review: dedupe lens discipline snippets into one staged disciplines file#247
review: dedupe lens discipline snippets into one staged disciplines file#247jwbron wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 6c496fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
…into one staged disciplines file
jwbron
force-pushed
the
jwbron/review-dispatch-tax
branch
from
8e015ff to
813767c
Compare
khan-actions-bot
requested review from
a team,
kevinb-khan and
somewhatabstract
and removed request for
a team
Review live A/BBaseline:
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
… into jwbron/review-dispatch-tax
… into jwbron/review-dispatch-tax
Review Guidancegithub-actions (1 file)
Common patterns1 file: The verbatim discipline blocks (bounded investigation, untrusted input, read-every-line, out-of-lane handoff, schema-rules trailer) were removed from each of the 11 specialist-lens definitions and replaced with a uniform "Shared disciplines first" pointer to the staged - **Bounded investigation.** ...
- **Untrusted input.** ...
+ **Shared disciplines first.** Read `/tmp/gh-aw/review/disciplines.md` ... Domain notes for §Bounded investigation: ...Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| lines). Stage them for the lens sub-agents with one mechanical extraction — the | ||
| engine writes this rendered prompt to the path in `$GH_AW_PROMPT`: | ||
| ``` | ||
| sed -n '/^<!-- BEGIN REVIEW DISCIPLINES -->$/,/^<!-- END REVIEW DISCIPLINES -->$/p' \ |
There was a problem hiding this comment.
suggestion (non-blocking): disciplines.test.ts's sedRangeExtract is a hand-written replica of the sed range, and the staging test never asserts the real sed command — so the load-bearing whole-line ^/$ anchoring is modeled, not verified. If a future edit drops those anchors, /BEGIN REVIEW DISCIPLINES/ matches the backtick-wrapped mention on line 311 and opens the range ~1075 lines before the real marker (line 1386), yet every assertion still passes. Consider pinning the command and exercising real sed:
it("real sed extracts the same section as the replica", () => {
const viaSed = execFileSync("sed", ["-n",
"/^<!-- BEGIN REVIEW DISCIPLINES -->$/,/^<!-- END REVIEW DISCIPLINES -->$/p",
join(__dirname, "..", "review.md")], {encoding: "utf8"}).replace(/\n$/, "");
expect(viaSed).toBe(sedRangeExtract(reviewMd));
});| themselves match, never this instruction or the sed command's own text.) | ||
| Then verify the staged file carries the schema section: | ||
| `grep -q '## Structured finding schema and hunts' /tmp/gh-aw/review/disciplines.md`. | ||
| If that verification fails (e.g. `$GH_AW_PROMPT` is unset in a future engine), fall |
There was a problem hiding this comment.
suggestion (non-blocking): The heredoc fallback (lines 322-326) reproduces the ~92-line disciplines section byte-for-byte, but the verify on line 321 greps for only one heading (## Structured finding schema and hunts). A paraphrased or truncated LLM transcription of the other six sections would still pass that single-heading check, so lenses could run on degraded discipline text with no signal. Consider a line-count or content-hash check so a degraded fallback fails loudly.
|
|
||
| The section between the markers below is the single copy of the discipline text | ||
| every **specialist lens** follows. It used to be stamped verbatim into all eleven | ||
| lens definitions and paid on every dispatch; now the lenses read it once from |
There was a problem hiding this comment.
question (non-blocking): Does staging net a per-dispatch token reduction, or relocate the cost? Each of the 11 lens pointers now Reads the ~5.2KB disciplines file as a tool result (still input tokens) plus a pointer block and an extra round-trip, where ten lenses previously carried a shorter condensed variant inline. Worth one measured run to confirm the dispatch-tax saving before the rest of the stack builds on it — or reframing the changeset around drift-elimination, which is the unambiguous win here.
| lines). Stage them for the lens sub-agents with one mechanical extraction — the | ||
| engine writes this rendered prompt to the path in `$GH_AW_PROMPT`: | ||
| ``` | ||
| sed -n '/^<!-- BEGIN REVIEW DISCIPLINES -->$/,/^<!-- END REVIEW DISCIPLINES -->$/p' \ |
There was a problem hiding this comment.
note (non-blocking): The test exercises sedRangeExtract against the source review.md, not the rendered $GH_AW_PROMPT that the runtime sed actually reads. If the compiler ever indents or transforms the marker lines, the anchored sed stages an empty file and the fallback fires on every run — behavior stays neutral, but the cost win is silently lost and no test covers that path. Mitigated by the verify+fallback guard and prior precedent for extracting $GH_AW_PROMPT, so non-blocking.
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it. ## Format A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON. - `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these. Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset. ## Cases Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them. The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit). ## Test plan: - `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
# Conflicts: # workflows/review/eval/corpus/live.ts
1 participant
Summary
Fourth of five PRs in the fold-in batch two stack (stacked on #245; the stack is based on #238,
jwbron/review-trial-skill). The top of the batch-two stack will bejwbron/review-skill-rule-quote.The v1.4.0 re-run's cost premium vs the v1.3.1 baseline sits at dispatch time (uncached input +51-65%, cache writes +18-34% per run), partly because the discipline snippets (bounded investigation, untrusted input, lens-owned skills incl. quote-the-rule, the schema-rules trailer, the tri-state hunt contract) are stamped verbatim into every one of the eleven specialist-lens definitions and paid on every dispatch.
The mechanics, grounded in the gh-aw v0.81.6 source:
## agent:sections are extracted from the markdown at compile time into per-agent definitions (pkg/parser/sub_agent_extractor.go), and each dispatch pays its definition's full text; the orchestrator's rendered prompt (the main body only) is written to disk at the engine-provided$GH_AW_PROMPTpath. So the shared text now lives once, in a marker-delimited "REVIEW DISCIPLINES" section at the end of the main body, and:sed -n '/^<!-- BEGIN ... -->$/,/^<!-- END ... -->$/p' "$GH_AW_PROMPT" > /tmp/gh-aw/review/disciplines.md. The patterns are whole-line-anchored on purpose, so the sed command's own text and prose mentions of the markers can never open the range. A grep verifies the staged file carries the schema section; the fallback is a byte-for-byte heredoc copy, never a paraphrase. Prompt and disciplines version atomically (both live in review.md), with no coupling to the lib-checkout ref.Behavior-neutral by design: the instruction content is unchanged. Where the eleven stamped copies had already drifted (security-auth carried a fuller bounded-investigation and untrusted-input wording than the compact copies in the other ten), the disciplines section unifies on the fullest wording, so no lens loses instruction it previously had. The label-shape reviewers (correctness, skill-auditor, claim-validator, and the whole-change reviewers) are deliberately out of scope: their variants differ materially (per-agent investigation-cap CLI id semantics,
discussionvsevidence_trace), so deduping them is not the mechanical win it is for the lenses.Failure scenario this guards against, and how it is pinned: if the extraction silently produced an empty file, every lens would run without its disciplines (no quote-the-rule, no schema rules).
lib/disciplines.test.tsreplicates the exact sed range semantics against review.md and asserts the extraction is non-empty and carries all seven section headings, that every lens points at the staged file and carries no residual copy, and that the label-shape reviewers still carry theirs; Step 1 additionally verifies the staged file at run time before any lens is dispatched.Net effect on review.md: 246 insertions, 474 deletions (about 16.6KB removed); each lens's dispatched definition shrinks by roughly 1.3KB. Whether that fully closes the measured dispatch premium is for the next lifecycle re-run to say; the duplication and the drift are gone either way.
cc @jeresig
Testing
npx vitest run workflows/review: 508 tests green (24 new inlib/disciplines.test.ts).npx tsc --noEmitclean.