feat(agent): add Codex Auto-review setting - #407
Conversation
There was a problem hiding this comment.
Clean, well-scoped change. Traced the new approvalsReviewer field end-to-end (protocol → CP DTO/repo/route → agentSpecAssembler → daemon write-agent/reconciler/acp-host → web), including both POST create and PATCH update paths on the CP route (verified against the actual PR head, not just the diff) — the field is threaded through correctly everywhere, and applySessionConfig correctly no-ops when approvalsReviewer is unset so it never resets an unconfigured runtime's default. Good test coverage (reconciler host-spawn-sig, acp-host fake-agent round-trip, CP integration test for create/patch/clear).
Nit (non-blocking): AgentRecord.approvalsReviewer is typed optional (approvalsReviewer?: ApprovalsReviewer | null, persistence/ports.ts) but agent.repo.ts's toRecord always sets it (ov.approvalsReviewer ?? null), same as the non-optional permissionMode: string | null sibling field. Consider dropping the ? for consistency.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
The main create/edit/detail → Control Plane persistence → AgentSpec replication → daemon session configuration path is coherent, and the advertised-option fallback correctly keeps unsupported runtimes usable. However, raw ACP evaluations currently report the new reviewer setting without applying it, so evaluation behavior and artifacts diverge. That correctness issue should be fixed before merge.
Validation performed on commit 26434054: protocol, daemon, Control Plane, and web typechecks; 99 focused daemon tests; 54 web data tests; 24 Control Plane MCP-schema tests; changed-file ESLint; and the repository’s GitHub CI suite all passed.
Non-blocking compatibility warning: the console exposes the setting for Codex by runtime identity even when an older adapter does not advertise the selector. The daemon safely skips it, which is acceptable for active development, but the UI may temporarily show a configured value that has no runtime effect.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| ...(typeof agent.runtimeOverrides?.model === 'string' ? { model: agent.runtimeOverrides.model } : {}), | ||
| settings: { | ||
| ...(typeof agent.permissionMode === 'string' ? { permissionMode: agent.permissionMode } : {}), | ||
| ...(typeof agent.approvalsReviewer === 'string' ? { approvalsReviewer: agent.approvalsReviewer } : {}), |
There was a problem hiding this comment.
[P2] Apply this setting to the raw ACP evaluation host
This adds approvalsReviewer to the evaluation subject metadata, but RawAcpEvaluationRunner later constructs its AcpHost with model, permission mode, effort, and fast mode only (configPrefs around lines 862–867). An agent configured with auto_review is therefore evaluated with codex-acp’s default user reviewer while the generated manifest/ATIF claims auto_review; any approval request then exercises the host fallback instead of the reviewer being evaluated. Please also pass approvalsReviewer: agent.approvalsReviewer into that host and cover the raw-runner path with a focused test.
There was a problem hiding this comment.
This revision addresses the nit from my previous pass (AgentRecord.approvalsReviewer is now non-optional, matching its sibling permissionMode), and fixes a real gap: RawAcpEvaluationRunner's configPrefs now includes approvalsReviewer (runner.ts:865), so raw-ACP evaluations actually apply the Auto-review selector to the ACP session instead of only recording it in the manifest's settings (which prepareSubject already did). New test coverage (echoConfigOptions fixture flag + assertions on the echoed _approvals_reviewer config option and manifest.subject.settings.approvalsReviewer) verifies the wiring end-to-end. No new issues found.
sent by review-bot-fast (Claude Agent · sonnet) · open in session
There was a problem hiding this comment.
Approved on commit 6bb2e6db. The synchronization fully addresses the prior blocking finding: RawAcpEvaluationRunner now forwards agent.approvalsReviewer through AcpHost.configPrefs, so raw evaluations apply the same reviewer setting that their manifest and ATIF metadata report. The expanded scriptable ACP fixture and evaluation-runner test verify the reconciled _approvals_reviewer option reaches currentValue: "auto_review", while the AgentRecord required-field cleanup matches the repository projection that always supplies a value or null.
Validation on the exact head: daemon and Control Plane typechecks passed; all 12 evaluation-runner tests passed; changed-file ESLint and diff checks passed. I found no new correctness issues in the four-file revision delta.
Non-blocking compatibility note: an older Codex adapter may not advertise this selector; the daemon’s existing advertised-option check safely skips it, which is acceptable during active development.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
Summary
User/Auto-reviewreviewer setting for Codex agents in create, edit, and detail views_approvals_revieweroption to new and restored ACP sessions, while safely falling back for runtimes that do not advertise itWhy
AgentConnect's managed
@agentconnect.md/codex-acpbuild now contains the reviewer capability from codex-acp #350, but AgentConnect had no configuration path for it. This exposes that capability as an independent reviewer choice; Auto-review does not widen the active sandbox or approval policy.Validation
Created by Codex . GPT-5.6 Sol