Skip to content

Configure Codex and Claude reviewer models and efforts independently - #754

Merged
wwind123 merged 2 commits into
mainfrom
codex/role-model-overrides
Sep 9, 2026
Merged

Configure Codex and Claude reviewer models and efforts independently#754
wwind123 merged 2 commits into
mainfrom
codex/role-model-overrides

Conversation

@wwind123

@wwind123 wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Why

When Codex is both coder and reviewer, --codex-model currently selects both seats. Issue mode compounds this: the approved-implementation handoff replaces that shared model with --implementation-coder-model, so a Luna implementation also makes the Codex reviewer use Luna. PR mode has no independent reviewer selection either.

This PR adds explicit reviewer overrides without changing existing defaults or checkout ownership.

Changes

  • Add --reviewer-codex-model and --reviewer-codex-reasoning-effort.
  • Add matching --reviewer-claude-model and --reviewer-claude-effort options.
  • Resolve model and effort independently for each role. Reviewer overrides apply to plan reviews, PR reviews, and discussion participants, not coder or analyzer turns.
  • Use the resolved selection in actual backend arguments, prompt signatures, logs, and configured invocation metadata. Verified runtime observations retain precedence in published signatures.
  • Preserve explicit reviewer selections through the issue-to-PR handoff and generated recovery commands; reject conflicting raw CLI arguments.
  • Make unsupported-model diagnostics honor the reviewer model and recommend the reviewer flag when applicable.
  • Document precedence, issue-mode implementation switching, and explicit PR resume configuration.

Example

Luna/xhigh implements and addresses feedback; Sol/medium reviews:

agent-loop pr 123 --repo OWNER/REPO \
  --coder codex --reviewer claude --reviewer codex \
  --codex-model gpt-5.6-luna --codex-reasoning-effort xhigh \
  --reviewer-codex-model gpt-5.6-sol \
  --reviewer-codex-reasoning-effort medium

For issue planning, keep the desired planning model in --codex-model, put Luna in --implementation-coder-model, and retain the same reviewer options. Model-only overrides preserve effort fallback; effort-only overrides preserve model fallback. Separate resume invocations must repeat the desired options.

Scope

This does not separate coder/reviewer checkouts, change permissions, select a new repair backend, modify the installed tool, or restart the audit queue. Existing behavior is retained when no reviewer overrides are supplied.

Validation

  • Regression coverage includes CLI parsing, actual Codex/Claude command arguments and identity, coder-reviewer-coder isolation, sequential/parallel issue-to-PR handoffs, full/compact/discussion prompt signatures, recovery commands, and validation failures.
  • Full suite: python -m pytest -q passed, 2,641 tests in 176.84 seconds, using the installed project virtualenv with a 300-second shell timeout. Some existing config tests require user-cache access, so the full suite was run with that permission.
  • git diff --check passed. No live model calls were needed.

-- OpenAI Codex

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Review verdict: Blocking

Independent reviewer model/effort selection is threaded cleanly through resolve_invocation, both backends, the registry signature, the recovery command, and the implementation handoff, with solid new coverage in tests/test_role_models.py. One behavior/documentation mismatch blocks merge: the discuss agenda analyzer turn runs with role="reviewer", so it silently picks up the reviewer overrides even though README and docs state the analyzer is excluded, and its prompt signature is computed without the role, so the instructed signature disagrees with the model that actually runs. GitHub PR checks are reported passing; I ran no tests locally.

Blocking issues

  • _run_discuss_analyzer invokes the analyzer agenda turn with role="reviewer" (src/coding_review_agent_loop/orchestrator.py:10228). Now that role selects the model and effort, that turn runs on --reviewer-codex-model/--reviewer-claude-model and the reviewer effort, while the same analyzer's other turns (round synthesis, final synthesis, evidence reconciliation, semantic comparison) use role="analyzer" and the agent-wide settings. This contradicts README.md ("not the coder or discussion analyzer") and docs/local_agent_loop.md ("not the discussion analyzer or repair backend"), and it splits one analyzer's turns across two models. It also desynchronizes the signature: build_discuss_agenda_prompt computes agent_signature(analyzer, config) with no role (src/coding_review_agent_loop/prompts.py:3079), so the prompt instructs the agent-wide label while the run's model_used carries the reviewer label. Either pass role="analyzer" at that call site (matching the discuss-analyzer-r* label and the documented contract) or change the documentation and give build_discuss_agenda_prompt the matching role so signature and execution agree; add a test pinning the chosen behavior.

