ums: three findings from a five-PR session - #1008
Conversation
Records what the merged run of #996/#999/#1003/#1004/#1005 taught that none of those five captured, plus one candidate declined. memories/github.md: list_pull_requests reports merged: false on every row, merged ones included, so merged_at is the field that discriminates. Verified across all three PR states and shown not to be an artifact of the fields projection. memories/preferences.md and skills/gip/SKILL.md: isolation: "worktree" needs the session's own cwd to be inside a git repository, which the remedy added by #1005 states without. Both places a reader meets that remedy now carry the precondition and the explicit-worktree fallback. shared/workflow/ardi.md: naming the specific gate when reporting a blocker. The neighbouring rule covers whether something is blocked; this covers why, and fires after that rule's remedy has already succeeded. Declined: a one-off review-cost delta, which is evidence for an upstream issue rather than a durable corpus learning.
c54da79 to
05d9f32
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| `git -C <repo> fetch origin main`, then | ||
| `git -C <repo> worktree add <path> main`. | ||
| Two details decide whether that fallback is actually clean. | ||
| Use `git -C` rather than `cd`, per the persisting-cwd bullet further down. |
There was a problem hiding this comment.
Forward reference, per this repo's own forward-references.md rule (cited from CLAUDE.md's "Remove forward-pointing phrases from prose, not just crossref divs" section).
"per the persisting-cwd bullet further down" points at content that appears later in the file (the "Bash's cwd PERSISTS across separate calls..." bullet, ~22 lines below) — the reader hasn't reached it yet at this point in a linear read. CLAUDE.md explicitly bans this pattern: "plain-text signposting — 'as discussed below', 'in the following section', 'we'll cover this later' — pointing at content the reader hasn't reached yet, in any prose."
Since the referenced bullet is pre-existing content the diff doesn't touch, the cleanest fix is to inline a brief, self-contained rationale rather than pointing forward:
| Use `git -C` rather than `cd`, per the persisting-cwd bullet further down. | |
| Use `git -C` rather than `cd` -- Bash's cwd persists across separate calls | |
| in a session, so a `cd` here would silently carry into later commands. |
There was a problem hiding this comment.
Addressed in 8105b6c.
Verified first: the target is the Bash's cwd PERSISTS bullet, which sat 22 lines below the reference, so a linear reader genuinely has not reached it. That is the pattern CLAUDE.md bans.
Took your suggestion essentially verbatim, inlining the rationale rather than pointing forward:
Use `git -C` rather than `cd`, because Bash's cwd persists across separate
calls in a session, so a `cd` here silently carries into later ones.
It now reads as one of the reasons the fallback command is spelled the way it is, alongside the origin/main and --detach reasons from the finding below, so the paragraph no longer depends on anything downstream.
Generated by Claude Code
| The rule is not wrong, it is stated without its precondition, in both places | ||
| a reader meets it: the bullet directly above, and | ||
| [`gip`](../skills/gip/SKILL.md)'s "give **every** subagent | ||
| `isolation: \"worktree\"`". |
There was a problem hiding this comment.
Rendering bug, not cosmetic: backslash escapes don't work inside Markdown code spans. Per the CommonMark spec §6.1: "Backslash escapes do not work in code blocks, code spans, autolinks, or raw HTML." So `isolation: \"worktree\"` will render literally as isolation: \"worktree\" (with visible backslashes), not isolation: "worktree" as intended.
This is inconsistent with every other occurrence of the same phrase in this file and in skills/gip/SKILL.md (lines 277, 306, 343 here, and SKILL.md lines 102/251), all of which write it unescaped. It's also misquoting gip/SKILL.md's actual text, which reads isolation: "worktree" with no backslashes. The escaping isn't needed at all here — the enclosing "..." are plain prose quotes with no Markdown meaning, so the nested " inside the code span needs no escape.
| `isolation: \"worktree\"`". | |
| `isolation: "worktree"`". |
There was a problem hiding this comment.
Addressed in 8105b6c.
Rendered it rather than taking the spec citation on trust, since the finding is a claim about output:
$ pandoc -f commonmark -t plain esc.md
gip's "give every subagent isolation: \"worktree\"".
$ pandoc -f gfm -t html esc.md
... <code>isolation: \"worktree\"</code>".</p>The backslashes survive into both plain text and HTML, exactly as you said, and skills/gip/SKILL.md:102 really does read isolation: "worktree" unescaped -- so it was misquoting its own source. The unescaped form renders isolation: "worktree", matching that line character for character.
Also confirmed the inconsistency claim across the file: preferences.md lines 277, 306, 319, 343 and SKILL.md 102/251 are all unescaped, and line 329 was the only body-prose exception. The only other backslash-escaped quotes in the corpus are inside YAML frontmatter description: strings, where the escape is required and correct -- so nothing else needed touching.
Generated by Claude Code
| `git -C <repo> fetch origin main`, then | ||
| `git -C <repo> worktree add <path> main`. |
There was a problem hiding this comment.
The prescribed fallback command routinely fails on the exact branch name it hard-codes. git worktree add <path> main refuses with fatal: 'main' is already checked out at '<other-path>' whenever main is already checked out anywhere else — which this repo's own git worktree guidance elsewhere already documents (memories/git.md: "Plain git checkout main in a linked worktree correctly refuses when main is checked out in the primary (or any other) worktree").
Two consequences:
- This corpus's own session-start convention actively triggers it.
CLAUDE.md's "Keep ai-config and repo checkouts fresh" point 1 says to check that the local clone "is onmain" — which is exactly the state that makes even the first invocation of this fallback fail. - The
gipfan-out use case is defeated entirely.skills/gip/SKILL.md:114uses this identical command to "create each worktree explicitly" for "every" subagent — but since the branch name is always the literalmain, only the first worktree can succeed; every subsequent one collides and errors.
Every other worktree instruction elsewhere in this corpus uses a new branch or --detach instead of reusing a shared branch name (e.g. memories/preferences.md:268: worktree add -b <branch> ... origin/main; skills/ums/SKILL.md:178: worktree add -b "ums-<topic>" ... origin/main; skills/cascade/SKILL.md:43: worktree add --detach <dir>). This new fallback is the only one that names a shared branch literally.
Suggested fix (also applies to the identical command in skills/gip/SKILL.md:114, and removes the need for the stale-local-main check two lines below since it no longer touches the local main ref at all):
git -C <repo> fetch origin main
git -C <repo> worktree add -b <slug> <path> origin/main
There was a problem hiding this comment.
Addressed in 8105b6c, and the finding is right -- but I did not take the suggested command, because measuring it showed it fails on exactly the concurrent fan-out it is meant to serve.
I built a throwaway repo (git 2.43.0) whose local main sat five commits behind origin/main, and ran five worktree creations per round, sequentially and concurrently.
| form | sequential | concurrent |
|---|---|---|
add <path> main, main checked out |
0/5 | 0/5 |
add <path> main, main free |
1/5 | see below |
add -b <slug> <path> origin/main (suggested) |
5/5 | failed in 4 of 5 rounds |
add -b <slug> --no-track <path> origin/main |
5/5 | 5/5 in all 5 rounds |
add --detach <path> origin/main |
5/5 | 5/5 in all 5 rounds |
Your diagnosis holds exactly. With main checked out -- this corpus's own session-start state -- every attempt refuses with fatal: 'main' is already used by worktree at '<path>'. With main free, the first agent claims it and the other four get the same refusal, which is the fan-out defeated as you described.
The concurrent row is worse than the error, and is the part I would not have predicted. Run genuinely in parallel, the already-checked-out guard is not atomic: three or four worktrees landed on main simultaneously in every one of six rounds, a state the sequential path refuses. Nothing errors. I then committed in two of them:
fr-1 committed: 40fc674
fr-2 sees main at: 40fc674 <- not its own base
fr-2 committed: e048679
So the agents stack onto each other's commits -- silently, which is the precise collision isolation: "worktree" exists to prevent. The loud refusal is the good outcome.
Why not -b <slug> ... origin/main. Branching with tracking from a remote-tracking ref writes upstream config, and that write takes a .git/config lock the racing agents lose:
error: could not lock config file .git/config: File exists
error: unable to write upstream branch configuration
Rounds scored 4/5, 4/5, 5/5, 4/5, 2/5. --no-track removes the config write and goes 5/5; --detach never creates a branch at all and also goes 5/5.
What landed: git -C <repo> worktree add --detach <path> origin/main, in both preferences.md and SKILL.md:114. --detach over --no-track because the subagent's own brief already tells it to work "on your own branch", so it cuts one inside the worktree -- and it matches cascade's existing worktree add --detach precedent you cited. The full flow (detach, checkout -b, commit, five at once) went 5/5 across three rounds, every worktree based on origin/main.
Your parenthetical about the stale-main check was right too, and I verified the mechanism rather than assuming it: fetch origin main advances origin/main (042c314..7a4a8b8) while leaving local main at 042c314. So basing on origin/main fixes the stale base in the same stroke, and the separate "verify with git log --oneline -1 and realign" step is gone. The measurement is recorded in the entry's case record, and the prose now says the concurrent mode is silent rather than loud.
Generated by Claude Code
|
|
||
| **A blocker that was true when you published it can stop being true while | ||
| the PR is open, and withdrawing it is your job, not the reviewer's.** | ||
| The bullet above covers a blocker that was never true. |
There was a problem hiding this comment.
This diff's insertion breaks a pre-existing back-reference into a direct contradiction. Before this PR, "the bullet above" (this line) correctly referred to the "Verify a blocker you assert..." section (the roxygen2/altdoc#76 case, about a blocker that was never true). This PR inserts a new section directly above this one — "Name the specific gate when you report a blocker..." — which is explicitly about the opposite case: its own text (line 570) says "the call was attempted, it genuinely failed, and the blocker is real. Only the attribution is wrong."
So as of this diff, "The bullet above covers a blocker that was never true" now describes a section that is explicitly about a blocker that is real — a direct contradiction between adjacent lines. This file's own convention is that bare "the bullet above" means the immediately preceding section (see e.g. lines 254, 453, 799), and names non-adjacent references explicitly (e.g. "the regression-test bullet above", "the blocker rule above") — so there's no looser reading that saves this. It also undercuts the sentence's own point: "This is the harder case, because the caveat was correct... " no longer contrasts against anything, since the new adjacent section is also about a correct caveat.
| The bullet above covers a blocker that was never true. | |
| The verify-a-blocker bullet above covers a blocker that was never true. |
There was a problem hiding this comment.
Addressed in 8105b6c, with one extension beyond the line you flagged.
Confirmed the contradiction: the new section's line 570 reads "the call was attempted, it genuinely failed, and the blocker is real", directly above a line calling the preceding section one about a blocker that was "never true". Your reading of the file's convention holds too -- bare "the bullet above" means the adjacent section at lines 254/453/799, while non-adjacent ones are named (the regression-test bullet above, withdraw-a-stale-blocker bullet below at line 277).
I took your fix and added a clause, because naming the target alone leaves the sentence's own contrast doing less work than it should. There are now three distinct blocker cases in a row, and the reader arrives at this one having just read the middle one:
The verify-a-blocker bullet above covers a blocker that was never true, and
the gate-naming bullet between it and this one covers a real blocker whose
mechanism was misnamed.
This is the harder case, because the caveat was correct and diligent when
written, ...
That distinguishes never-true / real-but-misattributed / true-then-stale, so "the harder case" contrasts against both rather than against nothing.
One more instance of the same breakage, which the finding did not name. Line 860 read "That rule and the blocker rule above both point outward" -- also referring to Verify a blocker you assert. That was already loose before this PR (two candidates); my insertion made it three. Since my diff is what degraded it, I fixed it in the same pass rather than leaving it: it now reads the verify-a-blocker rule above, matching the wording at 611. Flagging it explicitly in case you consider it out of scope -- it is a one-word change to a line the diff otherwise does not touch.
I also swept the neighbours of all four insertion hunks for the same class, mechanically rather than by eye. The remaining adjacency references all still resolve: github.md:382 ("the bullet below") and :401 ("the entry above") both point at bullets before the insertion point; github.md:423, preferences.md:327/332, and ardi.md:567 are inside the new text and point at genuinely adjacent targets -- I checked that the two bullets github.md:423 calls "about staleness" (lines 380 and 399) really are.
Generated by Claude Code
Code reviewReviewed the diff (+120/-0 across four files: The Four issues found, all independently verified before posting — see inline comments for full detail and citations:
VerdictNeeds more work. |
|
Claude finished review — View run Code reviewReviewed the diff (+120/-0 across four files: The Four issues found, all independently verified before posting — see inline comments for full detail and citations:
VerdictNeeds more work. |
- preferences.md: drop the forward reference to the persisting-cwd bullet and inline its rationale instead. - preferences.md: unescape the quotes inside an inline code span, which CommonMark renders literally and which misquoted gip/SKILL.md. - preferences.md + gip/SKILL.md: base the worktree fallback on origin/main with --detach rather than the bare branch main. Measured: the old form scores 0/5 with main checked out and 1/5 with it free, and concurrently its guard races several worktrees onto main at once with no error. The suggested -b <slug> origin/main form fails four rounds in five on a .git/config lock; --detach scores 5/5. - ardi.md: name the verify-a-blocker bullet explicitly at two back- references the new section made ambiguous.
Review round 1: all four findings AddressedCommitted as
Finding 3 is right, and its suggested fix is notThis one is mine originally -- I handed that command to the agent that wrote the entry. Rather than reason about
Two things the finding did not reach:
Landed Beyond the flagged linesPer the neighbours-of-every-hunk sweep, one further instance of finding 4's pattern: Also corrected something no finding raised: my first draft of the rewrite said the naive command "gets three separate things wrong" above an enumeration that did not number three. Dropped the count rather than fixing it, per Verification, on
|
|
Claude finished review — View run Code review — PR #1008I reviewed the current head ( Prior findings: verified resolved
I re-ran New finding
This wasn't raised in the prior review round. VerdictNeeds more work. One new finding: hardcoded |
The worktree-isolation fallback hard-coded the literal branch `main` in both the prescribed commands and the prose explaining them, in memories/preferences.md and skills/gip/SKILL.md. `gip` is repo-agnostic and already parameterizes `<repo>` in that very command, and already uses `<default-branch>` for the subagent's own branch step. On a repo whose default is `master` or `develop` the prescribed pair dies outright: git fetch origin main fatal: couldn't find remote ref main git worktree add --detach <path> origin/main fatal: invalid reference: origin/main That defeats the fallback in precisely the case it exists for. Measured against a throwaway repo whose default branch is `develop`; both commands succeed against `origin/develop`. Resolution is pointed at rather than duplicated: gip's step 0 already says to note the default branch, and ums already carries the `gh repo view --json defaultBranchRef` recipe with its own "don't hard-code main" note. The measurement record at the end of the preferences.md entry keeps its literal `main`, because those runs were made against a repo whose branch really was `main`; parameterizing a record of commands actually executed would falsify it. A sentence now says so, and records the new develop-repo measurement beside it.
Review round 2: the hardcoded-
|
command as prescribed at 8105b6c |
result |
|---|---|
git fetch origin main |
fatal: couldn't find remote ref main (rc=128) |
git worktree add --detach <path> origin/main |
fatal: invalid reference: origin/main (rc=128) |
Both succeed against origin/develop (rc=0). So the second error text the review predicted is exactly right, and the fetch half fails first with a different message, which the finding did not mention.
Correction to the finding: the placeholder citation
The finding cites gip lines "55, 144-145, 151" as already using <default-branch>. Only 144, 145, 151 do. Line 55 reads:
and note the default branch. Resolve `<owner>/<repo>` once so you can pass it to
That establishes the concept and the resolution step, but does not use the placeholder. The distinction matters for the fix, because line 55 is what I point at for resolving the value rather than a fourth place the placeholder appears.
Wider than the flagged lines
Per address-every-comment.md on a finding that is a pattern, I swept both files rather than editing the four sites named. Every occurrence, with provenance established from git diff origin/main...HEAD:
| site | authored by this PR? | disposition |
|---|---|---|
preferences.md prescriptive prose + Do/Don't (11 occurrences) |
yes | Addressed |
gip 114, 115 (named by review) |
yes | Addressed |
gip 117, 118, 120, 124 (not named by review) |
yes | Addressed |
gip 40, 71 |
no, pre-existing | Deferred to main, see below |
gip:117 is the one worth calling out: its whole subject is which ref to base a worktree on, and it hardcoded the branch while saying so, which makes it the sentence a reader is likeliest to copy.
Two pre-existing occurrences, deliberately not fixed here
gip:40 (must branch from that MR's tip, not main) and `gip:71` (`it can branch straight from `origin/main) are byte-identical to origin/main and untouched by this PR's diff. Both are triage-criteria prose rather than copyable commands.
Left alone per address-every-comment.md's main-sync rule and dont-incur-technical-debt.md's "authorship, not adjacency": fixing them on this branch would put it out of step with main on content this PR did not author, and the fix target is main itself. Flagging rather than silently dropping, since my edit does leave gip:71 and the new gip:121 describing the same choice differently. One line to overrule if you would rather I fold them in.
How it was parameterized
Resolution is pointed at, not duplicated -- gip's step 0 already says to note the default branch, and ums/SKILL.md:227-228 already carries the recipe with its own # discover the base -- don't hard-code main. So no third copy of gh repo view --json defaultBranchRef was added.
The measurement record at the end of the preferences.md entry keeps its literal main deliberately. Those runs were made against a repo whose branch really was main; parameterizing a record of commands actually executed would falsify it. A sentence now says so explicitly, notes that this is why those runs never surfaced the hardcoding, and records the develop-repo measurement beside it.
Verification, on 7abfed6
- Banned punctuation, three-dot range, in Python: 0 banned glyphs and 0 non-ASCII over 189 added lines -- quoting the examined count, since a bare zero is not evidence on its own.
check-new-line-breaks.py(NLB_BASE_REF=origin/main):No lines missing semantic breaks.Read from output, not exit status.test_check_memory_file_size.py: 13 passed, 0 failed -- run as the test suite exactly asvalidate.ymlinvokes it (python3 scripts/test_check_memory_file_size.py), not the advisory script. IncludesPASS: this repo's own memories/ is under the 1200-line default.preferences.mdis 811 lines;memories/github.mdis untouched by this round and stays at 1199.sync-codex-skill-wrappers.py: regenerated 175 wrappers,git status --porcelainempty afterward, so no drift. Run because this PR edits askills/file.validate-skills.py:all skills and manifests valid.check-links.py:no broken relative linksacross 863 links in 426 files -- which covers the two new../skills/ums/SKILL.mdreferences.- Neighbours of every hunk re-read. The
gip:128back-reference andpreferences.md's own wording were kept consistent when "stale-local-maintrap" became "stale local base". The "wrong in more than one way at once" phrasing carries no count, so adding a fourth way does not stale it. - No
claude-*model identifier in the diff.
Not verified this round: markdownlint-cli2 is not installed in this container (npx declined to fetch it), so unlike the previous round I have no result to quote for it. Only .md prose changed and no list or heading structure was touched.
Not merging.
Generated by Claude Code
|
One follow-on from Section 2 still says:
As of Not editing it myself for two reasons. The body is your correction log, and adding a fourth item to a numbered Suggested minimal edit, if you want it: change that sentence to name Generated by Claude Code |
2026-08-01 05:18 UTC |
Code reviewRe-reviewed at head Verified fixes:
Independently re-verified in this round (not just re-reading the diff):
VerdictReady for merge. |
|
Claude finished review — View run Posted the review comment: #1008 (comment) SummaryRe-reviewed PR #1008 at head
I also independently re-ran the repo's own validators ( VerdictReady for merge. |
PR #1008 parameterized the base branch across this file but left two prescriptive instances reading the literal `main`, so one file now gave the same instruction two ways. Deferred there rather than fixed, correctly: both lines were byte-identical to `main`, so editing them on that branch would have put it out of step. #1008 has since merged, which is the follow-up its own rule prescribes. Line 71 read `origin/main` where the surrounding steps prescribe `origin/<default-branch>`; line 40 named `main` as the base an issue branches from. Both now name the parameter. Left alone: the two occurrences that are evidence rather than instruction -- the sentence naming the literal string `main` as the thing not to hard-code, and the verbatim `fatal: invalid reference: origin/main` a reader will match against their own terminal. Line 71's em-dash became this diff's to fix once the line was edited, so it is now `--`. Co-authored-by: Claude <noreply@anthropic.com>
…and two self-contradictions Four findings, all correct, all verified against live API state rather than reasoned about. 1. fully-clean.md's case record claimed Copilot's check run on #1008 completed success at 04:50:41Z. There is no such check run. Neither #1005 nor #1008 carries ANY Copilot-attributable check context (8 and 10 checks respectively; filtering either for /opilot/ returns 0), and that holds both where Copilot posted a refusal and where it was silent. Rewrote the mechanism: the silence is invisible on the check surface by construction, not misread from a green Copilot check -- which is a stronger argument for the section's own advice. Left the error visible in the record rather than deleting it. 2. The prescribed lookup named user.login while tool-mappings.md's CLI fallback exposes author.login, and the two surfaces also disagree on whether the value carries [bot]. Measured on #1005: REST returns copilot-pull-request-reviewer[bot] under user.login, gh pr view returns copilot-pull-request-reviewer under author.login. Mixing them returns zero hits and reads as 'did not review' -- the exact false negative the section is about. Now a two-row table. 3. avoid-hardcoding-external-data.md said the judgment was 'never written down' four lines below quoting one of the two files writing it down. Both files state it, in fact. Replaced with the point that survives: an in-file rationale protects a reader, not a grep, so a sweep re-flags them regardless. 4. metacognitive-monitoring.md said 'four findings' then 'the five findings'. Reconciled, and corrected its closing claim: that entry is where 04:50:41Z came from, and it called its own conclusion 'exactly right'. It was not. A verification that adjusts a figure without asking whether the measured thing exists propagated the fabrication into fully-clean.md instead of catching it, so the record now reads five of six rather than five of five. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* ums: a handed finding's particulars are the half that is wrong Three findings from the six-PR session that merged #996, #999, #1003, #1004, #1005 and #1008, plus one candidate declined as already covered. metacognitive-monitoring: extends 'A premise you were handed is still a claim' to a premise handed by a reviewer rather than by a person. That section's only detector is a hedge in the source, and a review comment carries none, so the signal is silent exactly where the premise most often arrives. Records the asymmetry that says which half to check: across five findings the conclusion held five times and the particulars were wrong five times, and particulars are what decide the edit list. Adds the two reasons the check feels done when it is not, comprehension and delegation. fully-clean: a third state for the same external reviewer, alongside the fifth case's refusal. Its check run completes success having posted no review at all, so nothing on the PR reports that a configured reviewer never weighed in. Defeats the 'no verdict is its own state' remedy, which reads the job outcome and is scoped to a job that failed. avoid-hardcoding-external-data: the boundary the parameterize rule must not cross. A quoted command that was run, a verbatim error string, and the stated conditions of a measurement are claims about the past, so substituting a parameter falsifies them rather than generalizing them. Declined: that merge-base --is-ancestor reports non-ancestor for every branch in a squash-merge repo. Measured true here, 8 of 8, all merged, but CLAUDE.md already states it flatly at two sites. * fully-clean: name the query that does distinguish a silent reviewer The third-state case record closed by saying nothing anywhere on the PR distinguished a reviewer that had approved from one that never spoke. The sentence directly above it reports the opposite: the login-filtered review list returned eight reviews with none from Copilot, which is exactly the discriminator, and exactly what the section's own Do-bullet sends a reader to fetch. The body prose was already right -- nothing on the PR *reports* the gap on its own -- but the case record escalated that to *distinguished*, which contradicts the check the section exists to prescribe. Naming the query keeps the intended contrast with the green-check signal and makes the point land harder: every signal except the prescribed one was uninformative. * address review: an invented check run, a two-surface field mismatch, and two self-contradictions Four findings, all correct, all verified against live API state rather than reasoned about. 1. fully-clean.md's case record claimed Copilot's check run on #1008 completed success at 04:50:41Z. There is no such check run. Neither #1005 nor #1008 carries ANY Copilot-attributable check context (8 and 10 checks respectively; filtering either for /opilot/ returns 0), and that holds both where Copilot posted a refusal and where it was silent. Rewrote the mechanism: the silence is invisible on the check surface by construction, not misread from a green Copilot check -- which is a stronger argument for the section's own advice. Left the error visible in the record rather than deleting it. 2. The prescribed lookup named user.login while tool-mappings.md's CLI fallback exposes author.login, and the two surfaces also disagree on whether the value carries [bot]. Measured on #1005: REST returns copilot-pull-request-reviewer[bot] under user.login, gh pr view returns copilot-pull-request-reviewer under author.login. Mixing them returns zero hits and reads as 'did not review' -- the exact false negative the section is about. Now a two-row table. 3. avoid-hardcoding-external-data.md said the judgment was 'never written down' four lines below quoting one of the two files writing it down. Both files state it, in fact. Replaced with the point that survives: an in-file rationale protects a reader, not a grep, so a sweep re-flags them regardless. 4. metacognitive-monitoring.md said 'four findings' then 'the five findings'. Reconciled, and corrected its closing claim: that entry is where 04:50:41Z came from, and it called its own conclusion 'exactly right'. It was not. A verification that adjusts a figure without asking whether the measured thing exists propagated the fabrication into fully-clean.md instead of catching it, so the record now reads five of six rather than five of five. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fully-clean.md criterion 1 gains a block saying gh pr checks is not a complete enumeration of a head's check runs, naming the commit check-runs endpoint as the authority for "has everything finished", and distinguishing this from the existing workflow-run-vs-check-run gap two paragraphs above. It states plainly that the reason for the omission is unestablished, names the three untested candidates, and records the counts that disqualify each. The fifth case's "no check run at all / by construction" claim is corrected to what was measured, with both dates kept. On #1005 and #1008 the commit check-runs endpoint returns one copilot-pull-request-reviewer run each, conclusion success, while gh pr checks returns zero for both. That also reinstates the 04:50:41Z figure a later revision retracted: check run 91327863807 on 7abfed6 reads completed_at 2026-08-01T04:50:41Z, success. metacognitive-monitoring.md's sixth-instance paragraph asserted the same refuted claim, so it is corrected in the same pass. Closes #1074
…fifth case (#1079) * start: document gh pr checks incompleteness in fully-clean.md (closes #1074) * Record that gh pr checks omits check runs, and correct the fifth case fully-clean.md criterion 1 gains a block saying gh pr checks is not a complete enumeration of a head's check runs, naming the commit check-runs endpoint as the authority for "has everything finished", and distinguishing this from the existing workflow-run-vs-check-run gap two paragraphs above. It states plainly that the reason for the omission is unestablished, names the three untested candidates, and records the counts that disqualify each. The fifth case's "no check run at all / by construction" claim is corrected to what was measured, with both dates kept. On #1005 and #1008 the commit check-runs endpoint returns one copilot-pull-request-reviewer run each, conclusion success, while gh pr checks returns zero for both. That also reinstates the 04:50:41Z figure a later revision retracted: check run 91327863807 on 7abfed6 reads completed_at 2026-08-01T04:50:41Z, success. metacognitive-monitoring.md's sixth-instance paragraph asserted the same refuted claim, so it is corrected in the same pass. Closes #1074
3 participants
Three entries from the session whose five PRs merged tonight, plus one candidate declined. I read what #996, #999, #1003, #1004, and #1005 recorded on current
mainfirst, so none of this restates them.> [!NOTE]
> Three corrections since this PR opened, all to claims I published and then disproved. The body is rewritten rather than quietly patched, since the first version is what earlier readers saw.
>
> 1. A shell placeholder written in angle brackets was stripped by the API, leaving
git worktree add main. That is the documented loss inmemories/github.md; placeholders are unbracketed below, per its own remedy. Committed files are unaffected, which that entry also predicts and I verified.> 2. I wrote that the memory-file-size threshold is "advisory and exits 0, so it should not block an unrelated PR", and kept an over-length entry on that basis. That was wrong, and
validatefailed in CI on it. See the section below.> 3. Review round 1 found the prescribed fallback command itself wrong, and measuring it showed it fails in a second way neither the review nor I anticipated. Entry 2 below is rewritten accordingly; the superseded version is described in place rather than deleted.
1.
memories/github.md--list_pull_requestsreportsmerged: falsefor merged PRsPlaced beside the two sibling field-reliability bullets (
get_status,get_check_runs), because it is the third of that family and the contrast is the point: those two are about staleness, where a field is sometimes wrong. Here the value is constant, so it is wrong for every merged PR while looking correct on any unmerged one you spot-check it against. That is the same "a constant carries no information" argumentfully-clean.mdalready makes for review.state, so the entry cites it rather than re-deriving it.Verified myself rather than taken from the brief, on
d-morrison/ai-config, 2026-08-01:listmergedfalsefalsefalselistmerged_atgetmergedfalsetruefalsemerged: falseon all 101 rows across four list calls, including 1005, whichgetreportsmerged: true/merged_by: d-morrison.fieldsprojection. The brief flagged this as worth pinning down since the remedy differs, and it is not: a call passing nofieldsargument at all returns the samemerged: false.merged_byis not a fallback -- never served in a list response even when named explicitly infields.I caught myself writing the
getcell for the open PR before callinggeton it, and made the call rather than leaving an inferred value in a table presented as measurement.The mechanism (list returns GitHub's smaller pull-request representation, leaving a zero-value
false) is marked as inferred from which fields are absent, not read from the server source -- the same hedging discipline #1004 applied to the node-ID story it had to retract.2.
memories/preferences.md+skills/gip/SKILL.md-- the worktree remedy has an unstated preconditionisolation: "worktree"needs the session's own cwd to be inside a git repository. #1005's entry prescribes it citinggip, andgipsays to give every subagent that parameter; neither carries the precondition, so in this harness layout both prescribe a parameter that errors.Verified both clauses of the error message independently:
git rev-parse --show-toplevelin the default cwd/home/userreturnsfatal: not a git repository, withai-config,gha,qbt,qwt,rpt,workflowsone level below.settings.jsonexists at~/.claude/or/root/.claude/, so noWorktreeCreatehook is configured either.Stated limit: the error text is quoted from the parent session's attempt. This agent has no Agent tool and did not re-run it, and the entry says so rather than implying a reproduction.
The caveat went in both places a reader meets the remedy, per the brief: the full entry in
memories/preferences.mdbeside the rule it qualifies, and a short pointer ingipat the line that prescribes the parameter. The failure mode worth naming is that a reader hitting the error concludes isolation is unavailable and shares the checkout after all, which is the exact outcome #1005's entry exists to prevent.The fallback command, as corrected in review round 1
This PR originally prescribed
git worktree add PATH main, and that was wrong. Review found it hard-codes a branch name that a worktree cannot reuse; I then measured the alternatives rather than reasoning about them, on git 2.43.0, five worktree creations per round against a throwaway repo whose localmainsat five commits behindorigin/main.add PATH main,mainchecked outadd PATH main,mainfreeadd -b SLUG PATH origin/main(review's suggestion)add -b SLUG --no-track PATH origin/mainadd --detach PATH origin/mainTwo things beyond what the finding claimed:
mainat once in every one of six rounds. Committing in two of them showed the second readingmainat the first's new commit and stacking on top -- the exact collision isolation exists to prevent, with no error.-b SLUG ... origin/mainloses a.git/configrace, because branching with tracking from a remote ref writes upstream config:could not lock config file .git/config/unable to write upstream branch configuration.What landed is
git -C REPO worktree add --detach PATH origin/main, in both files. Basing onorigin/mainalso removes the stale-base trap the original entry documented -- confirmed thatfetch origin mainadvancesorigin/mainwhile leaving the localmainref untouched -- so the separate "verify and realign" step is gone rather than merely restated.3.
shared/workflow/ardi.md-- name the specific gate when reporting a blockerPlaced directly after "Verify a blocker you assert", which it refines. That rule governs whether something is blocked and its remedy is to attempt it once. This governs why, and fires after that remedy has succeeded: the call was attempted, it genuinely failed, the blocker is real, and only the attribution is wrong -- which is why nothing about it feels unverified.
I reported an unresolvable review thread as blocked "for scope reasons" across roughly six status updates. The failure actually seen under that spelling was the node-versus-declared-string comparison. Only the other spelling is scope.
The transferable point, which is what made this worth an entry rather than a note: a category word that is also the proper name of one specific mechanism cannot double as the generic term for its family. "Scope" names a real gate on this platform, so the wrong reading survives re-reading, and it is actionable in the wrong direction -- someone told a call failed on scope will reach for the other owner, which also fails.
memories/github.mdalready records both gates and their verbatim errors (#1004). What was missing was the usage habit, which I confirmed is absent rather than assuming it.Round 1 also found that inserting this section broke a pre-existing back-reference below it into a contradiction. Fixed by naming the target (
the verify-a-blocker bullet above) and distinguishing all three adjacent blocker cases; a second instance of the same reference at line 860 was fixed in the same pass, since this insertion is what made it ambiguous.Declined: the review-cost delta
Recommended as a comment on the gha issue, not recorded here, and I have posted nothing. My MCP scope is
d-morrison/ai-configonly.The measurement is real (pre-slide run
30673938581at $17.03, post-slide30676853877at $4.18, consecutive rounds on PR #1003). Three reasons it does not belong in this corpus:claude-code-review.yml, and the natural home is the issue that fix closes.total_cost_usdentries use cost as a diagnostic (cost 0 means quota exhaustion), never as a performance comparison, so this would also be a new kind of claim on thin evidence.timestamp-volatile-claims.md.The correction: the 1200-line threshold is a hard gate, not advisory
Entry 1 first landed at 36 lines, taking
memories/github.mdfrom 1175 to 1211. I ranscripts/check-memory-file-size.py, saw it report the finding and exit 0, read its own docstring saying a crossing "should not block an unrelated PR", and kept the entry on that basis, arguing in this body that trimming to satisfy an advisory instrument inverts what the instrument is for.validatethen failed:scripts/test_check_memory_file_size.pyasserts the corpus is clean, and that test is not advisory. The script exits 0; the test suite does not. I had run the first and not the second, so my evidence never covered the claim I made from it -- the shapefail-fast.mddescribes, where a check's scope is narrower than the conclusion drawn from it.Fixed properly rather than papered over:
merged_by, the hedged mechanism, and the Do/Don't.memories/github.mdis now 1199 lines andtest_check_memory_file_size.pyreports 13 passed, 0 failed.Searched, and concluded already covered or absent
Normalized for whitespace, backticks, asterisks and underscores on both needle and haystack, since this corpus breaks lines mid-phrase and the needle needs the same transform as the text.
merged_at,merged: false,list_pull_requests,fields projection. The only priormerged_athit isCLAUDE.md, unrelated.isolation: "worktree",not in a git repository,WorktreeCreate,give every subagent. Zero hits for either precondition string anywhere in the corpus.name the specific gate,which gate,two different gates,quote the error verbatim,paraphrase an error,attribute the failure,misattributed,a category word,generic descriptor,names one specific mechanism,superficially similar errors. The twowrong mechanismhits were read and are about choosing the wrong mechanism to build, not about misattributing a failure.ardi.md's "Verify a blocker you assert" (whether, not why),fully-clean.md's eighth case (the tool's message misleads; here the message was accurate and my paraphrase was not), andmetacognitive-monitoring.md's cause-claim type (which prescribes asking what else explains it, but not that two gates exist or that the error text separates them).Verification
Run after committing, on the current head
8105b6c(which also carries a merge ofmain, sincemainadvanced tob16fe72while this PR was open):gipedit where I had imitated the surrounding file's style -- the exact trapascii-punctuation-in-source.mdnames. Fixed with targeted edits, not a file-wide replace, per that same fragment's scope-creep warning.NLB_BASE_REF=origin/main check-new-line-breaks.py:No lines missing semantic breaks.Read from output rather than exit status, since this one genuinely is advisory.test_check_memory_file_size.py: 13 passed, 0 failed -- run as the test suite, asvalidate.ymlinvokes it, not the advisory script.memories/github.mdis untouched by round 1 and stays at 1199.test_validate_skills.py8/0,test_slb.py34/0,test_check_install.py36/0,test_rotate_claude_token.py23/0,test_find_near_duplicates.pyall passed,check-vendored-drift.pyclean.validate-skills.py:all skills and manifests valid.check-links.py:no broken relative linksacross 863 links.markdownlint-cli2:Linting: 445 file(s)/Summary: 0 error(s)-- quoting the scope line, since a bare zero is not evidence on its own.sync-codex-skill-wrappers.py: no drift. Run because this PR edits askills/file; the wrappers embed frontmatter rather than body, and my edit is body-only.claude-*model identifier anywhere in the diff.Merge order
None. #1006 has since merged and is folded in via the
mainmerge; nothing else open overlaps these four files.