fix(ce-code-review): take review criteria from a repo-owned standards file - #1572
Conversation
…owned standards file Proposes collapsing 16 reviewer personas into 6 dispatches across 5 report axes, and adding CODING_STANDARDS.md as a review-time criteria file the repo owns. Audit found three review rules each duplicated across three personas, structural review gated off below ~200 changed lines, and no agent for the "did we build what we said?" axis. Marked as a review draft rather than a ce-unified-plan artifact so lfg and ce-work do not try to execute it. Reviewed across three cross-model panel rounds; provenance and the open questions are recorded in the document.
… review criteria source Standards discovery globbed CLAUDE.md and AGENTS.md only, so a repo that wrote its rules in CODING_STANDARDS.md had them silently ignored at review time. An instruction file is steering, charged to every agent's context on every turn, so it stays small; review criteria want a file with no context tax and room to grow. CODING_STANDARDS.md is now the designated criteria source. Instruction files remain a fallback for changed files that no standards file governs, so existing repositories keep their standards review, and exactly one kind of criteria file governs any given file. The reviewer also states that the content is the contract and the format is not: a criteria file may be written by a person or another tool, so rules are extracted from prose, bullets, or tables alike, with no schema, identifier, or section layout required.
… cell mode Four scenarios pin the criteria-source contract: the designated source wins over an instruction file, a subtree standards file governs its subtree while the root instruction file still supplies criteria outside it, the fallback keeps working when no standards file exists, and rules are extracted from a file whose format is flowing prose. Each task ends in a deterministic CRITERIA: line so the grade fails on a wrong mapping rather than on wording. Adds --git-staged, because these cells need a real reviewed diff and a fixture cannot express one on its own. --git-init commits everything it copies, and untracked paths are out of scope for a diff-scoping skill, so --git-untracked produced an empty reviewed set. Named paths are now held out of the seed commit and re-added, landing as a staged change.
PR SummaryMedium Risk Overview Documents the broader 16→6 persona consolidation plan in a new draft plan doc and narrows Adds skill-eval-cell support for Reviewed by Cursor Bugbot for commit a9c4df5. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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: 100e529866
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ock cited findings Two review findings, both at layers the original change did not touch. The shared false-positive catalog told every reviewer to suppress code-quality concerns "not codified in CLAUDE.md / AGENTS.md", so a rule that lives only in CODING_STANDARDS.md matched both the instruction to report it and the shared instruction to suppress it. The catalog now states the condition the filename list stood for: a concern needs a rule from one of the review's designated criteria files behind it. Precedence was resolved in Stage 3b and then discarded. The dispatch payload carried a flat path list, and the persona stated per-file precedence only in its standalone-discovery branch, so a dispatched reviewer could grade a file against both kinds. Stage 3b now passes the pairing, the payload names it, and the persona applies it on both paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a96b07407
ℹ️ 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".
Stage 3b enumerated criteria files with a workspace glob, so a pr-remote or branch-remote review searched whatever happened to be checked out instead of the reviewed head. A standards file present only in the reviewed tree was missed, and one deleted there could suppress the instruction-file fallback. Second finding on this block, so it is restated rather than patched again: the step now states its goal, the reviewed-tree condition, the done bar, and the fail-closed direction, and no longer enumerates the steps that carried them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d30955e2a1
ℹ️ 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".
…d path The done bar said every changed file must end with exactly one kind of criteria behind it, which made an ordinary outcome look like a gap: a changed file that no criteria file governs is complete, not uncertain. It now states the property that actually matters, that no file is graded against two kinds. The remote-scope read rule named changed files only, so a criteria file resolved from the reviewed head was still read from the checkout. It now covers every file that belongs to the reviewed tree.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9b44f4c. Configure here.
…r reads it The previous commit widened the rule in dispatch-reviewers.md, which only the orchestrator reads. A spawned persona receives diff-scope.md, whose ban covered the changed-file list alone, so an unchanged criteria file resolved from the reviewed head was still opened from the checkout. diff-scope.md now owns it, covering every file that belongs to the reviewed tree, and the dispatch-reviewers wording is back to what it was so the rule is stated once at the layer that reaches the reviewer.
1 participant
Summary
Two things are wrong with
ce-code-review, and they turn out to be the same problem.It runs 16 reviewer personas, and they overlap badly. Three separate personas carry near-identical copies of the same three rules; one defines its own territory as "the space between these reviewers." Overlapping lenses cost a subagent each, cost tokens, and produce duplicate findings that then have to be reconciled downstream. Fewer, sharper reviewers would be both cheaper and better — there is no tradeoff to make here.
But you cannot just delete personas, because some are the only thing enforcing anything specific — a Swift reviewer, a frontend-races reviewer, an agent-architecture reviewer. Deleting them takes real coverage away from the teams that need it. Unless those teams can write the rules down themselves.
So this PR adds the mechanism that makes the consolidation safe, and it is useful on its own:
CODING_STANDARDS.md. Write a rule in that file at your repo root, andce-code-reviewenforces it on every review from then on. This is the first time the skill can be customized — until now, what it checked was entirely what we shipped, and the only place a repo could put a rule wasCLAUDE.md/AGENTS.md, which standards review audited alone.That separation matters beyond customization, because the two files have opposite cost profiles. An agent instruction file is loaded into every agent's context on every turn, so it is under permanent pressure to stay short and rules get cut for space. A criteria file is read once, by one reviewer, at review time, so it can hold as many rules as a team accumulates. Conflating them capped how much a review could ever enforce.
Where this goes.
docs/plans/ce-code-review-reviewer-consolidation-and-standards-file.md, added in this range, takes the roster from 16 personas to 6 dispatches across 5 report axes — and hands the stack-specific ones to teams as exampleCODING_STANDARDS.mdfiles rather than shipping one team's opinions to everyone. This PR is step 1: it builds the file mechanism, and changes nothing about the roster. Existing repos are unaffected — instruction files remain the criteria for any changed file noCODING_STANDARDS.mdgoverns, and the run names that fallback in Coverage when it is used.Design decisions
Precedence is per changed file, and per kind. Every
CODING_STANDARDS.mdon a changed file's ancestor path applies together, as standards files always have. What the precedence rule settles is that an instruction file supplies criteria only where no standards file governs — so a given file is graded against one kind or the other, never both. The orchestrator resolves that pairing during discovery and passes it to the reviewer rather than handing over a flat path list.The content is the contract; the format is not. A criteria file may be written by a person or another tool, so rules are extracted from prose, bullets, tables, or nested headings, with no schema, identifier, or frontmatter required. The eval includes a fixture whose rules are a paragraph of flowing prose.
The fallback stays. Dropping the instruction-file read outright would take every existing user's standards review to zero findings the day they upgrade.
AGENTS.md's recorded exception for naming instruction files on a read path narrows to match, in the same commit.Review findings addressed
CODING_STANDARDS.mdwas both reportable and suppressiblepr-remote/branch-remotescope it read a tree that is not the one under reviewscope.mdalready reports the degraded coverage. The alternative — reading criteria from the checkout — is the defect the previous finding fixeddiff-scope.mdFive were missing conditions or mechanisms at the wrong owning layer; one was answered with the condition it was a case against. All of them clustered on one root — resolving criteria under remote review scope — which is the part of this change the eval cannot reach.
Validation
Four eval scenarios added to the skill-eval catalog, run against Claude and Codex. The discriminating one is
standards-scoped-precedence: a rootAGENTS.mdalongside askills/CODING_STANDARDS.md, with changed files both inside and outsideskills/.799702cf0)missing required text: skills/demo.md=skills/CODING_STANDARDS.mdPre-change, both hosts found the standards file and deliberately excluded it; Claude named the rule that then went unenforced. Post-change both route
skills/demo.mdto the standards file andsrc/cart.tsto the fallback. The other three rows (designated source, fallback-only regression guard, prose format) pass on both hosts, and the precedence row still passes after the review fixes.Reproduce with
bun tests/skill-eval-cell/pack.ts --id ce-code-review/standards-scoped-precedence --arm ab.Known coverage gap. All four cells exercise standalone discovery, where the agent resolves criteria itself. None exercises the dispatched path, where the orchestrator resolves the pairing and hands it to a seeded persona — which is exactly why the second review finding escaped them. A cell that seeds the persona directly with a
<standards-paths>block is follow-on work, as is one that runs remote scope. That gap is why five of the six review findings on this PR were about remote-scope criteria resolution: the reviewers were reaching a path the cells structurally cannot.bun run release:validateand the skill-eval-cell suite pass.Test harness
These cells need a real reviewed diff, and a fixture cannot express one:
--git-initcommits everything it copies, and untracked paths are out of scope for a diff-scoping skill, so--git-untrackedyields an empty reviewed set.--git-stagedholds named paths out of the seed commit and re-adds them, landing a staged change. It is additive — no existing cell passes it — and reusable by any future code-review cell.Security Disclosure
Two changes touch file handling, neither involving secrets, credentials, or permissions. The review skill's standards discovery now also matches
CODING_STANDARDS.md, under the same ancestor filter as before, and is now explicitly scoped to the tree under review rather than whatever is checked out. The eval harness gains--git-staged, which runsgit rm --cachedandgit addagainst caller-named paths inside a throwaway workspace it already owns.One residual worth naming: criteria are read from the reviewed head, so a diff that edits the criteria file is reviewed against its own edited rules. That is intentional — the alternative means a PR that adds a rule is never checked against it — but a loosened rule can silently suppress findings. Disclosure of criteria-file changes is step 5 of the linked plan and is not in this PR.
Agent Disclosure
Claude Code · claude-opus-5[1m]