Skip to content

feat(code-review): review non-application files + three TestQuality shapes - #183

Open
mikeangstadt wants to merge 1 commit into
mainfrom
feat/non-application-file-review
Open

feat(code-review): review non-application files + three TestQuality shapes#183
mikeangstadt wants to merge 1 commit into
mainfrom
feat/non-application-file-review

Conversation

@mikeangstadt

Copy link
Copy Markdown
Collaborator

Summary

Extends the shared reviewer prompt to cover the file classes the fleet currently does not review, plus three TestQuality shapes that stay green while proving nothing.

Prompt-only. All behavior lives in tools/prompts/shared_prompt.txt, which every reviewer role loads. No dispatch, roster, category, or schema change; no new agent; no new stage.

Why: the fleet's coverage is categorically absent outside application source

Measured against five days of one consuming repo — 2,617 original review comments (replies excluded) across 326 PRs, 2026-07-30 → 2026-08-04, attributed per reviewer agent:

file class total findings from this fleet fleet share
CI guard/deploy scripts 101 0 0%
workflow definitions (.github/) 69 0 0%
docs / MDX 94 0 0%
migrations / schema 28 0 0%
test files 219 1 0.5%
app code .tsx 684 465 68%
prototypes 193 139 72%
app code .ts 1,099 301 27%

The fleet is strong where it is pointed and produces zero findings on four whole file classes. Those classes are 22% of the corpus and are carried almost entirely by other reviewer agents — so the capability gap is real, not a question of whether those files contain reviewable defects.

Note the limit of this measurement: it shows output, not cause. It does not by itself prove the reviewers would have found these defects if prompted — only that they are not reporting them today. If the zeros turn out to come from file-scope filtering in partitioning rather than from prompt silence, this PR is necessary but not sufficient, and the follow-up is in derive-spawn-spec/partitioning rather than here.

What changed

<non_application_files> — declares those files in scope and enumerates the generic defect classes per class, all repo-agnostic:

  • CI/pipeline: privileged jobs running repository lifecycle scripts before minting a write token; workflow-scope write permissions and persisted checkout credentials; the implicit "all dependencies succeeded" condition that is false for a skipped dependency; missing job timeouts; cancel-on-new-run over irreversible side effects; trigger filters that do not match what they gate; a shell step whose unguarded command pre-empts the branch owning its exit code.
  • Guard/meta-test scripts: guards that assert a predicate appears rather than executing the decision; guards anchored to the first matching block instead of the production node; allowlist entries broad enough to suppress future real violations.
  • Migrations: destructive statements selecting survivors by a proxy for canonicity; idempotency guards that no-op over an invalid object left by a failed run; an enum value added without updating every classifier that switches on it.
  • Docs/API specs: claims stronger than the code enforces (unconditional where the code is conditional, product-wide where the behavior is one lane); a behavior change whose describing doc is not updated; generated-client specs admitting combinations the runtime always rejects.

Pinned-file pair detection — repos routinely hold meta-tests asserting another file's literal content (an action-version pin, a config snapshot, a required-context manifest, an allowlist). Reviewers now grep for a test referencing the changed path or a distinctive changed literal and report when that test exists but is not in the diff. Emitted as Correctness, not CompanionChange, deliberately: it is a provable CI failure with a concrete assertion to cite, and it carries no rule-driven reasoning_certificate — which templates/companion_change.md interpolates and would render empty for an LLM-emitted finding.

<test_quality> — three shapes, all generic:

  1. Wiring unproven — helper tested directly, production caller never asserted; deleting the call site leaves the suite green.
  2. Assertion of existence — source grepped/AST-inspected for a predicate instead of executing that decision against synthetic inputs, so it cannot distinguish a correct implementation from a widened one.
  3. Input-invariant fixture — a mock or route handler returning identical results regardless of the params the test claims to cover.

New TestQuality subcategories route to the existing templates/testquality_specialized.md via the TestQuality/* fallback row, so /code-review fix needs no new template.

Test plan

  • python3 -m pytest test_code_review_helpers.py test_code_review_schema.py1,286 passed, 0 failed.
  • Checked that no test pins shared_prompt.txt content before editing it (the same rule this PR teaches): every test writes a synthetic prompt file to tmp_path; the real file is only ever copied verbatim into the CR dir by code_review_helpers.py:13719. No pinned-file pair broken.
  • Verified only .claude-plugin/plugin.json is bumped per release (.codex-plugin/plugin.json tracks separately and was last touched by an unrelated commit), matching the v3.7.0 release commit's file set. Bumped 3.7.0 → 3.8.0 with a CHANGELOG entry.
  • git diff --check clean; plugin.json parses.

Cost. shared_prompt.txt grows 317 → 409 lines (~17.5 KB → ~24 KB), about +1.6k input tokens per reviewer agent. It is identical across agents and stable across runs so it caches, but on a large fleet the uncached first-touch cost is real — /code-review cost will show the fleet-side delta.

What could still break. Prompt additions can shift reviewer attention: a fleet told to review workflow YAML may spend budget there on PRs where the application diff mattered more, and the new heuristics could produce false positives on repos whose CI conventions differ (a deliberate cancel-in-progress, an intentionally broad allowlist). The existing FLAG criteria and the verifier pass both still apply, and every new heuristic requires a concrete file:line, so these should surface as verifier-refuted rather than shipped — but the first few runs on a CI-heavy PR are worth watching.

…hapes

Extends the shared reviewer prompt to cover CI/pipeline definitions, guard and
meta-test scripts, migrations, and docs/API specs, plus the wiring-unproven,
assertion-of-existence, and input-invariant-fixture TestQuality shapes.

Prompt-only: no dispatch, roster, or schema change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@wongk wongk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this adds explicit things to review to the shared reviewer prompt, rather than assigning these things to specific reviewers. this means every review agent is going to duplicate these reviews, and it will dilute the agent-specific review areas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants