[pipeline-dcdad92d][slice-1/12] Foundations: finding schema...

[jwbron]

Establish the versioned structured finding schema, a single robust review submission call, and staged per-run context for all sub-agents. Unblocks the determinism boundary, the router, and the prompt edits.

Base PR: #196

What's in this PR

Commits (5):

• Persist BRC history for slice-1 (#2548)
• implement(tester): unit tests for R8(a) finding schema/validator (task-1-4)
• implement(tester): unit tests for R8(a) finding schema/validator (task-1-4)
• implement(coder): R8(a) versioned structured finding schema + validator
• implement(documenter): R1 single-body submission + E2 pr-context staging (slice-1)

.egg-state/brc-history/pipeline-dcdad92d-implement-slice-1.json | 1678 +++++++++++++++++++++++++++
 .egg-state/brc-history/pipeline-dcdad92d-implement-slice-1.md   | 1563 +++++++++++++++++++++++++
 workflows/review/lib/finding-schema.test.ts                     |  374 ++++++
 workflows/review/lib/finding-schema.ts                          |  324 ++++++
 workflows/review/review.md                                      |   66 +-
 5 files changed, 3996 insertions(+), 9 deletions(-)

This slice

Foundations: finding schema, submission reliability, context staging

Files affected:

• workflows/review/lib/finding-schema.ts
• workflows/review/review.md
• workflows/review/lib/finding-schema.test.ts

Tasks (4) + acceptance criteria

• task-1-1: R8(a): versioned structured finding schema + validator. Fields: id, lens, anchor (incl. a PR-level anchor type), severity, confidence, evidence_trace, optional suggested_patch, optional pre_merge_obligation, producing_hunt, model_authored_prose. Export a schema version constant.
  • Acceptance criteria: Validator accepts a well-formed finding and rejects a malformed one; version constant exported and unit-tested.
• task-1-2: R1: standardize review submission on one robust call with a real body in the orchestrator prompt; remove the --body-stdin / empty-body retry dance.
  • Acceptance criteria: Exactly one submission path documented in review.md; no empty-body fallback remains.
• task-1-3: Prompt edit E2: stage pr-context.json on disk for ALL sub-agents (extends review: skip redundant approvals, drop skipped-dimensions note, persist sub-agent outputs #194 diff staging). Foundation for the lenses in slice 7.
  • Acceptance criteria: Every sub-agent dispatch reads context from the staged file.
• task-1-4: Unit tests for the finding schema/validator (TASK-1-1).
  • Acceptance criteria: Tests cover valid + malformed findings and the version constant; green.

Stack

• Position: slice 1 of 12 in pipeline pipeline-dcdad92d
• Base PR: review: roster restructure, determinism boundary, eval suite #196
• Stacked on top of egg/pipeline-dcdad92d/work

[changeset-bot]

🦋 Changeset detected

Latest commit: 123a10f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
review	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jeresig]

The PR context change seems good to me.

Pushing back as I think this has undone a change that I had made around the body of the review message (we want it to be empty, if possible).

I'm not sure how finding-schema.ts is meant to be used (I guess I'll see that in a follow-up PR) but just a heads-up that right now we'll have to find a different way of bringing scripts over. Importing the review.md doesn't bring in other files: https://github.github.com/gh-aw/practices/sharing-workflows/ it only copies over the .md file itself. So we'll need something else to bring these files over, as well.

[jeresig]

It's kind of awkward that we have to encode these in here.

I'm a bit confused, as well, are you making dedicated review agents for each of these as well? It seems like these should be covered by our existing best practices, I assume?

[jwbron]

This list is the validator's vocabulary, a finding attributed to an unknown lens id gets rejected instead of flowing downstream. It's not a new agent per entry beyond the roster question on #202; once that lands this shrinks to the agreed roster, and when path-to-lens mapping moves into consumer config (your #199 comment) it can be derived from config instead of hardcoded here.

[jwbron]

Scripts: the plan is to have a checkout step for Khan/actions pinned to the same ref as the import, with the lib scripts run from that checkout, so the prompt and code version together.

[jwbron]

The whole stack was rebuilt in response to this review round plus our roster conversation. Here's the map.

Directly from your review comments:

