feat(ce-code-review): configurable model and effort for high-stakes reviewers - #1301
feat(ce-code-review): configurable model and effort for high-stakes reviewers#1301npwalker wants to merge 8 commits into
Conversation
…igh-stakes reviewers
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2a0cac63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- per-key fall-through: invalid/missing review_effort never drops a valid review_model pin - explicit unsupported-host outcome: no model selector -> omit override, inherit session model, disclose the unapplied pin - Stage 3d announce made self-sufficient with a minimal inline two-key config read
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b696af871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- alias-shape pre-bind check + rejected-selector fallback so a bad pin never costs a reviewer - template/docs state per-key fallback independently
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f0e257448
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Do not proceed until the final local roster is materialized. This is a routing boundary, not a preference: a started peer and the in-process adversarial reviewer must never both receive the same review brief. | ||
|
|
||
| Announce that final team before spawning, as a user-facing summary: name the always-on reviewers plainly, and for each conditional reviewer give the one-line reason it was added (the real concern, not the keyword that matched). Do **not** put local reviewer model-tier labels (`[session model]`/`[mid-tier]`) or scope-mode codenames in this announce — those are internal. Still decide each local reviewer's tier here and keep it in working state for Stage 4. The cross-model line is separate and follows the receipt-aware model/reasoning and route wording in its reference. This is progress reporting, not a blocking confirmation. | ||
| Before this announce, resolve the config pin directly with a minimal read — do not load `references/dispatch-reviewers.md` yet (Stage 4 below still gates that load): read `review_model` and `review_effort` from `<repo-root>/.compound-engineering/config.local.yaml` first, then `config.yaml` (first non-empty value matching the model-alias shape `^[A-Za-z0-9._-]{1,64}$` wins, `#`-commented lines ignored) — the same two-layer read the `docs_root` block above uses. Use these resolved values both for this stage's tier decision and for the announce line below. `references/dispatch-reviewers.md` (loaded at Stage 4) remains the authoritative statement of precedence and degradation for these keys and consumes the same resolved values; this Stage 3d read only lets the announce state the pin without loading that reference early. |
There was a problem hiding this comment.
When configuration contains an unsupported but alias-shaped effort such as review_effort: maximum, this new resolver accepts it because it applies the model-alias regex to both keys. Fresh evidence after the fallback wording was corrected is that the Stage 4 reference still never supplies the promised low | medium | high | xhigh validation or an effort-rejection retry, so a host exposing per-dispatch effort can reject the spawn instead of silently omitting the invalid effort. Resolve review_effort with its documented enum independently from the model alias.
AGENTS.md reference: AGENTS.md:L118-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolve
review_effortwith its documented enum independently from the model alias.
Parking this one for the author rather than patching a third round of resolution-edge prose. This and the sibling in-run-precedence thread share a root: how exhaustively the config-resolution prose should enumerate edge semantics (effort enum validation, effort-rejection retry, in-run precedence at Stage 3d) versus staying at the precedent's level of detail (plan_model/brainstorm_model carry none of this). Options: (a) add per-key enum validation + effort-rejection fallback wording now (most complete, but the prose keeps growing each round); (b) fold both remaining edge cases into one compact "resolution table" paragraph shared by Stage 3d and Stage 4 (one authoritative spec, no drift surface); (c) accept the current level of detail and track the residuals. Lean: (b). Leaving the thread open for the author's call.
| Do not proceed until the final local roster is materialized. This is a routing boundary, not a preference: a started peer and the in-process adversarial reviewer must never both receive the same review brief. | ||
|
|
||
| Announce that final team before spawning, as a user-facing summary: name the always-on reviewers plainly, and for each conditional reviewer give the one-line reason it was added (the real concern, not the keyword that matched). Do **not** put local reviewer model-tier labels (`[session model]`/`[mid-tier]`) or scope-mode codenames in this announce — those are internal. Still decide each local reviewer's tier here and keep it in working state for Stage 4. The cross-model line is separate and follows the receipt-aware model/reasoning and route wording in its reference. This is progress reporting, not a blocking confirmation. | ||
| Before this announce, resolve the config pin directly with a minimal read — do not load `references/dispatch-reviewers.md` yet (Stage 4 below still gates that load): read `review_model` and `review_effort` from `<repo-root>/.compound-engineering/config.local.yaml` first, then `config.yaml` (first non-empty value matching the model-alias shape `^[A-Za-z0-9._-]{1,64}$` wins, `#`-commented lines ignored) — the same two-layer read the `docs_root` block above uses. Use these resolved values both for this stage's tier decision and for the announce line below. `references/dispatch-reviewers.md` (loaded at Stage 4) remains the authoritative statement of precedence and degradation for these keys and consumes the same resolved values; this Stage 3d read only lets the announce state the pin without loading that reference early. |
There was a problem hiding this comment.
When config pins opus but the current request explicitly asks the reviewers to use another model, the new Stage 3d read binds and announces the config value without checking the in-run request, even though dispatch-reviewers.md later says that request outranks config. Because this line also requires recording the resulting tier before that reference is loaded, a literal execution can retain the config tier or at minimum announce a model that was not requested; include the direct-request precedence in this inline resolver before materializing working state.
AGENTS.md reference: AGENTS.md:L144-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
include the direct-request precedence in this inline resolver before materializing working state.
Parking for the author — same root as the effort-enum thread (see the decision context there): whether the Stage 3d inline resolver should carry the full precedence/validation spec or the prose should be consolidated into one authoritative resolution paragraph consumed by both stages. The finding itself is valid: the Stage 3d sentence should acknowledge an explicit in-run model request outranking the config keys. It will be folded into whichever consolidation shape the author picks. Leaving open.
- configuration.md no longer claims docs_root is the tracked layer's only consumer
|
@npwalker im reviewing this now but one request for future PR: have your agent follow the Pr template for the repo. Thanks! |
|
Apologies @tmchow! I started the process by forking a session I was in the process of not related to compound engineering so my agent claims we didn't know about the Claude.md instructions from this repo and then writing a subtly incorrect wrong zsh command to check for a PR template resulted in the other half of the failure. I'll keep this in mind going forward, I usually don't fork this way, was trying something new as I thought it might be context lighter ( and/or just simpler for me ) as my session would already be cached rather than asking my agent to read the session. Updated the PR body now. Have you considered adding the PR body as a CI check probably as an extension of the pr-title-check? I'd put a PR for the check but seems almost simpler to leave it to you if you want to. Let me know what you think either way. btw, love what you're doing with Compound Engineering and how much value you provide to all of us by continually improving it and sharing with us. ❤️ |
2 participants
Summary
Users who drive sessions on a cheap or low-effort orchestrator model can now pin ce-code-review's three high-stakes reviewers (
correctness-reviewer,security-reviewer,adversarial-reviewer) to a stronger configuration — e.g. Opus at high reasoning effort — instead of having them silently inherit the session model. Two new optional config keys do this:review_model(an exact pin) andreview_effort(applied where the harness's dispatch primitive exposes per-dispatch effort, silently skipped elsewhere). With the keys unset, behavior is unchanged.Design decisions worth reviewing:
review_modelapplies even when the session model is stronger, so the key can lower capability too; the template documents that the user owns the trade-off.plan_model/brainstorm_model(local-only), these keys also read the tracked.compound-engineering/config.yaml, so a repo-wide default reaches fresh worktrees that don't inherit the gitignored local file.docs/solutions/skill-design/dispatch-script-failure-degrade-outcome-not-boundary.md).Session-settled decisions carried from planning: trio-only scope (user-directed, over all-personas and two-key variants); model + best-effort effort keys (user-directed, over a compound
opus-highalias); two-layer local-first read (user-directed, over local-only); exact-pin semantics (user-approved, over floor-only elevation).Validation
Both new contract tests were observed red before the prose/template changes and green after; the pre-existing model-tiering test at
tests/review-skill-contract.test.ts:374passes unmodified; full suitebun test --parallelis green (2871 pass, 0 fail);skills/ce-setup/scripts/check-healthreports healthy with no new warnings.An independent multi-persona review of this branch validated four residual findings (one P1: the new Stage 3d announce line depends on config resolution prose that only loads at Stage 4; three test-assertion-strength items). They are recorded in
docs/residual-review-findings/nick-review-model-config.mdfor resolution during review.Security Disclosure
New input-handling surface, low risk: two optional config keys (
review_model,review_effort) are now read — including from the tracked.compound-engineering/config.yaml, so a repo can influence which model/effort review subagents run with. Values must match the existing model-alias shape^[A-Za-z0-9._-]{1,64}$before use (rejects empty, path-like, oversized, or shell-metacharacter values); anything failing the check is treated as unset and never passed through to dispatch. No shell/exec, path handling, secrets, permissions, or dependency changes.Agent Disclosure
Claude Code · claude-fable-5