From 1971f850e2cd26662e85ea81904bf2e0ae6b2937 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:09:21 +0100 Subject: [PATCH] fix: use maintained codex acp bridge (#1656) --- .github/workflows/e2e-tests.yml | 4 +- README.md | 2 +- .../internal/agent/agents/codex_acp.go | 49 ++++----------- .../agent/agents/codex_acp_cliflags_test.go | 59 ------------------- .../internal/agent/agents/codex_acp_test.go | 54 +++++++---------- .../agent/agents/helpers_catalog_test.go | 13 ++-- .../runtime/lifecycle/manager_launch_test.go | 8 +-- .../settings/controller/profile_crud_test.go | 28 ++------- .../settings/controller/reconciler_test.go | 46 +++++++++++++++ .../tests/settings/agent-profile-acp.spec.ts | 6 +- .../0034-agentclientprotocol-codex-acp.md | 27 +++++++++ docs/decisions/INDEX.md | 1 + 12 files changed, 129 insertions(+), 168 deletions(-) delete mode 100644 apps/backend/internal/agent/agents/codex_acp_cliflags_test.go create mode 100644 docs/decisions/0034-agentclientprotocol-codex-acp.md diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 8d71d3c2a0..1f8926e544 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -370,7 +370,9 @@ jobs: desktop-e2e: name: Desktop E2E Smoke runs-on: ubuntu-latest - timeout-minutes: 20 + # Includes Ubuntu WebKit/Tauri system package installation; GitHub apt mirrors + # can spend more than 20 minutes there before tests start. + timeout-minutes: 45 defaults: run: shell: bash diff --git a/README.md b/README.md index 7c8453ba61..a5cb762d91 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ Connect Kandev to the tools your team already uses — pull issues into the kanb | Agent | Launch | |:-------:|:----------:| | **Claude Code** | `npx -y @agentclientprotocol/claude-agent-acp` | -| **Codex** | `npx -y @zed-industries/codex-acp` | +| **Codex** | `npx -y @agentclientprotocol/codex-acp` | | **GitHub Copilot** | `npx -y @github/copilot --acp` | | **Gemini CLI** | `npx -y @google/gemini-cli --acp` | | **Amp** | `npx -y amp-acp` | diff --git a/apps/backend/internal/agent/agents/codex_acp.go b/apps/backend/internal/agent/agents/codex_acp.go index 426019c125..25311befc1 100644 --- a/apps/backend/internal/agent/agents/codex_acp.go +++ b/apps/backend/internal/agent/agents/codex_acp.go @@ -16,11 +16,7 @@ var codexACPLogoLight []byte //go:embed logos/codex_dark.svg var codexACPLogoDark []byte -const codexACPPkg = "@zed-industries/codex-acp" - -// CodexACPSandboxDiskFullReadCLIFlag is the seeded cli_flags text for disk-full-read sandbox access. -// Single quotes preserve inner JSON double-quotes through cliflags.Tokenise. -const CodexACPSandboxDiskFullReadCLIFlag = `-c 'sandbox_permissions=["disk-full-read-access"]'` +const codexACPPkg = "@agentclientprotocol/codex-acp" var ( _ Agent = (*CodexACP)(nil) @@ -28,41 +24,17 @@ var ( _ InferenceAgent = (*CodexACP)(nil) ) -// CodexACP implements Agent for the Zed Industries codex-acp package. -// It speaks the ACP protocol (JSON-RPC 2.0 over stdin/stdout) via a Rust binary -// wrapping OpenAI Codex. Used for A/B comparison against the native Codex agent. +// CodexACP implements Agent for the Agent Client Protocol codex-acp package. +// It speaks the ACP protocol (JSON-RPC 2.0 over stdin/stdout) through the npm +// bridge for OpenAI Codex. Used for A/B comparison against the native Codex agent. type CodexACP struct { StandardPassthrough } -// codexACPPermSettings seeds profile cli_flags for @zed-industries/codex-acp. -// Values are passed as -c key=value overrides (see codex-acp --help and -// https://developers.openai.com/codex/config-reference). Toggles default off; -var codexACPPermSettings = map[string]PermissionSetting{ - "config_approval_policy_never": { - Supported: true, - Default: false, - Label: "Skip approval prompts (config)", - Description: "-c approval_policy=never (allowed: untrusted, on-request, never, granular)", - ApplyMethod: PermissionApplyMethodCLIFlag, - CLIFlag: "-c", - CLIFlagValue: "approval_policy=never", - }, - "config_sandbox_disk_full_read": { - Supported: true, - Default: false, - Label: "Disk full read access (config)", - Description: `-c sandbox_permissions=["disk-full-read-access"] (legacy list; prefer sandbox_mode in config.toml)`, - ApplyMethod: PermissionApplyMethodCLIFlag, - CLIFlag: "-c", - CLIFlagValue: `'sandbox_permissions=["disk-full-read-access"]'`, - }, -} - // codexPassthroughPermSettings maps passthrough-only toggles to @openai/codex CLI -// flags. Not returned from PermissionSettings(): @zed-industries/codex-acp only -// accepts -c/--config overrides; ACP auto-approve uses agentctl approval_policy. -// The legacy --full-auto flag was removed; auto_approve uses --ask-for-approval never. +// flags. Not returned from PermissionSettings(): ACP auto-approve uses agentctl +// approval_policy. The legacy --full-auto flag was removed; auto_approve uses +// --ask-for-approval never. var codexPassthroughPermSettings = map[string]PermissionSetting{ PermissionKeyAutoApprove: { Supported: true, @@ -99,7 +71,7 @@ func (a *CodexACP) ID() string { return "codex-acp" } func (a *CodexACP) Name() string { return "Codex ACP Agent" } func (a *CodexACP) DisplayName() string { return "Codex" } func (a *CodexACP) Description() string { - return "OpenAI Codex coding agent using the ACP protocol via the Zed Industries bridge." + return "OpenAI Codex coding agent using the Agent Client Protocol bridge." } func (a *CodexACP) Enabled() bool { return true } func (a *CodexACP) DisplayOrder() int { return 2 } @@ -188,8 +160,7 @@ func (a *CodexACP) LoginCommand() *LoginCommand { } // Install both the user-facing OpenAI codex CLI (which `codex login` runs -// against) and the ACP bridge package — the bridge wraps codex internally -// and depends on it being on PATH. +// against) and the ACP bridge package used for chat sessions. func (a *CodexACP) InstallScript() string { return "npm install -g @openai/codex " + codexACPPkg } @@ -197,7 +168,7 @@ func (a *CodexACP) InstallScript() string { func (a *CodexACP) BillingType() usage.BillingType { return codexBillingType() } func (a *CodexACP) PermissionSettings() map[string]PermissionSetting { - return codexACPPermSettings + return emptyPermSettings } // InferenceConfig returns configuration for one-shot inference using ACP. diff --git a/apps/backend/internal/agent/agents/codex_acp_cliflags_test.go b/apps/backend/internal/agent/agents/codex_acp_cliflags_test.go deleted file mode 100644 index d6eeb2a2d9..0000000000 --- a/apps/backend/internal/agent/agents/codex_acp_cliflags_test.go +++ /dev/null @@ -1,59 +0,0 @@ -package agents - -import ( - "reflect" - "testing" - - "github.com/kandev/kandev/internal/agent/settings/cliflags" - "github.com/kandev/kandev/internal/agent/settings/models" -) - -func TestCodexACP_CLIFlagTokenise(t *testing.T) { - t.Parallel() - cases := []struct { - name string - raw string - want []string - }{ - { - name: "approval_policy", - raw: "-c approval_policy=never", - want: []string{"-c", "approval_policy=never"}, - }, - { - name: "sandbox_permissions", - raw: CodexACPSandboxDiskFullReadCLIFlag, - want: []string{"-c", `sandbox_permissions=["disk-full-read-access"]`}, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - got, err := cliflags.Tokenise(tc.raw) - if err != nil { - t.Fatalf("Tokenise: %v", err) - } - if !reflect.DeepEqual(got, tc.want) { - t.Fatalf("tokens = %#v, want %#v", got, tc.want) - } - }) - } -} - -func TestCodexACP_ResolveSeededCLIFlags_DefaultDisabled(t *testing.T) { - flags := make([]models.CLIFlag, 0, len(codexACPPermSettings)) - for _, s := range codexACPPermSettings { - flagText := s.CLIFlag - if s.CLIFlagValue != "" { - flagText = s.CLIFlag + " " + s.CLIFlagValue - } - flags = append(flags, models.CLIFlag{Flag: flagText, Enabled: s.Default}) - } - got, err := cliflags.Resolve(flags) - if err != nil { - t.Fatalf("Resolve: %v", err) - } - if got != nil { - t.Fatalf("expected no argv when defaults disabled, got %#v", got) - } -} diff --git a/apps/backend/internal/agent/agents/codex_acp_test.go b/apps/backend/internal/agent/agents/codex_acp_test.go index fe563bdfba..c9703ac666 100644 --- a/apps/backend/internal/agent/agents/codex_acp_test.go +++ b/apps/backend/internal/agent/agents/codex_acp_test.go @@ -17,40 +17,10 @@ func TestCodexACPRuntimeNoLongerBindMountsHostHome(t *testing.T) { } } -func TestCodexACP_PermissionSettings_CuratedConfigOverrides(t *testing.T) { +func TestCodexACP_PermissionSettings_NoBridgeCLIFlags(t *testing.T) { settings := NewCodexACP().PermissionSettings() - want := map[string]struct { - flagText string - defaultOn bool - }{ - "config_approval_policy_never": { - flagText: "-c approval_policy=never", defaultOn: false, - }, - "config_sandbox_disk_full_read": { - flagText: CodexACPSandboxDiskFullReadCLIFlag, defaultOn: false, - }, - } - if len(settings) != len(want) { - t.Fatalf("PermissionSettings() len = %d, want %d: %#v", len(settings), len(want), settings) - } - for key, spec := range want { - s, ok := settings[key] - if !ok { - t.Fatalf("missing %q in PermissionSettings()", key) - } - if !s.Supported || s.ApplyMethod != PermissionApplyMethodCLIFlag { - t.Fatalf("%q: unsupported or wrong apply method: %+v", key, s) - } - if s.Default != spec.defaultOn { - t.Fatalf("%q: Default=%v, want %v", key, s.Default, spec.defaultOn) - } - gotFlag := s.CLIFlag - if s.CLIFlagValue != "" { - gotFlag = s.CLIFlag + " " + s.CLIFlagValue - } - if gotFlag != spec.flagText { - t.Fatalf("%q: flag text %q, want %q", key, gotFlag, spec.flagText) - } + if len(settings) != 0 { + t.Fatalf("PermissionSettings() = %#v, want no Codex ACP bridge CLI flags", settings) } } @@ -64,6 +34,24 @@ func TestCodexACP_BuildCommand_NoCodexCLIFlags(t *testing.T) { } } +func TestCodexACP_UsesAgentClientProtocolBridge(t *testing.T) { + a := NewCodexACP() + want := []string{"npx", "-y", "@agentclientprotocol/codex-acp"} + + if got := a.BuildCommand(CommandOptions{}).Args(); !slices.Equal(got, want) { + t.Fatalf("BuildCommand = %#v, want %#v", got, want) + } + if got := a.Runtime().Cmd.Args(); !slices.Equal(got, want) { + t.Fatalf("Runtime Cmd = %#v, want %#v", got, want) + } + if got := a.InferenceConfig().Command.Args(); !slices.Equal(got, want) { + t.Fatalf("Inference Command = %#v, want %#v", got, want) + } + if got := a.InstallScript(); !strings.Contains(got, "@agentclientprotocol/codex-acp") { + t.Fatalf("InstallScript = %q, want agentclientprotocol bridge package", got) + } +} + func TestCodexACPSessionDirTemplate(t *testing.T) { a := NewCodexACP() cfg := a.Runtime().SessionConfig diff --git a/apps/backend/internal/agent/agents/helpers_catalog_test.go b/apps/backend/internal/agent/agents/helpers_catalog_test.go index a0b8ad9144..5fd865246d 100644 --- a/apps/backend/internal/agent/agents/helpers_catalog_test.go +++ b/apps/backend/internal/agent/agents/helpers_catalog_test.go @@ -26,12 +26,15 @@ func TestCatalogPermissionSettings_MergesCursorForce(t *testing.T) { } } -func TestCatalogPermissionSettings_MergesCodexCLIFlags(t *testing.T) { +func TestCatalogPermissionSettings_ExcludesCodexBridgeCLIFlags(t *testing.T) { catalog := CatalogPermissionSettings(NewCodexACP()) - if len(codexACPPermSettings) != 2 { - t.Fatalf("codexACPPermSettings len = %d, want 2", len(codexACPPermSettings)) + if _, ok := catalog["config_approval_policy_never"]; ok { + t.Fatal("catalog must not expose stale codex -c config override") } - if _, ok := catalog["config_approval_policy_never"]; !ok { - t.Fatal("missing codex config_approval_policy_never") + if _, ok := catalog["config_sandbox_disk_full_read"]; ok { + t.Fatal("catalog must not expose stale codex sandbox override") + } + if _, ok := catalog[PermissionKeyAutoApprove]; !ok { + t.Fatal("catalog missing universal auto_approve") } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go index 3f1194def9..942800e32e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go @@ -276,7 +276,7 @@ func TestConfigureAndStartAgent_DoesNotSendTaskDescriptionEnv(t *testing.T) { TaskID: "task-1", SessionID: "session-1", AgentProfileID: "profile-1", - AgentCommand: "npx -y @zed-industries/codex-acp", + AgentCommand: "npx -y @agentclientprotocol/codex-acp", WorkspacePath: t.TempDir(), Metadata: map[string]interface{}{ "runtime_env": map[string]string{"KEEP_ME": "yes"}, @@ -289,7 +289,7 @@ func TestConfigureAndStartAgent_DoesNotSendTaskDescriptionEnv(t *testing.T) { if err != nil { t.Fatalf("configureAndStartAgent() error = %v", err) } - if bootCommand != "npx -y @zed-industries/codex-acp" { + if bootCommand != "npx -y @agentclientprotocol/codex-acp" { t.Fatalf("bootCommand = %q, want agent command", bootCommand) } if configuredEnv["KEEP_ME"] != "yes" { @@ -311,7 +311,7 @@ func TestConfigureAndStartAgent_SpillsLargeWakePayloadEnv(t *testing.T) { TaskID: "task-1", SessionID: "session-1", AgentProfileID: "profile-1", - AgentCommand: "npx -y @zed-industries/codex-acp", + AgentCommand: "npx -y @agentclientprotocol/codex-acp", WorkspacePath: workspace, Metadata: map[string]interface{}{ "runtime_env": map[string]string{ @@ -388,7 +388,7 @@ func newConfigureCaptureAgentctlClient(t *testing.T, log *logger.Logger, capture *captured = req.Env _, _ = w.Write([]byte(`{"success":true}`)) case "/api/v1/start": - _, _ = w.Write([]byte(`{"success":true,"command":"npx -y @zed-industries/codex-acp"}`)) + _, _ = w.Write([]byte(`{"success":true,"command":"npx -y @agentclientprotocol/codex-acp"}`)) default: w.WriteHeader(http.StatusNotFound) } diff --git a/apps/backend/internal/agent/settings/controller/profile_crud_test.go b/apps/backend/internal/agent/settings/controller/profile_crud_test.go index d8bb8cbd82..dc5b820687 100644 --- a/apps/backend/internal/agent/settings/controller/profile_crud_test.go +++ b/apps/backend/internal/agent/settings/controller/profile_crud_test.go @@ -61,30 +61,12 @@ func TestSeedCLIFlags_EmptyForAgentWithNoCurated(t *testing.T) { } } -// TestSeedCLIFlags_FromCodexACP seeds codex-acp -c config overrides (off by -// default) and never @openai/codex passthrough flags like --ask-for-approval. -func TestSeedCLIFlags_FromCodexACP(t *testing.T) { +// TestSeedCLIFlags_EmptyForCodexACP ensures the Agent Client Protocol bridge +// does not expose stale @openai/codex -c overrides as ACP subprocess flags. +func TestSeedCLIFlags_EmptyForCodexACP(t *testing.T) { flags := seedCLIFlags(agents.NewCodexACP()) - - want := map[string]bool{ - "-c approval_policy=never": false, - agents.CodexACPSandboxDiskFullReadCLIFlag: false, - } - if len(flags) != len(want) { - t.Fatalf("expected %d seeded flags, got %d: %+v", len(want), len(flags), flags) - } - for _, f := range flags { - enabled, ok := want[f.Flag] - if !ok { - t.Errorf("unexpected seeded flag: %q", f.Flag) - continue - } - if f.Enabled != enabled { - t.Errorf("%q: Enabled=%v, want %v", f.Flag, f.Enabled, enabled) - } - if f.Description == "" { - t.Errorf("%q: missing Description", f.Flag) - } + if len(flags) != 0 { + t.Fatalf("expected no seeded flags for codex-acp, got %+v", flags) } } diff --git a/apps/backend/internal/agent/settings/controller/reconciler_test.go b/apps/backend/internal/agent/settings/controller/reconciler_test.go index 413258060e..2c054b4230 100644 --- a/apps/backend/internal/agent/settings/controller/reconciler_test.go +++ b/apps/backend/internal/agent/settings/controller/reconciler_test.go @@ -323,6 +323,52 @@ func TestProfileReconciler_HealsStaleModel(t *testing.T) { } } +func TestProfileReconciler_HealsStaleCodexModeAfterBridgeSwap(t *testing.T) { + st := newFakeStore() + dbAgent := &models.Agent{Name: "codex-acp"} + _ = st.CreateAgent(context.Background(), dbAgent) + existing := &models.AgentProfile{ + AgentID: dbAgent.ID, + Name: "Codex", + Model: "gpt-5.6-sol", + Mode: "auto", + } + _ = st.CreateAgentProfile(context.Background(), existing) + st.created = nil + + ag := &mockInferenceAgent{id: "codex-acp", displayName: "Codex", enabled: true} + caps := &fakeCapReader{ + caps: map[string]hostutility.AgentCapabilities{ + "codex-acp": { + AgentType: "codex-acp", + Status: hostutility.StatusOK, + Models: []hostutility.Model{{ID: "gpt-5.6-sol", Name: "GPT 5.6 Sol"}}, + CurrentModelID: "gpt-5.6-sol", + Modes: []hostutility.Mode{ + {ID: "read-only", Name: "Read Only"}, + {ID: "agent", Name: "Agent"}, + {ID: "agent-full-access", Name: "Agent Full Access"}, + }, + CurrentModeID: "agent", + }, + }, + } + r := newReconciler(t, st, caps, ag) + if err := r.Run(context.Background()); err != nil { + t.Fatalf("Run: %v", err) + } + if len(st.updated) != 1 { + t.Fatalf("expected 1 updated profile, got %d", len(st.updated)) + } + updated := st.updated[0] + if updated.Model != "gpt-5.6-sol" { + t.Errorf("model = %q, want gpt-5.6-sol", updated.Model) + } + if updated.Mode != "agent" { + t.Errorf("healed mode = %q, want agent", updated.Mode) + } +} + func TestProfileReconciler_CleansOrphanProfiles(t *testing.T) { st := newFakeStore() // Seed a DB row for an agent that is NOT registered in the registry. diff --git a/apps/web/e2e/tests/settings/agent-profile-acp.spec.ts b/apps/web/e2e/tests/settings/agent-profile-acp.spec.ts index dda3ea2bfc..369f2ac649 100644 --- a/apps/web/e2e/tests/settings/agent-profile-acp.spec.ts +++ b/apps/web/e2e/tests/settings/agent-profile-acp.spec.ts @@ -5,7 +5,7 @@ import { SessionPage } from "../../pages/session-page"; * Verifies the ACP-first profile editor: * * - Universal agentctl auto-approve toggle renders with danger styling. - * - Codex curated `-c` config toggles render (off by default). + * - Codex no longer renders stale `-c` config toggles unsupported by the ACP bridge. * - Profile name edits persist across reload (exercises the new AgentProfile * DTO shape with `mode` / `migrated_from` columns). * - Mode picker renders when the agent's capability cache advertises modes. @@ -38,10 +38,10 @@ test.describe("Agent profile — ACP-first", () => { if (agent.name === "codex-acp") { await expect( testPage.getByTestId("cli-flag-curated-config_approval_policy_never"), - ).toBeVisible(); + ).toHaveCount(0); await expect( testPage.getByTestId("cli-flag-curated-config_sandbox_disk_full_read"), - ).toBeVisible(); + ).toHaveCount(0); } // The mock agent advertises modes, so the mode picker is rendered. diff --git a/docs/decisions/0034-agentclientprotocol-codex-acp.md b/docs/decisions/0034-agentclientprotocol-codex-acp.md new file mode 100644 index 0000000000..ccfac62fb8 --- /dev/null +++ b/docs/decisions/0034-agentclientprotocol-codex-acp.md @@ -0,0 +1,27 @@ +# 0034: Agent Client Protocol Codex ACP Bridge + +**Status:** accepted +**Date:** 2026-07-10 +**Area:** backend, protocol + +## Context + +Kandev's `codex-acp` agent previously launched `@zed-industries/codex-acp`. That bridge exposed an older Codex model catalogue and did not advertise newer Codex models already available in the native OpenAI Codex CLI. The actively maintained ACP bridge is published as `@agentclientprotocol/codex-acp`. + +## Decision + +Kandev's `codex-acp` agent launches `npx -y @agentclientprotocol/codex-acp` for ACP chat and one-shot inference sessions. The install script still installs `@openai/codex` for `codex login`, then installs the ACP bridge package for Kandev sessions. + +No product spec update is required: the user-facing agent remains `codex-acp`; only the package that supplies the ACP bridge changes. + +## Consequences + +Codex ACP sessions receive the model catalogue and config options from the Agent Client Protocol bridge, including model IDs that the old bridge did not expose. Existing profiles using old model or mode IDs are reconciled by the profile healer: unavailable values are cleared or replaced with the probed current values. Auth method identifiers can differ between bridge implementations, so auth UI should keep consuming advertised methods instead of hard-coding bridge-specific IDs. + +Codex-specific ACP `cli_flags` are not advertised because the bridge entrypoint does not apply the native Codex `-c` config overrides to chat sessions. Kandev still exposes the universal agentctl auto-approve toggle for ACP permission requests and keeps native Codex flags scoped to passthrough mode. + +## Alternatives Considered + +- Keep relying on `@zed-industries/codex-acp` and assume npm or local symlinks redirect it. Rejected because Kandev should declare the package it intends to execute and not depend on external aliasing. +- Use native `@openai/codex` passthrough only. Rejected because passthrough does not provide the ACP chat integration Kandev needs. +- Wait for the Zed package to publish a newer release. Rejected because the maintained successor package already exposes the needed Codex capabilities. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index fc51039dbe..30c7e0ae83 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -39,3 +39,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 0031 | [Office skill reference files](0031-office-skill-reference-files.md) | accepted | backend | 2026-07-06 | | 0032 | [Configurable worktree branch names](0032-configurable-worktree-branch-names.md) | accepted | backend, frontend | 2026-07-07 | | 0033 | [Durable plan implementation start marker](0033-durable-plan-implementation-start.md) | accepted | backend, frontend | 2026-07-09 | +| 0034 | [Agent Client Protocol Codex ACP Bridge](0034-agentclientprotocol-codex-acp.md) | accepted | backend, protocol | 2026-07-10 |