review: add the per-lens consumer payload seam - #281
Conversation
…seam A consuming repo may now define .github/aw/review/lenses/<lens>.md for any of the eleven specialist lenses, plus lenses/correctness.md for the always-on correctness-reviewer. Each file is runtime-imported (optional form) into a new 'Repo-specific rules and hunts' section of the matching reviewer prompt, carrying that repo's surface-specific rules and extra tri-state hunts. Lens names stay generic and shared; only payloads vary per repo. Behavior-neutral for every current consumer: no repo carries a payload file yet, and gh-aw's runtime resolves a missing optional import to nothing. correctness-checks.md stays imported as a deprecated alias for lenses/correctness.md (frontend carries one today); a repo should migrate the file and carry at most one of the two. Also in this change: - Eval alignment: resolveRuntimeImports now matches production for the optional form (missing resolves to empty, not the '(not configured for this eval case)' note), so corpus case trees can carry payloads. The required-form fallback note is unchanged and remains the one documented deviation (production fails the run). - disciplines.test.ts pins the payload import in every specialist lens section so the seam cannot be silently dropped. - README documents the lenses/ surface and the three-way contribution rule (shared skeleton vs lens payload vs skills), and fixes two stale claims in the consumer-config section: correctness-checks.md was never documented, and the optional import form was said to have been dropped. First planned payload use: migrating the #271 unbounded-reads hunt from the shared skeleton to server repos' correctness/caching payloads, and frontend's client-surface lenses/security-auth.md.
🦋 Changeset detectedLatest commit: 87e38dd 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 |
This comment has been minimized.
This comment has been minimized.
…, inert-payload warnings, precedence) Addresses the four non-blocking reviewer findings on #281: - Pin the correctness payload imports: disciplines.test.ts now asserts the correctness-reviewer section carries both the lenses/correctness.md import and its deprecated correctness-checks.md alias, so neither can be dropped silently (the specialist-lens assertion covered only the eleven lenses). - Warn on silently-inert payloads: new lib/lens-payloads.ts computes fixed-format warnings for a lenses/ entry matching no imported payload (typo or unknown name), a specialist payload no ROUTING rule routes, and the correctness alias carried alongside its replacement. runCli readdirs the payload dir and appends these to routingConfig.warnings, the existing channel the orchestrator surfaces in the review body's note lines. Split into its own module to respect the max-lines cap on router.ts; the specialist roster is a parameter to avoid an import cycle. - State payload precedence in the prompts: the eleven lens payload sections and the correctness framing now say payload rules are additive and never relax or override the shared rules, which win on conflict. README states the same, plus the alias's removal point (next major release). - FsLike gains readdirSync; the router test fake answers it (and directory existsSync) from its input map.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
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. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…ound-two feedback) Addresses the second-round reviewer findings on #281: - ENOTDIR crash: existsSync is true for a regular file at .github/aw/review/lenses, and readdirSync would then throw before routing.json is written, failing every review run in that repo. The CLI now degrades that misconfiguration to a routing warning; the test fake's readdirSync mirrors node:fs and throws on a file path to prove it. - Alias-alone deprecation nudge: an unmigrated repo (frontend today) would go silently inert when the correctness-checks.md import is dropped at the next major. lensPayloadWarnings now emits a rename nudge whenever the alias is carried without lenses/correctness.md, so the cliff is signposted on every review until the file moves. - runCli wiring for the alias check is pinned by a fakeFs case carrying both correctness files; the both-files warning cannot silently drop out. - README: the alias row no longer implies the alias is ignored when both files exist (both import; the router warns), and the warning list names the nudge and the dir-as-file degrade. - Import order nit in router.test.ts fixed. The additive/never-override contract stays prose-level by design: payloads are consumer-owned trusted config in the same class as risk-classification.md (restored from the base branch, authored under normal code review), and a phrase blocklist or size cap would misfire on legitimate rule text. Answered on the thread.
Review Guidancegithub-actions (4 files)
Common patterns1 file: An identical optional "Repo-specific rules and hunts" import block was added before Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
| // alias import is dropped at the next major, an unmigrated repo | ||
| // would go inert with no signal, so nudge on every review until | ||
| // the file moves. | ||
| warnings.push( |
There was a problem hiding this comment.
question (non-blocking): The alias-alone deprecation nudge here fires on every review whenever correctness-checks.md exists without lenses/correctness.md. disciplines.test.ts notes frontend carries the alias today, so that one consumer sees a deprecation note on every PR the moment this ships — a per-PR visible output change, not the "behavior-neutral for current consumers" the description claims. Intended forcing function, or worth gating until the migration is coordinated?
Lower-confidence notes (2)
workflows/review/lib/router.ts:942—readdirSyncpasses entry names blindly, so a directory named<lens>.mdpasses the inert-payload detector with no warning;readdirSync(..., {withFileTypes: true}).filter(e => e.isFile())closes it. Contrived, non-blocking.workflows/review/README.md:161— "A payload cannot whitelist a defect or lower the evidence bar" overstates a prose-only guarantee (no mechanical payload-content check). The author has confirmed on the adjacent thread that the contract is intentionally advisory.
…tes (shared default) Four reviewer runs on four different PRs (#281 and the review-* stack) were killed at the 20-minute engine ceiling on 2026-07-21, all with the same signature: Execute Claude Code CLI dead at ~20m50s, after the review's safe outputs had been emitted (comments and verdict posted) but before the cache-memory update. Losing the memory update compounds: the next re-review starts cold, redoes more work, and runs even longer. Root cause, not just symptom: the high-tier runBudget.maxWallClockMinutes soft target is 20, exactly equal to the old hard ceiling. The prompt's run-out-of-budget-gracefully protocol treats the soft target as the point to START landing, but for high-tier runs that point and the kill point were the same minute, so shedding mathematically could not save a heavy run. This repo routes nearly every file to tier=high, which is why it hit the wall first; consumers inherit the same geometry on their high-tier PRs. The change: timeout-minutes 20 -> 40 in the shared source (ships to consumers at their next installed-reviewer bump, changeset included) and the same value in this repo's installed copy as a LOCAL OVERRIDE (content-guard marker; drop at the next install bump). The budget table's soft targets are deliberately unchanged, so shedding starts at the same points; the wider ceiling restores real headroom between 'start landing' and 'killed'. budgets.ts's calibration note records the new geometry.
…tes (shared default) Four reviewer runs on four different PRs (#281 and the review-* stack) were killed at the 20-minute engine ceiling on 2026-07-21, all with the same signature: Execute Claude Code CLI dead at ~20m50s, after the review's safe outputs had been emitted (comments and verdict posted) but before the cache-memory update. Losing the memory update compounds: the next re-review starts cold, redoes more work, and runs even longer. Root cause, not just symptom: the high-tier runBudget.maxWallClockMinutes soft target is 20, exactly equal to the old hard ceiling. The prompt's run-out-of-budget-gracefully protocol treats the soft target as the point to START landing, but for high-tier runs that point and the kill point were the same minute, so shedding mathematically could not save a heavy run. This repo routes nearly every file to tier=high, which is why it hit the wall first; consumers inherit the same geometry on their high-tier PRs. The change: timeout-minutes 20 -> 40 in the shared source (ships to consumers at their next installed-reviewer bump, changeset included) and the same value in this repo's installed copy as a LOCAL OVERRIDE (content-guard marker; drop at the next install bump). The budget table's soft targets are deliberately unchanged, so shedding starts at the same points; the wider ceiling restores real headroom between 'start landing' and 'killed'. budgets.ts's calibration note records the new geometry.
## What Bumps the reviewer agent job's `timeout-minutes` from 20 to 40: in the shared source `workflows/review/review.md` (ships to consumers at their next installed-reviewer bump; changeset included) and in this repo's installed copy as a LOCAL OVERRIDE with the content-guard marker (drop at the next install bump). ## Why Four reviewer runs on four different PRs (#281 and the review-* stack) were killed at the 20-minute engine ceiling on 2026-07-21, all with the same signature: `Execute Claude Code CLI` dead at ~20m50s, after the review's safe outputs were emitted (comments and verdict posted) but before the cache-memory update. Losing the memory update compounds: the next re-review starts cold and runs longer. Root cause rather than symptom: the high-tier `runBudget.maxWallClockMinutes` soft target is 20, exactly equal to the old hard ceiling. The prompt's run-out-of-budget-gracefully protocol treats the soft target as the point to START landing, so for high-tier runs the landing signal and the kill point were the same minute; shedding could never save a heavy run. This repo routes nearly every file to tier=high, which is why it hit the wall first, but consumers inherit the same geometry on their high-tier PRs. The budget table's soft targets are deliberately unchanged: shedding starts at the same points as before; the wider ceiling restores real headroom between "start landing" and "killed". `budgets.ts`'s calibration note records the new geometry. ## Evidence Timed-out runs, all agent job / Execute Claude Code CLI killed at ~20m50s: - run 29874895707 (#281, review-lens-payload-seam) - run 29879948520 (jwies/review-submission-plan) - run 29878269776 (jwies/review-decision-chokepoints) - run 29878208638 (jwies/review-pre-agent-staging) Khan/webapp's recent review runs show no timeouts, but webapp's high-tier PRs sit on the same soft-target-equals-ceiling geometry, so the shared default moves rather than a local-only override. ## Test plan - `npx vitest run`: 43 files, 1057 tests pass; `pnpm typecheck` clean; eslint clean on budgets.ts. - `gh aw compile` (v0.81.6): 0 errors, 0 warnings; the lock diff is exactly the two timeout values plus the frontmatter hash. - review-pins.test.ts and version-sync.test.ts pass (the pinned source/ref surface is untouched). Author: jwbron Reviewers: github-actions[bot], jeresig, jaredly, somewhatabstract Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ✅ 9 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #285
…ice 2) (#282) * [jwies/review-scripted-dispatch] review: script-driven dispatch behind the ROUTING dispatch dial (orchestrator slice 2) * [jwies/review-scripted-dispatch] review: sync the workspace lockfile with the agent-sdk dependency * [jwies/review-scripted-dispatch] review: parse the validator's claims-array contract; dispute cap as a mechanical floor; feedback hardening * [jwies/review-scripted-dispatch-local] review: lenient contract extraction, findings-key tolerance, and the malformed-output retry in the dispatcher (trial run 29893634730) * [jwies/review-scripted-dispatch-local] review: honest dedupe-test naming; dispatch-directive arity/duplicate and runCli dispatchMode coverage (re-review feedback) * [jwies/review-scripted-dispatch] review: enforce the label contract in fromLabelShape (salvage near-misses, reject unknown labels) and join subject/discussion with a sentence break Trial run 29897276810's correctness-reviewer emitted a ReportFindings-style shape ({file, line, anchor, summary, severity, verdict, category, discussion}) instead of the label contract. The parse accepted it, defaulted label to the empty string with no anchor, and produced four label-less anchor-less claims that rendered as bare '**:**' paragraphs in the review body and demoted a BLOCKING correctness finding to advisory. Now fromLabelShape salvages the near-miss fields first (path from anchor.path/file, subject from summary), then rejects any finding whose label is not in the Conventional Comments vocabulary, so parseWithRetry re-dispatches the reviewer once with its corrective note instead of accepting a shape the contract never allowed. The same run's prose join glued subject to discussion with a bare space ("...memory Both TestExpiration..."); joinProse now inserts a sentence break when the subject lacks terminal punctuation, which also keeps buildClaims' first-sentence split recovering the subject. * [jwies/review-scripted-dispatch] review: merge cross-source duplicate claims before validation (#245) Trial run 29897276810 posted the AddDate months-vs-days defect four times (correctness, completeness, first-principles, and a skill-auditor out-of-lane handoff), and each copy was separately validated; claim validation is the largest sub-agent cost line, so duplicates now merge before the validator dispatch, not after. The merge is conservative and deterministic: different sources only, same path within a two-line window, and a token-similarity floor (Jaccard, overlap, and shared-bigram thresholds calibrated on that run's real outputs, where same-defect pairs scored at least 0.20 Jaccard and different-defect pairs on the same lines at most 0.05). The survivor is the highest-severity copy; it gains an 'also flagged by' note, adopts a merged duplicate's suggestion or author dispute when it lacks its own, and every merge is recorded in dispatch-result.json so run reports can count them. Replayed against the run's real out/ files this merges 13 claims to 9: the AddDate group collapses into the blocking correctness finding, and the missing-composite-index defect (flagged by correctness and holistic in different framings) collapses into one. * [jwies/review-scripted-dispatch] review: raise the per-sub-agent dispatch timeout to 15 minutes and record timeouts readably Trial run 29901690493 shed both default finders: the correctness-reviewer and skill-auditor were aborted at exactly the 5-minute DEFAULT_TIMEOUT_MS while every lighter reviewer finished in 60-115 seconds, and the staged error record read only 'Claude Code process aborted by user' (the SDK's generic abort message). The heavy investigators routinely need 5-10 minutes; the prior pin's correctness pass ran about 8 minutes to completion. The cap is a hang backstop, not a budget (credit spend is metered by the sandbox api-proxy), so it now sits at 15 minutes, and the CLI runner translates a timer abort into 'sub-agent timed out after Xms' so the out-file record and the run report name the cause. * [jwies/review-scripted-dispatch] review: salvage a label-valid finding whose failure_scenario is missing Trial run 29906543140's correctness pass twice emitted findings with valid labels, anchor objects, and rich discussions but no subject and no failure_scenario; the schema requires a non-empty failure_scenario, so both replies were rejected and the correctness dimension was voided (disclosed, but a voided default finder is strictly worse than a weaker fallback). The salvage now falls back subject -> discussion for the failure_scenario, so the claim-validator attacks the discussion prose instead of nothing. * [jwies/review-scripted-dispatch] review: salvage a title-keyed subject in the label-shape near-miss path Trial run 29908199997's correctness pass drifted to {id, file, anchor, label, category, title, discussion}: valid label, but the subject lived under 'title', so the salvage produced an empty subject and (pre-c35675f) an empty failure_scenario, voiding the dimension a third time. 'title' now joins 'summary' in the subject fallback chain. * [jwies/review-scripted-dispatch] review: salvage suggested_patch in the label-shape near-miss path fromLabelShape read only the contract's `suggestion` key, so trial run 29943085279's correctness pass, which drifted into its ReportFindings-style shape (defect-13 lineage), salvaged the AddDate finding but dropped the one-line fix it carried under `suggested_patch`; the posted comment (Khan/webapp#41029) had no suggestion fence. Accept a non-empty `suggested_patch` when `suggestion` is absent, exactly like the anchor and summary/title salvages beside it; `suggestion` still wins when both are present. * [jwies/review-scripted-dispatch] review: drop the same-path line window in the cross-source duplicate merge dedupeClaims required anchors within two lines of each other, so trial run 29943085279 posted the missing-deletion-test defect four times in one review (expiration_test.go:15 and :58, expiration.go:62 and :38). For same-path pairs the calibrated similarity floor (describesSameDefect) now carries the precision alone, the stance suppressOpenThreadDuplicates already takes and documents. Replayed against that run's claims.json this merges exactly the :15/:58 pair and nothing else; the AddDate issue and the untested-behavior thought on the very same line stay unmerged (the false-merge guard), and the test-adequacy/first-principles pair scores below the floor and stays two comments. Cross-file merging stays out of scope: it needs its own strictly higher calibration, and a missed merge only costs a duplicate comment. * [jwies/review-scripted-dispatch] review: star-guard the duplicate merge so a chain-only member never drops Union-find chains A~B~C through a bridging claim that bundles two defects (measured: a test-adequacy finding naming both the missing test and the unbounded read clears the floor against each correctness finding separately while the two never clear it against each other), and the collapse silently dropped a distinct blocking finding; with the same-path line window gone the bridge can span a whole file. A group member now merges only when it clears describesSameDefect against the survivor directly; chain-only members stay their own claims (a missed merge only costs a duplicate comment). Both recorded trial merges are unaffected: run 29897276810's four-way group is pairwise-complete, run 29943085279's is a direct pair. * [jwies/review-scripted-dispatch] review: stage dispatch-result.json under out/ so the artifact carries it runDispatch wrote dispatch-result.json only to /tmp/gh-aw/review, which the Step 9 artifact upload does not include, so run 29943085279's post-hoc could not tell whether the correctness output arrived through the submit_result tool or the text fallback: perAgent (structuredFinal, retried, per-agent usd), merges, and threadSuppressions never left the runner. The same content now also stages as out/dispatch-result.json. The dispatch gate reads out/ files by fixed or routing-planned name only (rereview-plan.json already stages there as a non-agent file), so the extra file changes no gate rule. * [jwies/review-scripted-dispatch] review: malformed reviewFiles entries fail toward triage-unavailable The dispatcher filtered triage's reviewFiles to strings and treated the filtered-empty result as an empty review, dispatching no finders and rendering no disclosure, while the gate's empty-reviewFiles waiver reads the RAW staged array (dispatch-gate.ts triageEmptiedReview): a triage output like reviewFiles: [1, 2] therefore produced a guaranteed false block (rule 1 correctness-missing has no disclosure escape, plus shed-undisclosed for every planned extra) on a conforming dispatcher. A non-empty array with zero string entries now takes the triage-unavailable path: review everything, disclose the skipped triage. A genuinely empty array still empties the review and the gate waiver still applies. * [jwies/review-scripted-dispatch] review: show the correctness reviewer one complete example finding The correctness pass emitted the ReportFindings-style shape in run 29943085279 even with the submit_result instruction appended (defect-13 lineage), and the salvage path is demonstrably lossy (the same run's suggested_patch drop). The output contract now carries one filled-in example finding naming the required keys and the drift keys not to substitute. Format emphasis only; no review rule or severity guidance changes. Verified with gh aw compile against a scratch consumer layout (.github/aw/review/config.md staged): 0 errors, 0 warnings. * [jwies/review-scripted-dispatch] review: record the trial hardening in the scripted-dispatch changeset * [jwies/review-scripted-dispatch] review: recalibrate cross-source claim dedup on the 30301235749 trial Run 30301235749 (Khan/webapp#41118) had five sources flag one TTL-unit defect at expiration.go:38, merged none of them, and posted three blocking comments plus a note for a single defect; it also made one false merge, folding the skill-auditor's AddDate handoff at :38 into the unrelated missing-test todo at :62. Replaying the module over that run's real pre-dedup claims (out/claims.json is written AFTER the merge, so the input is that file with the recorded merge undone and the skill-auditor handoff restored from out/skill-auditor.json) shows the floors were not the cause. A label-shape reviewer that omits failure_scenario is handed its own subject back as one, so the default correctness pass, a one-line title with all of its evidence in discussion, reached the comparison as eleven content tokens repeated twice and scored 0.13-0.23 Jaccard against the four discursive copies on a 0.20 floor. No threshold can fix that: run 29943085279's distinct same-line issue/thought pair (0.192/0.350/3) outscores two of the true duplicates on both ratios, so any floor loose enough to catch the cluster admits it. A claim whose failure scenario only restates its subject is now compared on its discussion instead. Only those claims: feeding the discussion to every claim was measured and rejected, since it lifts that same-line negative to ten shared bigrams, above every real duplicate. The floors then split in two tiers, an identical (path, line) from two sources being evidence of one defect while a differing line is weak evidence of two: exact anchor at 0.14/0.34/4, any other line on the same path at 0.20/0.35/6. The tighter other-line tier is what rejects the false merge (five shared bigrams; both real different-line duplicates share six or more). Replayed over the two runs whose pre-validation claim sets survive as artifacts, all 58 same-path cross-source pairs land on ground truth: 30301235749 collapses its five :38 copies into the correctness issue and keeps the :62 todo, 29897276810's three-way :38 group merges exactly as before, and no other pair moves. Nothing over-merges. Run 29943085279 staged no claims.json, so its pairs stay fixture-held, checked against the comments it actually posted. Both floors are minima over real claims and the margins are one unit (4 bigrams against 3, 6 against 5); dedup.test.ts carries the run's real claim texts so the next recalibration re-derives them rather than nudging them. * [jwies/review-scripted-dispatch] review: validate the validator's corrected fields per key `applyVerifications` applied every `corrected` field through one string-or-number check, so `corrected.label` could drift out of the label vocabulary and `corrected.line` could land a string in a numeric anchor. An out-of-vocabulary label on a confirmed BLOCKING claim fails `isBlockingLabel` and silently drops it from the REQUEST_CHANGES verdict: the drift class `fromLabelShape` already gates on the finder side, left ungated on the validator side. Validate per key and keep the finder's original when a correction is rejected. * [jwies/review-scripted-dispatch] review: split the self-contained glob matcher and dispatch-mode tests out main's lens-payload seam (#281) left router.ts at 999 lines against the 1000-line cap, so this branch's twelve lines of dispatchMode plumbing breached it, and the unioned dispatch-mode tests did the same to router.test.ts. Move the section already marked self-contained into glob-match.ts (re-exported from router.ts so no importer changes) and the dispatch-directive CLI tests into router-dispatch-mode.test.ts, the same split dispatch-contracts.ts took from dispatch.ts.
Summary:
A consuming repo may now define .github/aw/review/lenses/.md for any of
the eleven specialist lenses, plus lenses/correctness.md for the always-on
correctness-reviewer. Each file is runtime-imported (optional form) into a new
'Repo-specific rules and hunts' section of the matching reviewer prompt,
carrying that repo's surface-specific rules and extra tri-state hunts. Lens
names stay generic and shared; only payloads vary per repo.
Behavior-neutral for every current consumer: no repo carries a payload file
yet, and gh-aw's runtime resolves a missing optional import to nothing.
correctness-checks.md stays imported as a deprecated alias for
lenses/correctness.md (frontend carries one today); a repo should migrate the
file and carry at most one of the two.
Also in this change:
optional form (missing resolves to empty, not the '(not configured for this
eval case)' note), so corpus case trees can carry payloads. The
required-form fallback note is unchanged and remains the one documented
deviation (production fails the run).
section so the seam cannot be silently dropped.
(shared skeleton vs lens payload vs skills), and fixes two stale claims in
the consumer-config section: correctness-checks.md was never documented,
and the optional import form was said to have been dropped.
First planned payload use: migrating the #271 unbounded-reads hunt from the
shared skeleton to server repos' correctness/caching payloads, and frontend's
client-surface lenses/security-auth.md.
Test plan:
npx vitest run: 43 files, 1058 tests pass, including the newresolveRuntimeImports optional-form test and the per-lens seam assertion added to
disciplines.test.ts (pins the payload import in all eleven lens sections).
pnpm typecheckclean; eslint clean on the changed TS files.(github/gh-aw-actions@v0.81.6, setup/js/runtime_import.cjs): a missing optional
import inlines nothing and only warns; import paths under .github/ pass its path
validation.
runs; with no payload files in any consumer, the prompt delta is the inert
optional-import sections only, so the arms should match.