ums: learnings from three review rounds and a cross-vendor pass on #2131/#2185 - #2207
Merged
Conversation
…2185 Five entries, from PR #2131/#2185's review lifecycle: - self-review-fallback.md + .cases.md: a CLEAN same-vendor verdict is a cross-vendor trigger, not only a failed one. Two same-vendor readings (11 pre-push adversarial rounds, and claude-review's "Ready for merge") cleared #2131 at b744d6a; codex then returned 11 findings, 8 blocking. Supplies the missing half of the existing case record, which names its own limit as covering a reviewer that was down rather than clean. - adversarial-self-review.md: a PR's visible review history is rationale you cannot withhold. The claude-review verdict named the prior rounds and the test file's round-N markers in its own justification. - least-flexible-tool.md: third occurrence of the regex-instead-of-lexer class, recorded per ums's recurrence step. Filed as #2189. - algorithmatize-checks.md: two more mutation misdiagnoses, both under outcomes already listed; count recorded, plus when to check an anchor. - fixtures-are-not-evidence.md: where the specification is a written rule, quote its literal text into the fixture. - claim-pr.md: second occurrence of the concurrent-session collision, with different parents rather than matching ones. Two adversarial review rounds ran pre-push; round 1 returned 11 findings and round 2 returned 10, all addressed.
…ree overreaching claims Round-3 adversarial review findings, all addressed: - least-flexible-tool.md: at 10909ff the regex was `-c\s`, which cannot match mid-word. The mid-word false positive was created by the widening to attached/= forms, and three rounds then patched the boundary. Restated as one widening forcing three narrowings, which is the same rule from the other side. - adversarial-self-review.md: nine verification steps, not ten (derived from the comment body, not restated from a reviewer's count); exact quotation where the prose said "in its words"; two bullets reworded to name observable actions rather than mental states. - self-review-fallback.cases.md: the case record asserted "why it probed no further", which the sibling section added in the same commit explicitly retracts; now scoped to the verdict's justification. A third live explanation (a different findings threshold) added. - algorithmatize-checks.md: the anchor-matched-nothing case belongs to the inapplicable-mutation outcome, not outcome four; Don't half added. - fixtures-are-not-evidence.md: cite the rule's file, not a volatile line.
…tion cases Round-4 adversarial review findings: - least-flexible-tool.md: cf195e4's parent is 10909ff, not 3b8d04e -- the two are concurrent siblings merged by 5c577a5, so no linear 'three rounds then patched' sequence exists. Three attempts at that narrative were each wrong in a new way. Rewritten to the claims that are derivable: the matcher's start state, its end state, the three lookbehind forms, and the count. Drops --request-changes from the false-positive pair, since COMMENT_FLAG_RE gates close/reopen and never sees a gh pr review segment. - algorithmatize-checks.md: the second instance is the parent entry's exception, not an example of it -- the mutation was fine and the suite lacked a discriminating fixture. Split, and the entry's ordering marked as a default the pass-condition entry's identity check settles. - self-review-fallback.cases.md: deduped a cross-reference stated twice in seven lines. - 'denies'/'contradict'/'by construction' softened to what the evidence carries.
…rop an inert evidence item Round-5 adversarial review findings: - least-flexible-tool.md: the 'order is unrecoverable' framing was a rationalization of three earlier failed narrations, and contradicted the same passage's own ordered account. The order IS recoverable in one command per commit; the entry now says to run 'git log -1 --format=%P' before narrating a fix history, which is the lesson four rounds actually taught. Count corrected to one writing plus two rewrites, and the intermediate unanchored form named, since the final regex correctly does not match the compliant command the passage cites. - claim-pr.md: '--comment=' was inert -- '--comment\b' already matches '--comment=x' because the boundary sits between t and '='. Executed both hook revisions to confirm. Replaced with the two verified items, and the inert one kept as the reason to execute rather than read both diffs. - self-review-fallback.md: 'depth in one family' imports learn-from-review-findings' term for a converging series' search space to describe a vendor blind spot, which spans families -- the cited record's own round markers span several. Reworded to the vendor's reachable region. - algorithmatize-checks.md: heading no longer calls both rows misdiagnosed, which the body supports for neither; the two discriminating questions are now stated once, in order. - adversarial-self-review.md: dropped an unsourced statistical claim. - self-review-fallback.cases.md: named the referent behind 'it answered twice over', and restored the justification-versus-effort scoping clause.
…x that actually warned Round-6 adversarial review findings. Five rounds each found a factual error in a different narration of which #2185 commit responded to which, while the count and the regex forms survived every round untouched -- so this applies the entry's own thesis to itself and changes the shape rather than tuning the same knob a sixth time. - least-flexible-tool.md: states what each revision WAS and drops what each was a RESPONSE to. The widened form is now 3b8d04e's actual '-c(?:\s|=|\S)', which is the only revision that warns on the compliant command (executed against all four); the previously quoted form shipped already anchored and matches nothing. The six rounds are recorded as the entry's second lesson, with 'git merge-base --is-ancestor' as the instrument, since 'git log --format=%P' reports immediate parents only and cannot separate a sibling from a descendant four commits down. - claim-pr.md: named the two commits instead of theirs/mine, and the Do bullet now says to execute each side rather than read its diff, which is the lesson the paragraph above it actually teaches. - algorithmatize-checks.md: one row was misread, not both; named the mutation instead of a pronoun whose nearest antecedent was the anchor. - self-review-fallback.cases.md: the threshold reading is not the only one with a cheap remedy.
…raming Round-7 adversarial review findings: - least-flexible-tool.md: 'went in to stop that' and 'a widening forced a run of narrowings' were the same causal claim the entry says it dropped, and git refutes it (cf195e4 branched from 10909ff, not from 3b8d04e). The entry now lists four forms and asserts nothing about which answered which. The self-referential paragraph claimed only the causal chain ever broke and named five prior narrations; both were false and both flattered the fix, so it is cut to the checkable core. Boundary form pinned to 15b63d9 rather than to a moving branch head, and the --is-ancestor bullet now names its operand order, which decides the answer. - claim-pr.md: 'their --comment= alternative' had cf195e4 as its nearest antecedent, which is the side that dropped it; named the commit and the direction. Deduped a clause repeated near-verbatim two lines apart. - self-review-fallback.cases.md: dropped a cost figure not sourced by anything the record links, and which carried no weight. - algorithmatize-checks.md: the new Do now names the sibling Do that prescribes the stronger remedy, and says why a check is still needed.
… produced five forms Round-8 adversarial review findings, the first two of them factual errors in the case record's headline: - self-review-fallback.cases.md: 'two same-vendor readings cleared that head' was false of one of them. The claim comment says eleven rounds ran before the FIRST push, and four substantive commits landed after it, three of them changing matcher or derivation logic. Only claude-review read b744d6a. Restated as one verdict at the head plus eleven rounds of same-vendor depth on an earlier state, which is what the record can support. self-review-fallback.md's 'eleven of them and one of them' inherited the same reading and is corrected with it. - least-flexible-tool.md: four edits produce five forms, and the passage said four forms while enumerating five. Separated the two counts. The meta-paragraph claimed the entry attributes nothing, while the sentence seven lines above attributes the quote-boundary form to the false positive its own commit message names -- a linear successor, so the attribution is sound and the meta-claim was the overreach. 'the layer that answers the question' hedged to 'would answer', since #2189 is a proposal and shlex.split raises on unbalanced quotes. --is-ancestor's exit status read three-valued, per errexit-is-not-uniform. - self-review-fallback.md: two framing overreaches, a scope claim over sentences that carry no trigger, and 'no artifact at all' over a case whose primary produced a full verdict comment.
Round-9 adversarial review findings: - self-review-fallback.cases.md: 'four substantive commits earlier' overstated the gap ~4x, in the record's own favour. The branch's commit messages name the state each round read (594fdce: 'One blocking finding on 5aa36bb'), so the last state read is 5aa36bb -- one branch commit (9 insertions, 4 deletions) plus the origin/main merge from the reviewed head. Recorded as the narrow gap it is, which strengthens the record rather than weakening it: the eleven rounds had substantially read the work codex found eight blocking defects in. Also fixed the cell's other defect, that eleven rounds read eleven different states so no single value could be right; and disambiguated 'readings' from 'explanations'. - least-flexible-tool.md: 'where a token begins' is refuted by the passage's own second edit, which asks what may follow the flag; widened to 'boundaries'. Attribution sentence given its real subject. - self-review-fallback.md: the narrowed scope claim still read as 'every trigger-naming sentence prescribes cross-vendor', which is false of three paragraphs above it that prescribe a self-review. - algorithmatize-checks.md: named ANY_BODY_FLAG_RE instead of a pronoun whose nearest antecedent was POST_RE. - claim-pr.md: 'no merge in it at all' is false one commit back on the 3b8d04e side; scoped to between the two fixes.
Round-10 adversarial review findings. The reviewer's closing observation is the fix: across ten rounds each repair to a sentence broke it on a new axis, because the sentences were carrying more precision than the argument needed. - least-flexible-tool.md: 'linear successor' is false -- every path from cf195e4 to 9336348 runs through the 5c577a5 merge, which the same paragraph names. Ancestry is what the attribution needs and what the entry's own Do bullet tests; --is-ancestor exits 0. Also disambiguated 'the preceding quote', which read as a quotation given earlier. - self-review-fallback.md: dropped the four-item appositive, which omitted the 'no review workflow configured' failure mode and listed 'contradicting itself', which the passage above names in order to exclude it. The head claim is true and self-sufficient. - claim-pr.md: 'no merge between the two fixes' is still false -- ed03e06 is a main-merge inside the divergence on the 3b8d04e side, and is why the parents differ. 'Matching parents nowhere in sight' is the contrast the section actually draws. - self-review-fallback.cases.md: 'reading' carried a review-pass sense and an interpretation sense five lines apart; the latter is now 'explanation' throughout. Named the noun behind 'Its remedy'. And the codex inference is now derived rather than hedged: every file codex raised a blocking finding in is byte-identical at 5aa36bb and b744d6a.
Round-11 adversarial review finding. The hand-typed list of files codex raised blocking findings in did not match what #2177 records, in both directions: it omitted shared/workflow/disclose-agent-authorship.md and skills/rescue-closed/SKILL.md, and included AGENTS.md, which #2177 names as the authority being violated rather than as a site. The conclusion happened to survive because both omitted files are also unchanged, which is a wrong derivation reaching a true conclusion by luck -- the exact shape the entry around it warns about, and the class round 10 was weakening sentences to eliminate. Replaced with the intersection, published as the command that computes it: the files #2185 changed, against the files that moved between the last state the rounds read and the branch's own last commit. Empty. Only the origin/main merge touches any of them, and only hooks/hooks.json, from another PR's work rather than this branch's.
Round-12 finding: 'the files #2185 changed to address those findings' is a restrictive clause that is false of at least one member -- #2185 also carries an unrelated preferences.md learning. The set is #2185's whole diff, which is deliberately wider, and width is the conservative direction when the result is an empty intersection. Said so. Also strengthened the merge leg's warrant. It rested on authorship ('somebody else's work'), where what the conclusion needs is that no line the rounds read changed. 'git diff 5aa36bb b744d6a -- hooks/hooks.json' removes zero lines, which supplies it directly.
Collaborator
Author
|
Working on this --- paws off until I'm done. |
CI's check-new-line-breaks runs with NLB_CLAUSE_BREAKS=true and NLB_CLAUSE_MIN_LENGTH=80, which flags a line of 80+ characters carrying a mid-line semicolon. scripts/semantic-line-breaks.py does not implement that rule and in fact wants these eight lines rejoined, so the local tool cannot be run over this diff without turning the check red again. CI is the authority here; validate.yml runs check-new-line-breaks against content and runs semantic-line-breaks.py only through its own unit tests.
Collaborator
Author
|
One finding from a verification pass on this branch, which survived into the current head
That prose line is the only occurrence of the string
Verified as pre-merge rather than merge-caused: Worth naming because it is the class the entry is about: a claim about corpus state written from what was in the author's working tree rather than from a query against the repo. Two options --- cite what exists on Everything else in that pass was clean, and the clean results bound the risk: the Recorded alongside a prose sweep on the same branch at #2191 --- worth checking which of those 24 findings the twelve review rounds already caught, since that sweep ran before them. Posted by Claude Code (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…a wrong direction claude-review round 1 on ff5a9da, both findings verified against the repo: - least-flexible-tool.md: the entry opened with a present-tense claim that COMMENT_FLAG_RE decides the close/reopen comment gate. It does not exist on main -- 'git grep COMMENT_FLAG_RE origin/main' returns nothing, and its introducing commit 10909ff is not an ancestor of main. It lives only on #2185, still open. Once this PR merges, a reader grepping the codebase for the construct the entry describes finds nothing. Scoped to that branch and dated, per timestamp-volatile-claims. - self-review-fallback.cases.md: 'in the section linked below' pointed forward at nothing -- the file ends three sentences later, and the adversarial-self-review link it meant sits two paragraphs above. Named the fragment and the direction, per forward-references.
Collaborator
Author
|
Resolved --- fixed in Worth keeping the diagnosis, though, because it is sharper than the fix: twelve pre-push rounds and a cross-vendor pass all verified the branch facts, and none asked whether the branch existed on That is It also explains why the round count did not help: thirteen readers all making the same substitution converge faster, not slower. That is the same shape as the same-vendor blind spot this PR documents, one level up --- the agreement measured the shared frame of reference rather than the claim. Posted by Claude Code (AI agent) --- not written by a human. |
Contributor
|
Claude finished review — View run Working tree is clean (only the pre-existing untracked All four independent review passes — two CLAUDE.md-compliance agents and two Opus bug-scan agents — returned zero findings, and this round went further than a typical pass: they actually executed nearly every checkable claim in the diff (regex behavior, Review of PR #2207Prior round's findings, verified fixed. The last review (on commit This round's findings: none. Four independent passes (two CLAUDE.md-compliance, two Opus bug scans) covered:
This is a pure-prose UMS documentation pass with no application code changes, and it holds up under an unusually thorough fact-check — nearly every specific, checkable claim in it was actually executed rather than taken on faith, and none failed. VerdictReady for merge. No new issues found; the two findings from the prior review round (commit Reviewed commit: 60c5b38 |
Contributor
d-morrison
added a commit
that referenced
this pull request
Aug 25, 2026
…2212) * ums(post-merge #2207): record the same-repo citation trap as a fifth occurrence Post-merge UMS top-up. #2207's own claude-review found that its prose described COMMENT_FLAG_RE in the present tense while the construct exists only on unmerged #2185. That is a fifth occurrence of the trap challenge-ambiguous-terminology.md already documents (gha#151 twice, #208, #217), and CLAUDE.md already says that section covers a same-repo sibling PR unchanged -- so this records the recurrence rather than opening a new shape, which a first draft wrongly did in verify-the-right-artifact.md. - The increment is why thorough review misses it: every reviewer verifies the claim against the branch it is ABOUT, which is correct for its truth, so it passes -- and the more carefully that branch is checked, the more settled the sentence looks. Nobody asks which artifact the READER lands on. - Corrected the section's own 404 claim at source. It read as though a link checker catches a premature cross-repo citation; on all four measured cases it could not, for two different reasons -- a backticked path is not a link, and the repo-root link resolves. - The Do bullet settles merge state from the PR's own state or an empty path-scoped git diff, per pr-on-claim. NOT git merge-base --is-ancestor: a review round demonstrated it exits 1 for #2205, #2202 and #2201, all merged, because this repo squash-merges and the squash commit excludes the branch's commits. pr-on-claim documents exactly that, and a first draft prescribed the broken check anyway. - least-flexible-tool.md: dropped a self-refuting clause #2207 shipped, which said `git grep` finds the constant nowhere on main while itself putting the string on main. Deferred to #2210: broadening reorganize-prose.md's content-preservation check, which is gated on a MOVE and so never fires on a scripted whole-file rewrite that drops the tail. A first draft of that entry was cut in review; the issue records what it got wrong. * ums: drop a forward reference, and name the two passes that raised the finding claude-review round 1 on 4112c6c, both findings addressed: - "as the measured cases below show" pointed forward, which forward-references.md says is not excused by pointing accurately. Now "on any case measured here". - "Two separately-dispatched same-vendor passes then raised it nine minutes apart" was accurate but unnameable, so the reviewer could not check it and flagged it as a possible inaccuracy. The two are a session verification pass at 05:27:25Z (comment 5405924657) and claude-review at 05:36:44Z -- 9m19s apart, both raising COMMENT_FLAG_RE. Named both, and said why the count looks wrong from the outside: the LATER claude-review round at 05:46:39Z reports zero findings and Ready for merge, so a reader comparing only the two claude-review comments sees one raise rather than two. That is exactly the reconstruction the reviewer attempted. * ums: name the constant before referring to it claude-review round 2 on 7b52990. The round-1 fix for a self-refuting clause introduced a forward reference in the same slot: "so the constant below is defined on that branch only" pointed ahead to COMMENT_FLAG_RE, which the sentence did not name until after the em-dashed aside. Split into two sentences so the constant is named first and its provenance second. Third pass over this one clause -- self-refuting, then forward-pointing, now neither -- which is the corpus's own "a later round can find a defect in the fix" running on a single sentence.
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
1 participant
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.
UMS pass banking learnings from PR #2131 / #2185's review lifecycle: three external review rounds plus a cross-vendor (codex) pass.
Six candidates were assessed. Four produced new entries, two were recorded as recurrences on entries that already covered the class, and none was written where the corpus already said it.
New entries
shared/workflow/self-review-fallback.md+.cases.md--- a clean same-vendor verdict is a cross-vendor trigger, not only a failed one.Every existing sentence in that fragment reaches for the cross-vendor reviewer when the primary fails. Nothing fired on the case where it answers and finds nothing.
claude-reviewreturned Ready for merge on #2131 atb744d6a2; acodexpass on that head returned 11 findings, 8 blocking, all verified real. The existing case record (#1895) names its own limit --- its primary produced zero verdicts, so it is evidence about a second reading rather than about when to reach for one. This supplies that missing half.shared/workflow/adversarial-self-review.md--- a PR's own review history is rationale you cannot withhold.That fragment already says never to brief a reviewer with your account of the change. The PR itself is a second channel you never chose to open: the claim comment's round count, and
# round-Nmarkers left in the test file. Theclaude-reviewverdict named both in its own justification for finding nothing. Scoped carefully to what is observable --- the history entered the justification --- rather than to reduced effort, which its nine enumerated verification steps weigh against.shared/workflow/fixtures-are-not-evidence.md--- where the specification is a written rule, quote its literal text into the fixture.The neighbouring section covers a fixture agreeing with a code bug. Here a fixture asserted that
_Posted by Codex (AI agent) -- not a human._discloses;AGENTS.mdsays substitute the name and keep the rest verbatim, so the tail was rewritten, not appended. It passed only becauseMARKER_REanchored a prefix and examined nothing after it.shared/workflow/claim-pr.md--- second occurrence, with different parents.Both branches of the existing rule turn on the parents matching. Two sessions fixing the same review round five minutes apart share only their base. Each side had a fix the other lacked, so either reset would have shipped a regression no check could see.
Recurrences recorded on existing entries
shared/coding/least-flexible-tool.md--- third occurrence of the regex-instead-of-lexer class, perums's recurrence step. Filed as #2189.shared/workflow/algorithmatize-checks.md--- two moreMISSEDrows read as coverage gaps, both under outcomes already listed. One is the entry's thesis and one is its exception, which is the half worth separating.Review
Twelve adversarial rounds ran pre-push, every one finding something real. Most of them landed on two passages --- the
least-flexible-tool.mdworked example and the case record's measurement --- and the pattern that ended it was the reviewer's own observation that each repair broke the sentence on a new axis, because the sentences claimed more precision than the arguments needed. Three were weakened to true statements and a hand-typed file set was replaced with the command that derives it.Everything is prose under
shared/.check-links.py,markdownlint-cli2,semantic-line-breaks.py,git diff --check, and a non-ASCII scan are green.Refs #2177.