autofix: handle the documentation reviewer's readability finding shapes - #335
autofix: handle the documentation reviewer's readability finding shapes#335jwbron wants to merge 2 commits into
Conversation
…ntation reviewer's readability finding shapes The review release that adds prose-readability clauses changes what a documentation thread can contain, and three of the fixer's Step 4 rules were calibrated for the old shape only. Batched instances: the reviewer caps readability at one thread per review and quotes up to three further instances in that thread's body, possibly in other files. The do-not-touch-other-files rule gains a second exception: a quoted instance is part of the finding wherever it lives; an instance alluded to without a quote is not. Without this the fixer could only fix the anchored instance and the thread could never fully resolve. Rewrite fidelity: a readability finding carries its plain rewrite, and that rewrite passed claim validation (meaning preservation checked). The fixer's improvised paraphrase passed nothing, so the rule is apply the reviewer's words verbatim, or leave the item unfixed and say so when the text has drifted out from under the quote. Duplicate paragraphs: the rewrite-before-delete bias exists for restated comments on unexplained code. Applied to a says-the-same-thing-twice finding it would mint a third phrasing of content that already exists twice, which is the defect the finding names. Deletion is the fix. The README documents the shape that stays out of reach by design: a PR title/description readability finding folds into the review body, never becomes a thread, and the thread-driven worklist cannot see it. The description is the author's voice; the review body already offers them the rewrite.
🦋 Changeset detectedLatest commit: 092c733 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - **Do not touch files no item points at.** Two exceptions. First, a change | ||
| mechanically forced by a fix (a caller that must be updated for a changed | ||
| signature). Second, the instances a batched documentation item quotes: the | ||
| documentation reviewer caps readability at one thread per review and |
There was a problem hiding this comment.
note (non-blocking): This encodes an unmerged companion PR's reviewer internals ('one thread per review', 'up to three instances, verbatim') as prose with no merge-order guard or cross-check. I grepped workflows/review/review.md on this checkout: it contains no readability, batching, or one-thread-per-review clauses — #331 is not in main, so this PR ships fixer rules keyed to a contract that exists only in a sibling PR. The operative test ('quoted → in scope') is robust on its own; the fragile part is the numeric/behavioral detail, and the repo already uses version-sync tests for exactly this class of cross-file drift.
A sketch, not a committable replacement:
Either sequence the merge (land #331 first, cite its merged text) or phrase the exception purely in fixer-verifiable terms ('an instance the body quotes verbatim, which you can locate exactly in the file') and drop the reviewer-internals numbers to a non-normative aside.
…-instances exception by the verbatim-rewrite rule Review feedback on #335: - The changeset said two new finding shapes; there are three. - The batched-instances exception and the verbatim-rewrite rule conflicted for a quoted instance without its own rewrite: the reviewer's batching instruction (review.md, the readability cap) requires quoting further instances, not rewriting them. The exception now defers to the rewrite rule: fix a quoted instance only when the fix needs no improvised words (deleting a duplicated paragraph) or the thread body carries a rewrite for that instance; otherwise leave it and report in Step 7.
|
Addressed the review feedback in 092c733:
|
There was a problem hiding this comment.
2 of 3 prior review threads resolved; 1 still unaddressed as of 092c733:
1 non-blocking thread still open
- note (non-blocking)
workflows/autofix/autofix.md:522: This encodes an unmerged companion PR's reviewer internals ('one thread per review', 'up to three instances, verbatim')...
| - **Do not touch files no item points at.** Two exceptions. First, a change | ||
| mechanically forced by a fix (a caller that must be updated for a changed | ||
| signature). Second, the instances a batched documentation item quotes: the | ||
| documentation reviewer caps readability at one thread per review and |
There was a problem hiding this comment.
suggestion (non-blocking): Change is calibrated to a reviewer contract (#331) not present in this repo. I checked the base tree (238c5b8): workflows/review/review.md's documentation reviewer has zero occurrences of coinage/says-the-same-thing/three further/one-thread-per-review, and it explicitly disclaims PR title/description scope ("title and description are not yours"). #331 is unmerged on this branch. autofix.md L522-524 asserts the reviewer "caps readability at one thread per review and enumerates up to three further instances, verbatim," and README L139-146 asserts a PR title/description readability finding "posts PR-level, folded into the review body" — neither behavior exists in the installed reviewer. The rules are harmless-but-dormant until #331 ships; the risk is release ordering. Cross-reference #331 and gate the autofix release so its docs never ship describing an absent reviewer contract.
| signature). Second, the instances a batched documentation item quotes: the | ||
| documentation reviewer caps readability at one thread per review and | ||
| enumerates up to three further instances, verbatim, in that thread's body, so | ||
| each **quoted** instance is part of the finding wherever it lives; fix those |
There was a problem hiding this comment.
thought (non-blocking): Batched-instances exception hands plan-owned scope decisions back to the model. Step 2 makes the plan authoritative ("The plan is final... Do not widen it... act on a finding it did not hand you") and Step 4 requires every hunk be "traceable to an item in plan.items"; the whole determinism boundary exists to keep scope out of model prose. The new exception (L519-532) directs the fixer to locate and edit quoted further instances "wherever they live," i.e. in files/locations plan.items does not carry, reintroducing the model-driven scope-widening the boundary was built to prevent. It's defensible (the quotes come from the item body the plan hands over verbatim), but the further instances' path/line are not in plan.items, so consider having plan.ts extract quoted-instance locations rather than leaving the fixer to grep and judge.
There was a problem hiding this comment.
2 of 3 prior review threads resolved; 1 still unaddressed as of 092c733:
1 non-blocking thread still open
- note (non-blocking)
workflows/autofix/autofix.md:522: This encodes an unmerged companion PR's reviewer internals ('one thread per review', 'up to three instances, verbatim')...
| - **Do not touch files no item points at.** Two exceptions. First, a change | ||
| mechanically forced by a fix (a caller that must be updated for a changed | ||
| signature). Second, the instances a batched documentation item quotes: the | ||
| documentation reviewer caps readability at one thread per review and |
There was a problem hiding this comment.
suggestion (non-blocking): Change is calibrated to a reviewer contract (#331) not present in this repo. I checked the base tree (238c5b8): workflows/review/review.md's documentation reviewer has zero occurrences of coinage/says-the-same-thing/three further/one-thread-per-review, and it explicitly disclaims PR title/description scope ("title and description are not yours"). #331 is unmerged on this branch. autofix.md L522-524 asserts the reviewer "caps readability at one thread per review and enumerates up to three further instances, verbatim," and README L139-146 asserts a PR title/description readability finding "posts PR-level, folded into the review body" — neither behavior exists in the installed reviewer. The rules are harmless-but-dormant until #331 ships; the risk is release ordering. Cross-reference #331 and gate the autofix release so its docs never ship describing an absent reviewer contract.
| signature). Second, the instances a batched documentation item quotes: the | ||
| documentation reviewer caps readability at one thread per review and | ||
| enumerates up to three further instances, verbatim, in that thread's body, so | ||
| each **quoted** instance is part of the finding wherever it lives; fix those |
There was a problem hiding this comment.
thought (non-blocking): Batched-instances exception hands plan-owned scope decisions back to the model. Step 2 makes the plan authoritative ("The plan is final... Do not widen it... act on a finding it did not hand you") and Step 4 requires every hunk be "traceable to an item in plan.items"; the whole determinism boundary exists to keep scope out of model prose. The new exception (L519-532) directs the fixer to locate and edit quoted further instances "wherever they live," i.e. in files/locations plan.items does not carry, reintroducing the model-driven scope-widening the boundary was built to prevent. It's defensible (the quotes come from the item body the plan hands over verbatim), but the further instances' path/line are not in plan.items, so consider having plan.ts extract quoted-instance locations rather than leaving the fixer to grep and judge.
Review Guidancegithub-actions (1 file)
|
There was a problem hiding this comment.
Approved — no blocking issues found.
5 of 5 prior review threads are still unaddressed as of 092c733:
5 non-blocking threads still open
- suggestion (non-blocking)
workflows/autofix/autofix.md:522: Change is calibrated to a reviewer contract (#331) not present in this repo. I checked the base tree (238c5b8): workflow... - suggestion (non-blocking)
workflows/autofix/autofix.md:522: Change is calibrated to a reviewer contract (#331) not present in this repo. I checked the base tree (238c5b8): workflow... - note (non-blocking)
workflows/autofix/autofix.md:522: This encodes an unmerged companion PR's reviewer internals ('one thread per review', 'up to three instances, verbatim')... - thought (non-blocking)
workflows/autofix/autofix.md:524: Batched-instances exception hands plan-owned scope decisions back to the model. Step 2 makes the plan authoritative ("Th... - thought (non-blocking)
workflows/autofix/autofix.md:524: Batched-instances exception hands plan-owned scope decisions back to the model. Step 2 makes the plan authoritative ("Th...
1 participant
Companion to #331, which adds prose-readability clauses (metaphor in place of the mechanism, says-the-same-thing-twice, undefined coinage) and PR title/description scope to the documentation reviewer. Three of the fixer's Step 4 rules were calibrated for the old finding shape only; this updates them. Prompt and docs only, no code changes.
The three gaps
1. Batched instances vs the file-touch rule. The reviewer caps readability at one thread per review and quotes up to three further instances in that thread's body, possibly in other files.
autofix.md's "do not touch files no item points at" rule would forbid fixing those, so the fixer could only ever fix the anchored instance and the thread could never fully resolve. The rule gains a second exception: a quoted instance is part of the finding wherever it lives; an instance alluded to without a quote is not. Extra files get reported in Step 7 like the existing mechanically-forced exception.2. Rewrite fidelity. A readability finding carries its plain rewrite, and that rewrite passed claim validation (the validator checks it preserves the original sentence's meaning; refuting rewrites that lose information is part of its documentation section as of #331). The fixer's improvised paraphrase passed nothing. New rule: apply the reviewer's rewrite verbatim, and when the file has drifted out from under the quote, leave the item unfixed and say so rather than composing a new paraphrase.
3. Duplicate paragraphs vs rewrite-before-delete. The existing rewrite-before-delete bias is calibrated for a restated comment sitting on unexplained code, where the rationale is recoverable and deletion erases it. Applied to a says-the-same-thing-twice finding, that bias would produce a third phrasing of content that already exists twice, which is exactly the defect the finding names. New rule: delete the quoted copy; never re-word it.
What deliberately does not change
A PR title/description readability finding folds into the review body and never becomes a thread, so the thread-driven worklist cannot see it. The README now documents that this is a design decision rather than a gap: the description is the author's voice, the review body already carries the rewrite for them to take or leave, and a bot editing PR metadata is a different trust decision than a bot editing comment text on a branch.
The
docsloop-ineligibility analysis inscope.ts/README is untouched: readability rewrites are still fixer-authored added lines that re-enter the reviewer's scope next cycle, so if anything they strengthen the existing conclusion.Testing
Prompt/docs/changeset only.
npx tsc --noEmitclean and the full canonical vitest suite green (including both version-sync tests; no version literals touched).