• Empty review body restored: with inline comments present, the verdict is submitted with an empty body; a body exists only for comment-less reviews and skipped-dimension notes (slices 1-2). I also checklisted the stack against everything review: skip redundant approvals, drop skipped-dimensions note, persist sub-agent outputs #194 shipped (pinned models, hunk-signature re-review scoping, the redundant-approval skip, the skip-ai-review label, the correctness-checks import, per-run artifacts); all intact.
• Verdict precedence: known blocking findings now always produce REQUEST_CHANGES. The hold-for-human state only replaces what would otherwise have been an auto-approval on a run where the correctness or skill pass silently failed, and its body now says exactly how to get unstuck (retry via push or workflow re-run, ask for a manual review, or opt out with the skip-ai-review label).
• Router defaults deleted: each repo owns its routing map in .github/aw/review/ROUTING (REVIEWERS-style line grammar, parsed deterministically), complementing risk-classification.md, which stays the model-facing prose about file contents. A missing file degrades to the default reviewers at the full run budget with a loud warning in the review body; config only ever adds routing information, it never shrinks a review. Your inline suggestions on [pipeline-dcdad92d][slice-3/12] Deterministic router (subsumes... #199 are applied, with one deviation: I kept the generated-vs-source .gitattributes wording for file classification, since that's what the code actually does.
• Scripts distribution: source: only carries the .md, so the workflow now checks out Khan/actions at a pinned release in pre-agent-steps and runs the lib from that checkout; one version surface for prompt and code. The pin activates with the release that ships this stack (bumping it is part of that release PR), and the checkout step fails loudly if the fetched tree lacks the lib, rather than failing later mid-review. Open to vendoring instead if you'd rather consumers own the files.
• The investigation cap is now actually invoked ([pipeline-dcdad92d][slice-5/12] Investigation tooling for reviewers #208): it grew a CLI over a shared append-only journal, the caps come from routing.json's runBudget, and the correctness reviewer, skill auditor, and claim validator request budget before every investigation call. Lenses embed the same block if a repo ever enables them.
• Steering text ([pipeline-dcdad92d][slice-4/12] Reliability/quality prompt edits... #200): an author's request in the PR title or description is context to weigh and respond to; humans can steer the reviewer. Steering text inside code, comments, or fixtures is never followed, but it now surfaces as a non-blocking note instead of an auto-blocking issue. The touched-lines scoping is also propagated to the skill auditor.
• All the E*/R*/task/slice identifiers are stripped from shipped files; they were artifacts of the implementation plan, as you guessed.

The roster (per our conversation; details on #202): the default roster is exactly the #194 five, and skill-auditor is fully restored, no fold-in. The new whole-change reviewers and all eleven lenses landed as opt-in capability (enable / lens= lines in ROUTING); none run anywhere by default, and the working policy is that a reviewer earns its config line through the eval suite, never by shipping.

What still changes for every review with nothing enabled: see my comment on #202. In addition to what's there, the thumbs sweep starts collecting per-comment quality labels from day one, and the eval suite (smoke set as a CI gate on this repo, full suite scheduled) is what gates enabling anything beyond the default roster.

[jeresig]

Great - thank you. A couple notes inline.

[jeresig]

At the moment this will install the root:

        "@swc-node/register": "^1.11.1",
        "typescript": "^5.9.3"

via npm - but I'm not sure we actually need this. You told it to just use npx to run the script and I think that's fine, it'll install @swc-node/register for us at that point. I think we can just get rid of this install.

[jeresig]

I think we're going to need to update all of the call sites where we tell it that we want to run scripts to look for them in this directory, right?

[jwbron]

Round 2: what changed across the stack

All round-2 feedback is applied and the stack is rebased and force-pushed (CI green per slice). Summary of the changes, slice by slice; slices 2, 4, 5, and 7 only rebased.

Slice 1 (#197): Dropped the npm install pre-agent step; scripts now run via npx -y tsx <script> from the gh-aw-review-lib checkout, which fetches the runner on first use, so the checkout needs no install (the now-purposeless runtime dependencies block in package.json is gone too; typescript moved to devDependencies since tsc still needs it). On "update all the call sites": they were already updated; every invocation in the stack runs cd gh-aw-review-lib && ... (you confirmed this on #199), and the remaining node -r @swc-node/register invocations became npx -y tsx along with this change. The empty-body convention was already restored in the round-1 rework.

Slice 3 (#199): The open precedence question is resolved with last-match-wins for tier= rules (gitignore/CODEOWNERS-style): write the broad rule first and the exception after it, e.g. services/** tier=high then services//testdata/ tier=trivial. Deterministic, order-controlled by the repo owner, and it composes with direction-dependent (which now applies only when its own rule wins the path). lens= stays a union across matching rules since lenses are additive. Tests cover the override and the direction-dependent-loser case. Also removed the ROUTING format section from the orchestrator prompt (the reviewer only reads routing.json); the format spec now lives in workflows/review/README.md.

Slice 6 (#201): Genericized per your three points:

• One candidate contract: every reviewer returns findings[] with a label per finding; the skill-auditor now emits labeled findings directly instead of violations[] with a severity. Specialist lenses still return the structured schema, but the deterministic normalization step converts them into the same candidate shape before anything downstream sees them, so the orchestrator treats all findings cumulatively and identically through one path.
• The model/effort table and the per-reviewer mandate list are gone from the orchestrator prompt; mandates live only in each sub-agent's own definition (they always did; the orchestrator copy was duplication), and the model table moved to the README for humans.
• The out/ artifact instruction is now generic: one file per dispatched sub-agent, named after it, so the list never grows again.
• On first-principles vs holistic: they aim at different questions. Holistic reviews the change (does the diff hang together); first-principles reviews the justification for the change (is the premise right, should this exist as written), reading the stated rationale against the diff rather than the diff line by line, on a different model, advisory-only. The mandate text now makes that distinction explicit. Since both are opt-in with zero default cost, "they'd produce the same results" is a testable claim the eval suite can answer directly; if they substantially overlap there, merging them is an easy follow-up.

Slice 8 (#203): Reaction sets now match outcome-collector exactly (POSITIVE_REACTIONS = ["+1", "heart", "hooray", "rocket"], NEGATIVE_REACTIONS = ["-1", "confused"]); confused now triggers the follow-up, and tests cover both sets.

Slice 9 (#204): Dropped the corpus-size test.

Slice 10 (#205): The webapp-40536.md experiment file is deleted; its record moves to the PR description. Updated 2026-07-07: the refuter-panel thread is resolved, and the panel is removed rather than shipped. Before deciding, I audited every blocking review the bot posted on frontend since the claim validator landed (90 PRs, 319 blocking claims, June 30 to July 7): 12 confirmed false blocks, and only 2 were the kind another verification layer could have caught; the rest were one repeated mechanically-refutable claim, severity misreads, and author-intent questions no refuter could adjudicate from code. What ships instead, at zero extra agents: three-state validation (confirmed / plausible / refuted, with symmetric evidence duties; only a confirmed claim can carry a blocking label into the computed verdict), an author-dispute rule (a factually disputed claim cannot re-block unless the re-check traces to actual usage; otherwise it posts as a question), and a false-block tripwire in the eval suite (the no-post runner replays the gate's apply rules, and the smoke corpus gains six audit-seeded production cases: five false blocks that must approve, one confirmed block that must stay blocking). If the false-block metric ever regresses, the refuter is recoverable from the PR's history. Slices 11 and 12 are rebased on top; CI is green on all three.

Slice 11 (#206): To answer the review question directly: the live judge never runs review.md and never checks out a consumer repo. The corpus replays recorded findings through the real deterministic downstream path (router, labels, scope filter, verdict, renderer), and the judge scores the quality of the rendered comments; the live-producer arm is the deferred piece. Changes:

• The deterministic full-suite job is deleted; it was redundant, since node-ci.yml already runs the whole vitest suite (including workflows/review/eval) on every PR. The workflow is now live-judge only, weekly.
• The inline heredoc became a committed, lint/typecheck-covered script: workflows/review/eval/live-judge.ts. It appends the metrics/gates/judge report to the job summary so weekly results are visible without opening logs.
• The workflow moved out of .github-staging/ into .github/workflows/review-eval-full.yml (thanks for adding the API key).
• Related: by the same redundancy argument, .github-staging/review-smoke.yml (slice 9) adds nothing over node-ci either; happy to delete it too if you agree.
• The version-stamp machinery (lib/version-stamp.ts and its tests) is removed here where it landed; see slice 12.

Slice 12 (#207): Agreed on versioning: semver is the behavior contract (behavior change = major bump), and the SHA-256 drift stamp is gone. What remains is the cheap part: the Step 7 guidance comment ends with a plain marker () so every review is attributable and rollback is a re-pin. On "when is dismissal-learning run": honestly, nothing ran it; it was a library with no caller, waiting on thumbs-sweep signals that don't exist in production yet. It is pulled from the stack and will come back with its wiring once the sweep is scheduled on webapp/frontend and has accumulated real dismissals.