Skip to content

feat(agent): add Codex Auto-review setting - #407

Merged
zfy0701 merged 2 commits into
mainfrom
codex/auto-review-setting
Aug 2, 2026
Merged

feat(agent): add Codex Auto-review setting#407
zfy0701 merged 2 commits into
mainfrom
codex/auto-review-setting

Conversation

@zfy0701

@zfy0701 zfy0701 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a separate User / Auto-review reviewer setting for Codex agents in create, edit, and detail views
  • persist and replicate the setting through the Control Plane and AgentSpec without changing the existing permission mode
  • apply codex-acp's _approvals_reviewer option to new and restored ACP sessions, while safely falling back for runtimes that do not advertise it

Why

AgentConnect's managed @agentconnect.md/codex-acp build 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

  • daemon focused tests: 99 passed, plus the raw ACP evaluation reviewer test
  • web data tests: 54 passed
  • Control Plane MCP schema tests: 24 passed
  • Control Plane Postgres integration tests: 50 passed
  • protocol, daemon, Control Plane, and web typechecks
  • full repository ESLint via the pre-push hook

Created by Codex . GPT-5.6 Sol

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } : {}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zfy0701
zfy0701 merged commit 0ea3405 into main Aug 2, 2026
9 checks passed
@zfy0701
zfy0701 deleted the codex/auto-review-setting branch August 2, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant