Skip to content

fix(acp): start Codex chats in the access mode Codex allows - #1457

Merged
mariusvniekerk merged 8 commits into
mainfrom
pr/codex-acp-start-mode
Oct 22, 2026
Merged

fix(acp): start Codex chats in the access mode Codex allows#1457
mariusvniekerk merged 8 commits into
mainfrom
pr/codex-acp-start-mode

Conversation

@rodboev

@rodboev rodboev commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

A new Codex ACP chat now starts in a mode your Codex policy allows. If the policy permits only read-only and full access, the first prompt no longer fails with WorkspaceWrite is not in the allowed set. Before, every chat started in codex-acp's default workspace-write mode.

Forge asks Codex which sandbox and approvals it resolves, through a throwaway thread that leaves config and trust alone. Full access with never starts in agent-full-access, but only when an untrusted project's config isn't what grants it. Read-only with on-request starts in read-only. Other pairs, or a failed lookup, keep the default and show a notice.

A saved mode, INITIAL_AGENT_MODE, or a launcher's own mode always wins, and the derived mode isn't saved. The lookup runs only for codex-acp, npx, and bunx launches and can add a few seconds to chat start.

Refs agentclientprotocol/codex-acp#310

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (f11d4b0)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/workspace/localruntime/acp_test.go:1220: Probe tests use fabricated replies or the test executable, so they cannot verify that the real Codex CLI loads workspace access settings correctly or leaves user trust configuration unchanged. This violates the repository requirement to test external CLI behavior against the real CLI with isolated configuration.

    Fix: Add an isolated Go integration test using the real Codex CLI/adapter. Exercise global and project access settings through EffectiveAccess, verify resolved policies and unchanged user configuration, and skip when the CLI is unavailable.

Reviewers: 3x codex, codex (security) | Synthesis: codex, 9s | Total: 4m19s

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (0b76d49)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/codexhooks/access_test.go:25; internal/codexhooks/access_test.go:59: The real-Codex test configures only global settings in an empty workspace. Project-local access resolution through the temporary trust override is tested only with fabricated replies, so failure to load .codex/config.toml could select the wrong startup mode while all tests pass.

    Fix: Extend the isolated real-Codex test with project-local access settings that differ from global settings. Verify EffectiveAccess returns the project settings without persisting trust or modifying user configuration, and skip when Codex is absent.

Reviewers: 3x codex, codex (security) | Synthesis: codex, 8s | Total: 5m13s

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (b1c2d21)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/codexhooks/access_test.go:32: Real-CLI tests skip when codex or codex-acp is unavailable, but neither mise manifest installs them. CI can therefore silently omit access-resolution and config-preservation coverage, contrary to context/testing-basics.md.

    Fix: Declare codex and codex-acp in the appropriate mise manifest so CI runs these tests; retain local skips when the tools are unavailable.

Reviewers: 3x codex, codex (security) | Synthesis: codex, 7s | Total: 4m42s

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (8cc3d8e)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/workspace/localruntime/acp_settings.go:118: A saved Codex mode disables startup mode derivation without being passed into startup. If policy rejects the adapter's default mode, session/new fails before saved settings can be restored. The current test permits the default startup mode and misses this failure.

    Fix: Pass the saved Codex mode into INITIAL_AGENT_MODE before session creation, preserving explicit-setting precedence. Add coverage where the default startup mode is rejected but the saved mode is allowed.

Reviewers: 3x codex, codex (security) | Synthesis: codex, 9s | Total: 5m34s

@rodboev

rodboev commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Reworking this to also catch npx and standalone codex-acp launches, and saved modes Codex does not offer.

@rodboev rodboev changed the title fix(workspace): start Codex ACP chats in the mode Codex resolves fix(acp): start Codex chats in a mode your Codex policy allows Oct 22, 2026
@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (4a68df3)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/workspace/localruntime/acp.go:359: On Windows, when codex-acp or npx resolves to an extensionless shebang script and sh is available, procutil.Command rewrites cmd.Args to start with sh. Passing those transformed arguments to codexACPInitialMode causes command detection to fail, silently skipping access derivation and retaining the adapter's default mode.

    Fix: Derive the mode from the resolved original executable and original arguments before procutil applies its shell transformation. Add coverage for this Windows launch path.

Reviewers: 3x codex, codex (security) | Synthesis: codex, 8s | Total: 7m50s

@rodboev
rodboev force-pushed the pr/codex-acp-start-mode branch from 4a68df3 to eab439a Compare October 22, 2026 13:38
@rodboev rodboev changed the title fix(acp): start Codex chats in a mode your Codex policy allows fix(acp): start Codex chats in the access mode Codex allows Oct 22, 2026
@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (eab439a)

Verdict: Changes require fixes for 2 findings.

