refactor(docs): one file per fork-change entry; ancestry-checked, merge-resolved shas (#473 #472 #476) - #480
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 159 files, which is 59 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (159)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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/473-fork-changes-per-entry
branch
from
September 11, 2026 05:14
753decf to
5a48baf
Compare
jphein
force-pushed
the
feat/473-fork-changes-per-entry
branch
from
September 11, 2026 05:29
dc43c65 to
fa1c15e
Compare
jphein
pushed a commit
that referenced
this pull request
Sep 11, 2026
Review fixes on #478. (a) `mempalace mine --daemon --no-tunnels` silently did the expensive thing. The --daemon local-job-queue branch returns BEFORE the daemon-strict warning below it, its payload carries no tunnels field, and service.execute_job calls mine() without compute_derived. It now warns on that branch too. Accepting a skip and not honouring it is the failure shape this flag exists to avoid. (b) Two tests pin that `_validate_palace_fts5_after_mine` still runs with compute_derived=False. It already did — the gate was placed inside `if not dry_run:` but outside the validation deliberately, because integrity work is not a derived analytic. Nothing pinned that placement, and it is exactly the kind of thing an editor's re-indent breaks silently. Both tests passed on first run; they are pins, not fixes. (c) The zero-upsert short-circuit already keys on DRAWERS upserted, not files processed; the docstring now says so and why. Since palace-daemon#262 most drain mines walk their files and upsert nothing because the stored source_mtime still matches, so a files-processed test would almost never fire. Also drops this branch's docs/fork-changes.yaml entry: #480 replaces the monolith with one file per entry under docs/fork-changes/, and the wave cascade lands that first, so an entry written here would land in a deleted file. The entry is re-added in the new format, with `commit: HEAD` and `fork_pr:`, once #480 merges. The README/CLAUDE test count bump stays — it is independent and check-docs requires it. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
Replaces the branch sha with 'HEAD' plus fork_pr: 479, so the entry stops pointing at a commit that the squash-merge will delete. Verified the pre-#480 renderer and check-docs both accept it: hash-resolution (check 2) passes because HEAD resolves, and the changelog renders unchanged. Test count 7156 -> 7157 for the wing-normalization test. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…ld (#478) * feat(cli): mine --no-tunnels — skip the post-mine derived-graph rebuild Every projects-mode mine recomputes three derived analytics for the whole wing, however few files changed: cross-wing topic tunnels, within-wing hallways, and cross-wing entity tunnels. The cost scales with the WING, not with the change. Measured on the palace host: a 31-file (148 KB) memory sweep spent 29+ min of CPU and 1.6-4.1 GB RSS in that block, wrote zero drawers in that window, and held the exclusive mine lock throughout, with twelve more sweeps queued behind it. A sweep of a handful of memory files does not need the cross-wing graph at all, so the cheapest fix is to not do the work. `mine(compute_derived=False)` / `mempalace mine --no-tunnels` skips it. The drawers filed are byte-identical either way — pinned by a test — and only the derived graph is left un-refreshed until the next full mine. All three steps are gated together on purpose. They are a dependency chain (entity tunnels read the hallways the step above wrote), and gating only the two `_compute_*_tunnels_*` calls the issue names would leave the hallways load + full rewrite in place, which is most of the I/O. The help text says so rather than letting the flag's name under-promise what it turns off. Default is unchanged and pinned in three places, including the two existing exact-call assertions, so a silent flip of the default fails loudly. In daemon-strict mode the flag is listed in the existing ignored-local-flags warning: the daemon's /mine body has no `tunnels` field yet (palace-daemon#262), and a skip the operator asked for and did not get is worse than one they were told about. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): mine --no-tunnels Entry + all three render targets regenerated; README/CLAUDE test count re-derived 7108 -> 7118. scripts/check-docs.sh clean on all seven checks. The entry also records the measured root cause for the follow-up: the cost is create_tunnel's per-call load+rewrite of tunnels.json (O(n^2)), not the 993 MB hallways.json (~37 s per mine at production size). Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * perf(miner): a mine that filed no drawers skips the derived-graph rebuild No flag needed: a mine that upserted nothing has nothing to recompute for. Measured on the palace host: a requeued CLAUDE.md mine wrote zero drawers — all 661 of that file's drawers still carried the earlier filed_at, so the stored-mtime check skipped the file — and still spent 14+ minutes at 3.5 GB in the derived-graph block, against a 4G cgroup cap, while the palace wrote nothing at all. This is a trade, not a free win, and the code says so: _compute_entity_tunnels_for_wing reads EVERY wing's hallways, so a no-op mine of wing A could in principle have picked up a cross-wing tunnel created by a mine of wing B. The next mine of A that actually files something, or a full recompute, picks it up. That is a fair price for not burning 14 minutes and 3.5 GB to rewrite identical output. The skip prints a line rather than being silent, so an operator reading mine output can tell "nothing to do" from "something went wrong". Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): record the zero-upsert short-circuit Entry body + test count extended; all three renderers re-run; README/CLAUDE count 7118 -> 7121. check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): --daemon must not accept --no-tunnels and ignore it Review fixes on #478. (a) `mempalace mine --daemon --no-tunnels` silently did the expensive thing. The --daemon local-job-queue branch returns BEFORE the daemon-strict warning below it, its payload carries no tunnels field, and service.execute_job calls mine() without compute_derived. It now warns on that branch too. Accepting a skip and not honouring it is the failure shape this flag exists to avoid. (b) Two tests pin that `_validate_palace_fts5_after_mine` still runs with compute_derived=False. It already did — the gate was placed inside `if not dry_run:` but outside the validation deliberately, because integrity work is not a derived analytic. Nothing pinned that placement, and it is exactly the kind of thing an editor's re-indent breaks silently. Both tests passed on first run; they are pins, not fixes. (c) The zero-upsert short-circuit already keys on DRAWERS upserted, not files processed; the docstring now says so and why. Since palace-daemon#262 most drain mines walk their files and upsert nothing because the stored source_mtime still matches, so a files-processed test would almost never fire. Also drops this branch's docs/fork-changes.yaml entry: #480 replaces the monolith with one file per entry under docs/fork-changes/, and the wave cascade lands that first, so an entry written here would land in a deleted file. The entry is re-added in the new format, with `commit: HEAD` and `fork_pr:`, once #480 merges. The README/CLAUDE test count bump stays — it is independent and check-docs requires it. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: jp <claude2@techempower.org> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Every pull request in a wave inserted at the top of `entries:` in one `docs/fork-changes.yaml`, so every PR conflicted with every other PR on that file and the four artefacts rendered from it — measured across a 10-PR wave on 2026-09-10/11 with ZERO source conflicts. Each merge then forced every remaining PR through a full docs rebuild. Entries now live one per file in `docs/fork-changes/<date>-<id>.yaml`, so concurrent PRs touch disjoint paths. `scripts/fork_changes.py` is the single loader — the renderer, check-docs and maintain-fork-changes all go through it, so they cannot drift on ordering or validation. Ordering is `seq` descending, ties on (date desc, id asc). #473 proposed sorting by (date, id) instead; MEASURED, that moves 112 of 137 entries, because the file is in hand-curated narrative order and is not even date-descending. That would have made FORK_CHANGELOG a full rewrite and the migration unreviewable, so ordering stays explicit. The key property is that a `seq` COLLISION is harmless: two lanes both picking max+1 write separate files and order deterministically. The conflict was never the number — it was a shared insertion line in a shared file. A numeric collision costs nothing; a textual one blocks a merge. So ordering stops being a coordination point while staying explicit, which is what preserves the narrative order. Also here: - The README fork-change table is UNNUMBERED. A leading row number meant one new entry renumbered every row below it, so any two PRs conflicted across the whole table rather than on one line. - `merged_upstream` (which renders the changelog's closing sections) moves to `docs/fork-changes-meta.yaml`, beside the entries directory rather than inside it: it changes when an UPSTREAM PR merges, not once per fork PR, so a shared file on that cadence does not recreate the problem. Caught because the first loader returned only `entries` and silently dropped 19 rendered lines. ACCEPTANCE: `FORK_CHANGELOG.md` renders BYTE-FOR-BYTE IDENTICAL from the new layout (4104 lines, `diff` clean against the pre-migration file). README changes only the numbering column and the generated-from comment. The entry files were generated from `HEAD:docs/fork-changes.yaml` and the loader is asserted to reproduce the original order exactly, with every field value round-tripped — so the migration is mechanical, not retyped. 14 tests. Part of #473 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ose (#476) The old resolver scanned the 12 lines after a `commit:` line for any `#NN` and mapped that number to a recent commit. Those lines are prose, and the first issue number in an entry's prose is usually not that entry's PR; `#NN` was also unqualified, so an upstream number was indistinguishable from a fork one. MEASURED on the 12 entries this repo actually needed fixed: it resolved 3 and got all three WRONG — one matched against upstream MemPalace#1829 — and silently left 9 on HEAD while reporting success. Two passes now, both entry-wise: 1. `commit: HEAD` resolves from the entry's `fork_pr:` via REST `pulls/N` → `merge_commit_sha`. Exact, because the API reports the commit the merge actually created. REST not GraphQL: GraphQL has been rate-limited for this account during waves, and this is the one lookup that must work at merge time. 2. A sha that is no longer an ancestor of the branch is repaired by matching the dangling commit's SUBJECT to the squash subject (squash subject = branch subject + trailing `(#PR)`), requiring a UNIQUE match. `fork_pr` is a NEW field and deliberately not `pr`: `pr` already means the UPSTREAM PR number (it renders as "*Upstream:*" and check-docs queries it against MemPalace/mempalace), so reusing it would have conflated the two repos — the same unqualified-reference bug in a new place. Both passes refuse ambiguity instead of picking, and an unresolved entry makes `--check` exit non-zero. The failure direction is the whole point: leaving an entry unresolved is harmless and visible, while a confident wrong sha is neither, and check-docs cannot catch it because asserting a sha *resolves* is satisfied by any real commit. Dedup is removed — one file per entry makes a duplicate id a hard error in the loader, not something to clean up afterwards. 16 tests, all offline (git and the API are injected). Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…test count (#472 #473) An earlier draft of this said check #2's `git cat-file -e` was too weak for entry shas. L2 (morpheus-single-file-mine) pointed out it is worse than that: check #2 drops any line matching `/(jphein|techempower-org)/[a-z-]+/commit/` as "cross-repo", which is EXACTLY how the renderer emits every entry's commit link. A reviewer re-check then corrected my correction, which had overshot. Measured: 0 of 137 entry commit-LINK lines survive the filter; the 25 hashes check #2 does verify are prose mentions inside entry bodies ("the first was ``b46f18d``"), 10 of which coincide with an entry sha — so no entry `commit:` field was ever checked AS SUCH, and none for ancestry. Corroboration, and it is the sharper number: 7 of check #2's own 25 hashes are NOT ancestors of main, and it green-lights all of them. `2ffe652` and `ba16b82` are in both sets at once — entry shas that check #2 does verify and passes, while being unreachable from main. So `cat-file -e` is not merely weak in principle; it is actively passing stale hashes today. That is why 13 stale entry shas shipped green: the check that looks like it covers them does not enumerate them, and the one hash class it does enumerate it tests with the wrong predicate. Step 2b is therefore the FIRST verification of entry shas. It reads them from the entries rather than by scraping markdown, so it cannot be fooled by how a line happens to be formatted, and a failure names the file a human has to edit. POSITIVE CONTROL (the check is proven to fail, not just to pass): with the pre-rebase sha `5383040` planted in one entry — git cat-file -e 5383040 -> succeeds (old rule would PASS it) git merge-base --is-ancestor ... -> fails (new rule REJECTS it) and step 2b reports the entry id. Restored afterwards. Sweep: `maintain-fork-changes.py` re-pointed 3 of the 27 non-ancestor entries by exact subject match and REFUSED the other 24 rather than guess. Those are recorded in `docs/fork-changes-legacy-shas.txt` — their change is on main but the commit carrying it cannot be named, because the squash subject was rewritten at merge. A wrong sha would pass every check and mislead a reader; an allowlist line is a visible diff. The header notes that a looser token-similarity pass would recover ~8 more and was deliberately not applied, since hand-applying matches the tool refuses would undercut its contract. Main stays green either way. A literal in README and CLAUDE.md meant every PR that added a test edited the same two lines, so a 10-PR wave conflicted there every time and each merge forced the rest to re-derive it. check-docs now derives the count and reports it (7128 at this commit) instead of asserting a number; README points at `pytest --collect-only -q`. A literal that creeps back is warned about, not failed, since it may be a deliberate release note — but nothing depends on it any more. `docs/fork-changes/README.md` documents the layout for the next author: the `seq` / date / id tie-break and why it is NOT a date sort (a date sort moves 112 of 137 entries), and that `commit: HEAD` + `fork_pr:` is correct on a branch. Requested so nobody "fixes" the ordering later. `tests/test_render_docs.py` updated: four tests asserted the row numbering this change removes, and one asserted a numbered first row. Reported-by: morpheus-single-file-mine (check #2 exclusion) Part of #472, #473 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on (id, sha) Two review fix-firsts on 5a48baf. ## ship-prep.sh step 2/6 would have died on this very branch The step grepped README for "<N> tests pass on `main`" and hard-failed when absent — and the commit that removes the count literal removes exactly that phrase, so the next release prep would have stopped to ask whether the wording changed. The step's other job, `sed`-ing the number in place, is equally obsolete. It now REPORTS the derived count (still useful during release prep) and treats the absent literal as the expected state, warning only if one creeps back. Same contract as check-docs step 1, so the two cannot disagree. Header and step title updated to match. ⭐ Worth naming: I removed a literal and updated the checker that asserted it, but not the *other* script that also asserted it. The grep that finds every reader of a thing you are deleting is the cheap step I skipped. ## The legacy allowlist was keyed on the entry id alone That granted an entry a PERMANENT exemption rather than recording one known-bad sha. Its `commit:` could later be edited to any value — including a plausible WRONG sha, which is a real ancestor and therefore passes every check — and nobody would be prompted again. The file would have quietly become a list of entries nothing verifies. Now keyed on the pair: `<id> <sha>`, and both the shell check and the resolver skip only on an exact match. A resolved or altered entry starts failing until its line is removed, which is the prompt we want. An id with no sha is a hard error rather than a silent full exemption. POSITIVE CONTROL (the new refusal is proven, not assumed): with `cli-cypher-readonly` allowlisted for `32a41b1`, planting a DIFFERENT dangling sha `0c34464` on it makes step 2b report it again instead of skipping. Restored afterwards. Also: `docs/fork-changes/README.md` documents the pair format and that the exemption covers one sha, not the entry. 2 tests (allowlist pair parsing, exemption lost when the sha changes), 18 in the resolver file. Full suite 7043 passed. Part of #472, #473 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lychee caught two dead links to the file the migration deletes —
docs/BIBLIOGRAPHY.md:82 and README.md:284. Grepping the repo for the rest
found nine LIVE instruction sites, not two, so this fixes the class
rather than the two failures CI happened to surface:
docs/BIBLIOGRAPHY.md dead link -> fork-changes/README.md
README.md (x3) dead link + machinery row + setup comment
scripts/render-docs.py module docstring, AND the "This file is
generated. Edit ..." header it EMITS into
FORK_CHANGELOG.md — so the generated file was
telling readers to edit a deleted path
scripts/ship-prep.sh step 1 comment (also retitled: de-dup is gone,
resolution is #476's job now)
website/public/llms.txt prose
docs/integrations/* two prose mentions naming the path
.claude/commands/verify-docs.md the worst one: a LIVE slash command
that told an agent to `yaml.safe_load(
open('docs/fork-changes.yaml'))`. Rewritten to
go through scripts/fork_changes.py, and it now
says `pr:` is the UPSTREAM number while
`fork_pr:` is this repo's. Verified by running
the rewritten snippet: 12 entries with
pr + pr_state reach it.
## CLAUDE.md "Documentation maintenance" rewritten
The repo's own instructions still described editing one YAML at the top
of `entries:`, so the next agent would have been sent to a deleted file
by the very document that is supposed to orient it. It now describes the
directory, why it is a directory, and points at
docs/fork-changes/README.md for the schema and the ordering rule.
The "Lint" subsection's numbered list of check-docs checks was also
stale in three of four items — it promised a README-test-count
comparison that no longer exists, described the hash check as
`cat-file -e` without the ancestry requirement, and called the manifest
"the YAML". Updated, including the new 2b and the (id, sha) allowlist.
A stale description of a checker is worse than none: it tells you the
guarantee you have, and you stop looking.
## Deliberately NOT changed, having read each one
- dated research/spec/integration docs (2026-05-05, 2026-05-15,
2026-05-24, 2026-05-26) — historical snapshots that were accurate when
written; rewriting them would falsify the record
- FORK_CHANGELOG.md body text in older entries, and in the new entry,
describing the old single file — historically correct, and generated
- docs/research/*.json and scripts/probes_v2_git_derived.json — retrieval
EVAL CORPORA with `"expected": "fork-changes.yaml"`. These are
benchmark fixtures; editing them would silently change eval results,
which is a worse outcome than a stale string in a fixture
- scripts/fork_changes.py and tests/test_fork_changes_loader.py
docstrings, which describe what the split replaced
No dead links remain: `grep -rnE '\]\([^)]*fork-changes\.yaml'` over all
markdown returns nothing. markdownlint clean on the five touched docs,
check-docs exit 0, full suite 7043 passed.
Part of #473
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
force-pushed
the
feat/473-fork-changes-per-entry
branch
from
September 11, 2026 05:49
5451dad to
e0dd4a5
Compare
This was referenced Sep 11, 2026
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
One new file under docs/fork-changes/ per #480's per-entry format, with `commit: HEAD` and `fork_pr: 482` so the merge step resolves the squash sha rather than the lane recording a branch sha that goes unreachable. `seq` 139 from `scripts/fork_changes.py --next-seq`. Renders FORK_CHANGELOG.md, the README table and website/public/llms-full.txt. No test-count literal: #480 removed it and check-docs derives the count. Part of #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
First entry in #480's format: one new file, seq from --next-seq, `commit: HEAD` plus `fork_pr: 487` so the merge step resolves the squash sha instead of the lane recording a branch sha that becomes unreachable the moment it lands. No test-count literal to bump — #480 removed it. Part of #461 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…n from a worktree (#454) (#482) * test(init): compare sys.path entries after resolving them against cwd `test_init_filters_sys_path_from_leaked_pythonpath` failed all five sentinel params in any linked worktree, so every lane tonight ran the suite with it deselected and paid a false red before working that out. The behaviour under test was fine throughout. The assertion was the problem: it iterated `for p in sys.path if p`, which deliberately excludes the empty string — and the empty string is the cwd marker `python -c` puts on the path, which is what actually makes the child's import resolve. Measured in both trees with PYTHONPATH set to the sentinel, as the test sets it: main tree RESOLVED_TO <main>/mempalace/__init__.py PARENT_ON_SYS_PATH True worktree RESOLVED_TO <wt>/mempalace/__init__.py PARENT_ON_SYS_PATH False both EMPTY_IN_PATH True The child imports the tree it was launched from, in both cases, via that cwd entry. The absolute-entry match succeeded in the main tree for an incidental reason: the shared venv's editable install adds exactly `/home/jp/Projects/memorypalace` to sys.path, so the comparison found it there. From a worktree the editable entry points at the main tree, the absolute match fails, and the test reports an over-strip that never happened. Fix: normalise each entry with `os.path.abspath(p or os.curdir)` before comparing, so the cwd marker counts as the entry it is. That is faithful to the assertion's stated purpose — "the mempalace package itself must remain importable, so its parent directory must survive on sys.path" — because the cwd entry is how it remained importable. It keeps its teeth for the real failure: the assertion can now only fail if the filter removed both the cwd marker and any absolute entry providing the package, which is exactly an over-strip. A child that cannot import mempalace at all still trips the earlier `returncode == 0` assertion. `MEMPALACE_IMPORTED_FROM` is printed for diagnosis and deliberately not asserted on: which tree the child resolves depends on its cwd, and pinning that would re-introduce the coupling this fixes. Verified both directions: 8/8 from a linked worktree, and 8/8 with cwd set to the main tree. The full suite now runs green from a worktree with no manual deselect — 7026 passed, 82 skipped. Fixes #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): entry for the test_init cwd-relative sys.path fix One new file under docs/fork-changes/ per #480's per-entry format, with `commit: HEAD` and `fork_pr: 482` so the merge step resolves the squash sha rather than the lane recording a branch sha that goes unreachable. `seq` 139 from `scripts/fork_changes.py --next-seq`. Renders FORK_CHANGELOG.md, the README table and website/public/llms-full.txt. No test-count literal: #480 removed it and check-docs derives the count. Part of #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
One entry file under docs/fork-changes/ per #480's layout, seq 140, commit: HEAD + fork_pr: 479 so the merge step resolves the squash sha. Replaces the five fork-changes.yaml commits this branch carried before the rebase — that file no longer exists, and per-entry files mean this branch no longer conflicts with any other lane's docs. No test-count literal: #480 removed that check, and with it the per-rebase arithmetic that broke three times on this branch alone. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…aming migration (#442) (#479) * feat(hallways): postgres-backed hallway store behind an opt-in flag hallways.json on the production palace host is 1,041,537,215 bytes (1.04 GB) holding ~797K records across 21 wings, and growing — it was 479 MB one week earlier. Every hallway operation loads and JSON-parses the whole thing: - list_hallways(wing=...) filters in Python AFTER the full load, so the wing argument does not reduce the work at all; - the daemon cannot fast-intercept it (palace-daemon#255): it runs under a 2 GB cgroup cap already sitting at 98.4%, and parsing a 1 GB JSON array materializes several GB of dicts; - compute_hallways_for_wing loads it to recover four dynamics fields per pair, and _compute_entity_tunnels_for_wing loads it AGAIN via list_hallways, so two parsed copies are live at once — that double parse is where a mine subprocess's 1.6-4.1 GB RSS comes from; - _save_hallways rewrites all 1.04 GB per mine that produces a hallway. Sized honestly: at production scale the two-loads-plus-one-save is ~37 s of JSON I/O per mine (measured: json.load 3.1 s, json.dump 8.4 s on a 900K / 396 MB file). An earlier draft of this message attributed #442's reported "29+ minutes" to this step. That was wrong. Those 29 minutes are create_tunnel calling _load_tunnels + atomic _save_tunnels on EVERY call from inside a loop (palace_graph.py :840/:861, called at :1145 and :1278) — O(n^2) in the tunnel count, against a tunnels.json that is only 837 KB at 2000 tunnels. The sweep's own open file was tunnels.json.tmp, which said so. Measured by the #474 lane, mechanism confirmed independently here. This change removes the ~37 s and the RSS and unblocks the daemon fast-intercept; it does not make a memory sweep faster. The postgres store makes each of those proportional to the wing rather than to the whole palace: an indexed WHERE wing = %s, a wing-scoped dynamics read, and a delete+insert of one wing. JSON REMAINS THE DEFAULT. The flag is hallway_backend (config.json) or MEMPALACE_HALLWAY_BACKEND (env); the production cutover is a separate, deliberate step after the migration is verified against real data. Nothing changes until someone opts in, and a chroma/sqlite install never needs postgres for hallways at all. A postgres selection with no DSN falls back to JSON with a warning rather than raising: a misconfigured flag must not take down a mine, and an empty store would read as "this wing has no hallways", which is worse than slow. Schema notes: - named columns for what is queried or ordered by, plus dynamics jsonb and extra jsonb so a record round-trips losslessly and a newer writer's fields are not truncated by an older reader (verbatim-always applies to derived records too); - created_at/last_activated stay text rather than timestamptz on purpose: the records carry ISO-8601 strings, and round-tripping them through a timestamp type normalizes offsets and sub-second precision — it rewrites the user's data to say something slightly different. ISO-8601 UTC sorts correctly lexicographically and nothing range-queries them. - writes scrub NUL and lone surrogates through the backend's own scrubber (#411). Entity names, labels and room names are all transcript-derived, so they carry exactly those bytes; one of them would otherwise abort a write, and on a 797K-record import that is #411's failure mode with a much larger blast radius. One scrub at the storage boundary, so every writer gets it. Connections are opened per operation rather than cached: hallway reads and writes are infrequent, so the per-call connect is cheap, and it makes this store immune by construction to the stale-cached-connection failure #385 had to add a retry seam for. The migration passes its own long-lived connection. Tests: tests/test_hallway_store.py, 16 cases — JSON is the default, postgres only by explicit flag, a DSN-less postgres flag falls back loudly, lossless record/row round trip including unknown fields and dynamics, unstorable bytes scrubbed, the wing filter and pagination are in SQL, ordering by co-occurrence, dynamics read scoped to one wing and keyed by the sorted pair, replace_wing touching only that wing (and still clearing an emptied one), delete reporting whether a row went, and idempotent schema creation that never drops. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(hallways): route reads and writes through the store list_hallways, delete_hallway and compute_hallways_for_wing now go through the configured store instead of _load_hallways/_save_hallways directly. On the JSON store the behaviour is byte-for-byte what it was; on postgres the wing-scoped paths stop reading the whole palace. The recompute change is the one that matters most. It loaded every record in the store to recover four dynamics fields for one wing — a full parse of a 1.04 GB file, and the second of the two parses the post-mine block performs, which together are where a mine subprocess's 1.6-4.1 GB RSS comes from — and now asks the store for that wing's dynamics only. (The 29 minutes #442 reports for the post-mine block is create_tunnel's per-call persist in palace_graph, not this load; see the first commit in this branch.) The lookup is still keyed by the SORTED entity pair, matching the symmetric id in _hallway_id: without that canonicalization a record persisted with the pair reversed misses the lookup and silently loses its accumulated weights on every recompute (PR MemPalace#1578 review, HIGH). The two early-return paths now clear the wing rather than returning without writing. Previously a wing whose pairs all fell below min_count kept its previous snapshot, because the function returned before reaching the persist step; the store call makes "no surviving pairs" mean no rows. list_hallways gains optional limit/offset. 797K records is not a sane payload at any speed, and the daemon needs a bounded page to answer mempalace_list_hallways at all (palace-daemon#255). _load_hallways, _save_hallways, _get_hallway_file and _legacy_hallway_file are deliberately left in place and unmoved: the JSON store calls back into them at call time, so the existing tests that monkeypatch _get_hallway_file keep working untouched. All 110 pre-existing hallway tests pass unchanged. Tests: tests/test_hallways_store_routing.py, 9 cases — the default config still reads the same JSON file, the monkeypatchable helpers still work, the wing filter and pagination reach the store, delete delegates, the recompute reads one wing's dynamics and never lists the whole store, writes only its own wing, preserves accumulated dynamics across a reversed-pair recompute, and clears a wing whose pairs all fell below min_count. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(hallways): streaming, resumable hallways.json -> postgres migration python -m mempalace.migrate_hallways [source] [--dry-run] [--restart] [--state PATH] [--batch-size N] Streaming is the load-bearing requirement, not a nicety. Measured here on a synthetic 201 MB / 400K-record file, which is the same shape as production's 1.04 GB / ~797K: json.load (what _load_hallways does) 1,072 MB peak RSS 2.28 s this streaming reader 31 MB peak RSS 1.92 s 34x less memory and slightly faster. A migration that has to allocate 4 GB to move data whose entire problem is that it allocates 4 GB is not a migration. Extrapolated to the real file, json.load is ~5.4 GB while the reader stays buffer-bounded and flat in the file size. The reader uses json.JSONDecoder.raw_decode over a sliding text buffer rather than a third-party streaming parser, so this adds no dependency and the local-first install is unchanged. Text mode is deliberate: Python's text IO will not split a multi-byte character across two reads, so entity names in any script survive chunking. The buffer is compacted only once the consumed prefix exceeds a chunk — slicing after every record made it quadratic in the chunk size and was measured 6x slower before the fix. Also: - idempotent: the upsert is keyed on the record id, so re-running converges; - resumable, and it REFUSES to resume when the source file's size changed. The saved position is an ordinal into this file's record sequence, so applying it to different content would skip the wrong records and silently under-import. --restart is the documented escape hatch, safe because of the upsert; - --dry-run reads and reports without opening a connection or touching a row, and deliberately does NOT require a configured postgres store. Requiring one would make the safe rehearsal harder to run than the real thing, which is backwards for the step whose job is to de-risk the other one. (Found by an end-to-end CLI run; every unit test injected a store and so could not see it.) - reports a count of records carrying NUL or lone-surrogate bytes. The scrub itself lives at the storage boundary so every writer gets it; what the migration owes the operator is visibility, so a surprising number is noticed rather than absorbed. This imports only. Flipping hallway_backend to postgres is a separate, deliberate step taken after the import is verified against real data. Tests: tests/test_migrate_hallways.py, 20 cases — both persisted file shapes, empty stores, records spanning chunk boundaries, multibyte and astral characters under an 8-byte chunk, malformed JSON raising rather than importing nothing, missing files, dry run touching nothing, full import, idempotence, resume, resume refusal on a changed file, --restart, state recording, bounded batches, schema ensured once, unstorable bytes counted without aborting, and a dry run with no store configured. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(hallways): report entity classes in the migration dry run, filter nothing Measured read-only against the live 1.14 GB store on the palace host: of 154,692 distinct entities only 12,484 -- 8% -- are word-shaped. Identifier, path, url and template shapes are 47% of distinct entities and appear in the large majority of records. The first record in the file links the entity ${this.baseUrl}/health. These are code tokens harvested from mined source, not names. Whether they belong in the palace is a judgement about the corpus, not about storage, so the dry run prints the class mix and the migration filters NOTHING. A silent filter would be an irreversible edit to the user's data made by the tool that was only asked to move it, and "we never summarize, we never paraphrase" covers deciding which of someone's entities were worth keeping. The table also makes those rows cheap to hold and cheap to query, so the filtering decision stays open after the import rather than being forced at it. `other` is a catch-all bucket, not a junk verdict -- it will include legitimate names carrying digits or punctuation -- so 47% is the floor for code-shaped, not the total. Ordering is most-specific-first, so a template containing a slash classifies as a template rather than a path. Also date-stamps the scale figures rather than quoting one as current. The file moved 479 MB -> 1,041,537,215 B / ~797K records / 21 wings -> 1,142,562,893 B / 1,903,306 records / 50 wings inside one evening. A duplication hypothesis was tested and refuted: record count, distinct ids and distinct (wing, sorted-pair) tuples all agree exactly at 1,903,306, so the growth is new wings plus N(N-1)/2 pair combinatorics, not a writer bug. The same streaming reader parsed that live 1.14 GB file end to end in 52 s at 366 MB peak RSS (most of it the analysis's dedup hash sets, not the reader) -- a positive control for the migration against real data, with no write to the host. Tests: 3 more in tests/test_migrate_hallways.py -- the dry run reports every class, the classifier's buckets are pinned individually, and reporting does not filter (every record still imports). Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(hallways): pin that the store does not normalize wing names hallways.py never calls normalize_wing_name — zero references in the module — so wing matching is exact string equality, and a wing spelled differently from the drawers' metadata matches nothing. The #474 lane measured what that produces: silence, not duplicates (the drawer filter at hallways.py:264 is also exact, so a mismatch returns [] before the store is ever opened). Normalizing in the postgres read path would be both an improvement and a behaviour change — queries that used to return nothing would start returning rows. A storage move is not the place to make it silently, so the store passes the caller's wing through verbatim and this test says so out loud rather than leaving it to be discovered by whoever changes it next. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hallways): a truncated hallways.json must raise, not read as complete Review catch (#479). The reader returned silently when input ran out without a closing "]" -- a file cut exactly at a record boundary (killed scp, full disk, interrupted write) yielded N records and no error. Mid-record truncation already raised; this boundary case did not. The compounding half is what makes it serious: migrate() would then write a resume state recording that count as the finished total, so a later run would skip straight past the missing tail. Silent tail loss on a 1 GB import, invisible until someone noticed hallways missing. Now the only clean exit from the loop is having seen "]". Running out of input raises with a message that says the file is truncated and that a partial read will not be reported as complete. A failed read therefore leaves no resume point behind, which is asserted directly. Also strengthens the resume fingerprint from size-only to size + mtime_ns. Size alone misses a same-length rewrite. Deliberately NOT a content hash: hashing 1 GB costs a full read, which is most of what resuming exists to avoid; both fields come free from one stat. A state file written before mtime was tracked still resumes on size alone rather than being refused. Positive control, because a guard that fires on good input would be worse than the bug: the shipped reader parsed the live 1,142,562,893-byte production hallways.json end to end -- 1,903,306 records, 27.2 s, 42 MB peak RSS, no exception. The record count matches an independent analysis run of the same file exactly, which cross-validates both instruments. Tests: 5 more -- unterminated array at a record boundary, unterminated after a trailing comma, no resume state written after a failed read, a properly terminated array still reading clean (the control), and resume refused when mtime moves at an unchanged size. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(hallways): replace_wing is one transaction; reads name a missing table Review catch (#479). replace_wing issued DELETE + N INSERTs on an autocommit connection -- N+1 separate transactions. A crash after the DELETE leaves the wing EMPTY; a crash midway leaves it partial; and the recompute that would repair it only runs on the next mine of that wing. The JSON path made that unreachable by writing a temp file and os.replace -ing it. My docstring said "as the JSON path did", which was true of SCOPE (other wings preserved) and false of ATOMICITY -- exactly the half that matters here. The storage move must not trade one for the other silently, so the whole swap now commits or rolls back together, and the docstring says both things explicitly. _run grows an `atomic` flag: it flips autocommit off, commits once at the end, and rolls back on any exception. Single-statement reads keep the cheaper autocommit path and a test pins that they do. When the caller supplies a connection the caller's transaction governs and `atomic` is ignored -- committing someone else's open work would be worse than not grouping ours. Also, the low item from the same review: reads on a table that does not exist yet returned an opaque UndefinedTable, and would have been indistinguishable from an empty store had they returned []. list, count and dynamics_for_wing now return empty and warn ONCE per store naming `python -m mempalace.migrate_hallways`. Same reasoning as the daemon's fast-intercept note (palace-daemon#255): a bare [] sends whoever is debugging it somewhere else entirely, and raising would take down a mine over an ordering mistake. Writes still fail loudly -- "run the migration first" should be enforced, not worked around. Tests: 5 more -- one transaction for the swap, rollback when an insert fails mid-batch, rollback when the delete fails, reads staying on autocommit, and the missing-table warning firing once across three reads. The transaction fake starts autocommit=True to match what _connect() actually returns, so the autocommit assertion is not vacuous. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(hallways): assert the wing's rows survive a failed swap, not just that rollback was called The #479 review asked for "a test where a failing insert mid-batch leaves the wing's previous rows intact". My existing tests asserted rollback() was called and commit() was not — which proves the code *asked* for a rollback, not that the data survived. Those are different claims, and only the second one is the property the JSON path's temp-file + os.replace actually gave. Adds a fake that models transaction VISIBILITY: committed rows kept separate from pending ones, DELETE/INSERT applied to pending, commit() publishing and rollback() reverting. The test can then assert the wing's previous rows are still committed after a mid-batch failure, and that no partial write is visible — which is the real acceptance criterion. Plus a control that a successful swap does replace the wing and does spare the other wings, so the new test cannot pass by the swap silently doing nothing. Verified both directions: with atomic=True removed, the new test fails along with the three existing ones; restored, all 24 pass. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): hallways to a postgres table (#442) One entry file under docs/fork-changes/ per #480's layout, seq 140, commit: HEAD + fork_pr: 479 so the merge step resolves the squash sha. Replaces the five fork-changes.yaml commits this branch carried before the rebase — that file no longer exists, and per-entry files mean this branch no longer conflicts with any other lane's docs. No test-count literal: #480 removed that check, and with it the per-rebase arithmetic that broke three times on this branch alone. Part of #442 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
One new file under docs/fork-changes/ per #480's per-entry format, `seq` 141 from scripts/fork_changes.py --next-seq, `commit: HEAD` and `fork_pr: 481` so the merge step resolves the squash sha from the right PR. The old single-file docs commit was dropped rather than replayed, and the docs changes that had been amended into the cmd_mined commit were stripped with it, so nothing in this branch touches docs/fork-changes.yaml. Part of #459 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
pushed a commit
that referenced
this pull request
Sep 11, 2026
One new file under docs/fork-changes/ in the post-#480 format: `commit: HEAD` (the squash sha does not exist yet; maintain-fork-changes resolves it from fork_pr after merge) and `fork_pr: 484`. seq 141 from --next-seq. All three renderers re-run; README/CLAUDE test count re-derived on the rebased base (now 7218 collected). check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…dir precheck (#459) (#481) * fix(cli): resolve the backend before the local-dir precheck in the shared seam #418 removed this precheck from `purge` and `sync`; it survived in two more places, so `prune`, `status --json` and `compress` still refused a Postgres palace. A Postgres palace directory holds only sidecars (dsn.env, hallways.json, tunnels.json, locks/, wal/) and `PostgresBackend.detect()` returns False unconditionally, because the drawers live in the service. Measured on a sidecar-only palace with MEMPALACE_BACKEND=postgres and a DSN pointing at a closed port, where a working command must fail with "connection refused": prune --stale-days 99999 before: "No palace found at <dir>" exit 0 after: "connection refused" exit 1 compress --dry-run before: "has no backend database yet" exit 1 after: "connection refused" exit 1 --json status before: hint "has no backend database" exit 2 after: hint "connection refused" exit 2 `status --json` still exits 2 — the palace genuinely is unavailable — but the hint now names the real reason instead of asserting a missing database that was never supposed to be there. - `palace._open_collection_or_explain` resolves the backend first and runs States A and B only for a local-mode store. State B is kept rather than dropped: its purpose is that some local backends create their DB file on first open, so probing one would mutate the filesystem during a read-only inspection. Neither hazard exists for a service-backed store. Fixing the shared seam covers `cli._emit_local_status_json`, `cli.cmd_compress`, miner status and searcher's local fallback in one place. - `cmd_prune` carried its own copy plus a hardcoded `ChromaBackend`; it now resolves the backend, gates the precheck on it, and opens through `palace.get_collection`. It also stops rejecting a local sqlite_exact palace for not being chroma. - Every prune outcome names its target: `Target: <palace> (<backend>)` in the text path, `palace` + `backend` keys in every `--json` payload including the error ones. An unreachable palace, a failed query and a failed delete now exit non-zero instead of 0. Fixes #459 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): route cmd_mined through the same backend resolution `mined` was the last cmd_* carrying its own copy of the local-directory precheck plus a hardcoded ChromaBackend. On a daemon-strict host its daemon route hides the local path, so the refusal only surfaced with --palace or daemon-strict off — and it printed "No palace found at <dir>" and returned 0, the "a refusal looks like a success" shape #418 removed. Measured on a sidecar-only palace with MEMPALACE_BACKEND=postgres and a DSN pointing at a closed port: before: "No palace found at <dir>" exit 0 after: "Error reading palace: ... Connection refused" exit 2 The resolve-gate-open sequence is extracted to `_mined_open_drawers_or_exit`, which also drops cmd_mined back under ruff's complexity ceiling. Named for `mined` on purpose: `cmd_purge` and `cmd_prune` hold near-identical sequences with different message shapes, and unifying all three is worth a deliberate PR rather than a drive-by in one under review. A general-looking helper with a single caller is how a fourth copy gets written. Both output paths now exit 2 on a refusal. The --json path always did; the text path returned 0, and one condition must not hand a text caller and a JSON caller different exit codes. An earlier draft of the extracted helper chose the text renderer by whether the backend was known, so a connection error — which knows its backend — printed "No palace found at <dir>" and hid the real cause: a refusal naming the wrong reason, the same defect family. Found by running the CLI against the closed port, not by a unit test, because the JSON payload carries the hint either way. `palace_state` is now an explicit argument and `test_an_open_failure_says_why_instead_of_no_palace_found` pins it. Also fixes the review nit: `opened.assert_not_called(), "msg"` is a tuple expression whose message is unreachable — rewritten as a real assert. Test updates, both contract changes rather than repairs: the two `TestCmdMinedJson` cases patched `mempalace.backends.chroma.ChromaBackend`, which this path no longer constructs, so they now patch `mempalace.palace.get_collection`; `test_cmd_mined_no_palace` expects exit 2. `test_cmd_mined_groups_by_wing_and_source` runs against a real chroma palace and passed unchanged throughout, which is what establishes the routing change is sound rather than just re-pointed mocks. Part of #459 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): entry for the local-dir precheck fix One new file under docs/fork-changes/ per #480's per-entry format, `seq` 141 from scripts/fork_changes.py --next-seq, `commit: HEAD` and `fork_pr: 481` so the merge step resolves the squash sha from the right PR. The old single-file docs commit was dropped rather than replayed, and the docs changes that had been amended into the cmd_mined commit were stripped with it, so nothing in this branch touches docs/fork-changes.yaml. Part of #459 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jphein
pushed a commit
that referenced
this pull request
Sep 11, 2026
One new file under docs/fork-changes/ in the post-#480 format: `commit: HEAD` (the squash sha does not exist yet; maintain-fork-changes resolves it from fork_pr after merge) and `fork_pr: 484`. seq 141 from --next-seq. All three renderers re-run; README/CLAUDE test count re-derived on the rebased base (now 7218 collected). check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
) * perf(graph): batch tunnel persistence — 2000 tunnels 37.9s -> 0.08s create_tunnel did a full _load_tunnels AND a full atomic _save_tunnels on every call, and it is called from inside two loops: the per-entity loop in entity_tunnels_for_wing and the per-wing loop in compute_topic_tunnels. N tunnels therefore cost N loads and N rewrites, with per-tunnel cost rising linearly with what was already on disk — O(n^2). Measured on a throwaway palace, before: 100 tunnels 0.61s 1000 tunnels 11.54s 500 tunnels 3.97s 2000 tunnels 37.87s extrapolating to ~16 min at 10K tunnels, which is the 29-minute mine on #474. The file was 837 KB at 2000 tunnels — this was never a big-file problem, it was a per-call-persist problem. After, same benchmark: 2000 tunnels 0.08s (454x, and the curve is linear) create_tunnel is public API with callers outside those loops, so its per-call semantics are deliberately unchanged: it is now a one-element batch. Measured on the same run, 500 one-at-a-time calls still cost 3.77s (was 3.97s). create_tunnels also fixes two smaller per-call costs the loops paid: the dedupe was a linear rescan of the whole list per tunnel (now an O(1) id index, or a batch would just trade one quadratic term for another), and kind="explicit" room validation opened a collection per call (now once per batch, and before the lock, so a rejected batch cannot half-write). Ten tests, including an equivalence test asserting the batch and sequential routes produce identical files from identical specs, and coverage for undirected identity within a batch, created_at/updated_at, and L7 dynamics preservation across a rebuild. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): batched tunnel persistence One new file under docs/fork-changes/ in the post-#480 format: `commit: HEAD` (the squash sha does not exist yet; maintain-fork-changes resolves it from fork_pr after merge) and `fork_pr: 484`. seq 141 from --next-seq. All three renderers re-run; README/CLAUDE test count re-derived on the rebased base (now 7218 collected). check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: jp <claude2@techempower.org> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jphein
pushed a commit
that referenced
this pull request
Sep 11, 2026
One new file under docs/fork-changes/ in the post-#480 format: seq 143 from --next-seq, `commit: HEAD` (the squash sha does not exist yet; maintain-fork-changes resolves it from fork_pr after merge), `fork_pr: 486`. No test-count step — that phrase is gone from README and CLAUDE.md, and check-docs check 1 now skips rather than comparing. All three renderers re-run; check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…purpose (#486) * feat(cli): tunnels --rebuild --wing W — refresh the derived graph on purpose After #478, palace-daemon#262 and palace-daemon#264, the derived graph (topic tunnels, hallways, entity tunnels) refreshes ONLY as a side effect of a full-directory mine that actually files drawers. That is the intended trade, but it left no way to say "refresh the graph" deliberately. This is that way: the same three steps, against a named wing, mining nothing. It runs locally and does not require the daemon — only the listing path does — so the branch sits ahead of that check. The three functions are called through the `miner` module rather than imported by name, so the post-mine block stays the single definition of what "the derived graph" means; this verb must not become a second, drifting copy of that list. Each step keeps the post-mine block's per-step fault tolerance: a derived analytic must never take down the whole operation, and a partial refresh beats none. No --all, by measurement rather than preference. A whole-palace sweep pays compute_hallways_for_wing's full load-and-rewrite of hallways.json once per wing. Extrapolated from a measured 396 MB point (load 3.1s, dump 8.4s) to the current 1.2 GB file: ~44s per wing, ~37 min across 50 wings — which would recreate the exact problem this verb exists to fix. `--rebuild` without --wing exits 2 and says so, naming #442. --all becomes cheap once the hallway store stops being a monolithic JSON. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): tunnels --rebuild — refuse when it would rebuild the wrong palace Review fixes on #486. F1 (red): under daemon-strict with no --palace the verb rebuilt the LOCAL palace and printed +0/+0/+0 exit 0. An operator runs this BECAUSE the graph is stale, so a confident "fresh" is worse than a refusal. Now exits 2 naming the daemon URL and the two ways out, matching cmd_purge's shape. An explicit --palace still rebuilds: it says which palace is meant. F2: no palace write lock. Hallways are a whole-file load / modify / save that carries other wings forward from its OWN load, so a rebuild of wing A racing a drain mine of wing B silently drops one side's new records — whichever os.replace lands second wins, with no error. Now wrapped in mine_palace_lock; MineAlreadyRunning surfaces and exits 1, as cmd_sync does. F3: get_collection defaults create=True, so a typo'd --palace MATERIALIZED an empty palace and then reported success against the thing it had just made. create=False. F4: the wing was passed raw while the miner normalizes (My-Project -> my_project), so a mismatched spelling matched no drawers and printed a confident +0. Normalized at the boundary. F5: --all is now a real parser flag that refuses with the measured reason and names #442, instead of a bare argparse usage error that teaches nothing. F6: the LIST of three steps was written twice, and my comment claimed it was not. Extracted miner.recompute_derived_graph — the post-mine block and this verb now call the same function, so a fourth derived step cannot land in one and be forgotten in the other. Per-step fault tolerance moves into it, failures come back in `errors` keyed by step, and each caller still formats its own output (the mine prints only non-zero counts; the verb prints all three, because a rebuild that prints nothing is indistinguishable from one that did nothing). The post-mine block's exact wording is preserved. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): tunnels --rebuild verb One new file under docs/fork-changes/ in the post-#480 format: seq 143 from --next-seq, `commit: HEAD` (the squash sha does not exist yet; maintain-fork-changes resolves it from fork_pr after merge), `fork_pr: 486`. No test-count step — that phrase is gone from README and CLAUDE.md, and check-docs check 1 now skips rather than comparing. All three renderers re-run; check-docs clean on all seven. Part of #474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: jp <claude2@techempower.org> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…ntry 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>
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
…k_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>
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>
jphein
pushed a commit
that referenced
this pull request
Sep 18, 2026
check-docs verified render parity, sha resolution, sha ancestry and upstream PR states — but never that a merged fork PR documented itself. #517 was green on every check with no entry at all, and nothing would have surfaced it later: --next-seq and the renderers are happy with any subset. The checker answered a narrower question than its name, which is #516's twin and #505's class. Step 8 lists squash-merge commits since a baseline by their trailing (#NNN) and requires either a `fork_pr: NNN` entry or a reasoned allowlist line. Baseline is 6da8775, the commit that introduced docs/fork-changes/ AND the fork_pr field (#480). Before it the field did not exist, so "missing" would be meaningless for the ~137 older entries; a baseline is what keeps this about drift rather than about history. git log only, never the GitHub API — a docs check that needs the network is one that gets skipped. Warn-only by default, --strict fails, so the residue can be worked without blocking. A bare number in the allowlist is refused: an allowlist records WHY or it is a mute button, and the next person cannot tell a deliberate omission from an abandoned one. Pre-registered before writing the step, by hand from git log: 20 squash commits since the baseline, 19 with fork_pr, missing exactly {495}. The step reports exactly that. #495 is the docs-tooling sweep (scripts/maintain-fork-changes.py) that rewrites landed `commit: HEAD` values across EXISTING entries and adds no change of its own — the producer this allowlist exists for, and the pair this check consumes. Note: #517, the PR the issue cites, now HAS an entry; it was backfilled after the issue was filed. The backlog is one PR, not several. Tests drive the REAL script over throwaway git repos built under the project's tmp/ (never /tmp — a 16 GB tmpfs here). Mutation-tested: a reasonless allowlist line exits 1; allowlisting a PR that DOES have an entry fails the producer/consumer test as a dead line; disabling the fork_pr scan reports all 19 as missing, proving the scan is load-bearing. Closes #519
jphein
pushed a commit
that referenced
this pull request
Sep 18, 2026
check-docs verified render parity, sha resolution, sha ancestry and upstream PR states — but never that a merged fork PR documented itself. #517 was green on every check with no entry at all, and nothing would have surfaced it later: --next-seq and the renderers are happy with any subset. The checker answered a narrower question than its name, which is #516's twin and #505's class. Step 8 lists squash-merge commits since a baseline by their trailing (#NNN) and requires either a `fork_pr: NNN` entry or a reasoned allowlist line. Baseline is 6da8775, the commit that introduced docs/fork-changes/ AND the fork_pr field (#480). Before it the field did not exist, so "missing" would be meaningless for the ~137 older entries; a baseline is what keeps this about drift rather than about history. git log only, never the GitHub API — a docs check that needs the network is one that gets skipped. Warn-only by default, --strict fails, so the residue can be worked without blocking. A bare number in the allowlist is refused: an allowlist records WHY or it is a mute button, and the next person cannot tell a deliberate omission from an abandoned one. Pre-registered before writing the step, by hand from git log: 20 squash commits since the baseline, 19 with fork_pr, missing exactly {495}. The step reports exactly that. #495 is the docs-tooling sweep (scripts/maintain-fork-changes.py) that rewrites landed `commit: HEAD` values across EXISTING entries and adds no change of its own — the producer this allowlist exists for, and the pair this check consumes. Note: #517, the PR the issue cites, now HAS an entry; it was backfilled after the issue was filed. The backlog is one PR, not several. Tests drive the REAL script over throwaway git repos built under the project's tmp/ (never /tmp — a 16 GB tmpfs here). Mutation-tested: a reasonless allowlist line exits 1; allowlisting a PR that DOES have an entry fails the producer/consumer test as a dead line; disabling the fork_pr scan reports all 19 as missing, proving the scan is load-bearing. Closes #519
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
…PR (#530) Closes #519. ## What `check-docs.sh` gains **step 8**: every squash-merge commit on `main` since a baseline must have a `fork_pr: NNN` entry under `docs/fork-changes/`, or a reasoned line in the new `docs/fork-changes-no-entry.txt`. The logic lives in a standalone `scripts/check-entry-coverage.sh` so it can run on its own (a pre-push hook, the sweep) and so its tests can drive the **real** script. - **Baseline `6da87755`** — the commit that introduced `docs/fork-changes/` *and* the `fork_pr` field (#480). Before it the field did not exist, so "missing" would be meaningless for the ~137 older entries. A baseline is what keeps this about drift rather than about history. - **`git log` only, never the GitHub API.** A docs check that needs the network is one that gets skipped. - **Warn-only by default; `--strict` fails.** Missing PRs are printed with their titles. - **A bare number in the allowlist is refused.** An allowlist records *why*, or it is a mute button — and the next person cannot tell a deliberate omission from an abandoned one. ## Pre-registered before the step was written Enumerated by hand from `git log --oneline 6da8775..HEAD`, stated in my report before any code: ``` squash PRs since 6da8775 (20): 477 479 481 482 484 486 487 490 491 492 493 495 505 509 510 511 512 517 520 522 fork_pr values in entries (19): the same, minus 495 MISSING (1): 495 ``` The step reports exactly `{495}` on this main: ``` examined 20 squash-merge commit(s) since 6da8775 documented: 19 fork_pr value(s) · allowlisted: 0 ! #495 has no docs/fork-changes entry — docs(fork-changes): resolve every landed `commit: HEAD` to its squash sha (#476) (#495) ``` Invariant, not a total: *every squash-merge commit since the baseline has an entry or an allowlist line*. The count moves with every merge; the invariant does not. ## The allowlist: 1 PR | PR | title | why no entry | |---|---|---| | #495 | `docs(fork-changes): resolve every landed commit: HEAD to its squash sha` | Docs-tooling sweep. It rewrites `commit:` values across **existing** entries and introduces no change of its own, so an entry for it would describe nothing a reader of `FORK_CHANGELOG.md` wants. | **One**, not several. Worth stating plainly because the issue implies a backlog: **#517, the PR the issue cites as the trigger, now HAS an entry** — it was backfilled after the issue was filed. I checked rather than assuming the issue's framing still held. ## The producer/consumer pair `scripts/maintain-fork-changes.py` is the **producer**: the sweep that resolves landed `commit: HEAD` values from `fork_pr`. Its own PRs add no entry by design — that is precisely why the allowlist exists. This check is the **consumer**. They are executed together in `test_every_allowlisted_pr_is_genuinely_missing_an_entry`, which runs against the real repo and requires every allowlisted PR to be (a) genuinely a squash commit in range and (b) genuinely without an entry. An allowlist that drifts from what the sweep produces would silence a real miss, so it is verified rather than trusted. ## Composition with the docs sweep — a rule, not a coincidence `scripts/maintain-fork-changes.py` **step 1** resolves `commit: HEAD` from an entry's `fork_pr:`. Two consequences follow, and both are load-bearing here: 1. **The sweep consumes entries and produces none of its own**, so every sweep PR that ever lands belongs in the allowlist. That is a standing rule, not a judgement call to re-make each time, and the allowlist file says so. 2. **The sweep runs LAST in a wave**, after every other PR. So this check will *always* land while unresolved placeholders exist. ⇒ **This step checks that an entry EXISTS (by `fork_pr:`), never that its `commit:` is resolved.** A `commit: HEAD` entry counts as present. Whether a sha is real and is an ancestor is the strict ancestry check's job — conflating the two would make a correct entry look missing for the entire window between a PR merging and the sweep running, which is exactly when a wave is busiest. Pinned two ways: a behavioural test with a fixture entry carrying an unresolved placeholder (with a control asserting the fixture really is unresolved), and a structural test that the script contains no `commit:` logic at all. > The brief asked me to cite the sweep's "step 5b". The script documents **steps 1 and 2** only — there is no 5b. The relevant step is 1, and that is what the allowlist cites. ## Tests **10 new**, driving the real `check-entry-coverage.sh` over throwaway git repos built under the project's own `tmp/` — never `/tmp`, which is a 16 GB tmpfs on this workstation. A `commit: HEAD` entry counts as present · the script contains no `commit:` logic · missing PR is named with its title · fully-covered repo is clean (positive control) · **an empty range reports the count it examined** rather than silently passing · warn-only vs `--strict` · allowlisted PR is not reported · comments and blanks ignored · a reasonless line is refused · the producer/consumer allowlist check. Suite: **7514 passed**, 82 skipped. `ruff check` + `ruff format --check` clean. `bash -n` on both scripts. `check-docs.sh` clean on all 8. Mutation-tested, each verified to apply: | mutant | result | |---|---| | allowlist line loses its reason | `--strict` exits 1 ✓ | | allowlist a PR that DOES have an entry (dead line) | producer/consumer test fails ✓ | | disable the `fork_pr` scan | all 19 reported missing ✓ (the scan is load-bearing) | | skip entries whose `commit:` is still `HEAD` | behavioural + structural tests red ✓ | > One earlier mutant only moved the text without changing behaviour; the structural test caught it and the behavioural one correctly did not. Recorded as inert, not as evidence. ## Note This entry is the first whose own check would have caught its absence.
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.
Closes the P6 docs-pipeline item: #473 (layout), #472 (ancestry check), #476 (resolver).
Three commits, each independently reviewable.
1.
efe8afc— one file per entry (#473)Every PR in a wave inserted at the top of
entries:in onedocs/fork-changes.yaml, soevery PR conflicted with every other one on that file plus the four artefacts rendered
from it — measured across a 10-PR wave on 2026-09-10/11 with zero source conflicts.
Entries now live in
docs/fork-changes/<date>-<id>.yaml;scripts/fork_changes.pyis thesingle loader for the renderer, check-docs and maintain-fork-changes, so they cannot drift
on ordering or validation.
ACCEPTANCE:
FORK_CHANGELOG.mdrenders BYTE-FOR-BYTE IDENTICAL from the new layout(4104 lines,
diffclean against the pre-migration file). The entry files were generatedfrom
HEAD:docs/fork-changes.yaml, with the loader asserted to reproduce the originalorder exactly and every field value round-tripped — the migration is mechanical, not
retyped. README changes only the numbering column and the generated-from comment.
One deviation from #473, with the measurement behind it
#473 proposed sorting by
(date, id). Measured: that moves 112 of 137 entries, becausethe file is in hand-curated narrative order and is not even date-descending — FORK_CHANGELOG
would have become a full rewrite and this PR unreviewable. #473's stated goal is
"concurrent PRs then touch disjoint files", which one-file-per-entry achieves regardless of
sort key, so ordering stays explicit via
seq.The property that makes
seqsafe: a collision is harmless. Two lanes both pickingmax+1write separate files and order deterministically on(date desc, id asc). Theconflict was never the number — it was a shared insertion line in a shared file. A
numeric collision costs nothing; a textual one blocks a merge.
Also: the README table is unnumbered (one new row used to renumber every row below it),
and
merged_upstreammoves todocs/fork-changes-meta.yamlbeside the entries dir — itchanges when an upstream PR merges, not once per fork PR. That one was caught because the
first loader returned only
entriesand silently dropped 19 rendered lines.2.
a8ad716— resolve shas fromfork_pr+ subject match (#476)The old resolver scanned the 12 lines after a
commit:line for any#NN. Those lines areprose. Measured on the 12 entries this repo needed fixed: 3 resolved, all three wrong,
one matched against upstream
#1829, and 9 were silently left onHEADwhile the runreported success.
Now:
commit: HEADresolves fromfork_pr:via RESTpulls/N→merge_commit_sha(exact— the API reports the commit the merge actually created); a stale sha is repaired by
matching the dangling commit's subject to the squash subject, requiring a unique
match. Both passes refuse ambiguity instead of picking.
fork_pris a new field and deliberately notpr:pralready means the upstream PR(it renders as "Upstream:" and check-docs queries it against
MemPalace/mempalace), soreusing it would have conflated the two repos — the same unqualified-reference bug in a new
place.
The failure direction is the design: an unresolved entry is harmless and visible, a
confident wrong sha is neither. 16 tests, all offline (git and the API injected).
3.
753decf— ancestry check + derived test count (#472, #473 item 3)check-docs.shassertedgit cat-file -e <sha>— but for entry shas it assertednothing at all, which is worse and is the real root cause. Credit to
@morpheus-single-file-mine (L2) for spotting it; measured:
Check #2 drops any line matching
/(jphein|techempower-org)/[a-z-]+/commit/as"cross-repo" — exactly how the renderer emits every entry's commit link. So no entry
commit:field was ever checked as such, and none for ancestry; 10 entry shas wereverified only incidentally, via prose mentions on non-URL lines.
The sharper corroboration is the last row: 7 of the 25 hashes check #2 does enumerate
are unreachable from main and it green-lights every one.
2ffe652andba16b82sit inboth sets at once — entry shas that check #2 verifies and passes while being
non-ancestors. So
cat-file -eis not merely weak in principle; it is passing stalehashes today.
Two distinct failures, then: the check that looks like it covers entry shas does not
enumerate them, and the hash class it does enumerate it tests with the wrong
predicate. Step 2b fixes both — it reads shas from the entries rather than by scraping
markdown, and asserts ancestry.
(Measurement corrected twice: I first said
cat-file -ewas too weak, @morpheus-single-file-mineshowed entry shas were excluded entirely, and the reviewer's re-check caught that "0 were
ever checked" overshot by 10.)
Step 2b is therefore the first verification of entry shas. It reads them from the
entries rather than by scraping markdown, so it cannot be fooled by how a line happens to
be formatted, and requires
git merge-base --is-ancestor <sha> HEADper entry so afailure names the file to edit.
Positive control — the check is proven to fail, not merely to pass. With the pre-rebase
sha
5383040planted in one entry:and 2b reports the entry id. Restored afterwards.
Sweep: the tool re-pointed 3 of 27 non-ancestor entries and refused the other 24.
Those are in
docs/fork-changes-legacy-shas.txt: their change is on main, but the commitcarrying it cannot be named because the squash subject was rewritten at merge. A plausible
wrong sha passes every check and misleads a reader; an allowlist line is a visible diff. A
looser token-similarity pass would recover ~8 more and was deliberately not applied —
hand-applying matches the tool refuses would undercut the contract commit 2 establishes. Main
stays green either way, and shrinking that list is a good follow-up.
Test count: the committed literal is gone from README and CLAUDE.md. It meant every PR
that added a test edited the same two lines. check-docs derives and reports it (7128 here)
instead of asserting it.
Net effect on the next wave
A PR now adds one new file and touches no shared docs source. The remaining shared
artefacts (
FORK_CHANGELOG.md, README table,llms-full.txt) are fully generated, so aconflict there is resolved by re-running the renderers rather than by hand-merging.
Verification
test_init::test_init_filters_sys_path_*failures are a linked-worktree artifact (the shared venv's editable install resolves
mempalaceto the main tree) — they pass in the main checkout and are unrelated to thisbranch.
scripts/check-docs.sh→ ✦ docs clean, including the new 2b (113 entry commits checked,24 documented-legacy skipped).
ruff checkandruff format --checkclean.Added after review feedback
docs/fork-changes/README.md— documents theseq/ date / id tie-break and, explicitly,why it is not a date sort (a date sort moves 112 of 137 entries), so the next author
does not "fix" it. Also documents that
commit: HEAD+fork_pr:is what belongs on abranch, and that
fork_prandprare different repositories.Part of #473, #472, #476