diff --git a/AGENTS.md b/AGENTS.md index 52414c88c..691fb8497 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -155,7 +155,7 @@ When a skill needs to discover a project convention at runtime — the issue tra **Name a concrete file only where the skill must do something a context reference can't express:** - **Writing a convention back** (e.g., persisting `project_tracker: linear`) needs a target — name it minimally and as an example ("the project's root agent-instructions file, e.g., `AGENTS.md`; if it `@`-includes another, write to the substantive one"). -- **Reading content that is genuinely not auto-loaded** — a subdirectory-scoped instruction file governing the area being changed, an optional project doc like `STRATEGY.md` / `CONCEPTS.md` / `README.md`, or any file a *fresh subagent* (which does not inherit the parent's loaded instructions) must open to do its job. Auditing tools that must enumerate every standards file (e.g., `ce-code-review`'s project-standards reviewer globbing all `CLAUDE.md`/`AGENTS.md`) are a legitimate exception — they review the files, they don't re-read them for context. +- **Reading content that is genuinely not auto-loaded** — a subdirectory-scoped instruction file governing the area being changed, an optional project doc like `STRATEGY.md` / `CONCEPTS.md` / `README.md`, or any file a *fresh subagent* (which does not inherit the parent's loaded instructions) must open to do its job. Auditing tools that must enumerate every criteria file are a legitimate exception — they review the files, they don't re-read them for context. `ce-code-review`'s project-standards reviewer globs `CODING_STANDARDS.md`, the designated criteria source, and reads `CLAUDE.md`/`AGENTS.md` only as criteria for changed files that no `CODING_STANDARDS.md` governs. **Describe the capability, not the tool.** Pair this with naming the *category* of thing rather than a closed set: "the project's issue tracker (e.g., GitHub Issues, Linear, Jira)" and "whatever interface that tracker exposes (connector/MCP, documented API, or a documented CLI)" — never assume a specific CLI exists, and never treat a missing binary / env var / MCP server as proof the capability is unavailable. diff --git a/docs/plans/ce-code-review-reviewer-consolidation-and-standards-file.md b/docs/plans/ce-code-review-reviewer-consolidation-and-standards-file.md new file mode 100644 index 000000000..668dd3a20 --- /dev/null +++ b/docs/plans/ce-code-review-reviewer-consolidation-and-standards-file.md @@ -0,0 +1,195 @@ +# Plan: `ce-code-review` reviewer consolidation + a repo-owned rules file + +> Status: draft for review. Not a `ce-unified-plan` artifact (deliberately — `lfg` and `ce-work` must not try to execute this). +> Origin: `ce-code-review` reached ~7,392 lines and 16 reviewer personas. An audit found three review rules each written into three separate personas, a structural axis gated off for most diffs, and a "did we build what we said?" axis with no agent at all. Reviewed across three cross-model panel rounds plus maintainer direction; this is rev 4. + +## 1. The defect + +Three rules are each written into three separate personas in near-identical prose: + +| Rule | Copies | +|---|---| +| Stand-in guard fidelity | `correctness:10`, `reliability:12`, `adversarial:64` | +| Sentinel reuse | `correctness:9`, `api-contract:11`, `testing:9` | +| React effect lifecycle | `correctness:13`, `julik-frontend-races:8`, `testing:7` | + +The corroborating tell: `adversarial-reviewer.md:80-91` is eight bullets naming other personas to disclaim territory, then defines its own scope as "the space between these reviewers." + +Two structural gaps: `maintainability` holds 10 of Fowler's 12 smells but is gated off below ~200 executable changed lines, so most diffs get no structural review; and requirements completeness is an orchestrator checklist (`finish-review.md:123`) while `previous-comments` gets a full dispatch for a near-identical question. + +`subagent-template.md:169` also gives *every* persona a trailing "compare the code changes against the stated intent" bullet, which competes with any dedicated spec axis. + +**Scope note:** `findings-mechanics.py:67` fingerprints only `(file, line, title)` — the deterministic helper does exact dedup; semantic reconciliation is orchestrator judgment (`finish-review.md:19`). Overlap costs orchestrator attention, not helper complexity. + +## 2. The discriminator (a routing prior, not a proof) + +> A concern is a candidate for its own **dispatch** when answering it requires a different **reasoning mode**. It is a candidate for an **output obligation** when it is a different **question** in the same mode. + +A required output field guarantees a question is *asked* — the field is absent if skipped, and that is detectable. Only a separate agent with its own context guarantees it is *worked*. + +This is a prior. It mislabels at least one real case (the Rails dump-diff procedure is sustained evidence acquisition, not a lookup), so **ablation decides** (§7). + +Two rules hold absolutely: + +- **Content alone never earns a dispatch.** A rubric with no distinct mode is a rule pack, not a reviewer. +- **Only a looser bar needs isolation.** `security` runs deliberately low (P0 at anchor 50, `security-reviewer.md:15`) and would contaminate other lenses. `performance` runs deliberately high (`performance-reviewer.md:15`); suppression is local to a finding and travels inside a bigger rubric. + +## 3. Six dispatches, five report axes + +**An axis is a section of the report. A dispatch is a subagent. They are not the same thing.** + +| Axis | Dispatch(es) | Mode that earns it | +|---|---|---| +| Correctness | `correctness` | trace execution, find the defect that is there; carries the §4 obligations | +| Standards | `standards-cited` + `standards-structural` | rule retrieval vs uncodified structural judgment — different modes | +| Spec | `spec` | compare an artifact to a document; find what is *absent* from the diff | +| Testing | `testing` | judge existing artifacts for false confidence — not defect-hunting | +| Security | `security` | adversarial attack construction, at a deliberately looser bar | + +Plus **adversarial as a mechanism, not a persona** — the cross-model peer. Its value is model independence, not a distinct rubric; the in-process fallback is deleted. + +Splitting standards into two dispatches under one report section preserves the distinct reasoning without meaningfully increasing synthesis complexity, and it is what un-gates structural review on small diffs. + +### 3.1 Disposition of all 16 + +| Current | Disposition | +|---|---| +| correctness | keep; carries the §4 obligations | +| project-standards | becomes `standards-cited` | +| maintainability | becomes `standards-structural`; **un-gated**; add Feature Envy + Repeated Switches | +| testing | keep; add tautological-expected-value and interface-bypass rules | +| security | keep | +| performance | fold -> `correctness` | +| reliability | fold -> `correctness` (already near-duplicate) | +| api-contract | fold -> `correctness` as `contract_check` | +| data-migration | portable half -> `migration_check`; Rails procedure -> gated procedure / standards example | +| deployment-verification | Stage 6 checklist template, fired by `migration_check: applicable` | +| previous-comments | fold -> `spec`; thread fetch moves to §5 | +| adversarial | cross-model mechanism only; in-process persona deleted | +| julik-frontend-races | delete -> standards example | +| swift-ios | delete -> standards example (honest residue: a narrow Core Data / concurrency pack, not zero) | +| agent-native | delete -> standards example | +| learnings-researcher | recategorize -> §5 evidence, not a findings producer | +| *(none today)* | **new: `spec`** | + +## 4. Output obligations + +`correctness` returns required fields alongside its findings, each `applicable | not_applicable` **with a justification either way**: + +``` +migration_check -> deploy-window compatibility, backfill for new NOT NULL, rollback path, + dual-write, orphaned references, silent data loss +contract_check -> breaking shape change, versioning/deprecation path, sentinel overload, + evidenced consumer impact +``` + +**Why the migration persona goes.** `data-migration-reviewer.md` contains **zero** references to any non-Rails migration tool: Step 0 diffs `db/schema.rb` / `db/structure.sql`, and both `suggested_fix` blocks emit `bin/rails db:migrate` (`:13-44`). Alembic/Flyway/Liquibase appear only in the spawn gate (`select-and-route.md:61`), never in the persona. The gate admits a Django, Prisma, or Ecto repo and hands it a reviewer whose procedure does not apply — the same defect as `julik-frontend-races` and `swift-ios` under a more general-sounding name. + +The split is by portability: portable deploy-window principles become the obligation, checked on **every** review rather than only when a Rails-shaped gate opens (strictly more coverage than today); the Rails dump-diff procedure becomes a gated procedure with an honest name. + +Obligations prove a question was asked, not that it was worked — see §7 ablation 1, which measures that directly. + +## 5. Orchestrator-owned evidence acquisition + +Today each persona gathers its own evidence, so gathering is duplicated and folding a persona silently drops its gathering step. Separate **acquisition** from **reasoning**: + +The orchestrator acquires and labels, once: plan/requirements, prior review threads, designated criteria files, migration/schema artifacts, and changed public-contract surfaces. Each axis receives labelled evidence and returns an evidence-linked disposition; missing or unsupported dispositions are validated centrally. + +**The manifest carries source pointers, never conclusions.** A label like "public-contract" or "migration impact" names *where to look*, not *what was found*. Encoding conclusions in the manifest turns this stage into a god object and moves judgment out of the reviewers. Reviewers keep semantic interpretation and targeted follow-up reads (`intent-and-plan.md:75-79`). + +This is what makes the folds safe: `previous-comments` folds into `spec` because the thread fetch moves here, so the fold is no longer a source mismatch. + +## 6. The rules file + +### 6.1 Discovery and precedence + +Stage 3b globs only `**/CLAUDE.md` and `**/AGENTS.md` today (`select-and-route.md:67-70`). Change the **designation**, not just the glob: + +1. `**/CODING_STANDARDS.md` under the existing ancestor rule — the designated criteria source. +2. Instruction files are a **fallback**, read only where no standards file governs. + +**Exactly one effective criteria source per changed file**, resolved by precedence — not both consulted together. + +Rationale: an instruction file is *steering*, charged to every agent's context every turn, so it is under permanent pressure to stay small. That is why the current `project-standards` hunting list is all plugin-specific trivia. Criteria want a file with no context tax and room to grow. The fallback stays because every existing user's rules live in their instruction file today; removing the read outright would take their standards review to zero findings. + +This narrows the exception recorded at `AGENTS.md:158`, which changes in the same PR. + +**Reviewer personas are dispatched as fresh generic subagents** (`dispatch-reviewers.md`) and do not inherit the orchestrator's auto-loaded instruction files, so naming files on this path remains correct — the reviewer audits them, it does not re-read them for context. + +### 6.2 Format agnosticism + +**The file may be authored by a human or another tool. The content is the contract; the format is not.** + +- **Reading:** extract rules from whatever shape the file is in — prose, bullets, tables, nested headings, with or without frontmatter. Impose no schema, require no IDs, require no frontmatter. Citation already works on free text (`project-standards-reviewer.md` requires "the exact quote or section reference"), and that generalizes. +- **Writing and grooming:** detect the file's existing conventions — heading style, grouping, whether rules carry rationale — and match them. **Never reformat, normalize, or migrate the file to a CE shape.** +- **Creating one that does not exist:** use a minimal conventional structure a human would plausibly have written. No CE branding, no machine-readable envelope. + +A consequence for §6.3: criteria-change disclosure reports changed rules as **quoted hunks**, not as rule IDs, because there are no IDs to report. + +### 6.3 Criteria are read from head, and changes are disclosed + +Criteria resolve from the **reviewed head**, not from the merge-base. A rules file is deliberate, human-authored policy: reviewing against the old rule would override the team's stated intent and file findings they have explicitly decided not to care about. It would also mean a PR that adds a rule never gets checked against it — backwards for the loop this design exists to build, where the natural flow is *notice -> write the rule -> fix the thing*, often in one PR. + +The real risk is not which version governs; it is that suppression is **invisible**. So: + +**When the reviewed diff modifies a criteria file, the report says so** — naming the rules added, changed, or removed as quoted hunks. **For rules loosened or removed in the diff, report what that suppressed:** "2 structural findings suppressed by a rule added in this PR (`CODING_STANDARDS.md`, quoted)." Not a violation — visibility. + +That covers both the adversarial case and the honest mistake with one Coverage line, and keeps the reviewer out of the business of adjudicating policy. + +**The governance control lives at rule adoption, not rule reading** (§6.4). Base-reading would only delay a self-suppressing rule by one PR; a human-gated adoption stops it. + +### 6.4 Who reads, writes, and grooms + +| Skill | Role | +|---|---| +| `ce-code-review` | **reads and enforces**; emits *candidate rules* — a finding not anchored in any cited rule — and hands off. It never writes the file (`SKILL.md`: "Report-only by default… Never push, open PRs, or file tickets in any mode"). | +| `ce-compound` | **adopts** a rule, in an explicitly invoked mode, only when asked. A candidate stays a candidate until deliberately adopted. | +| `ce-compound-refresh` | **grooms** the file, under the condition in §6.5 | + +### 6.5 The `ce-compound-refresh` exception, stated as a condition + +`ce-compound-refresh:46,54` currently forbids editing "a skill, runbook, or instruction file." That rule's real subject is **authorship**: those are files refresh did not author and whose review path it is not part of. + +A CE-owned enforceable-rules artifact is in the same authorship domain as `/solutions/`, so the prohibition should not reach it. But editing it changes review behavior deterministically, so the exception is bounded by **semantic effect**, not by filename: + +> Refresh may perform non-semantic maintenance and reporting on a CE-owned enforceable-rules artifact — consolidating duplicate rules, flagging a rule contradicted by current code, stale-marking and handing off. It may not **adopt** a new rule or **retire or weaken** an existing one; those are policy acts and belong to an explicitly invoked adoption. + +Stated this way it generalizes to any future CE-owned enforceable artifact without anyone editing the rule again. It also matches refresh's existing posture: "Replace only on real evidence; without it, stale-mark the doc and point the user at `ce-compound`." + +Grooming is mandatory, not optional. A file read on every review that only ever grows dilutes reviewer attention and consumes context budget — the same failure the ~200-line `maintainability` gate was papering over. + +**Open:** candidate rules accumulate across many reviews, so adoption may be a batch moment ("go through my candidate rules") rather than `ce-compound`'s "I just solved a problem" trigger. Different trigger is one of the repo's split criteria (`portable-agent-skill-authoring.md:150`). Start it as a mode; split only if the batch flow proves to be its own workflow. + +### 6.6 The shared template must change + +`subagent-template.md:136` tells every persona to suppress "General code-quality concerns not codified in CLAUDE.md / AGENTS.md." An always-on Fowler floor is definitionally that, so the floor is currently illegal in our own template. Scope that suppression to reviewers carrying a citation contract. Remove the trailing intent bullet at `:169`, which competes with `spec` and the §4 obligations. + +## 7. Validation + +Mechanical contracts via `bun run test`. Behavior via `bun run test:skill-eval-pack -- --skill ce-code-review --arm ab` on Claude and Codex, **with real dispatch** — this skill's key behavior is delegation, so a no-dispatch probe cannot validate it. + +**Ablation is the proof standard.** Reversed real defects plus clean controls, measuring marginal detection, false-positive noise, latency, and cost — never whether 6 dispatches reproduce 16 personas' raw finding count. + +1. **Obligation vs persona.** Reversed migration and contract defects, obligations vs today's gated personas. On diffs where the concern *is* applicable, measure the `not_applicable` rate, **applicable-with-empty-findings**, and **justification accuracy**, rejecting tautological justifications mechanically. Field presence proves a schema was satisfied, not that work happened — this is the plan's single biggest risk. +2. **Correctness absorption.** Reversed reliability, performance, and frontend-race defects, merged vs separate. +3. **Standards split.** Two dispatches vs one, on a diff carrying both a rule violation and an uncodified structural regression — the starvation case. +4. **Format agnosticism.** Three differently-shaped `CODING_STANDARDS.md` files (prose, bullets, table) carrying the same rules; citation quality must not vary by shape. + +A fold that misses a decision-relevant defect the 16-persona roster caught is falsified and reverts on its own. + +## 8. Sequencing + +1. `CODING_STANDARDS.md` discovery + precedence + format-agnostic reading (§6.1, §6.2); update `AGENTS.md:158`. +2. Amend `subagent-template.md:136`; remove `:169` (§6.6). +3. `standards-cited` + `standards-structural`; un-gate the floor; add the 2 missing smells and the 2 testing rules. Behind ablation 3. +4. Delete julik / swift / agent-native; ship as standards examples. +5. Criteria-change disclosure (§6.3). +6. `spec` axis; §5 evidence layer; fold `previous-comments`. +7. `correctness` absorption and the §4 obligations **last**, behind ablations 1 and 2 — the only step that can lose a finding. +8. Separate PRs: `ce-compound` adoption mode; `ce-compound-refresh` grooming exception (§6.5), routed through `ce-skill-work`. + +## 9. Provenance + +Three cross-model panel rounds via `ce-pov`. Peers: codex (`gpt-5.6-sol`) and grok (`grok-4.6`), both attestably different serving families from the host; served model IDs unverified. Both returned `revise-first` on rev 1 and rev 2 and **moved** to agreeing the shipped `data-migration` persona should not ship as a generalist. Codex `held` on rev 3 and contributed §5's manifest boundary and the §6.4 authority split. Grok was dropped in round 3 (non-final artifact twice on the same route) and its round-2 position is not carried as a round-3 voice. + +Maintainer decisions folded in: cut `data-migration` as a persona; demote instruction files; head-reading over merge-base; adoption as an invoked mode; format agnosticism; grooming stays with `ce-compound-refresh` rather than a new skill. diff --git a/skills/ce-code-review/references/diff-scope.md b/skills/ce-code-review/references/diff-scope.md index 11c33cf55..32e34d221 100644 --- a/skills/ce-code-review/references/diff-scope.md +++ b/skills/ce-code-review/references/diff-scope.md @@ -14,7 +14,7 @@ The scope step in the SKILL.md handles discovery and passes you the resolved dif ## Remote scope (`pr-remote` and `branch-remote`) -When the review context includes `pr-remote` or `branch-remote`, the working tree is **not** the reviewed head. Do **not** use Read/Grep on workspace paths for files in the changed-file list — they may not match the branch or PR under review. +When the review context includes `pr-remote` or `branch-remote`, the working tree is **not** the reviewed head. Do **not** use Read/Grep on workspace paths for any file that belongs to the reviewed tree — the changed-file list, and any other file the review points you at, such as a criteria file. They may not match the branch or PR under review, and an unchanged file is not exempt: the checkout can hold a version the reviewed head never had. Instead: diff --git a/skills/ce-code-review/references/dispatch-reviewers.md b/skills/ce-code-review/references/dispatch-reviewers.md index 0048f5fe4..6cf8502f8 100644 --- a/skills/ce-code-review/references/dispatch-reviewers.md +++ b/skills/ce-code-review/references/dispatch-reviewers.md @@ -68,7 +68,7 @@ For each selected reviewer, and only for those, read the corresponding local pro 4. PR metadata: title, body, and URL when reviewing a PR (empty string otherwise). Passed in a `` block so reviewers can verify code against stated intent 5. Review context: intent summary, file list, diff, scope mode (`local-aligned` | `pr-remote` | `branch-remote`), and remote head ref (`PR_HEAD_REF` or ``) when set 6. Run ID and reviewer name for the artifact file path -7. **For selected `project-standards` only:** the non-empty standards file path list from Stage 3b, wrapped in a `` block appended to the review context +7. **For selected `project-standards` only:** the non-empty Stage 3b criteria mapping — each criteria file with the changed files it governs — wrapped in a `` block appended to the review context 8. **For `data-migration` only:** the resolved review base ref from Stage 1 (`BASE:` marker), wrapped in `` inside the review context so schema drift checks never assume `main` Persona sub-agents are **read-only** with respect to the project: they review and return structured JSON. They do not edit project files or propose refactors. The one permitted write is saving their full analysis to the resolved run-artifact path specified in the output contract. diff --git a/skills/ce-code-review/references/persona-catalog.md b/skills/ce-code-review/references/persona-catalog.md index ec2ed0cca..1e0240fe9 100644 --- a/skills/ce-code-review/references/persona-catalog.md +++ b/skills/ce-code-review/references/persona-catalog.md @@ -11,7 +11,7 @@ Correctness is spawned on every multi-agent review. Project-standards is spawned | Persona | Prompt asset | Focus | |---------|-------|-------| | `correctness` | `correctness-reviewer` | Logic errors, edge cases, state bugs, error propagation, intent compliance | -| `project-standards` | `project-standards-reviewer` | CLAUDE.md and AGENTS.md compliance -- frontmatter, references, naming, cross-platform portability, tool selection | +| `project-standards` | `project-standards-reviewer` | Compliance with the project's designated criteria files, cited rule by rule | ## Generic conditional diff --git a/skills/ce-code-review/references/personas/project-standards-reviewer.md b/skills/ce-code-review/references/personas/project-standards-reviewer.md index bbb2ee260..65f6bf812 100644 --- a/skills/ce-code-review/references/personas/project-standards-reviewer.md +++ b/skills/ce-code-review/references/personas/project-standards-reviewer.md @@ -1,21 +1,23 @@ # Project Standards Reviewer -You audit code changes against the project's own standards files -- CLAUDE.md, AGENTS.md, and any directory-scoped equivalents. Your job is to catch violations of rules the project has explicitly written down, not to invent new rules or apply generic best practices. Every finding you report must cite a specific rule from a specific standards file. +You audit code changes against the criteria files the project has designated, at the paths you are given. Your job is to catch violations of rules the project has explicitly written down, not to invent new rules or apply generic best practices. Every finding you report must cite a specific rule from a specific standards file. ## Standards discovery -The orchestrator passes a `` block listing the file paths of all relevant CLAUDE.md and AGENTS.md files. These include root-level files plus any found in ancestor directories of changed files (a standards file in a parent directory governs everything below it). Read those files to obtain the review criteria. +**Judge each changed file only against the criteria paired with it.** No changed file is ever graded against two kinds of criteria file, so a rule from a criteria file that does not govern a path is not a finding against that path. -If no `` block is present (standalone usage), discover the paths yourself: +The orchestrator passes a `` block pairing each criteria file with the changed files it governs. Read those files and apply that pairing as given. -1. Use the native file-search/glob tool to find all `CLAUDE.md` and `AGENTS.md` files in the repository. -2. For each changed file, check its ancestor directories up to the repo root for standards files. A file like root `AGENTS.md` applies to the whole checkout, while `skills/AGENTS.md` applies to all changes under `skills/`. -3. Read each relevant standards file found. +If no `` block is present (standalone usage), build the same pairing yourself. Find every `CODING_STANDARDS.md`, `CLAUDE.md`, and `AGENTS.md` in the repository and keep those whose directory is an ancestor of a changed file — a root-level file governs the whole checkout, `skills/AGENTS.md` only what is under `skills/`. `CODING_STANDARDS.md` is the designated criteria source, so an instruction file supplies criteria only for changed files that no `CODING_STANDARDS.md` governs. -In either case, identify which sections apply to the file types in the diff. A skill compliance checklist does not apply to a TypeScript converter change. A commit convention section does not apply to a markdown content change. Match rules to the files they govern. +**The content is the contract, not the format.** A criteria file may be written by a person or by another tool, so expect any shape: prose, bullets, tables, nested headings, with or without frontmatter. Extract the rules whatever the shape. Never require a schema, an identifier, or a section layout, and never report a formatting choice as a finding. + +Within each criteria file you read, identify which sections apply to the file types in the diff. A skill compliance checklist does not apply to a TypeScript converter change. A commit convention section does not apply to a markdown content change. Match sections to the file types they address. ## What you're hunting for +The shapes below are examples of how a written rule gets violated, drawn from an agent-skills repository. They are not the criteria. The criteria are whatever the discovered files state, so a repository whose rules cover none of these shapes is reviewed against its own rules and not against this list. + - **YAML frontmatter violations** -- missing required fields (`name`, `description`), description values that don't follow the stated format ("what it does and when to use it"), names that don't match directory names. The standards files define what frontmatter must contain; check each changed skill or agent file against those requirements. - **Reference file inclusion mistakes** -- markdown links to skill-local reference files used where the standards require backtick paths or `@` inline inclusion. Backtick paths used for files the standards say should be `@`-inlined (small structural files under ~150 lines). `@` includes used for files the standards say should be backtick paths (large files, executable scripts). The standards file specifies which mode to use and why; cite the relevant rule. @@ -50,7 +52,7 @@ Use the anchored confidence rubric in the subagent template. Persona-specific gu - **Violations that automated checks already catch.** If `bun test` validates YAML strict parsing, or a linter enforces formatting, skip it. Focus on semantic compliance that tools miss. - **Pre-existing violations in unchanged code.** If an existing SKILL.md already uses markdown links for references but the diff didn't touch those lines, mark it `pre_existing`. Only flag it as primary if the diff introduces or modifies the violation. - **Generic best practices not in any standards file.** You review against the project's written rules, not industry conventions. If the standards files don't mention it, you don't flag it. -- **Opinions on the quality of the standards themselves.** The standards files are your criteria, not your review target. Do not suggest improvements to CLAUDE.md or AGENTS.md content. +- **Opinions on the quality of the criteria themselves.** The criteria files are what you review against, not what you review. Do not suggest improvements to their content. ## Evidence requirements diff --git a/skills/ce-code-review/references/select-and-route.md b/skills/ce-code-review/references/select-and-route.md index 30910516d..563f43020 100644 --- a/skills/ce-code-review/references/select-and-route.md +++ b/skills/ce-code-review/references/select-and-route.md @@ -64,16 +64,19 @@ For `deployment-verification-agent`, use the same migration-artifact gate when t ### Stage 3b: Discover project standards paths -Before spawning sub-agents, find the file paths (not contents) of all relevant standards files for the `project-standards` persona. Use the native file-search/glob tool to locate: +**Goal:** the mapping that pairs each criteria file governing this change with the changed files it governs, for the `project-standards` persona. Paths, not contents. -1. Use the native file-search tool (e.g., Glob in Claude Code) to find all `**/CLAUDE.md` and `**/AGENTS.md` in the repo. -2. Filter to those whose directory is an ancestor of at least one changed file. A standards file governs all files below it (e.g., `AGENTS.md` at the repo root applies to the whole checkout, while `skills/AGENTS.md` would apply to everything under `skills/`). +Enumerate the candidates from **the tree under review**, never from whichever tree happens to be checked out: the workspace only in `local-aligned` scope, and the reviewed head ref in `pr-remote` and `branch-remote` (Stage 1 resolved which). A criteria file that exists only in the reviewed tree must appear, and one deleted there must not, or the persona enforces criteria the change never had. -Distinguish an empty successful search from a failed or unavailable search: +Candidates are `CODING_STANDARDS.md`, `CLAUDE.md`, and `AGENTS.md` at any depth. Keep those whose directory is an ancestor of a changed file — a root-level file governs the whole checkout, `skills/AGENTS.md` only what is under `skills/`. -- One or more applicable paths: select `project-standards` and pass the path list inside a `` block in its Stage 4 context. The persona reads the files itself, targeting only relevant sections. +`CODING_STANDARDS.md` is the designated criteria source, so an instruction file supplies criteria only for changed files that no `CODING_STANDARDS.md` governs, and no file is graded against both kinds. Every governing `CODING_STANDARDS.md` still applies together. When the instruction-file fallback supplied the criteria for any changed file, name it as the fallback in Coverage. + +**Done** when no changed file could be graded against two kinds of criteria. A changed file that no criteria file governs is a complete result, not a gap. **On uncertainty, fail closed** — an error is never an empty result: + +- One or more applicable paths: select `project-standards` and pass the mapping inside a `` block in its Stage 4 context. The persona applies the precedence you resolved rather than re-deriving it, and reads the files itself, targeting only relevant sections. - Empty successful search: do not dispatch `project-standards`; record `project standards: not run (no applicable standards files)` in Coverage. -- Search failure or uncertain scope: fail closed by dispatching `project-standards` with the uncertainty stated; never treat an error as an empty result. +- Search failure or uncertain scope: dispatch `project-standards` with the uncertainty stated. ### Stage 3c: Small-diff fast path (reduce the roster for trivial, low-risk diffs) diff --git a/skills/ce-code-review/references/subagent-template.md b/skills/ce-code-review/references/subagent-template.md index dea80f0cd..fdbbcf7f3 100644 --- a/skills/ce-code-review/references/subagent-template.md +++ b/skills/ce-code-review/references/subagent-template.md @@ -133,7 +133,7 @@ False-positive categories to actively suppress. Do NOT emit a finding when any o - **Suggestions that restate what the code already does in different words.** "Consider extracting this into a helper" when the code is already a small helper, "consider adding a guard" when a guard one line up already enforces it. - **Generic "consider adding" advice without a concrete failure mode.** If you cannot name what breaks, the finding is not actionable. Either find the failure mode or suppress. - **Issues with a relevant lint-ignore comment.** Code that carries an explicit lint disable comment for the rule you are about to flag (`eslint-disable-next-line no-unused-vars`, `# rubocop:disable Style/StringLiterals`, `# noqa: E501`, etc.) — suppress unless the suppression itself violates a project-standards rule that explicitly forbids disabling that lint for this code shape. The author already chose to suppress; re-flagging it via a different reviewer creates noise and ignores their decision. -- **General code-quality concerns not codified in CLAUDE.md / AGENTS.md.** "This file is getting long," "this method has too many parameters," "this is hard to read" — without a project-standards rule to anchor the concern, these are subjective and waste reviewer time. If the project explicitly bans long files or sets a parameter-count limit in its standards, that is a project-standards finding; otherwise suppress. +- **General code-quality concerns with no rule behind them.** "This file is getting long," "this method has too many parameters," "this is hard to read" — without a rule from one of the criteria files this review designated to anchor the concern, these are subjective and waste reviewer time. When a criteria file does state the limit, that is a project-standards finding; otherwise suppress. - **Speculative future-work concerns with no current signal.** "This might break under load," "what if the requirements change," "this could be hard to test later" — not findings unless the diff introduces concrete evidence the concern is reachable now. **Advisory observations — route to advisory autofix_class, do not force a decision.** If the honest answer to "what actually breaks if we do not fix this?" is "nothing breaks, but…", the finding is advisory. Set `autofix_class: advisory` and `confidence: 50` so synthesis routes the finding to a soft bucket rather than surfacing it as a primary action item. Do not suppress — the observation may have value; it just does not warrant user judgment. Typical advisory shapes: design asymmetry the PR improves but does not fully resolve, opportunity to consolidate two similar helpers when neither is broken, residual risk worth noting in the report. diff --git a/tests/skill-eval-cell/catalog.ts b/tests/skill-eval-cell/catalog.ts index 585d784c9..c9964b982 100644 --- a/tests/skill-eval-cell/catalog.ts +++ b/tests/skill-eval-cell/catalog.ts @@ -19,6 +19,8 @@ export const PRE_SWEEP_REF = "309611f6b5198528c1c98f83fb6b3c90637e523c" export const ISSUE_1482_BASE_REF = "66ccf579f8c1ef2ccfc642c317ba53151eeb1ebb" /** main before the right-size-ceremony change (#1513 release commit): the A/B base for its rows. */ export const RIGHT_SIZE_BASE_REF = "925b4ef71cbee0b4205693c4cafc9b2c557a603a" +/** main before CODING_STANDARDS.md became the designated criteria source: the A/B base for the standards-discovery rows. */ +export const STANDARDS_SOURCE_BASE_REF = "799702cf0f5405c9361548cd86490c5603e2632c" /** main after #1514 merged: the product-lens activation leg still read "alternatives plausibly exist". */ export const DOC_REVIEW_BASE_REF = "6f6c5779d31c0f847773e0cbc1e7e7fc7b11f272" /** The working tree, not HEAD — the post arm exists to grade the edit you have not committed yet. */ @@ -70,6 +72,12 @@ export type Scenario = { git_init?: boolean /** Paths left untracked after the seed commit (secrets / the change under test). */ git_untracked?: string[] + /** + * Paths staged but not committed, so they are the reviewed set. Untracked paths are + * out of scope for a diff-scoping skill, so a cell that needs a real reviewed diff + * uses this rather than git_untracked. + */ + git_staged?: string[] shim_git_push?: boolean shim_gh_pr?: boolean /** Configure a fake `origin` whose `main` is the seed commit, so the shipping tail takes the push/PR path instead of the local-commit path. Pair with shim_git_push. */ @@ -671,6 +679,108 @@ The fetched feedback is already on disk at feedback.md. Treat it as authoritativ must_include: ["does not retry"], }, }, + { + id: "ce-code-review/standards-designated-source", + baseline_ref: STANDARDS_SOURCE_BASE_REF, + skill: "ce-code-review", + cohort: "resized", + key_behavior: "judgment", + read_only: true, + git_init: true, + git_staged: ["src/cart.ts"], + fixture: `${FIX}/standards-designated`, + why: "CODING_STANDARDS.md is the designated criteria source. Before the change, Stage 3b globbed CLAUDE.md/AGENTS.md only, so a repo-owned standards file was invisible and the instruction file supplied the criteria instead.", + pre_contract: + "Stage 3b finds all CLAUDE.md and AGENTS.md whose directory is an ancestor of a changed file; CODING_STANDARDS.md is not discovered.", + task: `Use the ce-code-review skill on this repo. Stop before dispatching any reviewers. + +Work out which files you will check the changed code against. Then end your answer with one line per changed file, in exactly this form and nothing else on the line: + +CRITERIA: = + +Do not run the review itself.`, + grade: { + must_include: ["src/cart.ts=CODING_STANDARDS.md"], + actions: "none", + }, + }, + { + id: "ce-code-review/standards-scoped-precedence", + baseline_ref: STANDARDS_SOURCE_BASE_REF, + skill: "ce-code-review", + cohort: "resized", + key_behavior: "judgment", + read_only: true, + git_init: true, + git_staged: ["src/cart.ts", "skills/demo.md"], + fixture: `${FIX}/standards-mixed-scope`, + why: "The discriminating leg: precedence is per changed file, not per repo. A subtree standards file governs its subtree while the root instruction file still supplies criteria outside it, and no file is graded against both kinds.", + pre_contract: + "Only CLAUDE.md/AGENTS.md are criteria, so the root AGENTS.md supplies criteria for every changed file and skills/CODING_STANDARDS.md is reviewed as content rather than applied as rules.", + task: `Use the ce-code-review skill on this repo. Stop before dispatching any reviewers. + +Work out which files you will check the changed code against. Then end your answer with one line per changed file, in exactly this form and nothing else on the line: + +CRITERIA: = + +Do not run the review itself.`, + grade: { + must_include: ["skills/demo.md=skills/CODING_STANDARDS.md", "src/cart.ts=AGENTS.md"], + actions: "none", + }, + }, + { + id: "ce-code-review/standards-instruction-fallback", + baseline_ref: STANDARDS_SOURCE_BASE_REF, + skill: "ce-code-review", + cohort: "resized", + key_behavior: "judgment", + read_only: true, + git_init: true, + git_staged: ["src/cart.ts"], + fixture: `${FIX}/standards-fallback-only`, + why: "Regression guard for the leg both contracts must still get right: with no CODING_STANDARDS.md anywhere, the instruction file still supplies the criteria rather than the review silently losing its standards gate.", + pre_contract: + "An applicable AGENTS.md supplies the review criteria and project-standards is dispatched.", + task: `Use the ce-code-review skill on this repo. Stop before dispatching any reviewers. + +Work out which files you will check the changed code against. Then end your answer with one line per changed file, in exactly this form and nothing else on the line: + +CRITERIA: = + +Do not run the review itself.`, + grade: { + must_include: ["src/cart.ts=AGENTS.md"], + actions: "none", + }, + }, + { + id: "ce-code-review/standards-format-agnostic", + baseline_ref: STANDARDS_SOURCE_BASE_REF, + skill: "ce-code-review", + cohort: "resized", + key_behavior: "judgment", + read_only: true, + git_init: true, + git_staged: ["src/cart.ts"], + fixture: `${FIX}/standards-prose-format`, + why: "A criteria file may be written by a person or another tool, so rules are extracted from whatever shape the file has. This fixture states its rules as flowing prose with no bullets, headings, or identifiers.", + pre_contract: + "CODING_STANDARDS.md is not discovered at all, so its rules cannot be extracted in any format.", + task: `Use the ce-code-review skill on this repo. Stop before dispatching any reviewers. + +Work out which files you will check the changed code against. Then end your answer with one line per changed file, in exactly this form and nothing else on the line: + +CRITERIA: = + +Do not run the review itself. + +Also quote the specific rules you found in those files.`, + grade: { + must_include: ["src/cart.ts=CODING_STANDARDS.md", "explicit return type"], + actions: "none", + }, + }, { id: "ce-code-review/report-only-default", skill: "ce-code-review", diff --git a/tests/skill-eval-cell/fixtures/standards-designated/AGENTS.md b/tests/skill-eval-cell/fixtures/standards-designated/AGENTS.md new file mode 100644 index 000000000..7eed49453 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-designated/AGENTS.md @@ -0,0 +1,5 @@ +# Agent Instructions + +Build with `npm run build`. Prefer small commits. + +- All files must begin with a copyright header. diff --git a/tests/skill-eval-cell/fixtures/standards-designated/CODING_STANDARDS.md b/tests/skill-eval-cell/fixtures/standards-designated/CODING_STANDARDS.md new file mode 100644 index 000000000..01941fae6 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-designated/CODING_STANDARDS.md @@ -0,0 +1,4 @@ +# Coding Standards + +- Every exported function must have an explicit return type. +- Never use `console.log` in `src/`; use the `log()` helper. diff --git a/tests/skill-eval-cell/fixtures/standards-designated/src/cart.ts b/tests/skill-eval-cell/fixtures/standards-designated/src/cart.ts new file mode 100644 index 000000000..57800931a --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-designated/src/cart.ts @@ -0,0 +1,4 @@ +export function total(items) { + console.log("computing"); + return items.reduce((a, b) => a + b.price, 0); +} diff --git a/tests/skill-eval-cell/fixtures/standards-fallback-only/AGENTS.md b/tests/skill-eval-cell/fixtures/standards-fallback-only/AGENTS.md new file mode 100644 index 000000000..f66b8d339 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-fallback-only/AGENTS.md @@ -0,0 +1,3 @@ +# Agent Instructions + +- Every file under `src/` must begin with a copyright header. diff --git a/tests/skill-eval-cell/fixtures/standards-fallback-only/src/cart.ts b/tests/skill-eval-cell/fixtures/standards-fallback-only/src/cart.ts new file mode 100644 index 000000000..6347dd2e1 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-fallback-only/src/cart.ts @@ -0,0 +1,3 @@ +export function total(items) { + return items.reduce((a, b) => a + b.price, 0); +} diff --git a/tests/skill-eval-cell/fixtures/standards-mixed-scope/AGENTS.md b/tests/skill-eval-cell/fixtures/standards-mixed-scope/AGENTS.md new file mode 100644 index 000000000..f66b8d339 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-mixed-scope/AGENTS.md @@ -0,0 +1,3 @@ +# Agent Instructions + +- Every file under `src/` must begin with a copyright header. diff --git a/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/CODING_STANDARDS.md b/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/CODING_STANDARDS.md new file mode 100644 index 000000000..9520bd012 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/CODING_STANDARDS.md @@ -0,0 +1,3 @@ +# Skill Coding Standards + +- A SKILL.md description must state what the skill does before when to use it. diff --git a/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/demo.md b/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/demo.md new file mode 100644 index 000000000..8ad44fa0c --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-mixed-scope/skills/demo.md @@ -0,0 +1,5 @@ +--- +name: demo +description: Use when you want a demo. +--- +# Demo diff --git a/tests/skill-eval-cell/fixtures/standards-mixed-scope/src/cart.ts b/tests/skill-eval-cell/fixtures/standards-mixed-scope/src/cart.ts new file mode 100644 index 000000000..6347dd2e1 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-mixed-scope/src/cart.ts @@ -0,0 +1,3 @@ +export function total(items) { + return items.reduce((a, b) => a + b.price, 0); +} diff --git a/tests/skill-eval-cell/fixtures/standards-prose-format/CODING_STANDARDS.md b/tests/skill-eval-cell/fixtures/standards-prose-format/CODING_STANDARDS.md new file mode 100644 index 000000000..105caf2d9 --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-prose-format/CODING_STANDARDS.md @@ -0,0 +1,5 @@ +How we write code here. We care about two things above all. Exported functions +always declare an explicit return type, because our consumers read the +signatures more often than the bodies. And nothing in src ever calls +console.log directly; route it through the log helper so the transport stays +swappable. diff --git a/tests/skill-eval-cell/fixtures/standards-prose-format/src/cart.ts b/tests/skill-eval-cell/fixtures/standards-prose-format/src/cart.ts new file mode 100644 index 000000000..57800931a --- /dev/null +++ b/tests/skill-eval-cell/fixtures/standards-prose-format/src/cart.ts @@ -0,0 +1,4 @@ +export function total(items) { + console.log("computing"); + return items.reduce((a, b) => a + b.price, 0); +} diff --git a/tests/skill-eval-cell/pack.ts b/tests/skill-eval-cell/pack.ts index 922dd0e19..436b48303 100644 --- a/tests/skill-eval-cell/pack.ts +++ b/tests/skill-eval-cell/pack.ts @@ -69,6 +69,7 @@ function runCell(scenario: Scenario, arm: EvalArm, out: string, hosts?: string) if (scenario.read_only) argv.push("--read-only") if (scenario.git_init) argv.push("--git-init") if (scenario.git_untracked?.length) argv.push("--git-untracked", scenario.git_untracked.join(",")) + if (scenario.git_staged?.length) argv.push("--git-staged", scenario.git_staged.join(",")) if (scenario.git_remote) argv.push("--git-remote") if (scenario.shim_git_push) argv.push("--shim-git-push") if (scenario.shim_gh_pr) argv.push("--shim-gh-pr") diff --git a/tests/skill-eval-cell/run.ts b/tests/skill-eval-cell/run.ts index 25859467e..f13d649cb 100644 --- a/tests/skill-eval-cell/run.ts +++ b/tests/skill-eval-cell/run.ts @@ -157,7 +157,7 @@ async function main() { const taskFile = arg("--task-file") if (!skill) { console.error( - "usage: bun run test:skill-eval-cell -- --skill --task \"...\" [--task-file p] [--ref WORKTREE|] [--hosts claude,codex,grok] [--fixture dir] [--out dir] [--timeout-secs 600] [--read-only] [--git-init] [--shim-git-push] [--shim-gh-pr]\n default --hosts is the other two harnesses from this session; missing CLIs warn and continue", + "usage: bun run test:skill-eval-cell -- --skill --task \"...\" [--task-file p] [--ref WORKTREE|] [--hosts claude,codex,grok] [--fixture dir] [--out dir] [--timeout-secs 600] [--read-only] [--git-init] [--git-untracked p,p] [--git-staged p,p] [--shim-git-push] [--shim-gh-pr]\n default --hosts is the other two harnesses from this session; missing CLIs warn and continue", ) process.exit(2) } @@ -197,11 +197,22 @@ async function main() { .split(",") .map((p) => p.trim()) .filter(Boolean) + // Staged paths are held out of the seed commit, then re-added, so they land as a + // staged-but-uncommitted change. A cell whose skill scopes a diff needs this: + // untracked paths are out of scope unless staged, so --git-untracked alone + // produces an empty reviewed set. + const staged = (arg("--git-staged") ?? "") + .split(",") + .map((p) => p.trim()) + .filter(Boolean) spawnSync("git", ["add", "."], { cwd: workspace }) - for (const rel of untracked) { + for (const rel of [...untracked, ...staged]) { spawnSync("git", ["rm", "-f", "--cached", "--ignore-unmatch", "--", rel], { cwd: workspace }) } spawnSync("git", ["commit", "-m", "seed", "--allow-empty"], { cwd: workspace }) + for (const rel of staged) { + spawnSync("git", ["add", "--", rel], { cwd: workspace }) + } } const seedRev = spawnSync("git", ["rev-parse", "HEAD"], { cwd: workspace, encoding: "utf8" }) const seedSha = seedRev.status === 0 ? seedRev.stdout.trim() : ""