review: read per-directory REVIEW.md contracts from the consuming repo - #212
Merged
Conversation
…ing (slice-1) R1 (task-1-2): standardize Step 6 review submission on one robust submit-pull-request-review call with a real, non-empty body; remove the empty-body branch and any fallback/retry path. E2 (task-1-3): stage /tmp/gh-aw/review/pr-context.json in Step 1 and wire all six sub-agent read blocks to consume it. Extends #194 diff staging; foundation for slice-7 lenses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
task-1-1: workflows/review/lib/finding-schema.ts - FINDING_SCHEMA_VERSION exported constant; each finding carries schema_version and the validator rejects an unrecognized version (breaking-shape guard). - Finding shape (snake_case wire keys, matching #194 per-run JSON artifacts): id, lens, anchor, severity, confidence, evidence_trace, optional suggested_patch, optional pre_merge_obligation, producing_hunt, model_authored_prose. - Anchor is a discriminated union with the required PR-level type (pr | file | line, line supports side + multi-line start_line..line range). - KNOWN_LENSES (11 specialist + always-on/triage) and SEVERITIES exported for reuse by the slice-3 router and slice-2 verdict; lens/severity validated against them. - validateFinding collects ALL violations (diagnosable per-lens drop rate), plus isValidFinding narrowing wrapper and throwing assertFinding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nts) - Restore the empty-review-body convention: with inline comments present the verdict is submitted with an empty body; a non-empty body exists only for comment-less reviews and skipped-dimension notes. Keeps the single-call submission rule. - Deliver workflows/review/lib/ to consumer repos: gh-aw's source: import only copies the .md, so the job checks out Khan/actions at the pinned review-v* release (single version surface for prompt + code) and installs its runtime deps. - Drop planning identifiers (E2, R8, slice numbers, operator-directive note) from shipped file comments.
… rendering (slice-2) task-2-1/2-2 verdict.ts: pure computeVerdict(labels + dimension availability + policy conflicts) -> APPROVE | REQUEST_CHANGES | HOLD_FOR_HUMAN. Consumes #194's mechanical label rule (isBlockingLabel); does not re-implement it. R2 gate: a missing core dimension (correctness/skill-severity) or a policy-named conflict holds for a human; a lost pattern-triage is note-and-continue. Documented default blocking threshold = 1 (tunable, not a HITL gate). task-2-3 render-comment.ts: code-owned Conventional-Comment label taxonomy + deterministic label-wrapping/templating from the finding schema; models own all prose. Review-body rendering mirrors review.md Step 6 + skipped-dimension notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pshot tests (task-2-4) Truth-table tests for computeVerdict (verdict.test.ts): APPROVE / REQUEST_CHANGES / HOLD_FOR_HUMAN precedence, the R2 core-dimension gate (correctness + skill-severity absence holds; hold dominates a blocking label with all reasons retained), pattern-triage note-and-continue, policy-named conflicts (verbatim detail passthrough), blocking-threshold clamping (>=1) + never-RC-on-zero, and purity (deterministic, non-mutating). Rendering snapshot tests (render-comment.test.ts): isBlockingLabel over the full label taxonomy + unknown-label safe default; labelForFinding severity x lens matrix (conventions => best-practice, specialists => plain); renderComment inline snapshots (label wrap + verbatim prose + optional suggestion block) with verbatim prose/patch passthrough asserted; renderReviewBody inline snapshots for every verdict x inline branch + skipped-dimension notes + non-empty safe-output guard. Fixtures validated through assertFinding so tests can't pass on schema-invalid findings. 50 tests green; full pnpm test suite 253/253; tsc --noEmit clean (node24). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Formatting-only follow-up to b2c4d90: applies @khanacademy/eslint-config prettier rules to verdict.test.ts + render-comment.test.ts so the lint check passes on the changed files. No test logic or inline-snapshot content changed — 50/50 still green under node24; tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ema (tester NACK v1)
Formatting-only; no logic change. Applies the repo lint gate's prettier 2.6.2
formatting (eslint-plugin-prettier@4.0.0 binds prettier 2.6.2) which had never
been run against these files:
- render-comment.ts / finding-schema.ts: string-arg + ternary line wrapping.
- typeof-indexed-access type aliases dropped the paren form
((typeof X)[number] -> typeof X[number]) — prettier 2.6.2 emits the
unparenthesized form; the parens were added by prettier 2.7.0. Semantics
unchanged (verified typecheck-clean under the repo tsconfig).
Verified: strict typecheck clean; verdict truth-table + render checks still pass;
prettier diff against these files is now empty except the exact 2.6.2<->2.8.8
typeof-parens delta, which the pinned CI formatter (2.6.2) resolves in favour of
the committed form.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…bodies) - Flip verdict precedence to REQUEST_CHANGES > HOLD_FOR_HUMAN > APPROVE: a blocking finding is actionable on its own, so the hold only ever replaces what would otherwise be an auto-approval. Missing-dimension and policy-conflict reasons are still recorded and rendered. - Empty review bodies when inline comments exist (APPROVE and REQUEST_CHANGES); a non-empty body remains only for comment-less reviews and skipped-dimension notes, matching review.md. - HOLD_FOR_HUMAN body now explains what failed and 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), and renders policy-conflict lines. - Drop planning identifiers from shipped comments.
…viewer-mapper (task-3-3)
Wire the orchestrator prompt to consume the router's routing.json
({lensesToSpawn, teams, perFileTier, runBudget}) for lens + team routing,
per the architect integration design (§8.4/§8.6):
- Add a 'Route first' step in Step 3 invoking the in-session router CLI
(node -r @swc-node/register workflows/review/lib/cli/router.ts) over the
Step-1-staged full.diff/files.json + .gitattributes/.github/REVIEWERS,
writing /tmp/gh-aw/review/routing.json; document the one-model-touch
two-invocation protocol for diff-direction-dependent tiers.
- Drop the reviewer-mapper sub-agent: remove its Phase-2 dispatch bullet, its
out/ artifact entry, its owned-files.json staging, and its agent definition.
- Repoint Step 7 (risks/patterns grouping) and Step 8 (reviewer request +
fallback) from reviewer-mapper owners/fallbackTeams to routing.json
teams.owners/teams.fallback.
- Phase 2 dispatch now spawns the whole-change reviewers plus every lens named
in lensesToSpawn (populated as lenses land in slice 7).
AC: only the router feeds lens/team routing in review.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eviewer_code NACK v1) The router CLI invocation in Step 3 pointed at workflows/review/lib/cli/router.ts, but the router is built at workflows/review/lib/router.ts (plan task-3-1/3-2 file-lists; every shipped sibling CLI — finding-schema.ts, verdict.ts, render-comment.ts — lives directly under lib/, there is no lib/cli/ dir). The bad path would have failed the orchestrator's bash call at runtime, defeating task-3-3's AC. Corrected to lib/router.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-3) task-3-1: file classification from .gitattributes (linguist-generated), path->lens mapping (11 specialist lenses), team mapping from REVIEWERS (most-specific-pattern-wins + fallback ranking, subsuming reviewer-mapper), and per-file risk tier. The one diff-direction-dependent judgement is externalised as pendingRiskQuestions for the orchestrator's small-model call (plan §8.6); the core stays pure/deterministic. task-3-2: ONE budget rule scaling by the highest touched tier + ONE floor for misrouted PRs. Documented default caps (DEFAULT_TIER_BUDGETS, 20min/$10 ceiling per operator); maxToolCallsPerFinding lives here for slice-5 (R9). No human-read prose authored in code (R8 tripwire, §8.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, budget, floor (task-3-4) Covers the five task-3-4 AC axes over fixtures plus the reviewer-mapper subsumption (R12) and the glob/config parsers routing is built on: - classification: linguist-generated vs source (parse + isGenerated + route). - path->lens: per-file union, deduped lensesToSpawn in canonical order, always-on lenses never spawned as specialists. - tier: default, highest-of-matching, diff-direction-dependent deferral (conservative tier + pendingRiskQuestions), resolvedTiers second pass. - budget scaling: monotonic across tiers; run budget = highest touched tier. - misrouted floor: docs-only PR floored to 'low'; already-higher tier not lowered. - teams: most-specific REVIEWERS rule wins, sorted union + file-count fallback rank. Pure/deterministic core (plan §8.6) — no I/O, no model call. Offline: could not run vitest (node_modules absent, network unreachable); hand-verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(slice-3 v2)
Addresses reviewer_code_holistic v1 NACK (BLOCKING-1 + BLOCKING-2):
BLOCKING-1: router.ts now has a CLI entrypoint. review.md Step 3 invokes
`node -r @swc-node/register workflows/review/lib/router.ts` directly, so the
file doubles as the executable: a require.main===module guard runs runCli(),
which reads the Step-1-staged files.json + .gitattributes/.github/REVIEWERS,
calls the (unchanged) pure route(), and writes /tmp/gh-aw/review/routing.json.
node:fs is loaded lazily inside the guard so the module stays import-clean for
the unit tests; runCli takes an injected fs so it is testable without real I/O.
BLOCKING-2: new toRoutingJson() adapter emits the exact shape review.md
consumes — teams:{owners:{path:[team]}, fallback:[{team,files}]} (owners over
source files only, mirroring the old owned-files scope) and perFileTier in the
documented capitalized casing (High|Medium|Low|Trivial), bridged from the
core's lowercase tiers at the I/O boundary. pendingRiskQuestions is carried in
routing.json so the two-invocation resolvedTiers protocol stays intact; the
optional second-pass answer is read from /tmp/gh-aw/review/resolved-tiers.json.
Pure route()/parsers/budget core is byte-for-byte unchanged (reviewers had no
correctness objection); this is purely the I/O+serialization shell. No
human-read prose authored in code (R8 tripwire §8.7).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/tierFromDisplay/runCli) Extends task-3-4 tests to the coder's v2 additions (34d88d5) — the routing.json I/O boundary review.md Step 3 depends on: - tierFromDisplay: display+lowercase casings, unknown -> low. - toRoutingJson: display-cased perFileTier, owners scoped to source files (generated excluded), fallback + pendingRiskQuestions passthrough. - runCli (fs injected, no real I/O): bare-array & {files} wrapper parsing, misrouted floor, .gitattributes/REVIEWERS parse, resolved-tiers second pass, mkdir + routing.json round-trips to the returned value. Offline (node_modules absent, network unreachable); hand-verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Remove the baked-in DEFAULT_LENS_RULES/DEFAULT_RISK_RULES: a repo's routing map now lives in its own .github/aw/review/ROUTING file (REVIEWERS-style line grammar, parsed deterministically in the new routing-config module), complementing the model-facing risk-classification.md. Missing config degrades safely: no specialist lenses, always-on reviewers still run, budget floored, and routing.json carries a loud routingConfig warning the orchestrator surfaces on the PR. - runCli reads repo files under REVIEW_REPO_ROOT so the router can run from the shared-lib checkout against the reviewed repo. - review.md: apply reviewer-suggested edits (drop stale reviewer-mapper references, fix the router-reads-the-diff claim), document the ROUTING format, the checkout-based invocation, and the back-to-back two-pass flow for direction-dependent tiers with a concrete example. - Drop planning identifiers from shipped comments.
…lice-4)
E1: correctness-reviewer names the high-risk trigger + one-line judgment in riskReason.
E3: untrusted-input rule broadened to all PR content; injection attempts to steer the
reviewer are themselves findings (issue (blocking)).
E5: deletions are findings -- flag removals that drop a guard/check/test.
R3b: pre-existing bugs on touched lines are fair to flag, scoped to touched lines,
building on #194 severity (does not reopen it).
E6: stage the full reply chain per bot thread; reconciler weighs author reasoning and
never re-raises a conceded point.
E7: stage open human-thread lines; reconciler emits skipLines and the orchestrator
defers there (verdict + Step 5 kept consistent).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e, gates, version stamp) task-11-1: four datasets loading via the shared loader — golden (human-comment + revert/follow-up ground truth), synthetic-mutations mapped to lenses, clean, and a non-smoke incident repro; holdout/fresh/adversarial tags for the gates. task-11-2: metrics.ts — five metrics (must-catch recall, golden precision, clean false-block, noise, calibration/ECE) computed over a corpus run, pure. task-11-3: judge.ts — Opus-4.8 LLM-judge with a pure request/aggregate core and an injected model seam; deterministic human-audit sample; thumbs (slice-8) calibration pass. task-11-4: gates.ts — golden holdout split + fresh mutations (overfitting report) and the adversarial set as a hard gate for automatic mode. task-11-5: version-stamp.ts — SHA-256 of prompt+config+schema rendered into the existing #194 pr-reviewer HTML marker; the single drift-guard surface (parse + hasDrifted for a consumer sync check). run-types.ts holds the shared EvalRun shape the three eval modules consume. No consumer-repo writes; no benchmark-gated construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…den aggregate join Addresses reviewer_code_holistic v1 NACK (dangling cross-slice build dependency): judge.ts imported `DownvoteReason` from ../lib/thumbs-sweep, but slice-8 (the thumbs sweep) is not in slice-11's ancestry, so that module does not exist on this branch. The judge consumes thumbs labels as data and never needs the sweep module at build time, so the import was gratuitous coupling. Declared a local `DownvoteReason` union (structurally identical to slice-8's) so judge.ts is self-contained and typechecks against the branch as it stands. Also cleared the two non-blocking advisories while re-proposing: - aggregate() now rejects duplicate finding ids across requests (the join is by bare finding id, only case-unique in the schema — a collision would mis-join), and throws on a score whose finding id matches no request. The docstring now matches the code (previously claimed an unknown-id throw that did not exist). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sk-11-6) Adds workflows/review/eval/suite.test.ts, the task-11-6 deliverable: - Suite self-tests over the coder's slice-11 modules (loader four-dataset load, five metrics, adversarial/overfitting gates, LLM-judge with a STUB model, and the version-stamp drift surface), exercised against the real corpus plus focused synthetic fixtures built through the public parseCase/runCase path. - Whole-suite green-on-baseline check: every corpus case meets its expected block; must-catch recall = 100%, clean false-block = 0, adversarial hard gate passes (automatic mode allowed). - CI-wiring guard: the smoke subset is a strict subset (the fast per-PR pnpm-test gate), and the ONLY model seam is judgeCorpus (injected model, async, off the deterministic path) -- so the live-judge full suite is scheduled/opt-in, never per-PR. Metrics+gates run over the full corpus with zero model calls. Deterministic: no model, no network, no clock, no randomness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1-6, resolves gap-1)
… is a committed script on a weekly workflow, version-stamp machinery dropped
…n + R14 drift-guard doc R13 (task-12-1): add the three-way per-finding resolution rule to the thread-reconciler in review.md — on re-review every actionable prior finding resolves as fixed / deferred-to-filed-issue / disagreed-with-reason, else keep. R14 (task-12-2): document the slice-11 version stamp in review/README.md as the single consumer-readable config-drift surface (interface §4.5); reads the existing pr-reviewer:version marker, adds no second mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er_code NACK v1) Reviewer_code correctly flagged that the README overstated the version stamp as 'already rendered into the posted review comment'. Grep confirms renderVersionMarker/ computeVersionStamp are invoked only in eval/suite.test.ts; review.md Step 6/7 emit no pr-reviewer:version marker. Reworded the 'Config drift guard' section to state accurately: the stamp is produced by lib/version-stamp.ts and exercised by the eval suite today; the marker is the DESIGNATED single drift surface (Step 7 approval comment / committed skill snapshot) but is NOT yet emitted by the workflow — wiring that emit point is pending follow-through. Still adds no new mechanism; no second surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nditional-approval task-12-3 (R15): counters.ts — pure aggregation over #194 per-run artifacts + run summary: validator drop rate per source, comments/PR, verdict mix, thumbs agree rate, cost/run. No new logging mechanism; best-effort fs loader over the conventional artifact layout, defensive normalization from loosely-typed JSON. task-12-4 (R16): dismissal-learning.ts — mines resolved-without-fix threads, 👎-with-reply, and correct-pushback signals into candidate do-not-flag rules, emitted as a unified diff against a committed config for human approval. Never auto-applied; min-occurrences noise guard; deterministic ordering + diff. task-12-5 (R17): render-comment.ts — renderObligationsComment() builds the prominent pre-merge obligations comment from the schema's pre_merge_obligation field (verbatim), for the existing add-comment safe output; APPROVE body reflects the obligation count. Determinism boundary preserved (code owns structure only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing groupKey Addresses NACKs from reviewer_code_holistic, reviewer_contract, reviewer_security, and tester (all the same defect): the groupKey() field joiner and its adjacent comment held 3 raw U+0000 (NUL) bytes, which made git classify the file as binary and hid its diff from BRC review. Replaced the raw NUL bytes with the \0 escape sequence in the template literal and the comment. Runtime delimiter is byte- identical (NUL remains a collision-safe joiner — it cannot appear in lens/path/ label), so this is semantics-preserving; the file is now plain UTF-8 text and diffs normally. No logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…contract); defer dismissal-learning until the thumbs sweep has real signals
🦋 Changeset detectedLatest commit: e683d5c 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 |
khan-actions-bot
requested review from
a team,
jeresig and
somewhatabstract
and removed request for
a team
July 7, 2026 21:35
… conflicts after #207 squash-merge)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #207.
Problem
webapp documents sub-tree-specific review expectations in
REVIEW.mdfiles (a root one plus one per documented top-level directory, next to eachAGENTS.md), but nothing in the review workflow ever reads them. They are not pulled in by default:claude --print, no--bare), so the repo'sCLAUDE.md(webapp symlinks it toAGENTS.md) IS loaded for the orchestrator and inherited by the inline sub-agents.AGENTS.mdreferences the contracts as plain Markdown links, not@-imports, so they are never inlined;REVIEW.mdis not a memory filename to Claude Code; and the sub-agent prompts point only at the diff, the.github/aw/review/runtime imports, and the skill files.So the review contract only reached the reviewer if a model happened to follow a link.
Change
correctness-reviewer: when the checkout carriesREVIEW.mdfiles, read the root contract plus the nearestREVIEW.mdabove each reviewed file, and use them to sharpenriskReasonwording and calibrate finding severity for that sub-tree.claim-validator: use the same contracts to calibrate claim labels (a contract calling a category a nit supports correcting an overstated label). Contracts never decideverification; that stays code-evidence-only.REVIEW.mdfiles are unaffected.Trust boundary
Unlike
.github/and the agent config folders (restored from the base branch by gh-aw before the agent runs),REVIEW.mdfiles are read from the PR head. The prompts therefore treat contract text as guidance that can adjust emphasis but never override the workflow's rules, and an edit to aREVIEW.mdin the diff is reviewed on its merits under the existing steering-text rule.