review: add an opt-in documentation reviewer - #299
Conversation
🦋 Changeset detectedLatest commit: 53d7ef2 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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (raw === undefined) { | ||
| return []; | ||
| } | ||
| if (!Array.isArray(raw) || !raw.every((n) => typeof n === "string")) { |
There was a problem hiding this comment.
suggestion (non-blocking): The non-array enabledReviewers shape guard here is untested — live-producer.test.ts pins the happy path and the unknown-name throw, but not this branch. A case with enabledReviewers: "documentation" (or [1]) asserting rejects.toThrow(/must be an array of strings/) would close it. Low priority: it is a defensive guard whose sibling throw is already tested.
| the comment must exist at the cited location and say what the claim quotes, and the | ||
| code the claim contrasts it with must say what the claim says it says. The | ||
| documentation reviewer's characteristic false positive is mistaking a real | ||
| constraint for a restatement, so **refute** whenever the comment carries information |
There was a problem hiding this comment.
question (non-blocking): This documentation-claim rule frames verification around an existing comment ("the comment must exist at the cited location"; "refute whenever the comment carries information the code does not show"). Does that leave the policy's "missing the non-obvious why" category — which flags an absent explanation, with no comment to quote — without a validation path, so it is refuted by default? A sentence covering the absence case (verify the unexplained constant exists on an added line and that nothing in the change explains it) would close it.
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 |
| --- | ||
| name: documentation | ||
| description: Advisory, opt-in check that code comments and prose docs in the diff document intent rather than restate code; returns findings as JSON. | ||
| model: claude-opus-4-8 |
There was a problem hiding this comment.
I'm having some trouble getting opus 5 working. Going to get that sorted out in #294
…ewer An advisory-only whole-change reviewer that checks the comments and prose docs a diff adds or changes against a documentation policy written inline in its definition. Off everywhere until a repo adds `enable documentation` to its ROUTING file. Its findings render with a new non-blocking label, `suggestion (non-blocking, documentation)`. The variant is not cosmetic: the autofix workflow selects threads by parsing the label off each posted comment, so it is the only channel by which a documentation-scoped autofix can tell a documentation thread from any other nit. Supporting changes: a documentation-claim rule for the claim-validator, and the live eval producer now dispatches the opt-in reviewers a case enables (before this, no opt-in reviewer had a live arm at all). New corpus pair: one change with two real documentation defects and a must-not-flag trap, one whose comments all earn their line.
…e autofix selects on it
…er with scripted dispatch Rebase integration after main removed task mode. Two of this branch's review.md hunks no longer had prose to edit, because main moved what they edited into code: - The shed ranking is now `SHED_RANKING` in `dispatch.ts`; `documentation` takes the first-to-shed slot there instead of in review.md's prose. - The label taxonomy is now `BLOCKING_LABELS`/`NON_BLOCKING_LABELS` in `render-comment.ts`, which already carries the new label. And one semantic gap the textual merge could not see: the scripted dispatcher code-assigns each label-shape reviewer's `lens`, hardcoded to `conventions` for `skill-auditor` and `correctness` for everything else, so a documentation finding reached the render stage with the wrong lens. Replaced with a named map (`LABEL_SHAPE_LENS`), so `lens: documentation` survives to the downstream consumers that key on it and `labelForFinding` agrees with the label the reviewer emitted.
76864d2 to
18fc3ec
Compare
…ymmetric arm, not a failed run The live A/B baseline arm extracts its agents from the base tip, so on the PR that adds a reviewer it cannot define that reviewer. `produceLive` threw on any roster name missing from the arm's review.md and `runArm` does not wrap its produce call, so selecting either new documentation case killed the whole A/B before it wrote a report. That made the corpus pair this PR adds unmeasurable, which is the one thing the pair exists for. An enabled opt-in reviewer the arm does not define is now recorded as an absent dimension (`PerAgentReport.absent`) and reported under a new "Arm asymmetry" section, so a missing dimension never reads as a reviewer that ran and stayed quiet. Tolerating absence cannot mask a typo, the failure mode the enabledReviewers validation exists to prevent: the name is already checked against ENABLEABLE_REVIEWERS, so absence can only mean the arm predates the reviewer. Every other roster member still hard-errors, the routed lenses included, since a lens name is validated nowhere.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
3 of 3 prior review threads are still unaddressed as of c2e3594:
3 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/eval/corpus/golden/golden-documentation-stale-and-narrated/case.json:5: Neither new corpus case exercises the reviewer's "missing the non-obvious why" or commented-out-code flag categories — b... - suggestion (non-blocking)
workflows/review/eval/live-producer.ts:214: The non-arrayenabledReviewersshape guard here is untested —live-producer.test.tspins the happy path and the unkn... - question (non-blocking)
workflows/review/review.md:1597: This documentation-claim rule frames verification around an existing comment ("the comment must exist at the cited locat...
| const roster = rosterNames.flatMap((name) => { | ||
| const agent = agents.get(name); | ||
| if (agent === undefined) { | ||
| if ((enabled as readonly string[]).includes(name)) { |
There was a problem hiding this comment.
suggestion (non-blocking): This absence-tolerance path applies to both arms, but on the candidate arm of a new-reviewer A/B agents.get() returning undefined can only mean the candidate's review.md is misconfigured (the enable directive is validated against ENABLEABLE_REVIEWERS, not against the arm actually defining the sub-agent) — yet the run still completes green and the report normalizes it as expected. runArm already knows which arm it is on; a hard error, or at least a distinct flag, for candidate-arm absence would catch a misconfigured candidate that otherwise measures nothing about the reviewer the A/B exists to graduate.
Lower-confidence observations (1)
workflows/review/lib/dispatch-contracts.ts:250— the label-shape reviewer→lens mapping now lives in two independent maps (productionLABEL_SHAPE_LENSdefaults tocorrectness; the evallabelLensnames each reviewer), which agree fordocumentationbut already diverge for the other opt-in reviewers, with no cross-check test tying them together.
…o the pooled report The first live A/B of the documentation pair proved the absent-dimension handling works (the baseline arm recorded `documentation` absent on both cases in all three repeats and still produced a report), and exposed a gap in the reporting half: a multi-repeat run renders through renderMultiMarkdownReport, which had no asymmetry section, so the caveat existed only in the JSON while the markdown a reader sees credited the delta with no note that the baseline arm never had the dimension. Extracted the section into a shared helper, deduplicated across repeats (one absence per repeat would otherwise print N times), and used it in both renderers.
There was a problem hiding this comment.
Approved — no blocking issues found.
4 of 4 prior review threads are still unaddressed as of 53d7ef2:
4 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/eval/corpus/golden/golden-documentation-stale-and-narrated/case.json:5: Neither new corpus case exercises the reviewer's "missing the non-obvious why" or commented-out-code flag categories — b... - suggestion (non-blocking)
workflows/review/eval/live-producer.ts:214: The non-arrayenabledReviewersshape guard here is untested —live-producer.test.tspins the happy path and the unkn... - suggestion (non-blocking)
workflows/review/eval/live-producer.ts:654: This absence-tolerance path applies to both arms, but on the candidate arm of a new-reviewer A/Bagents.get()returnin... - question (non-blocking)
workflows/review/review.md:1597: This documentation-claim rule frames verification around an existing comment ("the comment must exist at the cited locat...
…criminate the documentation reviewer The first A/B of the original pair read zero recall delta: the baseline arm caught both seeded defects in 6 of 6 samples with no documentation reviewer in it. Diagnosis is a corpus-shape error, not a reviewer failure. Both seeded defects have a truth-value problem (a comment this diff falsified, a comment that narrates the change), and a false claim sitting next to changed code is exactly what a correctness pass reaches for; the ten-line diff gave it nothing else to look at. These three seed defects with no truth-value problem, so a correctness pass has no lane for them, each inside a diff that carries real and correct logic work so the default roster's attention has somewhere to go: - restated-docstring: a docstring whose every line is recoverable from the signature, on a genuine paging fix (over-fetch by one, replacing a length === PAGE_SIZE test that could not tell a full last page from a truncated one). - missing-why: three added constants, two carrying the rationale the code cannot show, and BASE_DELAY_MS = 1_700 carrying none. The reviewer has to notice the one gap its neighbours make conspicuous. - commented-out-code: the same construct twice in one diff, kept with a dated reason and a ticket in one function and unexplained in the other. A reviewer that greps for commented-out lines fails it in both directions. Every case carries its must-not-flag traps inside the diff, so a false flag is reachable rather than dropped by the provenance gate for unrelated reasons, and every diff is verified against computeChangedLines with each added line matching the fixture tree byte for byte.
Review Guidancegithub-actions (6 files)
Common patterns5 files: Single-line addition of 5 files: New corpus Excluded from review (5 files)Not individually reviewed — generated, formatting-only, or
|
There was a problem hiding this comment.
1 of 4 prior review threads resolved; 3 still unaddressed as of 102ee15:
3 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/eval/live-producer.ts:214: The non-array enabledReviewers shape guard here is untested. A case with enabledReviewers: "documentation" (or [1]) asse... - suggestion (non-blocking)
workflows/review/eval/live-producer.ts:654: This absence-tolerance path applies to both arms, but on the candidate arm of a new-reviewer A/B agents.get() returning... - question (non-blocking)
workflows/review/review.md:1597: This documentation-claim rule frames verification around an existing comment. Does that leave the policy's "missing the...
… dispatch.ts, which is over its line cap (#304) main is red on lint, and neither PR that caused it could have seen it. `@khanacademy/eslint-config` sets max-lines to 1000. #302 took workflows/review/lib/dispatch.ts to exactly 1000 lines; the single line #299 added to SHED_RANKING took it to 1001. Each PR was green against its own base, so the violation existed only in the merge, which is invisible to a per-PR lint run. Every open PR in the repo inherits the failure, including #300. Fixed by extracting a concern rather than by reclaiming a line, so the next addition does not land in the same place, and rather than raising the cap, which lives in the shared Khan config and would be a house-rule deviation. DEFAULT_FINDERS, SHED_RANKING, Roster, RosterShed and computeRoster move to dispatch-roster.ts and are re-exported from dispatch.ts, which already advertises one import surface for the dispatch machinery. The moved code is byte-identical; dispatch.ts goes 1001 -> 920 lines and the new module is 108. Verified: 1573 tests pass unmodified, typecheck clean, and eslint over everything CI lints (actions, utils, workflows, minus the ignorePatterns paths) reports zero errors. Worth recording for next time: a plain local `pnpm run lint` cannot reproduce CI inside a git worktree under .claude, because eslint skips dot-directories by default and silently ignores the whole tree; `--resolve-plugins-relative-to . --no-ignore` scoped to the CI paths is what actually reproduces it.
2 participants
Adds an opt-in
documentationreviewer: advisory-only, whole-change, checking the comments and prose docs a diff adds or changes against a documentation policy. Off everywhere until a repo addsenable documentationto itsROUTINGfile. First of three PRs from the #ai-guild thread on LLM-generated comments; the premise is that comment quality is an enforcement problem, not a prompt problem.Flags: a comment that restates the code; one that narrates the change ("updated to…", "previously we…"); one this diff falsified; commented-out code; and the other direction, a specific unexplained constant, workaround, or ordering constraint the change introduces.
Does not flag: density preferences, anything CI owns, generated files, a
TODOwith a ticket, or comments the diff did not touch.Design notes
The policy lives inline in the agent definition, like the specialist lenses'
Review rules. It has to be written down somewhere, because every finder must quote the rule andclaim-validatordrops what it cannot verify against real rule text; an unwritten policy just yields{"findings": []}. A fifth required.github/aw/review/*.mdconsumer import was the alternative, rejected because the baseline is universal and a missing runtime import fails the whole review at run time, so every repo would have to write the file before its next PR could be reviewed. Repo-specific calibration rides the per-directoryREVIEW.mdcontracts that already exist, which also answers Jeff's "backend and frontend may want different outcomes": they can, per directory.It never reasons about who wrote the text. It cannot tell human from model, must not guess, and the policy is the same either way. Attribution is unreliable anyway (we strip Claude trailers by convention, so absence proves nothing), and a rule binding only bot-authored PRs is bypassed by deleting a trailer. If we want the author axis, it belongs in the counters as a measurement, not in the rubric.
The label is load-bearing. Findings render as
suggestion (non-blocking, documentation), code-assigned from the lens like the, best-practicevariants. The stacked autofix PR depends on it:worklist.tsselects threads by parsing the label off each posted comment, so nothing else about a finding survives to reach it, and without the variant a documentation-scoped autofix would widen to every nit on the PR. There is deliberately no blocking variant; minting one would enlargeBLOCKING_LABELS, the setautofix: blockingacts on. Blast radius is small because the taxonomy is centralized (thumbs-sweep-github.tsspreads both sets,verdict.tscounts blocking only, the eval runner goes throughlabelForFinding). The README notes the consumer ordering: autofix reads labels minted by whichever reviewer version a repo has installed.Anchoring. The best finding here is "the change falsified this comment", and that comment usually sits on an unchanged line, because what changed is the code below it; the provenance gate would drop it. So the prompt says to anchor on the changed code line and name the comment in the prose. The golden case pins it: the stale-comment finding anchors on
EXPIRY_DAYS = 90, not on the comment above.Supporting changes
claim-validatorgains a documentation-claim rule. A documentation claim is neither a code claim nor askillclaim, so it was being validated as a runtime-behaviour claim. The new bullet says what to verify and names the characteristic false positive: mistaking a real constraint for a restatement.enabled reviewers.produceLive's roster wasDEFAULT_FINDERS + routing.lensesToSpawn, so no opt-in whole-change reviewer has ever had a live arm and none could earn itsenableline the way the README requires.routerConfig.enabledReviewersnow adds them in canonicalENABLEABLE_REVIEWERSorder, an unrecognised name throws (a typo would otherwise produce a green, expensive run measuring nothing), andlabelLensgains all six. Existing cases set nothing, so the corpus is unchanged.review.mdprose into code, so those hunks moved with them. One gap the textual merge could not see: the dispatcher code-assigns each label-shape reviewer'slens, hardcodedcorrectnessfor everything butskill-auditor, so a documentation finding reached the render stage with the wrong lens. Replaced with a namedLABEL_SHAPE_LENSmap, with a test.Corpus
A matched pair, both live-enabled:
golden-documentation-stale-and-narrated: a sound behaviour change (expiry 30 → 90 days, clock injected) leaving a falsified comment and a change-narrating comment. Both must post, verdict stays APPROVE. Plus a must-not-flag trap, anEXPIRY_SLACK_MScomment that reads like a restatement but records a constraint the code cannot show; it is inside the diff, so a false flag on it is reachable rather than gate-dropped for unrelated reasons.clean-documentation-earned-comments: the precision guard, and the more important of the two. Every added comment earns its line, the run must post nothing, and both traps sit on added lines next to the code they explain.Both diffs verified against
computeChangedLines(zero orphan hunk lines); annotated line numbers match the fixture trees.Left for later
update-issuesafe output and a shrink-only guard. Separate PR, not stacked.ROUTINGchange ships here; it earns itsenableline through the live A/B, which the producer change above finally makes possible.Testing
1421 tests pass, typecheck clean. The corpus pair runs through the deterministic replay in
suite.test.ts, wherecheckExpectationenforces themustCatchids and the pinnedpostedCommentCount.Worth a second opinion
documentationis first-to-shed, ahead ofconventions. Advisory comment findings should go first under budget pressure, but the interior order is editorial.conventionspicks among four: exact autofix selection and nil verdict impact, at the cost of the nitpick/question/note nuance.claude-opus-4-8/ medium is copied fromconventions, not measured.