feat(doctor): --curated flags curated docs newer than what the palace indexed (#451) - #490
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 14 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
force-pushed
the
feat/451e-doctor-curated
branch
from
September 11, 2026 06:31
30355fb to
7541f1b
Compare
… indexed
The palace keeps serving a pre-edit copy of CLAUDE.md or docs/*.md until it
is re-mined, and nothing says so. `doctor --curated` now reports, per file,
whether the copy on disk has moved on since the palace read it.
Opt-in, because the budget is real: the five existing checks run in ~405ms
all together (measured), and the cheapest reliable source of recorded mtimes
is one `mempalace_mined` call. Default `doctor` is untouched — measured at
365-372ms with this branch.
Staleness is decided by `provenance.source_stale` and nowhere else. It
already owns the comparison, the 60-second grace window ("a file touched
within a minute of its own mine is the mine, not an edit") and the honest
`None` cases; a second notion here would drift from the one search uses.
Four outcomes, and only the first is a ✗: stale; never indexed (reliable,
because the source list is an ENUMERATION — "absent from a search's top N"
would not be); undecidable; fresh. Two sources of recorded mtimes, both
already speaking the same `{source_file: mtime|None}` shape — the daemon's
`max_source_mtime` (palace-daemon#266, one request) and
`palace.prefetch_mined_set` for local/palace-host mode.
Unknown is never clean. A daemon predating the field, a source with no
recorded mtime, a file this host cannot stat, a recorded time in the future
— all report as undecidable. Measured on production: of six sampled
curated-doc sources, TWO have no recorded mtime, so reading None as fresh
would silently clear a third of them. Note the deliberate disagreement with
`prefetch_mined_set`, whose docstring says to treat None as STALE: right for
the miner, where re-mining is cheap, and wrong here, where it would
fabricate "you edited this" out of missing bookkeeping.
Two things the real-binary probe found that the unit tests could not, per
COMMON-DRAIN's new rule:
`add()` coerced `ok` with `bool()`, so every warn was stored as False. That
made warns render as ✗ — the glyph map's "warn" entry was unreachable — and
flip the exit code, both against intent already in the code (`ok_all` tests
`is not False` so a None passes; the closing message points at ✗ lines
only). Unnoticed because all five checks are ✓ on a healthy host, so no warn
had ever been rendered. Fixed here rather than deferred because the curated
check's "cannot tell" outcome is unexpressible while it stands. Behaviour
change worth noting: a warn now exits 0.
And run from a linked worktree, every curated doc reads "never indexed" —
true, since the palace recorded the main checkout's absolute paths, and
useless as a signal. The check now says which situation it is in. An
alarming line that means nothing is how a check earns a deselect, which is
exactly what happened to the test in #454.
Verified against the real daemon, read-only: from the main checkout,
`49 never indexed; 1 cannot be checked; daemon predates max_source_mtime`,
exit 0 — matching a direct SQL count of 1 indexed curated source for this
project.
Part of #451
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cap returned only the capped list, so the truncated tail was unrepresentable: it could never populate stale / never indexed / undecidable, and the check printed ✓ "N up to date" exit 0 having examined a fraction of the files. That is the second unhandled inability-to-look in a module whose whole contract is that unknown is never clean — and `test_is_capped` asserted exactly the length that hid it. Measured live, reproducing the reviewer's numbers independently: memorypalace 72 curated docs 50 examined 22 unexamined 2g 795 curated docs 50 examined 745 unexamined realmwatch 148 curated docs 50 examined 98 unexamined 2g could therefore report a clean bill of health from 6.3% coverage. `_curated_doc_paths` now returns `(paths, total_found)`. A truncated run is capped at warn and can never be ok, and the unexamined count is stated on EVERY outcome rather than only the clean one — a ✗ that hides how much it did not look at is still an under-reported answer. A stale file still wins the verdict: truncation downgrades ✓ to !, it does not upgrade ✗ to !. `CLAUDE.md` now survives the cap BY CONSTRUCTION (placed first, then the docs list capped). Sorting the combined list happened to do that, because `C` sorts before `docs/`, but a project with a capitalised docs directory would have lost it silently. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One new file under docs/fork-changes/, `seq` 146 from scripts/fork_changes.py --next-seq, `commit: HEAD` and `fork_pr: 490` so the merge step resolves the squash sha from the right PR. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
force-pushed
the
feat/451e-doctor-curated
branch
from
September 11, 2026 07:51
1c0378d to
c200dff
Compare
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…ering it as a link (#476) lucid's finding: seven entries merged since #480 still read `commit: HEAD`, and step 2b could not see any of them. Verified, and the mechanism is worth stating precisely because the obvious fix would not have worked: fork_changes.iter_commit_refs SKIPS HEAD, so those entries never reach the ancestry predicate at all. Hardening the predicate alone changes NOTHING. (`git merge-base --is-ancestor HEAD HEAD` also exits 0, so both halves had to learn about the literal.) ⇒ 2b caught a WRONG sha and was blind to a MISSING one. `--strict-resolved` / `STRICT_RESOLVED=1` enumerates HEAD entries (`fork_changes.py --commit-refs --include-head`) and fails on them, naming the entry and telling you to run the sweep. Wired in CI to `push` + `refs/heads/main` only: a pull request legitimately carries HEAD, because the squash commit it will point at does not exist yet. Loud, never auto-fixing — resolving is the sweep's job. POSITIVE CONTROL, both directions, on the same tree (1 HEAD entry): permissive -> exit 0 --strict-resolved -> exit 1, "entry 'init-test-cwd-relative-syspath' still has `commit: HEAD` on main" STRICT_RESOLVED=1 -> exit 1 `https://github.com/techempower-org/mempalace/commit/HEAD` is a VALID GitHub URL that resolves to whatever is at main's tip. So the 8 links in FORK_CHANGELOG.md and 7 in the README table pointed at an unrelated commit while looking exactly like real references — a reader cannot tell them apart, which makes it worse than an obviously missing value. `commit_link()` now renders "`HEAD` — pending resolution" as plain text. After this: 0 such links in either file. Worth noting an old entry's body already described "broken ``[HEAD](commit/HEAD)`` links", so this had been seen before and came back. A rendering rule is a better place for it than a habit. More HEAD entries land after this (#477, #490, stage A, #485), so a resolution commit would be stale before it merged. The sweep is one dedicated PR at the very end of the cascade, and it is the first PR the strict check has to pass. docs/fork-changes/README.md now says the sweep is the lead's last step of every wave, and why per-lane resolution cannot work. Caught while wiring CI: my first edit added a SECOND `env:` key to the check-docs step, which YAML resolves by keeping the last one — silently dropping `GH_TOKEN`, which step 4 needs for upstream PR state. Merged into the existing block and asserted both keys survive by parsing the workflow. 9 tests (65 in the three docs-pipeline files). Full suite 7076 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
… for unresolved placeholders (#476) (#492) * fix(scripts): resolve `commit: HEAD` from the commit that added the entry file (#476) `fork_pr` could not carry this on its own: a lane cannot know its own PR number while writing the entry, which is the same chicken-and-egg as the sha. #480's own entry is the proof — I wrote `fork_pr: 0` as a placeholder, deleted it rather than ship a wrong number, and left an entry nothing could resolve. The commit that ADDED an entry's file is the squash commit by construction, since the file arrives with the pull request: git log --follow --diff-filter=A --format=%H -- docs/fork-changes/<file> Deterministic, offline, needs nothing from the author, and immune to a squash subject reworded at merge time — which defeated 4 of the 27 cases in the #472 sweep. `--follow` so a renamed entry file still resolves to its original add, and the LAST add is taken, since a rename history can list several. `fork_pr` becomes a cross-check rather than the key: when present, the API answer is compared and a DISAGREEMENT refuses to resolve, because two independent mechanisms disagreeing is the worst case in which to pick one. It also still works as the fallback when file-add cannot answer. This runs ONLY on an entry whose `commit` is literally `HEAD`. Every one of the 137 entry files that predates the one-file-per-entry split was created by the SPLIT's own commit. So asking "what added this file" about an already-resolved entry returns the migration commit. MEASURED on this branch, before the guard existed: entry `scripts-maintain-fork-changes` field says 9060e09 (correct) git log --diff-filter=A on its file → 6da8775 (the split) That would have rewritten 137 correct historical shas to one wrong value — and 6da8775 IS an ancestor of main, so it would have passed the new ancestry check forever and read as correct. Exactly the wrong-but-plausible failure the reviewer named on #480. A test pins the boundary in both directions, and asserts the resolver never even asks about an entry that already has a sha. Ran against main: resolved exactly ONE entry — #480's own, `commit: HEAD → 6da8775` — and left the other 137 untouched, which is the boundary holding in practice rather than only in the unit test. 6da8775 verified an ancestor of main. The 2026-05-28 `scripts-maintain-fork-changes` entry needed no work: its `commit` is already 9060e09 and already an ancestor. It looked like an unresolved HEAD entry only because the entry is ABOUT HEAD resolution, so the literal string `commit:HEAD` appears twice in its own prose — a field-shaped grep matches the changelog entry that documents the field. docs/fork-changes/README.md updated: `fork_pr` is documentation and a cross-check, not load-bearing, and the boundary hazard is written down so nobody widens it. 5 tests (23 in the file). Full suite 7065 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs+fix: fill fork_pr after `gh pr create`; surface an unverifiable one (#476) GitHub's next PR number is not predictable: a lane wrote `fork_pr: 483` before opening and the PR came back 490. A guessed number is the #472 failure in a new coat — a confidently wrong field instead of a visibly unresolved one — so the directory README now says to fill `fork_pr` in AFTER `gh pr create` returns it, and that leaving it absent is the safe default since file-add is primary. Worth stating what a wrong number can and cannot do, because it decides how much this matters. It cannot corrupt the sha: - points at another MERGED PR -> its different answer trips the existing cross-check refusal - unverifiable (404/unmerged) -> file-add stands alone and is already right MEASURED: PR 483 does not exist (404), so lucid's guess falls in the second bucket and resolves correctly anyway. But "resolves correctly anyway" is how a wrong field survives, so the second case is now printed as an ADVISORY — "resolved from file-add; fork_pr #483 is unverifiable — check it" — rather than silently ignored. Deliberately not folded into the blocking list: an advisory that fails `--check` is an advisory the next person deletes. The function returns `(changes, unresolved, notes)` so the advisory is actually reported. I had first appended to a `notes` list that nothing returned — dead code that LOOKED like it reported something, which is the same class of defect as a check that enumerates nothing. 1 test (24 in the file), full suite 7071 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(check-docs): reject the literal `commit: HEAD` on main; stop rendering it as a link (#476) lucid's finding: seven entries merged since #480 still read `commit: HEAD`, and step 2b could not see any of them. Verified, and the mechanism is worth stating precisely because the obvious fix would not have worked: fork_changes.iter_commit_refs SKIPS HEAD, so those entries never reach the ancestry predicate at all. Hardening the predicate alone changes NOTHING. (`git merge-base --is-ancestor HEAD HEAD` also exits 0, so both halves had to learn about the literal.) ⇒ 2b caught a WRONG sha and was blind to a MISSING one. `--strict-resolved` / `STRICT_RESOLVED=1` enumerates HEAD entries (`fork_changes.py --commit-refs --include-head`) and fails on them, naming the entry and telling you to run the sweep. Wired in CI to `push` + `refs/heads/main` only: a pull request legitimately carries HEAD, because the squash commit it will point at does not exist yet. Loud, never auto-fixing — resolving is the sweep's job. POSITIVE CONTROL, both directions, on the same tree (1 HEAD entry): permissive -> exit 0 --strict-resolved -> exit 1, "entry 'init-test-cwd-relative-syspath' still has `commit: HEAD` on main" STRICT_RESOLVED=1 -> exit 1 `https://github.com/techempower-org/mempalace/commit/HEAD` is a VALID GitHub URL that resolves to whatever is at main's tip. So the 8 links in FORK_CHANGELOG.md and 7 in the README table pointed at an unrelated commit while looking exactly like real references — a reader cannot tell them apart, which makes it worse than an obviously missing value. `commit_link()` now renders "`HEAD` — pending resolution" as plain text. After this: 0 such links in either file. Worth noting an old entry's body already described "broken ``[HEAD](commit/HEAD)`` links", so this had been seen before and came back. A rendering rule is a better place for it than a habit. More HEAD entries land after this (#477, #490, stage A, #485), so a resolution commit would be stale before it merged. The sweep is one dedicated PR at the very end of the cascade, and it is the first PR the strict check has to pass. docs/fork-changes/README.md now says the sweep is the lead's last step of every wave, and why per-lane resolution cannot work. Caught while wiring CI: my first edit added a SECOND `env:` key to the check-docs step, which YAML resolves by keeping the last one — silently dropping `GH_TOKEN`, which step 4 needs for upstream PR state. Merged into the existing block and asserted both keys survive by parsing the workflow. 9 tests (65 in the three docs-pipeline files). Full suite 7076 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(scripts): file-add resolution against real git, incl. the no-fork_pr case (#476) The existing tests stub `adding_commit`, so they prove the wiring but not the git invocation. These drive real `git log` in a temp repo. The case that matters is #480's own entry: `commit: HEAD` and NO `fork_pr`, because the entry was written before the PR existed and a guessed number would have been worse than none. It is the reason file-add is the primary mechanism rather than a fallback. VERIFIED live on this repo as well — the entry's file was added by 6da8775, which is #480's squash commit. Measured on main (7df8dee): 8 entries carry `commit: HEAD`, and exactly ONE of them lacks `fork_pr` — that one. Four tests: - no `fork_pr` resolves to the ADDING commit, not the tip, with two unrelated commits on either side of it, and emits no advisory (there is no number to cross-check or warn about) - a LATER EDIT of the entry does not move the answer (`--diff-filter=A` means the add, so a typo fix or reworded body must not re-point a resolved sha) - a RENAMED entry file still resolves to its original add. Entry filenames embed the date, so correcting a date renames the file; `--follow` is what keeps that from silently re-pointing the sha at the rename commit - an untracked path returns None and is reported, never guessed Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): entry for the sha-resolution hardening (#476) `commit: HEAD` and no `fork_pr` yet — deliberately. The PR number does not exist until `gh pr create` returns it, and a guessed number is a confidently wrong field (a lane guessed 483 and got 490). It is filled in immediately after the PR is opened, in the following commit. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scripts): never resolve an entry against a feature branch (#476) Filling in `fork_pr: 492` — the number `gh pr create` actually returned, not a guess — surfaced a way file-add could recreate #472 through a new door, so this closes it before the mechanism ships. Asked about a FEATURE BRANCH, "what added this file" truthfully answers *the branch commit*. Written into the entry, the squash would orphan it moments later — which is precisely the failure this mechanism replaced. MEASURED on this branch: `git_file_add_commit(<my entry>)` with the default branch returns f87f9c1, the branch commit. The default `--branch=origin/main` already prevents it, and that is the property rather than an accident: against `origin/main` an unmerged entry finds NO add, so it stays `HEAD` instead of acquiring a sha that is about to become unreachable. Verified end to end — running the sweep command on this branch resolved the 8 landed entries from main's history and left THIS PR's own entry untouched at `HEAD`, which is exactly the behaviour the sweep needs. A test now pins both halves in one place: asked about the branch it returns the branch commit, asked about main it returns None, and `resolve_head_by_file_add` reports rather than resolves. The README says resolve against `origin/main`, never a branch, and why. (The 8 entries that run resolved in my working tree were reverted — the sweep is its own PR, per the lead.) 1 test (29 in the file). Full suite 7304 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scripts): refuse to write a sha that is not an ancestor of origin/main (#476) Oracle's rider, and it is a better fix than what I had. Before any resolved sha is written, `git_is_ancestor(sha, origin/main)` must hold; otherwise the entry is reported and `--check` exits non-zero. The `--branch=origin/main` default already made a branch commit hard to write. But a default is one argument away from being wrong, and the question the caller actually needs answered is not "which ref did you ask about" — it is "is this commit on main". Asserting that directly makes the hazard IMPOSSIBLE rather than unlikely, and it holds whatever `--branch` is passed. Correcting my own framing while I am here, because the lead caught it and the record should be right: the previous commit ("never resolve an entry against a feature branch") changed NO code — `maintain-fork-changes.py` is byte-identical across it. It added a test and documentation for a property the `origin/main` default already had. Calling that "closing the hazard" was an over-claim: I documented a safeguard and described it as building one. THIS commit is the code fix. 3 tests: a branch-only sha is refused and never written; the verify ref stays `origin/main` even when `--branch` is a feature branch; an ancestor sha still resolves. The real-git fixtures pass `verify_ref` explicitly, since a temp repo has no `origin/main` — the real `git_is_ancestor` still runs, against that repo's own ref. Verified live: with the gate in place the sweep still resolves all 8 landed entries against origin/main (reverted here — the sweep is its own PR). Full suite 7307 passed. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scripts): bind via_api unconditionally so term order cannot break it (#476) Oracle's note: the unverifiable-`fork_pr` advisory reads `via_api`, which was assigned only inside `if pr and fetch is not None:` — so it was safe purely by SHORT-CIRCUIT ORDER in the later condition. Reordering those terms, a harmless-looking edit, would have turned it into a NameError on every entry that carries no `fork_pr`, which is exactly the entry the file-add mechanism exists for. Asked for a comment; binding it unconditionally instead, because that is Oracle's own principle applied one level down — the ancestry gate was preferred over a default plus a README for the same reason. Make the edit safe rather than forbid it. The comment stays too, saying why. PROVEN rather than argued: a copy of the module with the terms reordered (`if not via_api and pr and fetch is not None:`) resolves an entry with NO `fork_pr` and raises nothing. Before this, that reorder was a latent crash. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
mempalace doctor --curatedreports, per curated doc, whether the copy on disk has moved on since the palace read it. The palace keeps serving a pre-editCLAUDE.mduntil it is re-mined, and until now nothing said so.mempalace half of #451 item E; the daemon half is palace-daemon#266.
Opt-in, because the budget is real
mempalace doctorruns in 405ms for all five existing checks (measured, two runs). The cheapest reliable source of recorded mtimes is onemempalace_minedcall. So the check is behind--curated, and the default path is untouched — measured at 365–372ms on this branch.One staleness notion, not two
The comparison goes through
provenance.source_staleand nowhere else. It already owns the mtime-vs-index comparison, the 60-second grace window ("a file touched within a minute of its own mine is the mine, not an edit") and the honestNonecases. A test asserts the delegation, because a second notion drifting from the one search uses is the failure to avoid here.Recorded mtimes come from whichever source can answer, and both already speak the same
{source_file: mtime|None}shape: the daemon'smax_source_mtime(palace-daemon#266, one request per run) orpalace.prefetch_mined_setfor local / palace-host mode.Four outcomes, and only one is a ✗
Unknown is never clean. Measured on production: of six sampled curated-doc sources, two have no recorded mtime, so reading
Noneas fresh would silently clear a third of them.This deliberately disagrees with
prefetch_mined_set, whose docstring says to treatNoneas stale. That is right for the miner, where re-mining is cheap and safe, and wrong here, where it would fabricate "you edited this" out of missing bookkeeping. The disagreement is documented at both ends so a future reader does not "fix" it.Two defects the real-binary probe found that the unit tests could not
Per COMMON-DRAIN's new rule — and both were invisible to green tests:
1.
add()collapsed the doctor's tri-stateok. It coerced withbool(ok), so everywarnwas stored asFalse. Warns therefore rendered as ✗ — the glyph map's"warn"entry was unreachable dead code — and flipped the exit code. Both are against intent already present in the file:ok_alltestsis not Falseprecisely so aNonepasses, and the closing message points at the ✗ lines only. It went unnoticed because all five checks are ✓ on a healthy host, so no warn had ever been rendered. Reproduced in isolation before changing anything.Fixed here rather than deferred because the curated check's "cannot tell" outcome is unexpressible while it stands.⚠️ Behaviour change worth a reviewer's eye: a warn now exits 0 (a stale hook.log or a non-empty replay queue no longer fails the command). That matches the design the code already encodes; if any cron gates on
doctor's exit for warns, this changes it.2. Run from a linked worktree, every curated doc reads "never indexed". True — the palace recorded the main checkout's absolute paths — and useless as a signal. I found it by running the real binary from the worktree I was writing in:
50 never indexed, which is both correct and alarming for no reason. An alarming line that means nothing is how a check earns a deselect, which is exactly what happened to the test in #454. The check now names the situation.Live verification, read-only
From the main checkout against the real daemon (which does not yet carry #266's field):
Cross-checked against a direct SQL count: exactly 1 curated source for this project is indexed (
CLAUDE.md, 25 drawers). So 49 never-indexed and 1 undecidable is precisely right, and it exercises the older-daemon path end to end.Tests
21 new in
tests/test_doctor_curated_docs.py: enumeration (CLAUDE.md +docs/**/*.mdonly, absolute, deterministic, capped, missing project) ×4; the tri-stateok×3 (warn renders as!and exits 0; an error still exits 1; the!glyph is reachable); the daemon tier ×7 (stale, fresh, no recorded mtime, a daemon without the field, never-indexed named separately, exactly one call per run, a daemon error is undecidable not a failure); the worktree note ×2; the--curatedgate ×2 (absent by default, and an olderNamespacewithout the attr does not crash); the local tier ×2; theprovenance.source_staledelegation ×1.Full suite: 7086 passed, 82 skipped. ruff check + format clean; docs entry added in #480's per-entry format (
seq140,commit: HEAD,fork_pr: 483), all three renderers run,check-docs.shclean.Nothing was written to the palace and nothing was deployed; every probe was a read.
Part of #451