refactor(cli): one daemon-unreachable message; only cmd_wings JSON says less (#476) - #517
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…476) `palace daemon unreachable at` was written out by hand at 22 places. This routes 20 of them through `_fail_daemon` and leaves 2 deliberately. They were never 22 copies of one block. Reading them found four distinct messages: 12 interpolating the exception, 5 with no exception at all, 2 with `/cypher returned {status}` wording, and cmd_wings going through `_read_family_fail` with a leading blank line, an " ERROR: " prefix and no diagnostics clause. Functions holding two occurrences hold one of the first kind and one of the second, not two copies of one thing. The five exception-less sites are the `data is None` case: `_call_daemon_rest` returns None on 404/401/403 and nothing is raised, so there is nothing to interpolate. Those sites name the failing route in their JSON instead — the only identifying information a 404 leaves behind. The five prose sites and the five route-naming JSON payloads are the same five sites; that correspondence is design, not drift, and it is why the helper takes `route=` and `**extra`: def _fail_daemon(err, want_json, *, route=None, **extra) -> None A helper that could not express what the call sites already said would make the CLI report LESS than it does, which is the defect this wave exists to remove wearing the costume of a cleanup. Dropping a JSON field is a breaking change to a machine interface and is invisible to a prose diff: a script reading .status simply starts getting null. 17 of the 20 sites are byte-identical afterwards on BOTH channels. Three changes are declared rather than claimed away: * the two /cypher sites rearrange their prose — the status stays in the sentence and stays in {"status": …}, so their JSON is byte-identical; * cmd_wings normalises. It had drifted furthest and changes on both channels: it loses the blank line and the " ERROR: " prefix, gains the diagnostics clause, and its JSON error becomes the bare exception instead of a pre-rendered sentence embedding the daemon URL. That is the one place the machine output says less, deliberately, to match the other nineteen; * all 20 sites gain `_fail_daemon`'s "rejected the call" branch, so a server-side JSON-RPC error no longer prints "unreachable". A behaviour change, and a fix of the #499 class: an error named for the layer that noticed rather than the layer that failed. `_daemon_tool_or_fail` keeps its own copy and is NOT consolidated. It is not a duplicate — it exits `1 if unreachable else 2`, and its docstring says why: "the operator response differs and the sibling commands' single 'unreachable' message is actively wrong for the second". That is the same answered-vs- unreachable distinction #499 exists to make, already implemented; `_fail_daemon` exits 1 for both, so folding it in would have destroyed a deliberate distinction in six commands and shipped it as a cleanup. The two converge in the exit-code PR, in the other direction. cmd_wings appears in both lists because it calls that helper AND carried its own copy; only the copy moves. Verified: 7389 passed, 82 skipped; ruff check and ruff format --check clean. The literal count 22 -> 2 is asserted by a test, not a grep. Nine new tests pin the machine-facing half per route, which a prose diff structurally cannot see. Three mutations each kill a specific test, and each was asserted to have APPLIED before its run was believed: dropping every `route=` kills the three route tests, dropping `payload.update(extra)` kills the status test, removing the `err is not None` guard kills the bogus-"(None)" test. Before and after were captured with the real binary against a closed port. Worth noting: nothing in the existing suite broke. The two declared prose changes were untested behaviour, which is how cmd_wings drifted in the first place. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seq 151, `commit: HEAD` (the squash commit does not exist yet), `fork_pr: 517` taken from what `gh pr create` returned rather than guessed. The entry was missing and nothing could have told us: check-docs verifies render parity and sha ancestry — i.e. it validates the entries that EXIST. It has no "every merged fork PR has an entry" check, so an absent entry leaves no trace to detect, which is why this PR was green and undocumented at the same time. Tracked separately by the lead. Three renderers re-run; check-docs exits clean. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PART 35's coverage note. The parametrised route test drove cmd_list, cmd_graph
and cmd_stats — the three that share the `cmd_*(args)` shape. The other two
sites could not be parametrize rows: cmd_move needs a drawer_id and names its
route `PATCH /memory` rather than a bare path, and _gather_bulk_move_matches
is not a cmd_* at all, taking (wing, room, want_json) positionally. So each
gets its own test.
The cmd_move row failed on its first run, correctly: it patched
`_call_daemon_rest` while cmd_move goes through `_patch_daemon_rest`, so a
real request escaped the mock and the assertion caught it.
Mutation counts after this, for the record, because two different mutants give
two different right answers:
strip `route=` from the 5 call sites -> 5 tests die (was 3)
remove `route` from the helper payload -> 6 tests die (was 4)
The helper-sited mutant is strictly stronger: it kills everything the
call-site mutant kills PLUS the unit test that calls `_fail_daemon(None, True,
route="/list")` directly. Neither count was wrong; they measure different
mutations, and a mutation sited closer to the thing under test catches a
superset.
Part of #476
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--next-seq` returned 151 when the entry was written; #510 landed with 151 and #512 with 152 before this branch could push. Re-run after the final fetch, immediately before pushing, it returns 153. That is the rule rather than the anecdote: `--next-seq` reads a shared counter, so its answer is true for a moment, not for a branch. The second time in two pushes that a correct reading went stale in transit. Renderers re-run; check-docs clean. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#512 landed `_fail_daemon_unavailable`, a third writer of the message, so the count assertion moved 2 -> 3 during the rebase. The test's NAME and its summary line still said two — a name disagreeing with its own behaviour, which is the defect shape this wave exists to remove and which I renamed 32 tests for in the follow-up. Fixed here rather than carried. The docstring now names all three writers and why each exists, so a fourth copy has to argue for itself in this test rather than just bump an integer. Part of #476 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
force-pushed
the
refactor/fail-daemon-one-message
branch
from
September 18, 2026 04:50
0a60c58 to
0f2c2d6
Compare
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
`_fail_daemon` exits 1 — "no results" — for every daemon outage. It still does
on today's main; this is the first change that moves it. A command reporting
"no results" when the daemon is down sends the reader looking for missing data
instead of a missing daemon: the defect family this wave exists to remove,
sitting in the exit status after being fixed in the prose.
0 success
1 no results — the operation RAN and selected nothing
2 the operation could not run: palace unavailable, or a usage error
64 the daemon answered and REJECTED a well-formed request (a 4xx)
1 vs 2 is the load-bearing line: 1 means the palace was reachable and had
nothing to say; 2 means the question never got asked.
`_daemon_tool_or_fail`'s MESSAGE is consolidated into `_fail_daemon`; THE
WRAPPER REMAINS. It is still defined, still has its six callers, and still owns
the `DaemonRequestError` -> 64 branch. What moved is its duplicated
failure-rendering body, which now delegates with a new `tool=` kwarg carrying
the tool name into prose and JSON. That was not possible in #517: it exited
`1 if unreachable else 2` while the shared helper exited 1 for both, so merging
the message then would have forced one exit code onto two situations in six
commands, as a "cleanup". The two now CONVERGE rather than become identical —
transport moves 1 -> 2, JSON-RPC is unchanged at 2, and the distinction
survives in the prose only, plus the `reachable` flag on the JSON.
`_fail_daemon_unavailable` (#512) is NOT touched, and the reason is the useful
part. Its docstring said to delete it "if `_fail_daemon` ever does move to 2";
this moves it, and the cited test still passes — because it asserts only the
exit code. The JSON shapes differ: window/source use {"error": <key>,
"message": <prose>} across the family so a client can branch on `error`, while
`_fail_daemon` puts the prose in `error`. Consolidating would change two verbs'
machine output, leave them inconsistent with their own sibling, and every test
would have stayed green because nothing asserted the shape. A retirement
condition written in exit-code terms is satisfiable without being sufficient.
Its docstring now records the real one — agree on a JSON SHAPE — and
`test_transport_failure_json_keeps_the_window_family_shape` makes the next
attempt fail loudly. Convergence is filed as #521.
`cmd_pending`'s unknown-action guard moves 64 -> 2 and routes through
`_fail_client`. A verb group named without an action is a usage error, the same
class as argparse's "invalid choice"; 64 now means exactly one thing. The guard
also gains a JSON document: a hand-rolled print(..., file=sys.stderr) gives a
`--json` caller prose and no document at all.
#514: the header claimed 64 was "argparse default for parse errors". argparse
exits 2, and nothing in mempalace/ subclasses ArgumentParser or overrides
error(), so that row described a behaviour this CLI never had. It survived for
years because the contract lived in prose nothing executed, so two new tests
make it falsifiable: one asserts the header says what the code does (and that
the false sentence never returns), the other asserts the ABSENCE of an
ArgumentParser subclass.
THE WORKLIST WAS DERIVED BY EXECUTION, not grep — flip the helper, run the
suite, classify every failure — because two careful greps were 3-4x short. A
test named `test_daemon_unreachable_exits_1` asserts only the exit code and
never quotes the message, so no text search can find it.
The numbers, by unit, because they are not the same count:
files that pin this behaviour 19 <- the invariant
tests failing when the helper is flipped back 49
assertions moved 1 -> 2 43 line edits + 1 constant (_DAEMON_FAIL_EXIT,
covering 2 more)
tests failing at derivation, before later merges 42
An earlier revision of this message said "45", which was a real measurement of
the WRONG UNIT — assertions, presented against a tests-derived arithmetic
(42 + 2 + 3 = 47) that does not reach it either. Four denominators are in play:
assertions, tests-that-failed-at-derivation, tests-that-now-pin-it, and files.
Only the FILE SET is stable under renames, splits, and one constant covering
two assertions, which is why it is the stated invariant.
A derived list is a snapshot too: the 42 was correct when derived and was
invalidated twice within the hour, once by coverage rows added to #517 and once
by #510's diary split merging.
Every failure is an outage expectation. Zero no-results assertions appear — not
"none found" but CANNOT appear: `_fail_daemon` is only called from
daemon-failure branches, so a no-results path cannot reach it. The ~31 files
asserting 1 for genuine no-results are untouched and still pass.
The diary family is a one-line edit, as its author intended: two of its three
assertions go through `_DAEMON_FAIL_EXIT`, a constant whose comment says it
exists "so the rebase that changes it is a one-line edit rather than a hunt ...
this stays 1 until someone owns that cross-cutting change". This owns it. The
third test moved onto the constant, and `TestDiaryReadNoResultsExitCode` keeps
a literal 1 with a warning saying why.
33 tests renamed from `*_exits_1` to `*_exits_2`; a name asserting one number
while the test asserts another is a name disagreeing with its own behaviour.
`test_daemon_unreachable_exits_1` became
`test_daemon_unreachable_uses_the_daemon_failure_code`, because a name encoding
a number the constant exists to move will lie again.
ISSUE-NUMBER CORRECTION: this work and #517 were labelled #476 from a plan
note. #476 is "maintain-fork-changes.py: positional #NN resolver writes wrong
shas" and is unrelated; the real issue is #523. 15 references corrected here.
Four are deliberately NOT corrected — tests/test_maintain_fork_changes.py (2),
tests/test_render_docs.py (1), tests/test_fork_changes_loader.py (1) — because
they cite the REAL #476. A blanket replace would have corrupted all four, the
same mistake as a blunt sed over the diary tests earlier in this branch: the
scoped edit is the default, and blast radius scales with matches rather than
targets. #517's squash subject says #476 and cannot be rewritten; its changelog
entry never carried an issue number, so the rendered changelog was never wrong.
Residue, named rather than folded in:
#518 should 401/403 be 64 rather than 2? Needs `_call_daemon_rest` to stop
collapsing 404/401/403 to None first.
#521 cli.py has two JSON error conventions; decide one, then retire
`_fail_daemon_unavailable`.
Closes #514
Part of #523
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
seq 156 from `--next-seq` after the final fetch; `commit: HEAD`; `fork_pr: 522`. The entry records the #476 → #523 mislabel so a reader of #517's immutable squash subject is not misled. Wording corrected with the code: `_daemon_tool_or_fail`'s MESSAGE was consolidated and the wrapper REMAINS with its six callers, and the two helpers CONVERGE rather than become identical. The worklist is stated by unit — 19 files as the invariant, 49 tests failing on a flip-back — because an earlier revision quoted 45, which measured assertions against a tests-derived arithmetic that did not close. `website/reference/python-api/cli.md` moves because `_fail_daemon`'s and `_daemon_tool_or_fail`'s docstrings changed, not because of the entry. Part of #523 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
…ure helper (#523, #514) (#522) * fix(cli): a daemon outage exits 2, not 1 (#523, #514) `_fail_daemon` exits 1 — "no results" — for every daemon outage. It still does on today's main; this is the first change that moves it. A command reporting "no results" when the daemon is down sends the reader looking for missing data instead of a missing daemon: the defect family this wave exists to remove, sitting in the exit status after being fixed in the prose. 0 success 1 no results — the operation RAN and selected nothing 2 the operation could not run: palace unavailable, or a usage error 64 the daemon answered and REJECTED a well-formed request (a 4xx) 1 vs 2 is the load-bearing line: 1 means the palace was reachable and had nothing to say; 2 means the question never got asked. `_daemon_tool_or_fail`'s MESSAGE is consolidated into `_fail_daemon`; THE WRAPPER REMAINS. It is still defined, still has its six callers, and still owns the `DaemonRequestError` -> 64 branch. What moved is its duplicated failure-rendering body, which now delegates with a new `tool=` kwarg carrying the tool name into prose and JSON. That was not possible in #517: it exited `1 if unreachable else 2` while the shared helper exited 1 for both, so merging the message then would have forced one exit code onto two situations in six commands, as a "cleanup". The two now CONVERGE rather than become identical — transport moves 1 -> 2, JSON-RPC is unchanged at 2, and the distinction survives in the prose only, plus the `reachable` flag on the JSON. `_fail_daemon_unavailable` (#512) is NOT touched, and the reason is the useful part. Its docstring said to delete it "if `_fail_daemon` ever does move to 2"; this moves it, and the cited test still passes — because it asserts only the exit code. The JSON shapes differ: window/source use {"error": <key>, "message": <prose>} across the family so a client can branch on `error`, while `_fail_daemon` puts the prose in `error`. Consolidating would change two verbs' machine output, leave them inconsistent with their own sibling, and every test would have stayed green because nothing asserted the shape. A retirement condition written in exit-code terms is satisfiable without being sufficient. Its docstring now records the real one — agree on a JSON SHAPE — and `test_transport_failure_json_keeps_the_window_family_shape` makes the next attempt fail loudly. Convergence is filed as #521. `cmd_pending`'s unknown-action guard moves 64 -> 2 and routes through `_fail_client`. A verb group named without an action is a usage error, the same class as argparse's "invalid choice"; 64 now means exactly one thing. The guard also gains a JSON document: a hand-rolled print(..., file=sys.stderr) gives a `--json` caller prose and no document at all. #514: the header claimed 64 was "argparse default for parse errors". argparse exits 2, and nothing in mempalace/ subclasses ArgumentParser or overrides error(), so that row described a behaviour this CLI never had. It survived for years because the contract lived in prose nothing executed, so two new tests make it falsifiable: one asserts the header says what the code does (and that the false sentence never returns), the other asserts the ABSENCE of an ArgumentParser subclass. THE WORKLIST WAS DERIVED BY EXECUTION, not grep — flip the helper, run the suite, classify every failure — because two careful greps were 3-4x short. A test named `test_daemon_unreachable_exits_1` asserts only the exit code and never quotes the message, so no text search can find it. The numbers, by unit, because they are not the same count: files that pin this behaviour 19 <- the invariant tests failing when the helper is flipped back 49 assertions moved 1 -> 2 43 line edits + 1 constant (_DAEMON_FAIL_EXIT, covering 2 more) tests failing at derivation, before later merges 42 An earlier revision of this message said "45", which was a real measurement of the WRONG UNIT — assertions, presented against a tests-derived arithmetic (42 + 2 + 3 = 47) that does not reach it either. Four denominators are in play: assertions, tests-that-failed-at-derivation, tests-that-now-pin-it, and files. Only the FILE SET is stable under renames, splits, and one constant covering two assertions, which is why it is the stated invariant. A derived list is a snapshot too: the 42 was correct when derived and was invalidated twice within the hour, once by coverage rows added to #517 and once by #510's diary split merging. Every failure is an outage expectation. Zero no-results assertions appear — not "none found" but CANNOT appear: `_fail_daemon` is only called from daemon-failure branches, so a no-results path cannot reach it. The ~31 files asserting 1 for genuine no-results are untouched and still pass. The diary family is a one-line edit, as its author intended: two of its three assertions go through `_DAEMON_FAIL_EXIT`, a constant whose comment says it exists "so the rebase that changes it is a one-line edit rather than a hunt ... this stays 1 until someone owns that cross-cutting change". This owns it. The third test moved onto the constant, and `TestDiaryReadNoResultsExitCode` keeps a literal 1 with a warning saying why. 33 tests renamed from `*_exits_1` to `*_exits_2`; a name asserting one number while the test asserts another is a name disagreeing with its own behaviour. `test_daemon_unreachable_exits_1` became `test_daemon_unreachable_uses_the_daemon_failure_code`, because a name encoding a number the constant exists to move will lie again. ISSUE-NUMBER CORRECTION: this work and #517 were labelled #476 from a plan note. #476 is "maintain-fork-changes.py: positional #NN resolver writes wrong shas" and is unrelated; the real issue is #523. 15 references corrected here. Four are deliberately NOT corrected — tests/test_maintain_fork_changes.py (2), tests/test_render_docs.py (1), tests/test_fork_changes_loader.py (1) — because they cite the REAL #476. A blanket replace would have corrupted all four, the same mistake as a blunt sed over the diary tests earlier in this branch: the scoped edit is the default, and blast radius scales with matches rather than targets. #517's squash subject says #476 and cannot be rewritten; its changelog entry never carried an issue number, so the rendered changelog was never wrong. Residue, named rather than folded in: #518 should 401/403 be 64 rather than 2? Needs `_call_daemon_rest` to stop collapsing 404/401/403 to None first. #521 cli.py has two JSON error conventions; decide one, then retire `_fail_daemon_unavailable`. Closes #514 Part of #523 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): palace unavailable exits 2 everywhere (#523) seq 156 from `--next-seq` after the final fetch; `commit: HEAD`; `fork_pr: 522`. The entry records the #476 → #523 mislabel so a reader of #517's immutable squash subject is not misled. Wording corrected with the code: `_daemon_tool_or_fail`'s MESSAGE was consolidated and the wrapper REMAINS with its six callers, and the two helpers CONVERGE rather than become identical. The worklist is stated by unit — 19 files as the invariant, 49 tests failing on a flip-back — because an earlier revision quoted 45, which measured assertions against a tests-derived arithmetic that did not close. `website/reference/python-api/cli.md` moves because `_fail_daemon`'s and `_daemon_tool_or_fail`'s docstrings changed, not because of the entry. Part of #523 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.
One message, four variants removed; no call site reports less than it did.
palace daemon unreachable atwas written out by hand at 22 places. This routes 20 of them through_fail_daemonand leaves 2 deliberately.The premise was wrong, and reading the sites is what found it
They were never 22 copies of one block. There are four distinct messages:
… — see mempalace status for diagnostics ({e})… — see mempalace status for diagnostics— no exception interpolated… — /cypher returned {status} (see mempalace status for diagnostics)cmd_wings, via_read_family_fail: leading blank line,ERROR:prefix, no diagnostics clauseFunctions holding two occurrences hold an A and a B, not two copies of one thing.
⭐ B is not a degraded copy of A — it is the no-exception case
Nothing was raised, so there is no exception to interpolate. Those five sites instead name the failing route in JSON — the only identifying information a 404 leaves behind.
The five B prose sites are exactly the five route-naming JSON sites. That correspondence is design, not drift, and it is why the helper needed a wider signature than "take the exception".
The helper
err=None→ prose drops the parenthetical instead of printing(None);routesupplies the JSON errorroute=→ preserves the per-route JSON error (daemon /list unavailable) that five sites emitted**extra→ carries structured fields a site already published — todaystatusfrom the two/cyphersites.statussimply starts gettingnull.Before / after — 20 sites across 12 functions
Captured with the real binary against a closed port, on pre-change
mainand again after.cmd_listcmd_move_gather_bulk_move_matchescmd_graphcmd_statscmd_whycmd_tagscmd_tunnels_drawer_call_or_exitcmd_cyphercmd_overlapcmd_wings17 of 20 byte-identical on both channels. Every entry point now answers the same way:
The three declared changes
1. C ×2 — prose rearranged, JSON identical.
The status is still in the sentence and still in
{"status": 502}.2.
cmd_wings— normalised. This site had drifted furthest:Its JSON error no longer embeds the daemon URL. That is the one place the machine output says less, and it is deliberate: every other site emits the bare exception, and the URL remains in the prose and in the caller's own config.
3. All 20 sites gain the
"rejected the call"branch._fail_daemondistinguishes a transport failure from a JSON-RPC error the daemon itself returned, so a server-side error that used to print "unreachable" now prints "palace daemon at <url> rejected the call — …". This is a behaviour change, and it is a fix of the #499 class: an error named for the layer that noticed rather than the layer that failed.What is deliberately not touched
_daemon_tool_or_failkeeps its own copy of the literal. It is not a duplicate — it is a better helper:That is the same answered-vs-unreachable distinction #499 exists to make, already implemented.
_fail_daemonexits 1 for both, so folding it in would have destroyed a deliberate distinction in six commands and shipped it as a cleanup. The two converge in the exit-code PR — in the other direction, with_fail_daemonlearning the discrimination.cmd_wingsappears in both lists because it calls_daemon_tool_or_failand carried its own copy; only the copy is touched here.Verification
ruff check+ruff format --checkclean.route=→ the 3 route tests diepayload.update(extra)→test_extra_fields_survivedieserr is not Noneguard → the bogus-(None)test diesPart of #476