Skip to content

chore(skills): after a nit-only round, check only its fixes - #1612

Open
fuad-daoud wants to merge 1 commit into
mainfrom
chore/review-nit-rounds
Open

fuad-daoud wants to merge 1 commit into
mainfrom
chore/review-nit-rounds

Conversation

@fuad-daoud

@fuad-daoud fuad-daoud commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

In short: After a review round finds only nit:s, the next round still reads every angle over all that moved, and from the third round such rounds took 34% of reviewer CPU for 17% of the issue: findings (#1611). This lets that round check only the fixes, unless a commit is not a fix, changes what code, a script, a rule or normative text does, or a merge touches the change; the review still ends only on a round that finds nothing.

Once a review's round finds only nit:, the next round no longer reads everything references/rounds.md#later-rounds-read-what-moved lists. From the third round, where each commit of the change since fixes a finding, leaving unchanged what production code, a script, rule or normative text does, and no merge, the setup's too, touched the change's files, it reads CI's run, each fix against the tree and each body sentence against the fixes, and runs each open cell, instead of running the angles, planting otherwise only for a fix's test; a merge adds references/rounds.md#recheck-against-final-tree. An issue: returns the next round to the full reading, and a wrong brief this round. The review still ends only on a round that finds nothing, so the bar of no nit: stands.

Measured over 29 issues (#1611, with the query, the pattern and the limits): from the third round, rounds after a nit-only round took 34% of reviewer CPU for 17% of issue: findings, 334 CPU-minutes a finding against 107 to 164 for first rounds and rounds after an issue: or a clean round. Those 9 findings are the cost: a narrower round meets some of them later, or not at all. The sample is four days of CPU data. No cost has been measured for the narrower round, so what this saves is unknown, and 34% is its ceiling.

The new rule, nit-round-checks-its-fixes, follows later-rounds-read-what-moved, and each rule it meets stands as follows:

  • review-ends-with-nothing-left names both ways a next round can read.
  • later-rounds-read-what-moved opens "short of" the new rule.
  • missed-cell-widens-the-round excepts a nit: found in or just before a round under the new rule. An issue: in a filled cell still widens. Whether a nit: should widen anywhere else is skills: any miss in a filled cell sends the next round over the whole change #1563's, which this leaves open.
  • fresh-reader-for-the-second-round lets a reader's brief hold what the new rule asks, as a reader cannot otherwise know what the round before found.
  • audit-what-a-reader-cleared is unchanged: a cell it reopens is run in a narrow round, a plant included.
  • recheck-against-final-tree opens "short of" the new rule for a fix; a merge still takes it, as the new rule says. With main moving several times a day, most narrow rounds will carry that recheck, which this leaves as the safe side; its cost is unmeasured too.
  • first-round-finds-everything is unchanged: the first two rounds stay whole, so a nit-only first round is never a PR's only whole read. Starting at the second round is the alternative, which saves more and risks more.

A reader cannot tell what the round before found, so the rule has a round's brief say so, and no tool checks that a brief does. SKILL.md's third step cites both rules.

A commit no finding asked for also takes the full reading, as nothing the narrow round reads reaches it.

Before and after

flowchart LR
  subgraph before["Before"]
    direction TB
    b1["round 3 or later follows a nit-only round"] --> b2["every angle over each file that moved,<br/>each fix and the whole PR body"]
    b2 --> b3["it ends on a round that finds nothing"]
  end
  subgraph after["After"]
    direction TB
    a1["round 3 or later follows a nit-only round"] --> a2{"a commit is no fix or changes what production code,<br/>a script, rule or normative text does,<br/>or a merge touches the change?"}
    a2 -- "no" --> a3["each fix, its lines, each body sentence<br/>against them, cells left open;<br/>a merge adds recheck-against-final-tree"]
    a2 -- "yes" --> a4["every angle over what moved"]
    a3 --> a5["it ends on a round that finds nothing"]
    a4 --> a5
  end
  before ~~~ after
Loading

What it brings: a review stops paying a full round to confirm a reworded comment, while an issue:, a commit no finding asked for, a change to what production code, a script, a rule or normative text does, and the bar of no nit: keep what they had.

Test plan

  • python3.14 .claude/skills/morphic-curating-skills/scripts/skills_lint.py: skills_lint: clean (9 skills), which holds the new rule's 150 words, its keys and citations.
  • python3.14 .claude/skills/morphic-shipping-prs/scripts/subjects.py --base origin/main 1612 "<title>": clean.
  • The figures, rerun from skills: a round after a nit-only round reads far more than the fixes it follows #1611's query and pattern at the window it pins: after nits only, round >= 3: rounds 49 ... cpu_h 50.1 (34%) issues 9 (17%) ... cpu_min/issue 334.
  • No Go changed, so no lint or coverage result can move; TestDocuments_OpenNoLineWithAPlus reads these files as Markdown and passes at this head, and CI run 42358220625 passed on ab36a06c, the gate included (coverage gate: all 20085 statements covered).

Closes #1611

Related: #1522, #1530, #1563, #1588

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

skills: a round after a nit-only round reads far more than the fixes it follows

1 participant