Same-PR follow-ups

  • _format_unresolved_item_label attributes a prior unresolved item with _public_reviewer_name(item.reviewer, config) and no role (src/coding_review_agent_loop/comment_rendering.py:208). When coder and reviewer are the same provider with different models, an item raised by the reviewer model is rendered with the coder/agent-wide model label, disagreeing with the reviewer's own footer, which uses the role-aware model_used. Pass role="reviewer" through _public_reviewer_name for that attribution path (the footer paths at lines 577/643/1051/1077 already receive the ground-truth model_used, so they need no change).

-- Anthropic Claude: claude-opus-5 (medium)

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Review verdict: Approved

The reviewer-specific Codex and Claude model/effort overrides are applied consistently across plan, PR, and discussion reviewer turns, remain independent from coder and analyzer selection, survive implementation handoff and recovery-command rendering, and are covered by focused tests and updated documentation. GitHub checks are passing.

-- OpenAI Codex: gpt-5.6-sol (medium)

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

PR review round 1 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation.

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Coder follow-up

Implemented and pushed both requested model-role consistency fixes to PR #754 in commit 81439ea.

Addressed items

  • item-1: Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: _run_discuss_analyzer invokes the analyzer agenda turn with role="reviewer" (src/coding_revie...
    • Resolution: Changed the discussion agenda invocation from the reviewer role to the analyzer role, keeping its configured model, effort, prompt signature, and other analyzer turns aligned. Added direct role regression coverage; the focused role-model module and existing discussion-loop integration test cover this path.
  • item-2: Same-PR follow-up from Anthropic Claude: claude-opus-5 (medium), round 1: _format_unresolved_item_label attributes a prior unresolved item with `_public_reviewer_name(it...
    • Resolution: Added role-aware public reviewer-name resolution and rendered unresolved review-item attributions with the reviewer role. Added regression coverage using distinct coder and reviewer models; the focused role-model module and existing public-name test cover compatibility and correct attribution.

Remaining items

  • None.

Tests run

  • python3 -m pytest tests/test_role_models.py -q (agent-loop instrumented; whole-command timeout 120s)
  • python3 -m pytest tests/test_discuss_loop.py::test_discuss_parallel_artifacts_are_isolated_by_round_and_agent tests/test_agent_loop.py::test_public_reviewer_name_config_aware_no_leakage -q (agent-loop instrumented; whole-command timeout 120s)

-- OpenAI Codex: gpt-5.6-sol (medium)

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Review verdict: Approved

Round 2 resolves both round-1 items. The discuss agenda turn now runs with role="analyzer" (src/coding_review_agent_loop/orchestrator.py:10228), so the analyzer keeps one identity across its agenda, round-synthesis, final-synthesis, reconciliation, and semantic-comparison turns, and it now matches the role-free signature that build_discuss_agenda_prompt instructs and the documented contract in README.md and docs/local_agent_loop.md. Prior-item attribution passes role="reviewer" through _public_reviewer_name, so an item raised by a reviewer-specific model is no longer labeled with the coder model. Both changes are pinned by new tests, and I found no test elsewhere that depended on the old analyzer role. The rest of the feature (resolve_invocation role precedence, backend model/effort wiring, registry signatures, arg-conflict validation, recovery-command preservation, and the implementation handoff) is unchanged from round 1 and reads correct. GitHub PR checks are still pending (test in_progress); I ran no tests locally, so this approval does not assert a green suite.

Prior unresolved item dispositions

  • [item-1] Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: _run_discuss_analyzer invokes the analyzer agenda turn with role="reviewer" (src/coding_revie... -> resolved
  • [item-2] Same-PR follow-up from Anthropic Claude: claude-opus-5 (medium), round 1: _format_unresolved_item_label attributes a prior unresolved item with `_public_reviewer_name(it... -> resolved

-- Anthropic Claude: claude-opus-5 (medium)

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Review verdict: Approved

The round-1 analyzer-role and reviewer-attribution defects are fixed with focused regression coverage, and the full diff shows consistent reviewer model/effort propagation across CLI configuration, backends, prompts, handoff recovery, and documentation. No blocking issues were found in this read-only review; tests were not run as instructed, and the authoritative GitHub test check remains pending.

Prior unresolved item dispositions

  • [item-1] Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: _run_discuss_analyzer invokes the analyzer agenda turn with role="reviewer" (src/coding_revie... -> resolved
  • [item-2] Same-PR follow-up from Anthropic Claude: claude-opus-5 (medium), round 1: _format_unresolved_item_label attributes a prior unresolved item with `_public_reviewer_name(it... -> resolved

-- OpenAI Codex: gpt-5.6-sol (medium)

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

PR review round 2 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation.

@wwind123

wwind123 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Reviewers approved PR #754; watching GitHub checks in the foreground. No coder or reviewer agents will run while checks remain pending.

-- coding-review-agent-loop

@wwind123
wwind123 merged commit 335525d into main Sep 9, 2026
1 check passed
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