fix(check-docs): examine every mention of a PR, not just the first (#516) - #520
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 39 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 (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR updates ChangesPR-state validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The parser, regression coverage, and documentation are consistent, with no concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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
force-pushed
the
fix/516-check-docs-all-mentions
branch
from
September 18, 2026 04:43
97a4796 to
faf0b5a
Compare
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
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
force-pushed
the
fix/516-check-docs-all-mentions
branch
from
September 18, 2026 06:25
71241eb to
53f05db
Compare
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.
check-docsstep 4 answered "does the FIRST mention agree?", not "do all mentions agree?" —grep … | head -1per document meant a drifted claim appearing after a correct one was invisible.Reproduced before fixing
Appending
PR #1377 is still open upstream.to the end of README.md, with MemPalace#1377 MERGED upstream:The control was proven reachable before its silence was trusted
This is the part that makes the reproduction mean anything. A control that is present but never looked at produces "did not fire" for the wrong reason, and that reads identically to "no drift". So before believing the pass:
Predictions were pre-registered before any code was read for a fix (
scratch/refuted-claims-provenance/lucid-516-prereg.md) and all five held, including the one flagged in advance as most likely to ruin the change — that iterating every line must not add findings on the unmodified tree.Three defects, now one scan
#45matched#452;/45matched/459efab. The commentary scan beside it used[^0-9], so the two loops could read different lines and reach a conclusion neither supportedThe second and third were latent while only one line per document was examined, and the fix for the first amplifies them — so all three had to move together.
Measured differentially over the whole repo
Every PR stubbed MERGED, so every clean claim becomes checkable. 7 findings before, 7 after — and five different ones in each direction:
The unchanged total is the trap. Anyone checking whether the count moved would conclude nothing had.
On the real tree with real states, before and after are both clean — this change surfaces no new drift today; it removes the blind spot that would hide tomorrow's.
Also removes both shellcheck errors in the file (SC1087 —
$n[read as array indexing) and adds none. The two remaining warnings are pre-existing.A limit, asserted rather than implied
When one clean line claims the true state and another claims a different one, drift cannot be told from history, so the PR is skipped. I considered flagging disagreement — the issue's own sketch suggests it — and measured it instead: exactly one such pair exists in the repo,
#1024in FORK_CHANGELOG.md, reading "pushed to the open MemPalace#1024 PR branch (squash-merged upstream)" alongside an authoritative "(MERGED)". That is correct documentation of a merged PR, and it is textually the same shape as genuine drift. Flagging disagreement would fire on it, so the rule stays and a test records the cost.Tests
11 new in
tests/test_check_docs_pr_state.py, driving the real script over a fixture tree with a stubbedgh— no network and no shared API quota. Four guards mutation-verified, each mutation asserting its target string exists, so a mutation that fails to apply reports loudly instead of as "nothing to guard":head -1Three of these tests were not evidence when first written, and were rebuilt rather than kept:
closed, a state the MERGED branch ignores, so it passed with the commentary skip removed.#45/459efabcase.test_drift_on_the_first_mention_still_detectedis kept deliberately as the harness's own positive control: it must pass before the fix, which is only possible if the harness can see a finding at all.check-docs passes on itself with the fixed step 4 running live. Full suite 7443 passed, 82 skipped.
ruff check+ruff format --checkclean.Part of #516
Update: a state WORD is not a state CLAIM (nebula's measurement applied)
The first pass above fixed
head -1and matched state words as whole words. That was not enough, and I could only prove it by running the check with REAL upstream states.Two corrections to my own verification, which are why this was nearly missed
My "no new false positives" run was not a real-state check. The stub returned a state for one PR number and nothing for the rest, so every other PR was skipped and "clean" proved nothing. With real
gh, the first pass added a warning:The control line nebula cited (
FORK_CHANGELOG.mdL246) is blank in this tree — that measurement was taken onfix/505b-check-docs-pr-matcher, so the line number did not transfer. Found by content instead: it is L358 here. And L356 is a worse case that the line-number coordinate could not point at.What the two real warnings actually were
README.md:297,FORK_CHANGELOG.md:356459efab. There is no#459on either line —/459matched inside the commit hash, and "open-and-refuse" supplied a whole-word "open". A right boundary cannot help:/459is followed bye.FORK_CHANGELOG.md:81self-quoting-retractionshape from the #503 spec, exactly as #511 went green and then warned again once its own entry landed.Three rules now
/pull/$n, never a bare/$n. A commit hash is not a PR reference.(OPEN), or a copulais/was/stays/remains/now [still] open. Validated against all 13 cases in this repo: every real claim kept (docs: post-pgvector-merge housekeeping (YAML + FORK_CHANGELOG + row inventory) #23 "is still OPEN", Publish fork-side REPORT.md + fix README cross-link — Engram-2 E2E QA run done in multipass-sme#72/#74 #168 "stays open", Add optional PostgreSQL backend with pg_sorted_heap support MemPalace/mempalace#665/feat(cli): addmempalace purge— delete drawers by wing/room MemPalace/mempalace#1087 "(OPEN)", refactor(backends/chroma): coerce None metadatas to{}at backend boundary (closes #1020) MemPalace/mempalace#1094 "(open upstream"), every prose case dropped ("open-and-refuse", "open the drawers", "opencode", "openai-compat", "reopened").The requirement, measured
With real
ghon the repo itself, both the base script and this one report zero PR-state warnings. Worth being precise about the base's: its cleanliness is incidental —head -1happens to land on a line without a claim, and only moved there because #509, #512 and this entry changed the changelog. This one is clean for a reason.shellcheckfindings 4 → 2; both remaining are pre-existing warnings, and 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, with two more mutations verified via the target-exists assertion:/pull/N→/NOne test asserted something the check cannot do — a PR referenced only by URL is never examined, because the number list is harvested from
#NNNNalone. 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.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests