Repository navigation
chore(skills): make review rounds fewer and reach every cell - #1543
Merged
Merged
Conversation
#1532's review ran past seven rounds at 16 to 64 minutes of a reader's time each, and four things kept it going: defects in the previous round's own fixes, cells a reader named instead of reaching, slow commands, and calls that batched nothing while the main session's context grew. The review skills addressed none of them. Readers now start at effort xhigh rather than inheriting the session's, and reach every cell their brief names, on a budget summed from those cells, or state why one cannot be reached. A brief also has the reader batch independent calls, keep bulky output in files, bound slow commands and build trial merges as a worktree of a commit-tree commit, which trial-merge-happened now prescribes. A scoped round carrying whole reads, such as closing issues and tracker searches, splits between two parallel readers, and no round divides its files between readers. Before each hand-off the author reruns the plants a fix can have changed, derives each body number from a command, and reruns the trial merges. Every harness lives under one directory with one fingerprint. A PR's scope settles before its second round, the main session compacts only at a clean round boundary and rereads its state afterwards, and each round's report gives its readers' pace. A placeholder in a prescribed command is also filled with the hardest value it admits. The reader rules move from rounds.md into a new readers.md, as rounds.md would otherwise pass its 4,000-word cap; each moved rule keeps its key, and every citation follows it.
closingIssuesReferences is empty on a stacked PR, so a Probe or a read listing a PR's issues from it found none there. The scope rule's Probe and closing-issue-read-whole now name each issue a closing keyword in the body names, by closing-keywords. recheck-before-hand-off restated which plants to rerun and dropped those whose code between plant and test a fix rewrote. It now cites verdict-outlives-unchanged-files for the set, whose own Probe gains the fixtures and the files between, read as literal paths, since a bracket in a pathspec is a glob. A reader's effort is set on the call that starts it, not in its brief, so start-readers-at-xhigh now holds that and reads the effort from the reader's transcript; its Why claims only that #1532's readers, started with none set, ran at the session's. reader-batches-and-bounds splits into reader-batches-its-calls and reader-bounds-its-commands, the latter bounding a command by what it walks or a limit of its own, as a tool's timeout did not stop one. The split round says which half matches a Direction to the head and to the body's reasons, the compaction rule names who compacts, and trial-merge-happened's ancestry check gives a reason that fits a worktree built from a commit.
run-prescribed-commands named only characters as the hardest values a placeholder admits, so a Probe listing a PR's issues was run on a PR into main alone and passed, while on a stacked PR it read nothing.
verdict-outlives-unchanged-files' Probe read its paths literally, so a glob of fixtures, the form this codebase writes them in, matched nothing and passed as unchanged. The flag is gone, and the Probe now also runs git ls-files --error-unmatch at the head, so a path matching nothing, or a test the change deleted, fails instead of passing. The rule now covers a commit as well as a merge, as recheck-before-hand-off applies it to a round's fix commit. The scope rule's Probe compared a PR's closing issues with a first round's report that a later reader never holds. It now compares them with the body as the PR opened, read from the body's edit history. recheck-before-hand-off's Probe named an undefined hand-off note, which read as the reader's brief would hand verdicts to the reader kept unanchored. The record now lives in the round's saved state and never in a brief. trial-merge-happened removes each worktree once read, as a merge per open PR otherwise piles up checkouts. run-prescribed-commands names a path given as a glob among the hardest values, the case the literal pathspec failed on. Two Probes say "the reader's brief" again, and one long line is rewrapped.
trial-merge-happened confirmed a merge by ancestry, which a worktree still holding the merge of a PR stacked on this one also passes. It now compares the merge's second parent with the PR's head. Its Probe, which listed both parents, repeated that check and is gone. start-readers-at-xhigh read the effort from the reader's transcript, which records whatever effort the reader inherited, so a reader started with none set passed whenever the session ran at xhigh. The Probe now reads the Agent call in the starting session's transcript. one-harness-directory's single checksum could not say which harness to rebuild; its Probe now diffs a sorted per-file list. The split round's diff half takes every carried cell that is not a whole read, and the whole-read half reads a closing issue as closing-issue-met-at-its-site says rather than restating it. The scope rule exempts a stacked child's issues a landing PR carries, and its Probe reads the opening body with userContentEdits(last:1), which returns it directly. recheck-before-hand-off becomes author-pass-before-hand-off, a key that no longer echoes recheck-against-final-tree. The previous commit's message overstated one effect: a test the change deleted already showed in the Probe's diff before git ls-files joined it. What ls-files adds is failing a path that matches no file.
trial-merge-happened confirmed a merge by its second parent alone, so a worktree still holding a merge of the same PR at an earlier head passed. That is the leftover a rerun after a fix commit meets, and its stale tree lints clean. The check now compares both parents with the head and the PR's head, and the heading says the rule builds the merge too. The previous commit's message was wrong to say the removed Probe repeated the second-parent check: it checked the first parent as well. one-harness-directory's list must sit outside the directory it fingerprints, or it names itself as changed. The split round's whole-read half reads closing issues by closing-issue-read-whole, which holds their comments, rather than closing-issue-met-at-its-site, which holds only the title and the Direction; its heading drops "scoped", which no other rule uses. run-prescribed-commands names a directory an earlier run left full among the hardest values, and the effort Probe says plainly what a reader's own transcript records.
trial-merge-happened said a stale <dir> could hold a merge "of this PR at an earlier head or one stacked on it", which reads either way round: the PR being merged in, or the change being trial-merged. It now names the leftovers by the placeholders the check compares, a merge of another <pr head> or of an earlier <head>, and the Rule names what it builds rather than leaning on its heading. The earlier message's "a PR stacked on this one" meant a PR stacked on the one being merged in, the case an ancestry check let through. one-harness-directory's Probe gave "it" two referents and diffed "after it", which allowed a diff taken before the hand-back. It now keeps the list outside the directory and diffs after the hand-back.
trial-merge-happened ran its confirm inside <dir>, where a <head> given as HEAD names the trial's own commit, so every correct merge built from HEAD was refused. The confirm now runs from the worktree that built the merge and reads the merge's parents with git -C <dir>. The rule kept reaching its word cap carrying two lessons, so the build moves into trial-merge-by-commit-tree, whose Why now says the form trial-merged all 27 open PRs (12 of them conflicted, so 15 were linted), and trial-merge-happened keeps the confirm and its key, citing the build, so every rule citing it still reaches both. one-harness-directory's fingerprint passed on an empty listing: find lists nothing in a directory named through a symlink, and a missing one leaves an empty list behind a pipeline that exits 0. It now runs find -H and refuses an empty list with test -s. run-prescribed-commands' hard values read as an illustration, not a closed list. 2bc686e's message corrects "the earlier message's" wording without naming the commit: that message is 434a367's.
one-harness-directory's find -H followed only the directory named on the command line, so a harness file linked from elsewhere was no regular file to it, and an edit to that file's target passed unseen. The Probe now uses find -L in both commands and writes the second in full, sort included, where "the same find into diff" left the sort out. closing-issue-read-whole now takes the issues from the body's closing keywords, which may name another repository's issue as <owner>/<repo>#N. gh issue view refuses that form, and its number alone reads this repository's issue of the same number, with no error. The rule reads an issue by its number or its URL.
fan-out-an-angle-to-a-reader started readers without citing brief-a-reader-to-its-cells, so a requested fan-out reached the effort, batching and bounding rules only through the fresh reader it names as the default. It now briefs each reader by that rule. brief-a-reader-to-its-cells asks for a budget summed from a share per cell, and its Probe now checks that the stated budget is their sum: a brief can state about 95 tool calls over shares that add to 86. Bullets this change added whose lines broke before a word that fitted are refilled at 100 columns, words unchanged.
b484ea3 refilled audit-what-a-reader-cleared, a rule this change only moves, so its per-rule diff against main showed rewrapped lines that the PR body's "each moved rule unchanged bar the edits above" does not cover, and the move was harder to check line by line. Its wrapping is main's again. That commit's message also said it refilled bullets this change added; it refilled ones the change edits as well, and this one it only moves. brief-a-reader-to-its-cells' Probe said "the budget, their sum", where "their" had no plural to refer to; it now says the budget is the shares' sum.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#1532's review ran past seven rounds, and #1542 traced what kept it going: defects in the previous round's own fixes, cells a reader named instead of reaching, slow commands, and calls that batched nothing while the main session's context grew. This puts each of #1542's ten Direction steps into the review and shipping skills as rule text, with no new script.
morphic-reviewing-prs/references/readers.md, new):xhigh, set on the call that starts it (start-readers-at-xhigh), gets a tool-call budget summed from its cells, and reaches every cell or says why it cannot (brief-a-reader-to-its-cells);reader-batches-its-calls), and bound slow commands by what they walk and build trial merges by the fast form (reader-bounds-its-commands);one-harness-directory);split-whole-reads-from-what-moved), and no round divides its files between readers (no-split-by-file).rounds.md):author-pass-before-hand-off, cited fromreview-own-fixes);compact-at-a-round-boundary);claims-and-process.md#run-prescribed-commands): a prescribed command also runs with each placeholder holding the hardest value it admits.commits-and-prs.md#scope-settled-by-the-second-round), and a trial merge is built as a worktree of agit commit-treecommit (merging-main.md#trial-merge-by-commit-tree, new) and confirmed by both its parents from the worktree that built it (merging-main.md#trial-merge-happened, which cites the build, so both trial-merge Probes reach the two through their citation of it).closing-issue-read-wholenamed a PR's issues fromclosingIssuesReferences, which a stacked PR leaves empty, and now names each issue a closing keyword in the body names, as the new scope rule does.verdict-outlives-unchanged-files' Probe diffed only the mutated file and the test, though its Rule also keeps the fixtures and the files between them; it now diffs all four and fails a path that matches no file, and the rule covers a commit as well as a merge, asauthor-pass-before-hand-offapplies it to a round's fix commit.Where this departs from the Direction as drafted:
fresh-reader-for-the-second-roundandreader-owns-its-directory, at 150 and 149 of their 150 words onmain, so each lands in a new rule the named one reaches by citation. Step 2 goes tobrief-a-reader-to-its-cells, a rule naming what a brief holds, as its Direction allows. Step 6, which asks for one rule or an edit ofreader-owns-its-directory, lands in two,reader-batches-its-callsandreader-bounds-its-commands, as one would pass 150 words.verdict-outlives-unchanged-files, which already names the plants a change can alter, a test, its fixtures and the code between a plant and its test included, rather than every plant.trial-merge-by-commit-tree, whichtrial-merge-happenedcites, rather than restated in each trial-merge Probe.closing-keywordshas it add whatever the round.fan-out-an-angle-to-a-readerandSKILL.mdnow name a split round's two readers, andfresh-reader-for-the-second-roundandlater-rounds-read-what-movedallow them, which all four would otherwise contradict.fan-out-an-angle-to-a-readeralso briefs each reader it starts bybrief-a-reader-to-its-cells, so a requested fan-out reaches the effort, batching and bounding rules too.The reader rules move from
rounds.mdintoreaders.md, the four already there with the new ones, asrounds.mdwould otherwise pass its 4,000-word reference cap. Each keeps its key, and every citation follows it.Closes #1542
Test plan
python3.14 .claude/skills/morphic-curating-skills/scripts/skills_lint.pyis clean. A citation of a moved rule planted at its old file drawsreferences/rounds.md holds no rule reader-owns-its-directory, so the lint holds every moved citation.main, keyed by rule rather than by file, shows each moved rule unchanged bar the edits above and its citations' file.<dir>holding a space, brackets and a plus, a conflicting merge stopping its chain, a<dir>still holding an earlier merge, of ci: minimize a fuzz input for 1s, not 10s #1385, which is stacked on ci: move the fuzz search from pull requests to a nightly job #1382, or of the same PR at an earlier head:--is-ancestorpassed the first and a second-parent check the second, and the both-parents check refuses each; and a merge built from a<head>ofHEAD, which the confirm accepts from the building worktree and would refuse inside<dir>;xhighfor each reader this PR's review started andunsetfor each of chore(skills): scope later review rounds, isolate their readers #1532's seven, whose own transcripts nonetheless recordxhigh;verdict-outlives-unchanged-files' Probe on a glob of fixtures, a path matching nothing, a file the head deleted, and files nameda[1].pyandb+c d.py: the glob diffs its files, the unmatched path and the deleted file failgit ls-files --error-unmatch, and the hard names pass;userContentEdits(last:1)query on this PR, chore(skills): scope later review rounds, isolate their readers #1532, a stacked PR and a PR whose body was never edited: the node it returns carries the PR's creation time, and an empty list leaves the body itself;gh issue viewasclosing-issue-read-wholeruns it, andlater-rounds-read-what-moved'sgit diff --name-onlyandgit grep -n '#<key>'.trial-merge-by-commit-treeandtrial-merge-happenedagainst every open PR: 15 merge cleanly, each with both heads as parents, a clean lint and no citation of a moved rule at its old file. The other 12 conflict, each already conflicting withmainand none in a file this change touches. The whole run took about 11 seconds.