diff --git a/.compound-engineering/config.local.example.yaml b/.compound-engineering/config.local.example.yaml index 23e532ff6..389821bcd 100644 --- a/.compound-engineering/config.local.example.yaml +++ b/.compound-engineering/config.local.example.yaml @@ -74,6 +74,29 @@ # plan_model: fable # model alias for the plan-authoring step (no default) # brainstorm_model: fable # model alias for approach generation (no default) +# Pin the model (and, best-effort, the reasoning effort) for ce-code-review's +# three high-stakes reviewers only -- correctness-reviewer, security-reviewer, +# and adversarial-reviewer. Every other persona (mid-tier, ce-doc-review, +# ce-simplify-code) is untouched. This is an exact pin, not a floor -- once +# set, it replaces session-model inheritance for the trio even when the +# session model is more capable, so a high or low choice here is a trade-off +# the user owns deliberately. Unlike plan_model/brainstorm_model above, this +# key is read two layers deep and local-first -- the gitignored +# config.local.yaml wins if set there, otherwise the TRACKED config.yaml +# supplies a repo-wide default that reaches fresh worktrees too. An explicit +# in-run model request (e.g. naming a model in your prompt) overrides either +# layer for that run only. review_effort only takes effect alongside a +# resolved review_model route, and only on a harness whose dispatch primitive +# exposes a per-dispatch effort control -- elsewhere it is silently skipped. +# Invalid, commented, or missing values fall through independently per key -- +# for review_model, straight to today's behavior (trio inherits the session +# model); for review_effort, it is simply omitted and never affects model +# resolution, so a valid review_model pin is never discarded because +# review_effort was absent or bad. + +# review_model: opus # model alias for the trio (no default; exact pin, can lower capability too) +# review_effort: high # low | medium | high | xhigh (no default; best-effort, trio-only) + # --- PR concept teaching (ce-commit-push-pr) --- # When a PR introduces a concept new to the codebase, the generated PR # description gains a "New concepts" section teaching it. The section toggle is diff --git a/docs/plans/2026-07-31-005-feat-review-model-config-plan.md b/docs/plans/2026-07-31-005-feat-review-model-config-plan.md new file mode 100644 index 000000000..b8eb5cc28 --- /dev/null +++ b/docs/plans/2026-07-31-005-feat-review-model-config-plan.md @@ -0,0 +1,193 @@ +--- +title: High-Stakes Reviewer Model Config - Plan +type: feat +date: 2026-07-31 +topic: review-model-config +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +product_contract_source: ce-brainstorm +execution: code +--- + +# High-Stakes Reviewer Model Config - Plan + +## Goal Capsule + +- **Objective:** Let users configure, via the CE config file, the model (and optionally reasoning effort) used for ce-code-review's three high-stakes reviewer personas (`correctness-reviewer`, `security-reviewer`, `adversarial-reviewer`), instead of always inheriting the session model. +- **Product authority:** This plan owns only the high-stakes trio's model selection. Mid-tier persona selection, ce-doc-review, ce-simplify-code, and the cross-model peer passes are not active scope. +- **Open blockers:** None. + +--- + +## Product Contract + +**Product Contract preservation:** changed: R8 — repo convention discovered in planning: model-elevation keys (`plan_model`, `brainstorm_model`) receive no check-health validation; the template/example byte-sync check is the only enforcement, so R8 now requires only silent runtime fall-through plus that sync, not new validation logic. R9 — CHANGELOG.md removed from the documentation surface: it is release-please-generated from conventional commits, never hand-edited per PR. All other requirements and every Key Decision unchanged. + +### Summary + +Add two optional config keys, `review_model` and `review_effort`, that pin the model and reasoning effort for ce-code-review's three session-model reviewers. Unset keys preserve today's behavior byte-identically. This lets a user drive an inexpensive session model (e.g. a low-effort orchestrator) while the highest-stakes review analysis runs on a stronger configuration (e.g. Opus at high effort). + +### Problem Frame + +The model tiering in `skills/ce-code-review/references/dispatch-reviewers.md` hard-codes two tiers: the trio inherits the session model, everything else gets the platform mid-tier. "Session model" was always a proxy for "most capable available," but that proxy breaks when a user deliberately runs a cheap or low-effort session model as the orchestrator: the highest-stakes analysis silently runs at the orchestrator's low capability, and the only workaround is per-session instruction overrides that don't persist or propagate. Config precedent exists (`plan_model`, `brainstorm_model`, `cross_model_peer`) but none of those keys reach the review personas. + +### Key Decisions + +- KD1. **Scope is the high-stakes trio only.** (session-settled: user-directed — chosen over all-personas and two-key variants: the trio is the only tier where session-model inheritance misfires; mid-tier stays as designed.) Governs R1, R6. +- KD2. **Two keys, model plus best-effort effort.** (session-settled: user-directed — chosen over model-only and a compound `opus-high` alias: captures the full "Opus 5 high" intent without inventing new alias grammar.) Governs R2, R3. +- KD3. **Read both config layers, local-first.** (session-settled: user-directed — chosen over the local-only convention of `plan_model`: fresh worktrees don't inherit the gitignored `config.local.yaml`, so a tracked repo-wide default matters for worktree-heavy workflows.) Governs R4. +- KD4. **Exact pin, not a floor.** When set, the key's model is used even if the session model is more capable — the user owns the trade-off. Governs R1. + +### Requirements + +**Config keys** + +- R1. When `review_model` is set to a valid model alias, ce-code-review dispatches `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` with that model override; when unset, commented, or invalid, the trio inherits the session model exactly as today. +- R2. When `review_effort` is set (`low` | `medium` | `high` | `xhigh`), it is applied to the trio's dispatches on harnesses whose subagent primitive exposes a per-dispatch effort override; where the primitive exposes none, the dispatch proceeds with the model override alone and no error. +- R3. `review_effort` has effect only when the dispatch carries an explicit model route for the trio (via `review_model` or an in-run request per R5); it never alters the default inherit-session-model dispatch. +- R4. Both keys are read from `.compound-engineering/config.local.yaml` first, then the tracked `.compound-engineering/config.yaml`; first non-empty value per key wins (the `docs_root` precedence shape). + +**Precedence and transparency** + +- R5. An explicit in-run request naming a model for the reviewers outranks the config keys for that run, in either direction (elevate or lower). +- R6. Mid-tier persona dispatch is unchanged: every non-trio persona keeps the platform mid-tier override regardless of these keys. +- R7. When the pin is active, the Stage 3d reviewer-team announce includes one line naming the configured model as *requested* (and effort, when applied); when the keys are unset the announce is unchanged. + +**Validation and docs** + +- R8. Invalid or commented key values fall through silently to defaults at runtime; no check-health validation logic is added (matching the `plan_model`/`brainstorm_model` convention). The existing check-health template/example byte-sync check must pass with the new keys documented. +- R9. The keys are documented in `skills/ce-setup/references/config-template.yaml`, its byte-identical mirror `.compound-engineering/config.local.example.yaml`, `docs/skills/configuration.md`, and `docs/skills/ce-code-review.md`, including an explicit note that the pin can lower capability as well as raise it. +- R10. The existing contract-test literals in `tests/review-skill-contract.test.ts` (the three reviewer names, "platform's balanced mid-tier model", "omit the override") remain satisfied, and new assertions pin the `review_model`/`review_effort` config-resolution prose in `dispatch-reviewers.md` and the keys' presence in the config template. + +### Acceptance Examples + +- AE1. **Covers R1, R7.** Given `review_model: opus` in `config.local.yaml`, when ce-code-review dispatches its reviewer team from a Fable-low session, then the trio is dispatched with `model: opus`, mid-tier personas with the Sonnet-class override, and the announce names Opus for the high-stakes reviewers. +- AE2. **Covers R1.** Given no `review_model` key (or only the commented template line), when ce-code-review runs, then the trio inherits the session model and behavior is identical to today. +- AE3. **Covers R2.** Given `review_model: opus` and `review_effort: high` on a harness without per-dispatch effort, when the trio is dispatched, then the model override applies, effort is skipped silently, and the review completes normally. +- AE4. **Covers R4.** Given `review_model: opus` in the tracked `config.yaml` and `review_model: sonnet` in `config.local.yaml`, when ce-code-review runs, then the trio runs on Sonnet (local wins). +- AE5. **Covers R5.** Given `review_model: opus` in config, when the user's review request says "run the reviewers on fable", then the trio runs on Fable for that run. +- AE6. **Covers R3.** Given `review_effort: high` in `config.local.yaml` but no `review_model` key and no in-run model request, when ce-code-review runs, then the trio inherits the session model and `review_effort` is silently ignored. + +### Scope Boundaries + +- Mid-tier persona model selection (`review_mid_model` or similar) — deferred; nobody has asked for it. +- ce-doc-review and ce-simplify-code persona models — unchanged. +- Cross-model peer model/effort (hard-coded per provider in the cross-model scripts, deliberately) — unchanged. +- A user-global `~/.compound-engineering/` config layer — outside this change's identity; the two repo layers suffice. +- Off-host dispatch (CLI fallback) when the harness can't serve the configured model natively — not in scope; the native subagent override is the only route. +- CHANGELOG.md — release-please-generated; carried by the conventional-commit PR title, never hand-edited. + +### Dependencies / Assumptions + +- Assumes the target harness's subagent primitive accepts a per-dispatch model override (true for Claude Code's Agent tool; harnesses without a recognized selector follow the existing degradation rule in `dispatch-reviewers.md`: omit and inherit). +- Assumption: per-dispatch reasoning effort is not exposed by Claude Code's Agent tool today; `review_effort` is forward-compatible there and immediately effective only on harnesses that expose effort. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. **Prose-only mechanics, no new scripts.** (session-settled: user-approved — chosen over adding a config-reader helper script: the skill is instruction-prose; the two-layer read is expressed the same way the `ce-docs-root` block expresses `docs_root`, and reviewers are dispatched by the orchestrating agent, not code.) Cites R1, R4. +- KTD2. **Config resolution lands in `dispatch-reviewers.md`'s Model tiering subsection; the Spawning Session-model bullet gets the one-sentence override hook.** The Mid-tier bullet and the three-name exception list stay verbatim so `tests/review-skill-contract.test.ts:374-407` literals keep passing. Cites R6, R10. +- KTD3. **Template style follows the Model elevation block, not the docs_root block.** Alias-based, commented-out examples, no validation, no default — `# review_model: opus` / `# review_effort: high` appended to the "Model elevation" section of `config-template.yaml`, mirrored byte-identically into `.compound-engineering/config.local.example.yaml` (enforced by check-health and `tests/skills/ce-setup-check-health.test.ts:55`). Cites R8, R9. +- KTD4. **Degrade the outcome, never the boundary** (per `docs/solutions/skill-design/dispatch-script-failure-degrade-outcome-not-boundary.md`): effort silently no-ops on unsupported harnesses, but a configured model pin never silently falls back to session-model inheritance. Cites R1, R2, R3. +- KTD5. **Announce names the model as requested, not verified** (per `docs/solutions/skill-design/requested-vs-verified-model-identity.md`): the Stage 3d line claims what was configured/requested; it does not assert the harness served it. Cites R7. + +### High-Level Technical Design + +Resolution order the enriched prose must encode, evaluated once per ce-code-review run at Stage 3d tier-decision time: + +``` +in-run explicit model request (either direction) + > review_model in config.local.yaml + > review_model in config.yaml (tracked) + > inherit session model (today's default) + +review_effort (same two-layer read) applies only when an explicit +model route resolved above, and only on harnesses exposing a +per-dispatch effort override; otherwise silently skipped. +Scope: correctness-reviewer, security-reviewer, adversarial-reviewer. +Mid-tier personas: untouched by all of the above. +``` + +--- + +## Implementation Units + +### U1. Config-resolution prose in dispatch-reviewers.md + +- **Goal:** The Model tiering and Spawning subsections resolve `review_model`/`review_effort` and route the trio's dispatch accordingly. +- **Requirements:** R1, R2, R3, R4, R5, R6, R7 (KD1-KD4, KTD1, KTD2, KTD4, KTD5). +- **Dependencies:** None. +- **Files:** `skills/ce-code-review/references/dispatch-reviewers.md`, `skills/ce-code-review/SKILL.md` (Stage 3d announce sentence, per R7/KTD5 — the announce guidance lives only at SKILL.md:484). +- **Approach:** + 1. In `#### Model tiering`: add a short "Config override" paragraph — read both keys per the two-layer local-first read (mirror the `ce-docs-root` block's wording shape: `config.local.yaml` then `config.yaml`, first non-empty wins, root via `git rev-parse --show-toplevel`, `#`-commented lines ignored); a set `review_model` replaces session-model inheritance for the three named reviewers only; an explicit in-run model request outranks the keys in either direction; invalid/missing values are treated as unset and fall through silently — a typo'd alias is never passed through as a pin (per R1, R4, R5). Also qualify the two now-absolute statements this paragraph contradicts: line 26's "inherit the session model with no override" gains "by default", and line 42's "the tier is a deterministic function of the persona" becomes "of the persona and the resolved `review_model` config". + 2. In `#### Spawning`, edit only the **Session model** bullet: it inherits the session model *unless* `review_model` resolved above — then pass that model, plus `review_effort` where the dispatch primitive exposes an effort override, omitting effort silently where it does not (per R2, KTD4). Leave the Mid-tier bullet and every existing test literal untouched. + 3. Add one sentence to the Stage 3d announce guidance at `skills/ce-code-review/SKILL.md:484`: when the pin is active, name the configured model as requested (per R7, KTD5); keep the existing no-tier-labels prohibition intact. +- **Patterns to follow:** `ce-docs-root` block in `skills/ce-code-review/SKILL.md:37-43` (two-layer read wording); `skills/ce-plan/references/reasoning-elevation.md` activation-resolution section (precedence wording). +- **Test scenarios:** `Test expectation: none in this unit -- instruction-prose change; behavior is pinned by U3's contract tests.` +- **Verification:** `bun test tests/review-skill-contract.test.ts` passes with the line-374 test unmodified. + +### U2. Config template, example mirror, and docs + +- **Goal:** The keys are discoverable and documented everywhere config keys live. +- **Requirements:** R8, R9 (KTD3). +- **Dependencies:** U1 (cites its settled wording). +- **Files:** `skills/ce-setup/references/config-template.yaml`, `.compound-engineering/config.local.example.yaml`, `docs/skills/configuration.md`, `docs/skills/ce-code-review.md`. +- **Approach:** + 1. Append commented `# review_model: opus` and `# review_effort: high` entries to the template's "Model elevation" section, with a comment block covering: trio-only scope, exact-pin semantics (can lower capability too — user owns it), two-layer local-first read (unlike `plan_model`, which is local-only), effort best-effort degradation, in-run request precedence (per R9). + 2. Copy the template byte-identically over `.compound-engineering/config.local.example.yaml` (per R8). + 3. Add a `docs/skills/configuration.md` options-table row: `[ce-code-review](./ce-code-review.md)` | `review_model`, `review_effort` | one-line description with precedence and degradation. + 4. Add a short "Configuring the high-stakes reviewer model" subsection to `docs/skills/ce-code-review.md`. +- **Patterns to follow:** "Model elevation" comment block (`config-template.yaml:61-75`); configuration.md row format; the docs_root prose in configuration.md for two-layer phrasing. +- **Test scenarios:** `Test expectation: none in this unit -- docs/config-comment change; the byte-sync and key-presence invariants are asserted in U3.` +- **Verification:** `bun test tests/skills/ce-setup-check-health.test.ts` passes; `skills/ce-setup/scripts/check-health` emits no new warnings in a configured repo. + +### U3. Contract-test updates + +- **Goal:** The new prose and template entries are pinned by tests so future edits can't silently drop them. +- **Requirements:** R10 (KTD2, KTD3). +- **Dependencies:** U1, U2. +- **Files:** `tests/review-skill-contract.test.ts`, `tests/skills/ce-setup-check-health.test.ts`. +- **Approach:** + 1. New test in `review-skill-contract.test.ts` (same read-and-slice style as the line-374 test): slice the Model tiering/Spawning region of `dispatch-reviewers.md` and assert it contains `review_model`, `review_effort`, the two-layer read (`config.local.yaml` then `config.yaml`), the three reviewer names in the override's scope, and the silent fall-through wording. + 2. Extend the key-advertisement test in `ce-setup-check-health.test.ts` (line-111 style) to assert the template contains `review_model` and `review_effort`. +- **Patterns to follow:** `tests/review-skill-contract.test.ts:374-407`; `tests/skills/ce-setup-check-health.test.ts:55,111`. +- **Test scenarios:** + - New prose test fails against the pre-U1 file (revert the U1 paragraph locally to confirm red), passes after U1. + - Existing "Stage 4 spawning restates model-override imperative" test passes unmodified against the U1 wording. + - Template key-advertisement test fails when `review_model` is absent from `config-template.yaml`, passes after U2. + - Template/example byte-sync test passes after U2. +- **Verification:** `bun test --parallel` green. + +--- + +## Verification Contract + +| Gate | Command | Applies to | Done signal | +|---|---|---|---| +| Contract tests | `bun test tests/review-skill-contract.test.ts` | U1, U3 | pass; line-374 test untouched | +| Health-check tests | `bun test tests/skills/ce-setup-check-health.test.ts` | U2, U3 | pass, incl. template/example byte-sync | +| Full suite | `bun test --parallel` | all | green | +| Manual prose walk | read `dispatch-reviewers.md` Model tiering + Spawning end-to-end | U1 | resolution order matches the HTD block; no contradiction with `SKILL.md:484` | + +## Definition of Done + +- All three units landed; `bun test --parallel` green. +- `config-template.yaml` and `.compound-engineering/config.local.example.yaml` byte-identical. +- AE1-AE6 each traceable to prose that produces that behavior (AE1/AE2/AE4/AE5/AE6 via U1's resolution paragraph, AE3 via the effort-degradation sentence). +- No change to mid-tier persona dispatch wording, ce-doc-review, ce-simplify-code, or the cross-model scripts. +- Conventional-commit PR title carries the feature for release-please (e.g. `feat(ce-code-review): configurable high-stakes reviewer model`). + +--- + +## Sources & Research + +- `skills/ce-code-review/references/dispatch-reviewers.md:24-47` — current two-tier rule; the Spawning Session-model bullet is the single edit point. +- `skills/ce-code-review/SKILL.md:37-43` (ce-docs-root block), `:484` (announce tier-label prohibition). +- `skills/ce-setup/references/config-template.yaml:61-75` — Model elevation block, the style template for the new keys. +- `skills/ce-setup/scripts/check-health` — `read_flat_config_value` (~39-60); model-elevation keys have no validation there (convention adopted by R8); template/example sync check (~403-410). +- `tests/review-skill-contract.test.ts:374-407`; `tests/skills/ce-setup-check-health.test.ts:55,111`. +- `docs/solutions/skill-design/dispatch-script-failure-degrade-outcome-not-boundary.md` (KTD4); `docs/solutions/skill-design/requested-vs-verified-model-identity.md` (KTD5); `docs/solutions/skill-design/validate-skill-prose-behavior-with-cross-host-evals.md` — contract tests pin literals, not agent behavior; a cross-host behavioral eval is a worthwhile follow-up outside this PR. +- CHANGELOG.md is release-please-generated (package.json `release:*` scripts) — no hand edit. diff --git a/docs/residual-review-findings/nick-review-model-config.md b/docs/residual-review-findings/nick-review-model-config.md new file mode 100644 index 000000000..60c027a15 --- /dev/null +++ b/docs/residual-review-findings/nick-review-model-config.md @@ -0,0 +1,12 @@ +# Residual Review Findings — nick/review-model-config + +Source: ce-code-review run 20260731-214329-4ae4d74b (mode:agent, plan docs/plans/2026-07-31-005-feat-review-model-config-plan.md). All findings validated by an independent validation pass; none met the autonomous apply bar (anchor-75, single reviewer), so they are recorded here for resolution during PR review. + +## Residual Review Findings + +- ~~P1 Stage 3d announce sequencing gap~~ — resolved during PR review: Stage 3d now performs a minimal inline two-key read (Codex review thread raised the same defect; fixed on the PR). +- **P2** `tests/review-skill-contract.test.ts:409` — the new contract test never pins the dispatch-time Session-model bullet that actually applies `review_model`; reverting that bullet alone would not fail any test. Suggested fix: assert the bullet's override wording directly. (testing, confidence 75) +- **P3** `tests/review-skill-contract.test.ts:417` — two-layer precedence asserted by presence (`toContain` x2), not order; reversing local-first precedence in prose would not be caught. Suggested fix: one ordered regex. (testing, confidence 75) +- **P3 (advisory)** `tests/review-skill-contract.test.ts:453` — the trio-order regex is satisfied by the pre-existing line-26 enumeration, so it does not pin the new Config override paragraph. Suggested fix: scope the regex to the new paragraph. (correctness + testing, confidence 75) + +Tracker filing: skipped — fork has issues disabled; upstream issues inappropriate for unmerged branch residuals (no_sink). diff --git a/docs/skills/ce-code-review.md b/docs/skills/ce-code-review.md index ffbc5692d..4b4e5ee7d 100644 --- a/docs/skills/ce-code-review.md +++ b/docs/skills/ce-code-review.md @@ -90,6 +90,14 @@ This shares the provider/route kernel with `ce-doc-review` (parity-tested in CI) Large diffs stay on the same single-peer route without being serialized into one enormous prompt. The orchestrator sends a compact semantic review map — intent, material risk divisions, generated-tree treatment, and cross-division interactions — while the worker keeps the exact diff outside the prompt as a private, selectively readable artifact. Deterministic code never invents risk groups or cuts semantic shards; the adversarial agent works within the orchestrator's divisions and narrows its reads again when needed. +### 1c. Configuring the high-stakes reviewer model + +By default the three high-stakes reviewers — `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` — inherit whatever model is running the current session, same as every other persona. Set `review_model` (a model alias, e.g. `opus`) to pin those three, and only those three, to a specific model regardless of the session model. This is an exact pin, not a floor: it can lower capability as well as raise it, so the choice is a trade-off the user owns deliberately. + +Unlike `plan_model`/`brainstorm_model`, `review_model` is read two layers deep, local-first: the checkout-local `config.local.yaml` wins if it sets the key, otherwise the **tracked** `config.yaml` supplies a repo-wide default — useful here specifically because fresh worktrees don't inherit the gitignored local file. An explicit in-run model request (naming a model in your prompt, in either direction) overrides either layer for that run only. + +`review_effort` (`low | medium | high | xhigh`) refines the pin's reasoning effort, but only when a `review_model` route actually resolved (from config or an in-run request) and only on a harness whose dispatch primitive exposes a per-dispatch effort control — elsewhere it's silently skipped. Fall-through is independent per key: an invalid, commented, or missing `review_model` falls through silently to today's behavior (trio inherits the session model), while an invalid, commented, or missing `review_effort` is simply omitted and never discards a valid `review_model` pin. See the [configuration reference](./configuration.md) for the shared config-layer contract. + ### 2. Severity (P0-P3) and autofix class are orthogonal Severity answers **urgency** (P0=critical breakage, P3=user discretion). The autofix class is **signal** about follow-up shape (not apply permission): diff --git a/docs/skills/configuration.md b/docs/skills/configuration.md index a0c4d06e3..e3dc64984 100644 --- a/docs/skills/configuration.md +++ b/docs/skills/configuration.md @@ -8,7 +8,7 @@ Run `/ce-setup` to create or repair the file and its `.gitignore` coverage. The By default every CE-written artifact folder lives under `docs/` — `docs/plans/`, `docs/solutions/`, and the rest. `docs_root` relocates that root to any repo-relative folder, for projects where `docs/` is already tracked content owned by something else (an Obsidian vault, a docs site). Unset, behavior is byte-identical to today. -`docs_root` reads from two layers, first non-empty wins: the checkout-local `config.local.yaml` above, then a **tracked** `.compound-engineering/config.yaml`. Prefer the tracked file — it is committed, so the setting reaches every clone and worktree of the project; the local file is per-checkout and would have to be re-set in each new worktree. (The tracked `config.yaml` is a general layer; today `docs_root` is its only consumer.) +`docs_root` reads from two layers, first non-empty wins: the checkout-local `config.local.yaml` above, then a **tracked** `.compound-engineering/config.yaml`. Prefer the tracked file — it is committed, so the setting reaches every clone and worktree of the project; the local file is per-checkout and would have to be re-set in each new worktree. (The tracked `config.yaml` is a general layer; its consumers today are `docs_root` and the `review_model`/`review_effort` keys.) Two things make `docs_root` unlike the other settings: @@ -40,6 +40,7 @@ All settings are optional. Commented examples are documentation, not active valu | [`ce-plan`](./ce-plan.md), [`ce-brainstorm`](./ce-brainstorm.md) | `plan_model`, `brainstorm_model` | Model elevation: send the reasoning-heavy step to a named model (e.g. `fable`, `opus`) instead of the session model. Value is a model alias; a prompt request or an orchestrator's `plan_model:` carrier (e.g. from `lfg`, honored even in pipeline mode) overrides it. Takes effect on every harness — natively where the host serves the model, else via the Claude CLI, else inline. No default (elevation off). | | [`ce-work`](./ce-work.md), [`lfg`](./lfg.md) | `work_engine_mode`, `work_engine_preferences` | Ordered implementation-author preferences. Mode is `off`, `prefer`, or `require`; each entry has a `harness` and optional `model`. See [Implementation routing](#implementation-routing). | | [`ce-code-review`](./ce-code-review.md), [`ce-doc-review`](./ce-doc-review.md) | `cross_model_peer` | Preferred cross-model review target: `codex`, `claude`, `grok`, `cursor`, or `composer`. The review skills still apply host-independence and route-availability gates. | +| [`ce-code-review`](./ce-code-review.md) | `review_model`, `review_effort` | Exact-pin model (and best-effort reasoning effort) for the high-stakes trio only — correctness-reviewer, security-reviewer, adversarial-reviewer; every other persona is unaffected. Read two layers, local-first (`config.local.yaml` then the tracked `config.yaml`, unlike `plan_model`/`brainstorm_model`'s local-only read); an in-run model request overrides both for that run. The pin replaces session-model inheritance even when it lowers capability — the user owns that trade-off. `review_effort` applies only alongside a resolved model route and only where the harness exposes per-dispatch effort; otherwise it is silently skipped. No default (elevation off). | | [`ce-commit-push-pr`](./ce-commit-push-pr.md) | `pr_teaching_section`, `pr_teaching_archive`, `auto_babysit` | Toggle PR concept teaching, opt into explainer archival, or opt out of the default babysit handoff. Defaults: `true`, `false`, and `true`. | | [`ce-product-pulse`](./ce-product-pulse.md) | `pulse_product_name`, `pulse_lookback_default`, `pulse_primary_event`, `pulse_value_event`, `pulse_completion_events` | Product identity, reporting window, and the events that represent engagement, value, and completion. The setup interview writes these values. | | [`ce-product-pulse`](./ce-product-pulse.md) | `pulse_quality_scoring`, `pulse_quality_dimension`, `pulse_analytics_source`, `pulse_tracing_source`, `pulse_payments_source`, `pulse_db_enabled` | Optional quality scoring and read-only data-source routing. | diff --git a/skills/ce-code-review/SKILL.md b/skills/ce-code-review/SKILL.md index 10244cdb0..c7fcba60a 100644 --- a/skills/ce-code-review/SKILL.md +++ b/skills/ce-code-review/SKILL.md @@ -481,7 +481,9 @@ When a job ID is returned and task tracking is active, add a distinct task that 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 `/.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. + +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. When a `review_model` config pin is active for the high-stakes trio, include one line naming the configured model (and effort, when applied) as *requested* for those reviewers. 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. ### Stage 4: Dispatch and collect reviewers diff --git a/skills/ce-code-review/references/dispatch-reviewers.md b/skills/ce-code-review/references/dispatch-reviewers.md index 1d6172b09..103b23879 100644 --- a/skills/ce-code-review/references/dispatch-reviewers.md +++ b/skills/ce-code-review/references/dispatch-reviewers.md @@ -23,10 +23,12 @@ When Stage 3c selected the lite roster, the fast pass still runs. #### Model tiering -Three reviewers inherit the session model with no override: `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer`. These perform the highest-stakes analysis — logic bugs, security vulnerabilities, adversarial failure scenarios — and should run at whatever capability level the user has configured. If the user is on Opus, these get Opus. +Three reviewers inherit the session model with no override by default: `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer`. These perform the highest-stakes analysis — logic bugs, security vulnerabilities, adversarial failure scenarios — and should run at whatever capability level the user has configured. If the user is on Opus, these get Opus. All other persona subagents and CE local prompt assets use the platform's mid-tier model to reduce cost and latency. See the Spawning subsection below for the exact dispatch-time override. +**Config override.** Read `review_model` and `review_effort` the same two-layer local-first way `docs_root` is read: `/.compound-engineering/config.local.yaml` first, then `config.yaml` (`` = `git rev-parse --show-toplevel`); the first non-empty value per key wins; `#`-commented lines are ignored. A resolved `review_model` must additionally match the model-alias shape `^[A-Za-z0-9._-]{1,64}$` (the same grammar the plan_model elevation carrier uses) before it is treated as a pin; a value that fails this shape check is treated as unset. This is a deterministic pre-bind check that catches malformed values (empty, whitespace, path-like, oversized) — it does not and cannot validate that a well-formed alias actually names a real, capable model; that is caught only by the rejected-selector fallback below. Fall-through is independent per key — an invalid or missing `review_model` falls through silently to session-model inheritance, exactly like a typo'd alias that is never passed through as a pin; an invalid or missing `review_effort` never touches model resolution at all, it only means effort is omitted — a valid `review_model` pin is never discarded because `review_effort` was absent or bad. A resolved `review_model` replaces session-model inheritance for `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` only — an exact pin, applied even when the session model is more capable than the configured one. An explicit in-run request naming a model for the reviewers outranks these keys in either direction (elevate or lower) for that run. `review_effort` takes effect only when an explicit model route resolved for the trio (via `review_model` or an in-run request) and only on a harness whose dispatch primitive exposes a per-dispatch effort override; otherwise it is omitted silently — degrade the outcome, never the boundary. Once a model pin is configured it is never *silently* dropped: on a harness whose dispatch primitive exposes no model selector at all, the pin is dropped with disclosure (see the Spawning subsection's unsupported-host rule) rather than silently, and the effort refinement on top of a resolved pin can still be omitted silently. + The orchestrator (this skill) also inherits the session model; it handles intent discovery, reviewer selection, finding merge/dedup, and synthesis. #### Run ID @@ -39,9 +41,9 @@ Use the run ID and absolute run dir already created at the Stage 3d routing boun Omit the `mode` parameter when dispatching sub-agents so the user's configured permission settings apply. Do not pass `mode: "auto"`. -**Model override at dispatch time — this is a correctness guarantee, not cosmetics.** Omitting the override on a top-tier parent session (e.g. Opus) silently runs that reviewer at the expensive tier — the regression this prevents. The tier is a deterministic function of the persona, so as you select reviewers in Stage 3, **record each reviewer's tier in an internal working list** — that list is your external memory (the role the old printed `[session model]`/`[mid-tier]` labels served) and it must exist and be honored even though it is no longer rendered in the user-facing announce: +**Model override at dispatch time — this is a correctness guarantee, not cosmetics.** Omitting the override on a top-tier parent session (e.g. Opus) silently runs that reviewer at the expensive tier — the regression this prevents. The tier is a deterministic function of the persona and the resolved `review_model` config, so as you select reviewers in Stage 3, **record each reviewer's tier in an internal working list** — that list is your external memory (the role the old printed `[session model]`/`[mid-tier]` labels served) and it must exist and be honored even though it is no longer rendered in the user-facing announce: -- **Session model** (no override; inherits the session model) — `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` only. +- **Session model** (inherits the session model, unless the Model tiering `review_model` config resolved above — then pass that model instead, plus `review_effort` on a harness whose dispatch primitive exposes a per-dispatch effort override, omitting effort silently where it does not) — `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` only. **Unsupported-host degradation:** when the active dispatch primitive exposes no model or custom-agent selector at all (e.g. Codex task wording, which cannot carry a model pin), passing the resolved `review_model` is impossible — mirror the Mid-tier rule below: omit the override, let the reviewer inherit the session model, and disclose the unapplied pin by name in Coverage/the announce. This is the pin dropped *with disclosure*, not the silent drop the Model tiering paragraph rules out. **Rejected-selector fallback:** the shape check above only catches malformed values, not a well-formed but wrong alias (e.g. `opuz`) that a selector-capable host rejects at spawn time. If dispatch fails with an unrecognized-alias/unknown-model error (not a capacity error — see the backpressure rule above), re-dispatch that reviewer once with the override omitted so it inherits the session model, and disclose the unapplied pin by name in Coverage. A rejected pin must never cost the reviewer its dispatch. - **Mid-tier** — every other persona and CE agent: pass the platform's balanced mid-tier model. In Claude Code, that is the Sonnet class. In Codex, apply this tier only when the active dispatch primitive exposes an explicit model or custom-agent selector; task wording alone does not select a different model. Otherwise omit the override and inherit the parent model — a working review on the parent model beats a broken dispatch on an unrecognized name. Apply this on **every** Agent / `spawn_agent` / subagent call. A missed override is a silent cost-and-quality regression, so treat the internal tier list as load-bearing — moving it out of the user-facing output removed the *display*, not the discipline. diff --git a/skills/ce-setup/references/config-template.yaml b/skills/ce-setup/references/config-template.yaml index 23e532ff6..389821bcd 100644 --- a/skills/ce-setup/references/config-template.yaml +++ b/skills/ce-setup/references/config-template.yaml @@ -74,6 +74,29 @@ # plan_model: fable # model alias for the plan-authoring step (no default) # brainstorm_model: fable # model alias for approach generation (no default) +# Pin the model (and, best-effort, the reasoning effort) for ce-code-review's +# three high-stakes reviewers only -- correctness-reviewer, security-reviewer, +# and adversarial-reviewer. Every other persona (mid-tier, ce-doc-review, +# ce-simplify-code) is untouched. This is an exact pin, not a floor -- once +# set, it replaces session-model inheritance for the trio even when the +# session model is more capable, so a high or low choice here is a trade-off +# the user owns deliberately. Unlike plan_model/brainstorm_model above, this +# key is read two layers deep and local-first -- the gitignored +# config.local.yaml wins if set there, otherwise the TRACKED config.yaml +# supplies a repo-wide default that reaches fresh worktrees too. An explicit +# in-run model request (e.g. naming a model in your prompt) overrides either +# layer for that run only. review_effort only takes effect alongside a +# resolved review_model route, and only on a harness whose dispatch primitive +# exposes a per-dispatch effort control -- elsewhere it is silently skipped. +# Invalid, commented, or missing values fall through independently per key -- +# for review_model, straight to today's behavior (trio inherits the session +# model); for review_effort, it is simply omitted and never affects model +# resolution, so a valid review_model pin is never discarded because +# review_effort was absent or bad. + +# review_model: opus # model alias for the trio (no default; exact pin, can lower capability too) +# review_effort: high # low | medium | high | xhigh (no default; best-effort, trio-only) + # --- PR concept teaching (ce-commit-push-pr) --- # When a PR introduces a concept new to the codebase, the generated PR # description gains a "New concepts" section teaching it. The section toggle is diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index 8ff958b2e..1785272f6 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -406,6 +406,25 @@ describe("ce-code-review contract", () => { expect(content).toContain("adversarial-reviewer") }) + test("Model tiering resolves review_model/review_effort config override for the trio", async () => { + const content = await readRepoFile( + "skills/ce-code-review/references/dispatch-reviewers.md", + ) + + expect(content).toContain("review_model") + expect(content).toContain("review_effort") + + // Two-layer local-first read, same shape as the ce-docs-root block. + expect(content).toContain("config.local.yaml") + expect(content).toContain("config.yaml") + + // The override is scoped to the three named reviewers only. + expect(content).toMatch(/correctness-reviewer.*security-reviewer.*adversarial-reviewer/s) + + // Invalid or missing values fall through silently -- never a passed-through typo'd pin. + expect(content).toMatch(/fall(s)? through silently/i) + }) + test("Stage 4 concurrent-batch dispatch preserves cap-safety and determinism", async () => { const content = await readRepoFile( "skills/ce-code-review/references/dispatch-reviewers.md", diff --git a/tests/skills/ce-setup-check-health.test.ts b/tests/skills/ce-setup-check-health.test.ts index c728f3676..f9b86a70e 100644 --- a/tests/skills/ce-setup-check-health.test.ts +++ b/tests/skills/ce-setup-check-health.test.ts @@ -118,6 +118,13 @@ describe("ce-setup check-health", () => { expect(template).not.toContain("fable_nudge") }) + test("advertises the review-model config keys", async () => { + const template = await readFile(configTemplate, "utf8") + + expect(template).toContain("review_model") + expect(template).toContain("review_effort") + }) + test("routes retired and malformed dormant engine settings into preference repair", async () => { const skill = await readFile(path.join(repoRoot, "skills", "ce-setup", "SKILL.md"), "utf8") const step3 = skill.match(/### Step 3:[\s\S]*?(?=### Step 4:)/)?.[0] ?? ""