Repository navigation
chore(skills): scope later review rounds, isolate their readers - #1532
Merged
Merged
Conversation
Review rounds after the second reread the whole change and re-derived its mechanical facts by hand, so each cost about as much as the first: on #1522 each later round's reader spent about 240,000 tokens. A delegated reader also had no directory of its own, and on one review it rewrote the reviewer's go vet harness. facts.py prints, a line each, the facts a round settles: each rule added, removed, renamed, moved or reworded, the lines citing it, the commands edited rules prescribe, the numbers the change and the body state, the issues the PR closes, the open PRs sharing its files and whether they merge, and, with --since, what moved since the tree an earlier round reviewed. The first two rounds still read the whole change. From the third, the fresh reader runs every angle over what moved, each fix, each cell left unreached or reopened by an audit of what a reader cleared, and the body. A defect in a filled cell that nothing changed sends the next round back over the whole change. A delegated reader writes only in a directory of its own, and a PR body states the head instead of logging rounds.
The review worktree merges both a stacked PR's base and origin/main, so reading the change from its merge base with the stack's base counted main's commits as the PR's own whenever that base lagged main: on such a PR the script printed rules, commands, numbers and shared files that were main's. The change is now read against what the head holds of the base merged with what it holds of main. Where the two conflict, the conflicted files are named as not read, and the head's resolution of them counts as the change's. The change's last commit, which dates a late issue update, is likewise its own, neither base's. A conflict another PR has with either base is labelled with each base it shares. A closing keyword before this repository's owner/name#N closes, as GitHub reads it. Tests now hold the branches no earlier test reached: a same-file citation outside its rule, a path that cites no rule, renames in each diff, fences indented under a bullet, a merge newer than the change's own last commit, and a held head with no pull ref. The rules state where a delegated reader may write without forbidding the objects a ref-free fetch adds, let a later round's brief name the cells to read, and give the --since command whole. CLAUDE.md's review line ends on the same condition review-ends-with-nothing-left sets.
With --since, facts.py printed every fact of the whole change, while the rules have a later round read only what moved and count each unsettled line as unreached, so a third round could not end without rereading the change. Its rule, cite, command and number lines now describe what moved since the reviewed commit, and its issue lines only the issues updated after it; the stale lines keep the files and the files they pair with. A file the stack's base and main conflict in is now left out of every fact, rather than read from conflict markers, and named as not read. The issues GitHub lists a PR as closing count only when they belong to this repository, whose owner and name the issue query now takes from the same remote. Tests hold each branch a planted defect showed unheld: no origin remote, a head behind main, a stack commit after the change's own, a dotted repository name, an issue updated at the reviewed commit's second, and a reference spelled in another case. The fresh-reader rule audits a reader's clearances as the fan-out rule does, a reader may add objects and worktrees of its own, and CLAUDE.md's review line names reopened cells beside unreached ones.
With --since, facts.py left out every closing issue last updated before the reviewed commit, but linking a PR to an issue leaves the issue's update time alone, so an issue the body began to close after that round was never printed and its asks never read. It now reads the body as it stood at the reviewed commit from the edits GitHub keeps, each a whole body, and leaves out only the issues that body already closed and that were not updated since. A PR opened after the commit, or a body file given before the PR exists, has every issue read anew, and so does a history longer than one page, which is named as not read. Under --since, a file the stack's base and main conflict in was marked as outside the change even when the PR edits it. It is now marked as where the two conflict, and its moves since the round are still read, as they differ between two resolved trees. Every gh call now names origin's repository, its owner and name passed as strings, so a repository named by digits is not read as a number. Tests hold GitHub's references from the same owner or of the same name, a remote naming no repository, the order of a body's edits, an edit at the reviewed commit's second, and a body file read since a round.
skills_lint now holds each rule a Markdown or Go file outside the skills cites by its full path, as CLAUDE.md does, to the rule its file holds; before, a renamed or removed rule left such a citation pointing at nothing with every check green. plant-in-a-throwaway-worktree restores with reset --hard and clean -fd, as a checkout left a git mv plant staged into the next verdicts. review-without-origin sets GH_REPO, which gh and its placeholders read without a remote, and takes a local clone where the repository must stay unwritten, as an unpacked copy holds none of the branch's history; the copy's own rule now says it serves a plant alone. moved-comments also reads the doc.go of each package code moves into. The assert.Equal departure's Probe reads each compared type with go doc -u, as no grep of names can see whether a value holds an unexported field. Six facts a review's verdict rested on unchecked each get a rule: a surviving plant is shown to fire, a table row's kill is run alone, a cited plant gives its literal edit, a recorded decline's fact is disproved like a finding, a generator's exclusions are held to what admits the input, and a mechanism that turns on a type is swept with the types.
facts.py printed facts that each already had a rule and a one-line command: the commands an edited rule prescribes, the numbers to derive, the issues a PR closes, the open PRs sharing its files, and what moved since a round. Its edge cases, stacked bases, conflicted merges and GitHub's API among them, had grown it to some 600 lines with 900 of tests that no CI runs. It is removed, and a later round now reads each file both git diff --name-only <merged> HEAD and the change's own diff list, with the lines citing a rule changed there. The curating skill's bar for a script now asks that no one command lists the set it checks, which is the bar facts.py should have met.
skills_lint read only Markdown and Go files outside the skills and left testdata out, so a citation in a YAML or Makefile comment, or in a hand-written keyed document under testdata, went unchecked. It now reads every tracked file outside the skills bar a binary one, and passes over unread a file that never names a skill's path. Tests hold a file under .claude beside the skills, a byte no encoding reads, and the order of the findings. The moved-comments Probe filtered with AM, which drops the rename a whole-file move between packages shows as, so it named no receiving package; it filters with AMR. The assert.Equal departure's Probe now names the go/packages sweep its neighbour rule describes, as go doc cannot read a type a test file declares, and that sweep first reports a site planted for it. rebuild-lost-plants no longer presumes a body cites plants by count. A row run alone is quoted so that a quote in its name survives, and a local clone for review switches to the PR's head.
a-kill-in-a-row-run-alone gave its row in double quotes, where the shell expands a $ in the name, and 47 row names in the tree hold one; the run then reached no test and read as a pass. The pattern is now anchored and single-quoted, its row spelt as -v prints it, and one === RUN line must name the row. The typed sweep's Probe first reports a site planted for each form the mechanism takes, nested and behind a pointer too, and passes a planted near miss, which the assert.Equal departure's Probe, now naming require.Equal as well, relies on. rebuild-lost-plants' Probe leaves out the file the condition is in, and testing.md re-plants a PR's listed mutants from their literal edits where the body gives them. A test now holds two citations on one line, and the outside-citation pass's docstring says it reads each file the tree holds.
The lint's test of citations outside the skills gave each citing file a single citing line, so a scan that stopped after a file's first citing line passed it. The fixture's CLAUDE.md now cites on a second line too, and that plant fails the test. Three Probes handed literal text to git grep as a regular expression: the condition rebuild-lost-plants greps for, the old name that sweep-old-name-after-rename-or-delete greps as a plain substring, and the old values widened-field-values greps beside a field. A condition holding a backslash and a name holding a bracket matched nothing, and a media type holding a plus was refused; each Probe now passes -F. The assert.Equal departure's Rule names require.Equal beside assert.Equal, as its Probe and #1534 do, and CLAUDE.md's testing line says testify's Equal. The message of dda91d6 counted 47 row names holding a $. No command derives that number, so it is withdrawn; the fix needs only that such names exist, as the #750 row shows.
The last commit reworded CLAUDE.md's testing line to name testify's Equal, which made that line conflict with #1381, an open PR that rewrites it. The departure in morphic-checking-style, which names both assert.Equal and require.Equal, holds the rule's detail, so the line returns to main's wording here.
The departure's Rule and widened-field-values' Probe each gained a line past the 100 columns their files wrap at, where no code span forces it, and both are rewrapped. The Why of later-rounds-read-what-moved read #1522's figure of about 240,000 tokens as what a round spent, where it is the context each reader ended holding. The Why argues where the findings sat, so the figure goes.
OmarAlJarrah
added a commit
that referenced
this pull request
Nov 6, 2026
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.
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
Review rounds after the second cost as much as the first, because each reread the whole change. On #1522, a change of 41 files and later 42, each later round's reader reread all of it and ended holding about 240,000 tokens of context, as #1530's table records. Separately, a delegated reader had no directory of its own, and on one review it rewrote the reviewer's
go vetharness. This change makes a round after the second read what moved since the round before, and gives each reader a directory of its own. It also settles the six other skill issues open beside them.Rounds (
morphic-reviewing-prs/references/rounds.md):first-round-finds-everythingkeeps the first two rounds over the whole change.later-rounds-read-what-moved: from the third round on, the fresh reader runs every angle over a narrower set:git diff --name-only <merged> HEADand the change's own diff list, where<merged>is the commit the round before reviewed;missed-cell-widens-the-round: a defect found in a cell that an earlier round filled, with nothing changed there since, sends the next round back over the whole change. That rule also states the limit this leaves: a miss in a filled cell nothing changes stays until a reader meets it.audit-what-a-reader-cleared: the main session reads each cell a reader cleared against what its rule asks, and reopens one cleared on less, such as a mutant judged equivalent on this tree's inputs alone, or an issue read in part.review-ends-with-nothing-left: the review ends on a round with noissue:ornit:and no cell unreached or reopened.CLAUDE.md's review line now ends on the same condition and cites the rule.Readers (
reader-owns-its-directory): a delegated reader's brief names a new directory only it writes, holding a detached worktree of the merged tree. It forbids writing anywhere else, the repository's refs included, bar the objects and worktrees the reader adds. The reviewer fingerprints each harness it will rerun before the hand-off, and rebuilds one whose fingerprint changed.fresh-reader-for-the-second-roundandfan-out-an-angle-to-a-readercite it, and a later round's brief carries the names of the cells to read and nothing else from an earlier round.PR bodies (
morphic-shipping-prs,body-states-the-head): a body states the head's Summary and Test plan. What a review round fixed goes in that round's commit messages, not into a growing log in the body.The other open skill issues:
skills_lint.pynow holds each rule a file outside the skills cites by its full path, asCLAUDE.mddoes, to the rule its file holds. It reads each file the tree holds, as the lint already lists them (each one git lists, untracked ones included and ignored ones not), bar a binary one, and testdata too, as testdata holds hand-written keyed documents. Until now no check held these citations.plant-in-a-throwaway-worktreerestores withreset --hardandclean -fd, which undo agit mvplant that acheckoutleft staged.review-without-originsetsGH_REPO, whichghand the placeholders ofgh apiread without a remote. Where the repository must stay unwritten, the review takes a local clone, which holds the branch's history, and switches it to the PR's head.plant-in-an-unpacked-copysays the copy serves a plant alone.moved-commentsalso reads thedoc.goof each package that code moves into. Its Probe filters the change's files with--diff-filter=AMR, so a whole-file move between packages counts.assert.Equaldeparture now namesrequire.Equalbeside it, as skills: the assert.Equal departure's probe cannot see the unexported fields it rules on #1534 counts both, and its Probe names thego/packagessweep thatsweep-a-typed-mechanism-by-typedescribes. That sweep first reports a site planted for each form the mechanism takes, nested ones included, and passes a planted near miss, as skills: the assert.Equal departure's probe cannot see the unexported fields it rules on #1534 asks. No grep of names can see whether a value holds an unexported field, andgo doccannot read a type a test file declares. An analyzer ininternal/lintruleswaits on test: assert.Equal compares values holding unexported fields, which the departure bars #1533, as skills: the assert.Equal departure's probe cannot see the unexported fields it rules on #1534 sequences it, and the skills' scripts, in standard-library Python, cannot read Go's types. The tree's existing calls stay with test: assert.Equal compares values holding unexported fields, which the departure bars #1533.CLAUDE.md's testing line keeps namingassert.Equalalone: test: move fuzz targets to packages that link no test tooling #1381, an open PR, rewrites that line, and the departure holds the rule's detail.a-survivor-needs-a-plant-that-fires);a-kill-in-a-row-run-alone);cite-a-plant-by-its-edit);disprove-a-decline, whichread-standing-reviewsnow cites);generator-exclusions-are-claims);sweep-a-typed-mechanism-by-type).Probes that grep literal text now pass
-F:rebuild-lost-plants,sweep-old-name-after-rename-or-deleteandwidened-field-values. A condition, a name or a value can hold a character a regular expression reads, such as a backslash, a bracket or a plus, and then matches nothing or is refused.SKILL.mdroutes to these rules, and an angle the change cannot reach is now settled without reading its reference.Why later rounds can read only what moved
#1530 asked whether scoping a round to its fixes would lose defects. I measured that against #1522's own review. For each
issue:ornit:that rounds 3 to 5 found, I asked whether the round before had changed its file or a rule it cites, and whether an earlier report had cleared or skipped it. The four reviewers' reports were the record.-Bclaim and the bytecode rule inmutation.md(fix-introduced); the body's list of PRs sharing files; the checklist's base, reached through the changedtest_subjects.py; twoskills_lint.pytest gaps, reached through the changedtest_skills_lint.pynormpathmutant judged equivalent "on this tree"; #1266's asks cleared on lines cut at about 450 characters--max-same-issuescommand; themerges secondsweep; the last-base-only mutant; the Probe counting mergesThe first three rows hold 12 of the 13, and scoped rounds keep those once members are enumerated and clearances audited. The 13th is the limit
missed-cell-widens-the-roundstates. A review that keeps finding such misses falls back to rereading everything, because a defect in a filled cell escalates the next round to the whole change.Where this departs from #1530's direction
run-prescribed-commands,sweep-each-claim-class, thegit grep -n '#<key>'oflater-rounds-read-what-moved, the trial-merge rules andmoved-prose. What moved since a round isgit diff --name-only. A script printing them would have to handle stacked bases, conflicted merges and GitHub's API itself, and no CI runs a skill script's tests. The curating skill's bar for a script now asks that no one command lists the set it checks. Step 1's planting each script fix beside an unplanted control stays with the Testing angle, andmutation-controlsalready requires the control.git diffagainst it gives what moved, so no hand-kept ledger is needed.-Bfalse, and round 4's found that the copy its fix prescribed failed a test. Every fix still goes to the next round's fresh reader; what shrinks is what that reader reads.CLAUDE.md's one-logical-change rule stands.Further changes to how a round is briefed, staffed and run, measured on this PR's own rounds, are #1542, for after this merges.
Test plan
python3.14 .claude/skills/morphic-curating-skills/scripts/skills_lint.py→ clean.python3.14 -m unittest discover -s .claude/skills/<skill>/scripts→ OK for the three skills with scripts.go test ./internal/archtest -run TestDocuments -count=1→ ok, as it readsCLAUDE.md.skills_lint.py's new check: a bad key planted inCLAUDE.md's citation ofreview-ends-with-nothing-leftdrawsCLAUDE.md:559: ... holds no rule review-ends-nowhere, and the tree with it put back is clean. Twelve plants incheck_cited_from_outsideand the code it calls, in.claude/skills/morphic-curating-skills/scripts/skills_lint.py, each from a copy with no__pycache__, run with-Bbeside an unplanted control that passes:self.check_cited_from_outside()deleted: killed;if rel.startswith(".claude/skills/"):→if rel.startswith(".claude/"):: killed;if False:: killed;if b"\0" in data or b".claude/skills/" not in data:→if b".claude/skills/" not in data:: killed;data.decode(errors="replace")→data.decode(): killed;self.known = sorted(known)→self.known = list(known): killed;if path.startswith(".claude/skills/"):→if True:: killed;if why or f"\n`{key}`\n" not in "\n" + held.read_text():→if why:: killed;split("\n"), 1):→split("\n")):: killed;breakadded afterself.check_cite(...): killed;if RULE_CITE.search(line): breakadded at the line loop's level, which stops a file's scan at its first citing line: killed;if b"\0" in data:: equivalent, as a citation the pass counts holds.claude/skills/in its file's bytes.go/packagesprogramsweep-a-typed-mechanism-by-typedescribes (test: assert.Equal compares values holding unexported fields, which the departure bars #1533's scan, run from a throwaway worktree's root) reports the calls planted for it:assert.Equalon an unexported field,require.Equalon one nested through a map, a slice and a pointer,assert.Equalon a type with anEqualmethod, andassert.NotEqualon an unexported field. It passes a plain struct and a plain map, and finds 117 existing calls beside them;go test -v -run '^<Test>$/^<row>$'on a row nameda_$ref_to_a_property's_schema_before_the_model_declaring_it_(GitHub_#750), escaped as the rule says, runs that row alone, with one=== RUNline naming it;reflect.DeepEqualgrep exits 1, finding none;git grep -lFfinds the conditionif b"\0" in data or b".claude/skills/" not in data:inskills_lint.py, where-lalone, reading it as a regular expression, exits 1;git grep -nF 'map[string]int' -- '*.go'lists 112 lines, where the regular expression lists none;git grep -nF -e Framings -e '+json-seq' -- docsfinds both+json-seqlines, where-Erefuses the pattern;reset -q --hard && clean -qfdrestores a tree aftergit mv, leavingstatusempty;git diff --name-only --diff-filter=AMRnames the file a whole-file move between packages brings, whichAMleaves out;git clone,update-ref refs/remotes/origin/main <base>andswitch --detach <head>each exit 0, leaving a head that holdsorigin/main;GH_REPO=dexpace/morphic gh api 'repos/{owner}/{repo}'resolves outside any repository;find <harness> -type f -exec cksum {} + | sort | cksumchanges when a harness'sgo.modis rewritten;git diff --name-only <merged> HEAD, intersected with the change's own diff, leaves out the files a merge ofmainbrought in.git merge-tree --write-tree, and the skills lint is clean on every merge tree, conflict markers left in place. Six share a file with this one: ci: run coverage in set mode and -race in a run of its own #1311, feat(irfacts): derive a correlation key for every entity #1354, ci: reuse test results across checkouts, keep main's cache warm #1373, test: move fuzz targets to packages that link no test tooling #1381, ci: move the fuzz search from pull requests to a nightly job #1382 and test(lintrules): refuse writes to pipeline package variables #1447. Twelve conflict, each only in files it also conflicts in againstorigin/main, none of them a file this PR touches; of the six, only feat(irfacts): derive a correlation key for every entity #1354 does, in sixdocs/irfiles.gh issue list --state all --limit 1000 --searchon review rounds, readers, briefs, PR bodies andmorphic-reviewing-prsreturns no open issue these rules settle other than the eight closed here.Closes #1529
Closes #1530
Closes #1534
Closes #1537
Closes #1538
Closes #1539
Closes #1540
Closes #1541