autofix: add a docs scope that fixes only documentation threads - #300
Conversation
🦋 Changeset detectedLatest commit: e652bf9 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}
|
|
||
| ### Why `docs` is the safest scope | ||
|
|
||
| Its edits cannot change program behaviour, which no other scope can say. That |
There was a problem hiding this comment.
thought (non-blocking): The "edits cannot change program behaviour" property here is enforced only by the Step 4 prompt rule ("changes text, never code") — nothing in plan.ts/stage.ts inspects the edits the agent actually produces. Since this line sells docs as the safest scope to trial first, consider either backing the claim mechanically (fail closed if a docs-only diff touches a non-comment line) or softening the wording to "is instructed never to".
| export const findingLabelsForScope = ( | ||
| scope: AutofixScope, | ||
| ): readonly string[] => { | ||
| switch (scope) { |
There was a problem hiding this comment.
note (non-blocking): This switch is described as exhaustive, but there is no default arm with a const _: never = scope guard, and nothing else enforces exhaustiveness: workflows/** is outside the typecheck project and the tests hardcode the three scopes rather than iterating AUTOFIX_SCOPES. A future token added to AUTOFIX_SCOPES without a matching arm would return undefined at runtime — arming that scope with no labels — and nothing would flag it. A never default (or a test iterating AUTOFIX_SCOPES) would make it fail loud.
| * cannot show this containment, so it is stated here and in the README. | ||
| */ | ||
| export const AUTOFIX_SCOPES = ["blocking", "nits"] as const; | ||
| export const AUTOFIX_SCOPES = ["blocking", "nits", "docs"] as const; |
There was a problem hiding this comment.
question (non-blocking): docs is the first scope token that selects by lens label rather than by severity class, and the containment argument in the docstring applies verbatim to suggestion (non-blocking, best-practice) and to every future lens variant. What bounds this axis from growing one token per lens label? It may be worth writing the boundary down now (e.g. "docs is the only lens-selection token" with the reason, or committing to the generalization) before a second lens token forces the choice ad hoc.
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 |
e30fd9c to
e3628dc
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approved — no blocking issues found.
4 of 4 prior review threads are still unaddressed as of e3628dc:
4 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:128: The "edits cannot change program behaviour" property here is enforced only by the Step 4 prompt rule ("changes text, nev... - note (non-blocking)
workflows/autofix/lib/scope.ts:149: This switch is described as exhaustive, but there is nodefaultarm with aconst _: never = scopeguard, and nothing... - question (non-blocking)
workflows/autofix/lib/scope.ts:65:docsis the first scope token that selects by lens label rather than by severity class, and the containment argument i... - note (non-blocking)
workflows/review/lib/router.ts:106: AddingdocumentationtoALWAYS_ON_LENSESdoes not make it always-on, and the addition is in fact required — `SPECIAL...
631c840 to
4573ef3
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
4 of 4 prior review threads are still unaddressed as of 4573ef3:
4 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:207: The "edits cannot change program behaviour" property here is enforced only by the Step 4 prompt rule ("changes text, nev... - note (non-blocking)
workflows/autofix/lib/scope.ts:189: This switch is described as exhaustive, but there is nodefaultarm with aconst _: never = scopeguard, and nothing... - question (non-blocking)
workflows/autofix/lib/scope.ts:89:docsis the first scope token that selects by lens label rather than by severity class, and the containment argument i... - note (non-blocking)
workflows/review/lib/router.ts:106: AddingdocumentationtoALWAYS_ON_LENSESdoes not make it always-on, and the addition is in fact required — `SPECIAL...
| case "blocking": | ||
| return BLOCKING_LABELS; | ||
| case "docs": | ||
| return [DOCUMENTATION_LABEL]; |
There was a problem hiding this comment.
nitpick (non-blocking): Arming autofix: nits and autofix: docs together lists suggestion (non-blocking, documentation) twice in findingLabels — docs's label set is a subset of nits', and resolveTokens builds the field with flatMap and no dedupe. Harmless today since buildWorkList collapses it to a Set, but the field rides on AutofixRequest, so a future consumer that renders or counts it inherits the duplicate.
| return [DOCUMENTATION_LABEL]; | |
| findingLabels: [...new Set(scopes.flatMap((scope) => findingLabelsForScope(scope)))], |
…y documentation threads `autofix: docs` / `/autofix docs` acts on the threads carrying the `documentation` reviewer's label and nothing else. It is a subset of `nits`, not a peer: documentation findings are non-blocking, so `nits` already covers them. The containment runs one way, and `docs` exists because arming `nits` to clear three stale comments also invites the fixer into every other cosmetic thread on the PR. Not loop-eligible, despite looking like the exception to "nits never loop": its deletion half converges, its missing-explanation half answers with prose, and prose can always be wanted better. One prompt rule comes with it: a documentation item changes text, never code. If the honest fix would touch an executable line, the item is left unfixed and reported. Edits that cannot alter behaviour is the property that makes this the safest scope to trial first.
…nale and its enforcement point
Documentation only; no behaviour change. Two claims in the loop-eligibility
notes were wrong, and both are the kind whoever adds the cadence axis would
inherit.
The enforcement point. "isLoopEligible is enforced in code rather than left to
convention" overstates a function whose only callers are its own tests. What
actually stops a loop in v1 is the token table: `loop` is in
UNIMPLEMENTED_TOKENS, so `autofix: loop` is rejected and no cadence can be armed
at all. isLoopEligible is a documented intent with a test, which is the right
thing to have built now, but it is not a gate. Both the README and the module
header now say so, and say that calling it is the cadence author's job.
The mechanism. Both notes attributed re-flagging to the fixer's own prose ("the
reviewer will always find something cosmetic in the autofixer's own output").
Khan/webapp#41194 shows otherwise: one blocking-scoped cycle fixed its finding
and had the thread resolved and the PR approved, then drew two fresh
non-blocking findings against code the fixer never wrote (counts.go:7 is
func MergeCounts, twelve lines above its first added line; counts.go:16 is a
context line in its own hunk). That run planned no-prior-fingerprint, so the
reviewer's newly-changed-code scope filter was a no-op and the whole diff was
re-derived with no memory of the previous review. Open non-blocking threads went
3 to 5 in one cycle with no nits-scoped work done. The generator is a memoryless
re-derivation over the whole diff, which does not need the fixer to have written
anything. The conclusion is unchanged and better supported: nits still cannot
converge.
What that costs blocking is worth writing down next to the predicate that
returns true for it. The merge gate is a claim about a human eventually merging,
not a termination proof, and nothing in the reviewer backs it: applyScopeFilter
keeps plain `issue (blocking)` / `todo (blocking)` findings whether or not they
land on newly-changed code, so blocking is the one class that can be re-raised
on previously-reviewed, untouched lines every cycle. The scope filter bounds
nits; it does not bound blocking. A cadence axis needs its own stop condition
and cannot inherit one from isLoopEligible.
No guard is added here. A cycle cap and a no-progress guard both have their
substrate already (summariseLedger's cycles/nextCycle and attemptedThreadIds),
but adding uncalled guards beside an uncalled predicate is not an improvement
until there is a cadence axis to call them.
…ata point Documentation only. Both loop-eligibility notes said `docs` ineligibility was "a claim about evidence we do not have, not a claim about the domain". After Khan/webapp#41194 that is no longer true, and it is the same class of stale claim as the two the previous commit fixed. The trial's second fresh finding is a missing explanation: counts.go:16 flags a doc comment that covers tie resolution but not the empty/nil-map case, raised unprompted against the fixer's own PR. That is exactly the half these notes name as non-convergent, so the evidence exists now and it points the same way the ineligibility call already did. Two limits stated rather than glossed, because they are what keep this an observation instead of a measurement. It did not come from the documentation reviewer: webapp runs a `review` release too old to mint the `documentation` label, so the finding was a plain `note (non-blocking)` and `autofix: docs` would not have selected it. And one finding says nothing about which half dominates, which remains the thing to measure first if a cadence axis is ever built. The conclusion is unchanged: `docs` stays ineligible. The changeset needs no edit; it already says "ineligible until something measures which half dominates", which one data point does not do.
…ion condition, not a property The nits-never-loop note says the generator is a memoryless re-derivation over the whole diff. True of the run it cites, but stated as a property of the generator a reader takes memorylessness as the normal state, and it is not: it was a consumer version artifact with a named cause and a named remedy. Khan/webapp pins review-v1.7.0 (2026-07-21). That release carries no stage-pr.ts and no stampSource at all, so it has no cache-memory fingerprint carrier and its only carrier is the review-body stamp that gh-aw's ingest sanitizer strips. Hence no-prior-fingerprint on every re-review, hence the scope filter no-op, hence the whole diff re-derived each cycle. review-v1.8.0 (2026-07-30) has both carriers, so a consumer on it can anchor a re-review and scope one. That does not make the convergence question go away, and the clause says what survives it: a cadence axis must not assume any given consumer is on a release that can scope. It is a condition to check, not a property to design around. The clause also says what it is NOT, because the two degradations look alike and are not. Autofix's own currency check is degraded for an unrelated and non-version reason: cache memory is scoped per workflow, so the reviewer's carrier is unreachable from autofix at any version. staleness.ts already documents that; conflating them would make the reviewer's fixable problem look permanent and autofix's permanent one look like a version bump away.
4573ef3 to
e652bf9
Compare
… 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.
Review Guidancegithub-actions (2 files)
Common patterns2 files: 4 files: Excluded from review (2 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
There was a problem hiding this comment.
Approved — no blocking issues found.
1 of 5 prior review threads resolved; 4 still unaddressed as of e652bf9:
4 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:207: The "edits cannot change program behaviour" property here is enforced only by the Step 4 prompt rule ("changes text, nev... - note (non-blocking)
workflows/autofix/lib/scope.ts:189: This switch is described as exhaustive, but there is nodefaultarm with aconst _: never = scopeguard, and nothing... - nitpick (non-blocking)
workflows/autofix/lib/scope.ts:193: Armingautofix: nitsandautofix: docstogether listssuggestion (non-blocking, documentation)twice in `findingLa... - question (non-blocking)
workflows/autofix/lib/scope.ts:89:docsis the first scope token that selects by lens label rather than by severity class, and the containment argument i...
2 participants
Adds one value to the scope axis:
autofix: docs//autofix docsfixes only thedocumentationreviewer's threads.Both dependencies have landed: #298 (autofix itself) and #299 (which mints the label this scope selects on). Rebased onto main 07-30, so the four commits below are this PR's own and the diff is just the scope change:
scope.ts, its tests, the label allowlist, and docs. The lock was regenerated withgh aw compile autofixrather than hand-merged, and it comes out byte-identical to the replayed patch; the twoautofix.mdcopies are byte-identical to each other.Why this is small
Because #299 did the load-bearing half.
worklist.tsselects threads by parsing the Conventional-Comment label off each posted comment — autofix reads the PR's threads, not the reviewer's run artifact, so nothing else about a finding survives to reach it. Once the reviewer mintssuggestion (non-blocking, documentation), a documentation-scoped autofix is exactly "the threads carrying that label", and this PR is a table entry plus a switch arm.docsis a subset ofnits, not a peer of itThe one thing worth arguing about. Documentation findings are non-blocking, so
nitsalready covers them: arming both is the same as armingnits. The containment runs one way only, which is the whole point — armingnitsto clear three stale comments also invites the fixer into every other cosmetic thread on the PR.The flat token namespace cannot show that (
blocking,nits, anddocsall read as peers), so it is stated inscope.ts, in the README's axis table, and pinned by a test that assertsdocs ⊂ nitsand that no blocking label can ever land in docs scope. No new axis: union semantics,UNIMPLEMENTED_TOKENS, and the two arming surfaces are untouched.findingLabelsForScopebecomes an exhaustive switch rather than a blocking/non-blocking ternary, so the next scope value cannot silently inheritNON_BLOCKING_LABELSby falling through.Not loop-eligible, and that is the interesting call
isLoopEligibleexists to encode the rule most likely to be violated by whoever adds the cadence axis, so a new scope value has to answer to it.docslooks like the exception to "nits never loop" and is only half one. Its deletion half genuinely converges: a comment that restates the code is either gone or it is not. Its other half does not — the documentation reviewer also flags a missing explanation, the fixer answers with prose, and prose is the thing a reviewer can always want written better. Sodocsstays ineligible, and the comment says why in those terms: it is a claim about evidence we do not have, not about the domain. If a cadence axis is ever built,docsis the first candidate to re-examine and the thing to measure is which half dominates.The prompt rule that comes with it
A documentation item changes text, never code. Deleting a comment the finding calls redundant is the expected fix, not an overreach — and such findings often carry no suggestion block precisely because a deletion cannot be expressed as one (the reviewer's
suggested_patchmust be non-empty). But if the honest fix would touch an executable line (renaming the symbol a comment misdescribes, changing the constant it contradicts), that is a code change wearing a documentation label: leave it unfixed and report it.Prompt-level, not code-enforced, and flagged as such — the same standing as "never weaken a test". A deterministic "comments only" check would have to be language-aware, and getting it wrong in either direction is worse than the prompt rule.
That property is also the argument for this scope existing: its edits cannot change program behaviour, which no other scope can say. It is the natural first thing to trial in a repo that has not run autofix before.
Version coupling worth knowing before trialling it
Autofix selects on labels minted by whichever
reviewrelease a repo has installed.autofix: docstherefore finds threads only once that repo is on a reviewer carrying the documentation label. Against an older reviewer it is not broken; it is always empty. Documented in both READMEs.Compiled output
remove-labels.allowedgainedautofix: docs, so.github/workflows/autofix.lock.ymlis recompiled (gh-aw v0.83.4, matching #298). The lock diff is exactly the two metadata hashes, the description, and the allowlist in its three baked locations..github/workflows/autofix.mdis kept byte-identical toworkflows/autofix/autofix.md, as #298 has it.Testing
1371 tests pass across the repo (133 in
workflows/autofix), typecheck and prettier clean. New tests cover the token on both arming surfaces, the label mapping, the subset relation, the no-blocking-label-in-docs-scope guard, and loop ineligibility.Not in scope
PR descriptions and titles. That is the third piece, it needs the
update-issuesafe output and a shrink-only guard, and it does not belong on this stack.