High

  • internal/workspace/localruntime/acp_test.go:1218: Access-mode tests replace the resolver or supply canned RPC replies, leaving actual Codex access resolution untested. This violates the required real-CLI boundary coverage and cannot detect broken forwarding, incorrect workspace policy resolution, or unintended config persistence.

    Fix: Add a focused integration test using real codex-acp with isolated configuration, skipping when unavailable. Exercise the production probe for distinct workspace policies, assert resulting modes and unchanged user config, and ensure CI installs the CLI.

  • internal/workspace/localruntime/acp_codex_test.go:31: The "plain" case in TestCodexACPProbeUsesLaunchContext duplicates the "full access" case at line 24, with identical inputs, mocked response, execution path, and assertions.

    Fix: Delete the "plain" table case and retain "full access".

Reviewers: 3x codex, codex (security) | Synthesis: codex, 12s | Total: 6m0s

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (911914a)

Verdict: No findings at or above medium severity.

Reviewers: 3x codex, codex (security) | Synthesis: codex | Total: 5m12s

@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (11379d4)

Verdict: No findings at or above medium severity.

Reviewers: 3x codex, codex (security) | Synthesis: codex | Total: 5m23s

@rodboev
rodboev marked this pull request as ready for review October 22, 2026 16:02
@mariusvniekerk mariusvniekerk self-assigned this Oct 22, 2026
@rodboev @mariusvniekerk
fix: start Codex chats in the resolved access mode
3b2c9f9 
Codex's workspace-write adapter default can reject the first prompt when the host permits only full access or read-only. Choose a matching mode after the adapter identifies itself, while preserving saved and explicit choices.

Keep derived choices temporary so later chats re-read the host policy. Policy mismatches and derivation failures should leave a usable chat with a notice rather than prevent startup.
@rodboev @mariusvniekerk
fix: preserve launcher-selected Codex chat modes
cb92ac0 
A launcher can choose read-only inside the adapter without exposing that choice in Forge's environment. Overriding it with full access defeats the user's explicit setting.

Use the mode advertised by the adapter as the authority for whether an automatic default is appropriate. Saved choices still take precedence, and derived choices remain temporary.
@rodboev @mariusvniekerk
fix: preserve Codex launch choices when deriving chat modes
a99dd93 
An explicit agent mode can look identical to the adapter default. Replacing it with full access defeats the user's launch choice.

Access probes must use the same launch context as the chat. Wrapper commands can supply settings that their CLI passthrough ignores, so deriving a mode from those probes can override the chat's actual policy. Bind executable lookup the same way as the session before probing.
@rodboev @mariusvniekerk
test: keep Codex access coverage focused on distinct behavior
980dcb4 
Repeated assertions made one mode or request change fail several tests, while the first-prompt fixture asserted its own rejection rule. Keep coverage with the tests that own each startup decision, access mapping, and request contract so failures point to the behavior that changed.
@rodboev @mariusvniekerk
test: verify Codex access resolution with real CLIs
885933c 
Canned replies and resolver stubs cannot catch drift in how Codex loads access settings. Exercise the real CLI boundary with isolated config so global defaults, project overrides, and the adapter's bundled CLI remain covered without rewriting user settings.
@rodboev @mariusvniekerk
fix: keep Codex chat access within trusted settings
c29b520 
A session-only trust override can make an untrusted project's settings appear to authorize Full access. That override must preserve restrictive project settings without turning project preferences into permission grants.

Keep startup notices focused on access settings that prevent a matching mode. Workspace-write settings already use the adapter's default Agent mode.
@rodboev @mariusvniekerk
fix: clarify Codex mode defaults and preference scope
87b9713 
Mode choices apply to the configured agent on its execution host across workspaces. The startup notice described a workspace-only choice and used a default-mode label that differs from the adapter's UI, which could mislead users about the mode and where their choice takes effect.
@rodboev @mariusvniekerk
test: catch Codex launch context regressions with focused cases
0d7b443 
Overlapping cases obscured which access decision changed, while the mode restoration stub accepted incorrect launch inputs. Keep distinct behavior covered and verify the session context reaches the access probe.

A missing sandbox field represents an older Codex reply more accurately than an empty sandbox type. The combined config case also checks that probing leaves the caller's settings intact.
@mariusvniekerk
mariusvniekerk force-pushed the pr/codex-acp-start-mode branch from 11379d4 to 0d7b443 Compare October 22, 2026 20:03
@roborev-ci

roborev-ci Bot commented Oct 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (0d7b443)

Verdict: No findings at or above medium severity.

Reviewers: 3x codex, codex (security) | Synthesis: codex | Total: 5m1s

@mariusvniekerk
mariusvniekerk merged commit 7eb1f61 into main Oct 22, 2026
63 of 64 checks passed
@mariusvniekerk
mariusvniekerk deleted the pr/codex-acp-start-mode branch October 22, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusvniekerk mariusvniekerk Awaiting requested review from mariusvniekerk

Assignees

@mariusvniekerk mariusvniekerk

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rodboev @mariusvniekerk