fix(scripts): check-docs must not match a PR number against a commit-sha prefix (#503) - #511
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
Follow-up on the same PR, applying nebula's measurement from the issue
thread. The first pass fixed `head -1` and matched state words as whole
words; that was not enough, and I could prove it only after running the
check with REAL upstream states.
Two corrections to my own verification first, because they are why this
was nearly missed:
* My "no new false positives" run used a stub that returned a state for
ONE pr number and nothing for the rest, so every other PR was skipped.
"Clean" there proved nothing. With real `gh` the fix ADDED a warning.
* The control line nebula cited (FORK_CHANGELOG.md L246) is blank in this
tree — the measurement was taken on another branch. Found by content
instead: it is L358 here, and L356 is a worse case nebula predicted but
could not see.
Measured with real states, before this commit:
NEW script: 2 warnings (MemPalace#1377, #459)
OLD script: 1 warning (MemPalace#1377)
Both causes are the same defect from the other end. `head -1` decided
WHICH lines are read; this decides WHAT counts as a claim on a line.
#459 README.md:297 and FORK_CHANGELOG.md:356 are the heading "purge /
prune / mined share one open-and-refuse sequence", linking commit
`459efab`. There is no #459 on either line — `/459` matched inside
the COMMIT HASH, and "open-and-refuse" supplied a whole-word "open".
A right boundary does not help: `/459` is followed by `e`.
MemPalace#1377 FORK_CHANGELOG.md:81 was MY OWN changelog entry, quoting the
control sentence verbatim. The entry documenting the defect
reproduced it — the `self-quoting-retraction` shape from the #503
spec, which is how #511 went green and then warned again once its
own entry landed.
So, three rules now:
* `/pull/$n`, never a bare `/$n`. A commit hash is not a PR reference.
* A state word must appear in a CLAIM SHAPE — a parenthesised marker
"(OPEN)", or a copula "is/was/stays/remains/now [still] open". Checked
against all 13 cases in this repo: every real claim kept (#23 "is still
OPEN", #168 "stays open", MemPalace#665/MemPalace#1087 "(OPEN)", MemPalace#1094 "(open upstream"),
every prose case dropped ("open-and-refuse", "open the drawers",
"opencode", "openai-compat", "reopened").
* The entry for this change does not quote its own control sentence. It
states a MERGED claim for MemPalace#1377, which is what MemPalace#1377 is.
Result with real `gh` on the repo itself: both the base script and this one
report zero PR-state warnings. The base's cleanliness is incidental — its
`head -1` happens to land on a line without a claim, and moved there only
because #509/#512/this entry changed the changelog. This one is clean for a
reason.
shellcheck findings 4 -> 2 (both remaining are pre-existing warnings; both
former ERRORS are gone).
Tests: 5 more (state word without a claim; nebula's "open the drawers" with
the PR alone on the line, since the real one is spared only incidentally by
a second PR sharing it; a commit hash is not a PR reference; a /pull/ URL
still counts; a parenthesised marker is still a claim). 16 total, and two
more mutations verified with the target-exists assertion:
claim shape -> bare word -> 3 tests
/pull/N -> /N -> 2 tests
One test asserted something the check cannot do — a PR referenced only by
URL is never examined, because the number list is harvested from `#NNNN`
alone. Pre-existing and out of scope; the test now says so rather than
pretending to cover it.
Full suite 7448 passed, 82 skipped. check-docs passes on itself.
Part of #516
…sha prefix The PR-state step extracted candidates as `#NNNN` but matched them per-document as `(#$n|/$n)`. A PR number is also a valid commit-sha prefix, so `/459` matched `.../commit/459efab` on a README line reading "one open-and-refuse sequence". Upstream MemPalace#459 is CLOSED, the substring test saw that state word, and the checker reported drift against a document with no such reference (`grep -c '#459' README.md` -> 0). Now requires `#$n` or `/pull/$n`. Deleting the slash alternative would have blinded the check on real `/pull/N` URLs, which these docs do contain; narrowing keeps the function and drops the collision. Twelve referenced numbers prefix a referenced sha, so eleven were latent rather than absent, and STRICT_PR_STATE=1 would have promoted them to errors against accurate docs. Controls both ways, because the obvious wrong fix is to silence the check: after the fix check-docs reports "all 243 PR references match upstream state" with no warnings; a fixture claiming OPEN for MERGED MemPalace#1000 still warns, and a negative control with no state word stays silent. Records five instances in the #503 spec and adds a `direction` field (false-confidence | false-alarm) to its record schema -- the corpus is almost entirely false-confidence, so an index without it answers "my check says the fix broke it" with cards about distrusting green. Re-derives the seed count from the sources rather than a summary tally line, which is what produced the original 25/21: 32 on record, 28 citable, with the derivation stated per source. Part of #503 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntry Part of #503 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…minator Oracle ruled PART 27 §27.4 in-class. Adds a record carrying two instances one review apart, by the same reader: a grep for 'failure-shape' returning 0 against rendered text reading "failure SHAPES", read as a missing render; and a grep of tests/ for the literal 1.10.0 returning 0, concluded "the claimed version test does not exist", when the test asserts [major, minor] >= [1, 10] -- a floor, not a pin. Searching for the value instead of the property, twice. States the discriminator the spec lacked. This class and the keyed-on-terms regime of section 2 produce an identical zero from a reader aimed at the searcher's own vocabulary, so they cannot be separated by how the zero was produced -- only by what it is consumed as. Known to be a miss: a retrieval problem, try another phrasing. Mistaken for a fact: a false finding, and what follows is a verdict or a CHANGES against correct work. That makes direction a property of the consumer rather than of the mechanism, which is the thing retrieval keys on -- so it belongs on the record, not inferred at read time. Second false-alarm on record. Count re-derived: 33 on record, 29 citable. Part of #503 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
force-pushed
the
fix/505b-check-docs-pr-matcher
branch
from
September 18, 2026 05:17
eb03a33 to
185f584
Compare
Replaces the claim that direction is a property of the consumer, per Oracle PART 32b. The reasoning was wrong: pgrep self-match always fabricates a live process, so its direction is fixed by the mechanism, while sha-prefix-match flips with the question asked. Direction belongs to the pair, not to either alone. The conclusion -- record it rather than infer it -- survives for a different reason: the asker's intent is not captured by shape, asked/answered, control_passed or remedy, so nothing at read time can reconstruct it. Where a shape's instances agree the direction may be carried on the shape as a modal value; where they can differ it belongs on instances[]. Notes that sha-prefix-match is already mixed in principle -- false-alarm inside check-docs, false-confidence against "does this doc reference #459?" -- and marks the earlier direction summary as modal rather than a partition, which it could no longer be read as. Also corrects a stale figure in that section: the corpus grew 25 to 33, not 25 to 31, between reviews. No count change, no seq change. Part of #503 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
Follow-up on the same PR, applying nebula's measurement from the issue
thread. The first pass fixed `head -1` and matched state words as whole
words; that was not enough, and I could prove it only after running the
check with REAL upstream states.
Two corrections to my own verification first, because they are why this
was nearly missed:
* My "no new false positives" run used a stub that returned a state for
ONE pr number and nothing for the rest, so every other PR was skipped.
"Clean" there proved nothing. With real `gh` the fix ADDED a warning.
* The control line nebula cited (FORK_CHANGELOG.md L246) is blank in this
tree — the measurement was taken on another branch. Found by content
instead: it is L358 here, and L356 is a worse case nebula predicted but
could not see.
Measured with real states, before this commit:
NEW script: 2 warnings (MemPalace#1377, #459)
OLD script: 1 warning (MemPalace#1377)
Both causes are the same defect from the other end. `head -1` decided
WHICH lines are read; this decides WHAT counts as a claim on a line.
#459 README.md:297 and FORK_CHANGELOG.md:356 are the heading "purge /
prune / mined share one open-and-refuse sequence", linking commit
`459efab`. There is no #459 on either line — `/459` matched inside
the COMMIT HASH, and "open-and-refuse" supplied a whole-word "open".
A right boundary does not help: `/459` is followed by `e`.
MemPalace#1377 FORK_CHANGELOG.md:81 was MY OWN changelog entry, quoting the
control sentence verbatim. The entry documenting the defect
reproduced it — the `self-quoting-retraction` shape from the #503
spec, which is how #511 went green and then warned again once its
own entry landed.
So, three rules now:
* `/pull/$n`, never a bare `/$n`. A commit hash is not a PR reference.
* A state word must appear in a CLAIM SHAPE — a parenthesised marker
"(OPEN)", or a copula "is/was/stays/remains/now [still] open". Checked
against all 13 cases in this repo: every real claim kept (#23 "is still
OPEN", #168 "stays open", MemPalace#665/MemPalace#1087 "(OPEN)", MemPalace#1094 "(open upstream"),
every prose case dropped ("open-and-refuse", "open the drawers",
"opencode", "openai-compat", "reopened").
* The entry for this change does not quote its own control sentence. It
states a MERGED claim for MemPalace#1377, which is what MemPalace#1377 is.
Result with real `gh` on the repo itself: both the base script and this one
report zero PR-state warnings. The base's cleanliness is incidental — its
`head -1` happens to land on a line without a claim, and moved there only
because #509/#512/this entry changed the changelog. This one is clean for a
reason.
shellcheck findings 4 -> 2 (both remaining are pre-existing warnings; both
former ERRORS are gone).
Tests: 5 more (state word without a claim; nebula's "open the drawers" with
the PR alone on the line, since the real one is spared only incidentally by
a second PR sharing it; a commit hash is not a PR reference; a /pull/ URL
still counts; a parenthesised marker is still a claim). 16 total, and two
more mutations verified with the target-exists assertion:
claim shape -> bare word -> 3 tests
/pull/N -> /N -> 2 tests
One test asserted something the check cannot do — a PR referenced only by
URL is never examined, because the number list is harvested from `#NNNN`
alone. Pre-existing and out of scope; the test now says so rather than
pretending to cover it.
Full suite 7448 passed, 82 skipped. check-docs passes on itself.
Part of #516
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
) (#520) * fix(check-docs): examine every mention of a PR, not just the first Step 4 did `grep … | head -1` per document, so only the FIRST line mentioning a PR number contributed to that document's claimed state. The check answered "does the first mention agree?" rather than "do all mentions agree?", and a drifted claim appearing after a correct one was invisible. Reproduced on the real repo before fixing: appending PR MemPalace#1377 is still open upstream. to the end of README.md, with MemPalace#1377 MERGED upstream, left check-docs reporting "✓ all 245 PR references match upstream state". The control was confirmed reachable BEFORE its silence was trusted: the appended line is in the instrument's own match set (line 477 of 5 matches), while `head -1` selects line 30 — which mentions MemPalace#1377 and claims nothing. A control that is present but never looked at produces "did not fire" for the wrong reason, and that reads identically to "no drift". Three defects, now one scan: - Only the first line was read. Every matching line is now, with the multi-PR commentary skip applied per LINE rather than zeroing a whole document's claims. - No right boundary. The claim scan used (#$n|/$n), so `#45` matched `#452` and `/45` matched `/459efab`, while the commentary scan beside it used [^0-9] — the two loops could read different lines and reach a conclusion neither line supported. One scan, one regex, anchored on a non-digit or end of line. - State words matched as substrings. "opencode", "openai-compat" and "reopened" all contain "open" and were read as a claim of OPEN. Latent while one line per doc was examined; amplified the moment every line is. Measured differentially over the whole repo with every PR stubbed MERGED: 7 findings before, 7 after — and five different ones in each direction. removed (false positives): #45 read off a line about #452 #56 "OpenCode adapter smoke test" #463 "openai-compat embedding" MemPalace#1567 ".opencode/opencode.json" MemPalace#2062 v3.8.0 sync line found (never examined): #23 "PR #23 is still OPEN but" #168 "#168 itself stays open" MemPalace#665 "*Upstream:* [PR MemPalace#665] (OPEN)" MemPalace#1087 "(OPEN)" MemPalace#1094 "(open upstream, jp-authored)" The unchanged total is the trap: a reader checking whether the count moved would conclude nothing had. Also removes both shellcheck errors in the file (SC1087 — `$n[` read as array indexing) and adds none; the two remaining warnings are pre-existing. Known limit, asserted in a test rather than left implicit: when one clean line claims the true state and another claims a different one, drift cannot be distinguished from history, and the PR is skipped. MemPalace#1024 in FORK_CHANGELOG.md is exactly that shape — "pushed to the open MemPalace#1024 PR branch (squash-merged upstream)" alongside an authoritative "(MERGED)" — and is correct documentation of a MERGED PR. It is the only such pair in the repo, which is why flagging disagreement was rejected. Tests: 11 new in tests/test_check_docs_pr_state.py, driving the real script over a fixture tree with a stubbed `gh` (no network, no shared API quota). Four guards mutation-verified, each mutation asserting its target exists so a mutation that fails to apply reports loudly instead of as "nothing to guard": restore head -1 -> 3 tests remove the word boundary -> 2 remove the commentary skip -> 1 drop the right boundary -> 1 Three of those tests were not evidence when first written and were rebuilt: the harness sliced output between step headings while findings go to stderr (so every "no finding" assertion passed vacuously); the commentary test claimed a state the MERGED branch ignores; and the boundary test used a number the check never queried. Each now fails under the old behaviour. check-docs passes on itself. Full suite 7443 passed, 82 skipped. Part of #516 Fixes #516 * fix(check-docs): a state WORD is not a state CLAIM; /pull/N, not /N Follow-up on the same PR, applying nebula's measurement from the issue thread. The first pass fixed `head -1` and matched state words as whole words; that was not enough, and I could prove it only after running the check with REAL upstream states. Two corrections to my own verification first, because they are why this was nearly missed: * My "no new false positives" run used a stub that returned a state for ONE pr number and nothing for the rest, so every other PR was skipped. "Clean" there proved nothing. With real `gh` the fix ADDED a warning. * The control line nebula cited (FORK_CHANGELOG.md L246) is blank in this tree — the measurement was taken on another branch. Found by content instead: it is L358 here, and L356 is a worse case nebula predicted but could not see. Measured with real states, before this commit: NEW script: 2 warnings (MemPalace#1377, #459) OLD script: 1 warning (MemPalace#1377) Both causes are the same defect from the other end. `head -1` decided WHICH lines are read; this decides WHAT counts as a claim on a line. #459 README.md:297 and FORK_CHANGELOG.md:356 are the heading "purge / prune / mined share one open-and-refuse sequence", linking commit `459efab`. There is no #459 on either line — `/459` matched inside the COMMIT HASH, and "open-and-refuse" supplied a whole-word "open". A right boundary does not help: `/459` is followed by `e`. MemPalace#1377 FORK_CHANGELOG.md:81 was MY OWN changelog entry, quoting the control sentence verbatim. The entry documenting the defect reproduced it — the `self-quoting-retraction` shape from the #503 spec, which is how #511 went green and then warned again once its own entry landed. So, three rules now: * `/pull/$n`, never a bare `/$n`. A commit hash is not a PR reference. * A state word must appear in a CLAIM SHAPE — a parenthesised marker "(OPEN)", or a copula "is/was/stays/remains/now [still] open". Checked against all 13 cases in this repo: every real claim kept (#23 "is still OPEN", #168 "stays open", MemPalace#665/MemPalace#1087 "(OPEN)", MemPalace#1094 "(open upstream"), every prose case dropped ("open-and-refuse", "open the drawers", "opencode", "openai-compat", "reopened"). * The entry for this change does not quote its own control sentence. It states a MERGED claim for MemPalace#1377, which is what MemPalace#1377 is. Result with real `gh` on the repo itself: both the base script and this one report zero PR-state warnings. The base's cleanliness is incidental — its `head -1` happens to land on a line without a claim, and moved there only because #509/#512/this entry changed the changelog. This one is clean for a reason. shellcheck findings 4 -> 2 (both remaining are pre-existing warnings; both former ERRORS are gone). Tests: 5 more (state word without a claim; nebula's "open the drawers" with the PR alone on the line, since the real one is spared only incidentally by a second PR sharing it; a commit hash is not a PR reference; a /pull/ URL still counts; a parenthesised marker is still a claim). 16 total, and two more mutations verified with the target-exists assertion: claim shape -> bare word -> 3 tests /pull/N -> /N -> 2 tests One test asserted something the check cannot do — a PR referenced only by URL is never examined, because the number list is harvested from `#NNNN` alone. Pre-existing and out of scope; the test now says so rather than pretending to cover it. Full suite 7448 passed, 82 skipped. check-docs passes on itself. Part of #516 * docs(fork-changes): renumber to seq 157 after the #511/#522 rebase Rebased onto 4c6a8d0. `--next-seq` re-run after the final fetch says 157; taken from the tool rather than assumed. Generated artefacts re-rendered from main's side, never hand-merged. Part of #516
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.
What
scripts/check-docs.sh's PR-state step now matches#$nor/pull/$n, never a bare/$n. Plus five instances and adirectionfield in the #503 failure-shape spec.Why — the measured incident
The step extracted candidate numbers as
#NNNNbut then matched them per-document as(#$n|/$n). A PR number is also a valid commit-sha prefix, so/459matched.../commit/459efab, and the README line carrying that sha reads "purge / prune /mined share one open-and-refuse sequence". Upstream
MemPalace/mempalace#459isCLOSED, the substring test saw that state word, and the checker reported drift against
a document that contains no such reference at all —
grep -c '#459' README.md→ 0.Twelve referenced numbers currently prefix a referenced sha (#22, #45, #46, #47,
#59, #63, #69, #82, #86, #167, #428, #459). Only #459 warned, because only that line
happened to carry a contradicting state word — the other eleven were latent, not
absent.
STRICT_PR_STATE=1promotes these to errors; enabling it would have gone redon false positives and invited someone to "correct" accurate documentation.
A correction to my own reasoning, recorded as asked
I first proposed deleting the slash alternative as dead weight. The docs falsified
that:
/pull/1000,/pull/1021and others are real references it legitimately catches.Deletion would have blinded the check on genuine URLs. Narrowing to
/pull/$nkeeps thefunction and drops the collision — the right fix, for a reason I initially had wrong.
Controls — both directions, because the obvious wrong fix is to silence the check
Reproduce:
bash scratch/refuted-claims-provenance/nebula-505b-matcher-control.sh <repo>for (a),
… --control-bfor (b). The negative control is the load-bearing one — withoutit, (b)'s WARN could have been a constant and would have proved nothing.
Two failures this change produced while being made
The harness indicted the shipping behaviour. My first two-way control reported
BLINDfor both arms, which reads as "the fix breaks the check" and would have discarded a
correct change. The cause was mine —
|used as a field delimiter in strings containing|. The tell: OLD failing is implausible, because OLD is the shipping behaviour and isknown to warn on #459. ⭐ When an instrument indicts the shipping behaviour, that is
the implausible arm — check the instrument before the subject. First
false-alarmonrecord, which is why
directionis now a field.The changelog entry re-triggered the warning it documents. After the fix, check-docs
went green — then warned again once the entry describing the defect landed, because that
prose re-supplied
#459and a state word on one line. 2g cards this as a retractionquotes its target. Fixed by keeping the real state adjacent to the reference; both
are recorded in the spec.
Spec changes (#503)
direction: false-confidence | false-alarmadded to the record schema. The corpus isalmost entirely false-confidence, so an index without it answers an agent arriving with
"my check says the fix broke it" using cards about distrusting green.
bare-path-receipt(PART 26),tally-line,sha-prefix-match,harness-indicts-shipping,self-quoting-retraction.Oracle PART 25's closing tally line rather than its section headers, which dropped
§25.1 and §25.10 and admitted two §25.14 near-misses. Now 32 on record, 28 citable,
as a per-source table naming the unit counted (PART 25 is 8 sections, ≥10
sub-instances; §25.6 and §25.9 each hold two).
reviews, and the spec's own §2 rule forbids citing by a position that moves.
Tests
None — shell script + docs.
scripts/check-docs.shexit 0, zero warnings, all threerenderers idempotent.
Part of #503