feat(session): add Auto to permission controls - #413
Conversation
There was a problem hiding this comment.
Reviewed the Auto-permission-preset composition across daemon/protocol/web. Logic is sound: the synthetic agent:auto-review value never leaks onto the ACP wire (always decomposed to mode+_approvals_reviewer before set_config_option), and reviewer-disable-before-mode-widen ordering correctly avoids a transient Auto+full-access window. Test coverage is solid across all four packages.
Two small non-blocking notes:
AcpHost.setSessionPermissionPresetreturnsmodeApplied || reviewerApplied— if the mode-set call fails but the preceding reviewer-reset succeeded, this returnstrueeven though the requested mode never took effect. No current caller inspects the bool (daemon.ts discards it), so no live impact, but worth tightening if a future caller starts relying on it.- Every non-Auto permission change on a reviewer-capable runtime unconditionally re-sends
approvals_reviewer=user, even when alreadyuser— a redundant ACP round-trip on plain mode switches (harmless/idempotent, just extra chatter).
Nothing blocking; approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
This revision only touches packages/daemon/test/daemon-commands.test.ts: wrapping the full access and ask for approval permission-mode assertions in vi.waitFor (since the host apply is a fire-and-forget async chain, asserting immediately after onInbound was racy), and clearing the setSessionPermissionMode/setSessionApprovalsReviewer mocks before the Auto-preset step so that assertion can't be trivially satisfied by a leftover call from the prior step. Good test-robustness fix, no logic changes. My prior approval of the permission-preset composition logic (unchanged since the last revision) stands — approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
|
Changes requested: one permission-ordering gap remains.
Please use the same conditional ordering for new/load configuration and add a load regression test. Validated on sent by |
There was a problem hiding this comment.
Requesting changes for one permission-ordering gap.
setSessionPermissionPreset() safely clears Auto-review before switching a warm session to an ordinary (especially wider) mode, but restored sessions do not use that ordering. applySessionConfig() still applies configured mode before _approvals_reviewer. If a session is loaded with mode=agent and auto_review, then the Agent configuration has changed to Full Access + user review, the load path first applies Full Access while Auto-review is still active and only clears the reviewer afterward. That recreates the exact widened intermediate state this change is intended to prevent. Please make the load preference pipeline use the same conditional ordering—user reviewer before an ordinary/wider mode, auto_review only after Agent mode—and add a load regression test.
Validation on head 04002e28: all GitHub CI checks are green; daemon, web, protocol, and control-plane typechecks passed locally; focused daemon suites passed (222 tests); focused web suites passed (17 tests); protocol passed (240 tests); git diff --check passed. Compatibility and migration concerns were treated as non-blocking.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| const reviewerAvailable = this.approvalsReviewerOptions(sessionId) !== null | ||
| // Disable Auto-review before changing to an ordinary mode (especially Full | ||
| // Access); enable it only after the requested Agent mode is in force. | ||
| let reviewerApplied = false |
There was a problem hiding this comment.
This ordering only protects live preset changes. applySessionConfig() still orders mode before _approvals_reviewer for session/load, so loading a session that currently has Auto and restoring Full Access + user review first creates Full Access + Auto-review, then clears it. Route configured permissions through the same conditional ordering and cover the load case.
There was a problem hiding this comment.
This revision extends the "disable Auto-review before widening, enable it only after Agent mode is set" ordering (already correct in the live setSessionPermissionPreset mid-session switch reviewed earlier) to the initial config-preference application in AcpHost used on session/new/session/load. Previously that path always applied mode before _approvals_reviewer regardless of direction, which could fail against a runtime that rejects widening to full-access while auto-review is still active on a restored session — exactly the scenario the new fake-acp-agent.mjs behavior (AC_REJECT_AUTO_FULL_ACCESS) and the new acp-host.test.ts case simulate and verify. The conditional ordering is symmetric with the existing mid-session logic, and the undefined/no-reviewer-preference case (the common path for non-Codex runtimes) is unaffected since the array position doesn't matter when the reviewer entry is a no-op.
No issues found — approving.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved on a8f089cf.
The restored-session ordering issue is fixed: applySessionConfig() now clears auto_review before applying an ordinary/wider mode and enables it only after Agent mode, and the new load regression makes the fake adapter reject the previously unsafe Full Access + Auto sequence. The broader composite-preset handling across daemon, chat controls, protocol metadata, and Web surfaces remains coherent.
Validation: focused daemon suites passed (223 tests), focused Web suites passed (17 tests), protocol passed (240 tests), daemon typecheck passed, and git diff --check passed. GitHub Build and Check are green; the remaining CI jobs were still running when reviewed.
Non-blocking migration warning: older Agent configs that omit approvalsReviewer do not explicitly coalesce that value to user inside the load preference pipeline, so a restored prior Auto value can survive until the daemon's per-turn configured-setting reconciliation. Per the active-development guidance, this compatibility edge does not block approval.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
Summary
Autopermission preset to Home and session composers/permissioncontrolsmode=agentplus_approvals_reviewer=auto_reviewuserwhen leaving Auto, before widening to another permission modeThis follows up #410 so Agent configuration and every session-level control now use the same merged permission UI.
Validation
Created by Codex . GPT-5.6 Sol