-
Notifications
You must be signed in to change notification settings - Fork 1
autofix: handle the documentation reviewer's readability finding shapes #335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| --- | ||
| "autofix": minor | ||
| --- | ||
|
|
||
| The fixer learns the documentation reviewer's three new readability finding | ||
| shapes (review's prose-readability release: metaphor in place of the | ||
| mechanism, says-the-same-thing-twice, undefined coinage). | ||
|
|
||
| Three rule changes in `autofix.md` Step 4, no code changes: | ||
|
|
||
| - **Batched instances are in scope.** The reviewer caps readability at one | ||
| thread per review and enumerates up to three further instances, verbatim, in | ||
| that thread's body. The do-not-touch-other-files rule gains a second | ||
| exception for them: each *quoted* instance is part of the finding wherever | ||
| it lives, while an instance merely alluded to without a quote is not. | ||
| Without this, the fixer could only ever fix the anchored instance, the | ||
| thread could never fully resolve, and the batching (which exists to spare | ||
| the author five separate threads) would trade author attention for fixer | ||
| blindness. The verbatim-rewrite rule below still governs each quoted | ||
| instance: the reviewer quotes it but need not rewrite it, so the fixer | ||
| touches it only when the fix needs none of its own words (deleting a | ||
| duplicated paragraph) or the thread body carries a rewrite for that | ||
| instance; a quoted metaphor or coinage without one is left unfixed and | ||
| reported. | ||
| - **Readability rewrites are applied verbatim.** A readability finding carries | ||
| its plain rewrite, and that rewrite passed claim validation, which checked | ||
| it preserves the original sentence's meaning. The fixer's improvised | ||
| paraphrase passed nothing, so improvising is now off the table: apply the | ||
| reviewer's words, or (when the file has drifted and the quote no longer | ||
| fits) leave the item unfixed and say so. | ||
| - **Duplicate paragraphs are deleted, never re-worded.** The existing | ||
| rewrite-before-delete bias is calibrated for a restated comment sitting on | ||
| unexplained code. Applied to a says-the-same-thing-twice finding it would | ||
| produce a third phrasing of content that already exists twice, which is the | ||
| defect the finding names. Deletion of the quoted copy is the fix. | ||
|
|
||
| The README documents the one shape that stays out of reach by design: 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 description | ||
| is the author's voice; the review body already offers them the rewrite. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,16 +494,42 @@ Edit the files directly in the workspace. Rules, all hard: | |
| a rationale.** A plausible-sounding reason you cannot source is worse than no | ||
| comment, because the next reader will trust it and the reviewer cannot tell | ||
| the difference. | ||
| - **A readability item is fixed with the reviewer's own words.** A documentation | ||
| finding may flag prose readability (a metaphor that hides the mechanism, a | ||
| paragraph restating an earlier one, shorthand the document never defines) | ||
| rather than comment content, and it carries the plain rewrite, quoted in its | ||
| body or as a suggestion block. Apply that rewrite verbatim: it survived claim | ||
| validation, which checked that it preserves the original sentence's meaning, | ||
| and a paraphrase you improvise did not. If the file has drifted and the quoted | ||
| rewrite no longer fits the text, leave the item unfixed and say so in Step 7 | ||
| rather than composing your own. | ||
| - **On a says-the-same-thing-twice item, delete; never mint a third phrasing.** | ||
| The rewrite-before-delete bias above is calibrated for a restated comment | ||
| sitting on unexplained code. A duplicated paragraph is the opposite case: the | ||
| content already exists in the copy that stays, so the fix is deleting the copy | ||
| the finding names, and rewording the duplicate into different words is the | ||
| failure mode the finding exists to stop, not a fix for it. | ||
| - **Say when a deletion left a hole.** If you delete a comment off a constant, | ||
| magic value, or workaround without being able to state why the value is what | ||
| it is, the thread is fixed and the code is still unexplained. Report both in | ||
| Step 7, naming the symbol, so the author knows there is a sentence only they | ||
| can write (`staleAfter`'s restating comment is gone; nothing records why the | ||
| window is that long). Reporting it as plainly fixed hides the one thing a | ||
| human still needs to do. | ||
| - **Do not touch files no item points at.** The one exception is a change that | ||
| is mechanically forced by a fix (a caller that must be updated for a changed | ||
| signature); note any such file in Step 7. | ||
| - **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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| 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 | ||
|
khan-actions-bot marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| too. An instance the body merely alludes to without quoting is not part of | ||
| the finding. The verbatim-rewrite rule above still governs each quoted | ||
| instance: the reviewer is only required to quote it, not to rewrite it, so | ||
| fix a quoted instance when its fix needs no words of yours (a duplicated | ||
| paragraph: delete the quoted copy) or when the thread body carries a rewrite | ||
| for that instance; a quoted metaphor or coinage with no rewrite of its own is | ||
| left unfixed and reported in Step 7, exactly like a drifted quote. Note any | ||
| file either exception led you into in Step 7. | ||
| - **Do not amend, rebase, or force-push.** You produce working-tree changes; | ||
| the push is a safe output. | ||
| - If a fix would require a design decision the reviewer did not make for you, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: