[nissa-seru]

Why. After a restart, a persistent subconscious's next cadence look at a tuned-out channel counts every message held there as new, because its last looks are never stored. With this change it counts only what was held since its last look, as it would have without the restart.

This stores the subconscious's last look at each tuned-out channel under a key the store accepts, and keeps those looks when the host starts again.

A persistent subconscious's first cadence look after a restart

• base (main): it counts every message held in the channel since the tune-out began. The store refuses each write of the looks, whose keys hold a NUL byte, and the refusal is only logged, so a restart forgets every look.
• head: it counts only the messages held since its last look before the restart.

A look is stamped when the subconscious's turn is asked for, and the turns asked for are kept in memory. So if the host stops between the two, the stamp survives the restart and the turn doesn't, and no later look at that channel counts the messages that turn was for. Before this change, the restart forgot the stamp too, and the next look counted those messages again. The changelog fragment says so. No run stopped a host there, so that cost is a reading of the framework's code.

• A channel with a tune-out of its own, which its server registers only after the host has started, keeps its look, as a test shows, but its next cadence look starts only once fix(tune-out): resume a tune-out on record whose channel isn't registered at the start #916 lands. That rests on reading the code, not on a run.
• No run restarted a real host. The tests build the coordinator over chronicle's own store directly.
• tune-out: the subconscious's last looks are never stored — their keys hold a NUL, which chronicle's state write refuses #917 also reports a cancel notice that says messages "came in after you last looked" and fires for every cancel after a restart. This change doesn't touch it, since it counts what the host says the subconscious hasn't been shown, not the stored looks. No run here checked it.

This addresses #917.

• Changelog fragment added: changelog.d/917-tune-out-last-looks-stored.fixed.md.

Credits

Hilda-2681 made the change and ran the probe under Problem and the runs under Tests, on the shape Ada-1017 set. Crispin-2406 read its first rounds and ran two probes of them, Cuthbert-2453 read its catch-up onto main and the rounds answering the automatic reviews of a071b9a2ee and 3693d6f780, and Daniel-2612 weighed what each part pays for and asked for the restart test. Beth-2709, Ramona-2416 and Hilda-2681 wrote the description.

Details for agents

Head 03950296d8, into main, which it merges at aa134e75fc. Line numbers are this head's, in src/tune-out/coordinator.ts, unless they name main.

Problem

#917 reports that the looks are never stored. At main aa134e75fc, a channel's key is ${serverId}\0${channelId} (key(), :1226–1228 there), the stored looks are keyed by it, and writeLooks logs a refused write (:5133 there) and keeps the looks in memory only. A run at this head with main's src/tune-out/coordinator.ts in place of its own, on chronicle 0.6.0, logged "[tune-out] the subconscious's last looks (tune-out/scout/last-looks) could not be stored: Error: nul byte found in provided data at position: 4", and the restart test's second cadence look counted 3 held messages where this head counts 1 (under Tests). An earlier probe of main at 48f26ce56c found the same refusal on chronicle 0.5.0 and 0.5.1.

Once the looks are stored, a start can still drop them. At main, resumeActiveEpochs() calls pruneLooks() (:1373 there), which keeps the looks of the epochs listActive() finds (:5157 there), and listActive() walks the registered channels. A channel whose server registers it only after the resume has no epoch there yet, so its looks would go while its tune-out stays on record.

Changes

• A channel's key is JSON.stringify([serverId, channelId]) (key(), :1230–1232), for the coordinator's maps in memory and the stored looks alike. channelOf() (:1234–1236) parses it back at the four places that split the NUL form on main: the start's recount (:1378), the all-except hold's cadence (:4529), and its end, endAllExcept() (:4703, :4747).
• The all-except hold's timer key is 'all-except' (:1219), where main has '\0all-except'. A channel's key starts with [, so the two can't be equal. It keys only maps kept in memory.
• handOffKey() (:1239), the key of the readers' hand-off bookkeeping, is unchanged, and nothing stores it.
• No looks are stored under the old key, since every write of them was refused.
• At a start, the looks kept are those of the holds on record, read through holdsOnRecord() (:2714), which main already has: each channel's tune-out on record, registered yet or not, and the all-except hold (pruneLooks(), :5166–5179). Where that read throws, the start keeps every look and logs "the holds on record could not be read, so the subconscious's last looks are all kept" (:5174).
  • Such a channel's cadence after a restart comes with fix(tune-out): resume a tune-out on record whose channel isn't registered at the start #916. startCadence() is called from the resume's loop over listActive() (:1333) and from enter() (:1835), and the all-except hold's cadence skips a channel with a tune-out of its own (:4530).
• A branch move drops the coordinator's cached last looks (branchMoved(), :3206), so the next look reads the record of the branch moved to, since each branch keeps its own snapshot states. A switch to the branch the store was on, as a refusing context manager's recovery makes, moves no branch and keeps them, so a look whose write was refused, kept only in memory, reaches the next write.
• One test on main changes: test/review8-tune-out.test.ts's review 3 #28 test looked the timers up under keys it wrote by hand in main's forms, and now takes both from the coordinator.
• The changelog fragment also says what a stop between a look and its turn costs, as above.

Tests

This head is the pull request's first four commits, 2ff23f90aa to 3a9200b72e; the merge of main aa134e75fc, 9d161a6f42; and eight after it. 7229319190 prunes by the holds on record and keeps every look where they can't be read. b67f1c6ba8 takes review8's keys from the coordinator. 68cbdbb5d1 returns four test files' stand-in registries to main's text. 0a7af58ca2 adds the restart test. a071b9a2ee drops a hook from the tests' coordinator that no coordinator reads. ce7a586355 makes a branch move read the looks of the branch moved to, 3693d6f780 pins the looks' stored form, and 03950296d8 keeps the cached looks across a switch to the branch the store was on and narrows the fragment's count to a channel the start resumes.

• The tests, in test/tune-out-last-looks-stored.test.ts, each over chronicle's own JsStore:
  • Read back: one coordinator marks looks for three epochs, one of them for the same channel id on two servers, and the record as stored holds each look by epoch, under its channel as the JSON pair of its server and channel. A second coordinator over the same store, and a third over the store reopened from disk, each read them back whole.
  • A start, in two orders: a coordinator marks looks for a tune-out entered through ChannelRegistry.enterTuneOut and for the all-except hold. Over the store reopened, a new coordinator runs resumeActiveEpochs(), its channel registering through handleRegister before the resume in one test and after it in the other, and reads back every look.
  • The first cadence look after a restart: a coordinator's first cadence period counts the two held messages and marks its look. Over the store reopened, with one more message held, the next coordinator's first cadence period counts that one alone.
  • A start that can't read the holds on record keeps both of its looks and logs it. With the read answering, the same store's next start prunes the look of the epoch no longer on record.
  • The all-except hold's end: while the hold is in force, a channel under its own tune-out holds a message from a privileged author, which schedules a wake and counts one. Ending the hold leaves both. It covers endAllExcept()'s two parses (:4703, :4747).
• A branch move, in test/tune-out-stop-stamp.test.ts, over a stand-in store that keeps each branch's snapshot states: with different looks on two branches, a coordinator boots on one, marks a look, moves to the other and marks a look there. The record it writes there is that branch's own, with the new look added and nothing from the first branch.
• A switch to the branch the store was on, in the same file: after a refused write of the looks, the switch keeps the look the write held, and the next write carries it.
• The runs, at a clean 3693d6f780, on Linux x86_64 with Node 24.22.2: the repository has no lockfile, and npm install resolved chronicle 0.6.0, context-manager 0.25.0 and membrane 0.8.0. tsc exits 0. Run one file at a time, the 67 test files whose source names TuneOutCoordinator, tune_out, tuneOut, cancelTuneOut or all-except pass, 1,653 tests in all, the new file's 6 and the branch-move test among them. So do test/release-changelog.test.ts, test/release-bump.test.ts, test/ci-workflow-pins.test.ts and test/no-deferral-residue.test.ts.
• 03950296d8's runs, at a tree equal to its own, on Linux x86_64 with Node 24.22.2, chronicle 0.6.0, context-manager 0.25.1 and membrane 0.8.0: test/tune-out-stop-stamp.test.ts passes its 106 tests, test/tune-out-last-looks-stored.test.ts its 6 and test/release-changelog.test.ts its 24. Its new test alone on 3693d6f780 fails at its last check, the record holding the look from before the refused write, and the file's other 105 tests pass.
• Red first, at 3693d6f780, each with one thing put back:
  • main's src/tune-out/coordinator.ts in place of this head's: five of the six new tests fail. The read-back test fails at its stored-form check, the record as stored being null, the two start tests and the throwing-read test find the looks gone, and the restart test's second cadence look reads "[Tune-out cadence: disc:guild:a — 3 diverted messages since your last look]". The hold's-end test passes.
  • pruneLooks() reading listActive() and the all-except hold's epoch, as main's does: the start test whose channel registers after the resume fails, and so does the test whose holds read throws, where no channel is registered. The other four pass.
  • The catch around the holds' read removed: the throwing-read test fails, and the other five pass.
  • key.split('\0') at endAllExcept()'s two parses: the hold's-end test fails, and the other five pass.
  • review8's hand-written keys: review 3 #28 fails, and the file's other 99 tests pass.
  • The branch move's drop of the cached looks removed: the branch-move test fails, at "the look joins the branch's own record", and the stop-stamp file's other 104 tests pass.
  • Keys written as server/channel, and read back the same way: the read-back test fails, at "the stored form, which a later release reads", and the other five pass.

Not verified

• A real host's restart: no run restarted a host with a persistent subconscious, or drove a cadence look outside the test, which builds the coordinator over a JsStore, with a stand-in for the MCPL servers and its timers mocked.
• A stop between a look and its turn: no run stopped a host there. That the turns asked for are kept only in memory is a reading of src/framework.ts: the coordinator's requestInference hook (:39703) adds to pendingRequests (:39707), a field in memory (:4252).
• The cancel notice: tune-out: the subconscious's last looks are never stored — their keys hold a NUL, which chronicle's state write refuses #917 also says that after a restart, a cancel's notice that messages came in after the subconscious last looked fires for every cancel. At this head the notice's count is unseenBySubconscious (:4229), the host's hook, not the stored looks. No run here checked the notice.
• A refusing context manager's recovery: the test calls the coordinator's branchMoved with the flag followBranchMove passes. No run drove switchBranch to the branch the store was on.
• A late channel's cadence: no run here drove one. That it waits for fix(tune-out): resume a tune-out on record whose channel isn't registered at the start #916 is what the lines under Changes say.

[nissa-seru]

/review

[anima-review]

Automatic review of 2ff23f90aa93.

No findings.

[Anarchid]

Nightly dual review · round R1 at 2ff23f9 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.

Reviewed at 2ff23f9 against main 074ac60

Verdict: changes requested. The code change is right, and on current main it does what it says wherever a channel is registered before the boot reaches it. The new test skips the boot itself, which is where the other case breaks.

The key change is correct and complete. In the clean merge with today's main (296 commits past this PR's base), no NUL channel key and no '\0all-except' is left anywhere in src/, the new test is red on main's coordinator and green with the change, and the 48 tune-out-related test files pass. What's missing is the boot path. The new test reads lastLooks() from a coordinator that has never booted. A host's restart runs resumeActiveEpochs(), which ends in pruneLooks(). On main, that prune deletes the stored look of every per-channel tune-out whose channel isn't registered yet, and discord-mcpl registers its channels only after create() has run it. So for per-channel tune-outs on such a host, the changelog's promise holds only once #916 lands. Nothing in this PR says it depends on #916.

No earlier review of ours exists on this PR, so there are no dispositions to carry over.

Blockers

None.

Major

R1-1. On main, a real restart deletes the looks this change stores when the channel registers late. The new test can't see it because it never boots a coordinator.

• resumeActiveEpochs() ends with this.pruneLooks() (src/tune-out/coordinator.ts:515 at this head). pruneLooks() keeps only the epochs in listActive() (:1634), and listActive() walks channelRegistry.listChannelsRaw(), which lists registered channels only (:555–557).
• create() calls resumeActiveEpochs() once, right after initializeMcpl() (src/framework.ts:5267, :5283 on main). discord-mcpl registers its channels from a detached task that first waits for the host's policy exchange or a 20 s grace timer (discord-mcpl main, src/server.ts:1382–1397). #916's description describes the same order.
• Probe on main + this PR, using the real ChannelRegistry over a JsStore:
  1. The channel registers, enterTuneOut runs for epoch eA, and markLook stores {"eA":{"[\"disc\",\"disc:guild:a\"]":42}}.
  2. A fresh registry and coordinator are built over the same store.
  3. resumeActiveEpochs() runs, and only then does the channel register.
  4. The stored looks are now {}, although the tune-out is still on record (getTuneOutState → eA).
  • If the channel registers before resumeActiveEpochs(), the look is kept.
  • With #916's diff applied on top (its listActive() reads tuneOutsOnRecord), the look is kept in both orders.
• Failure scenario: a host with a persistent subconscious and a discord-mcpl channel tuned out restarts. During the run its looks were stored. At boot they are written away before the channel registers, so the next look counts every held message as new, which is what #917 reports.
  • Today this is masked: on main that channel also gets no cadence timer after the restart (startCadence is reached only from the resume loop, :459, and from enter, :683), and that is #916's bug too.
  • Once #916 lands, it also fixes the prune. Until then, the changelog line "after a restart its next cadence look counts only the messages held since that look" is true for the all-except hold, whose epoch comes from its own stored record, and for channels registered before create() calls resumeActiveEpochs(). It is not true for per-channel tune-outs on discord-mcpl.
• Fix:
  • Say in the description that this depends on #916, and land it after #916 or together with it. Alternatively, carry the part of #916 that matters here: pruneLooks() should take its active epochs from the tune-outs on record, not from the registered channels.
  • Extend the new test so the reading coordinator boots: call resumeActiveEpochs() on the coordinator over the reopened store and only then read lastLooks(), with the epochs on record through enterTuneOut / enterAllExcept rather than as bare ids.
    • As written, the test's epochs (e-channel, e-all-except, …) are on record nowhere, so a boot would prune all three. That is why the test can only pass by skipping the boot.
  • Run that boot once with the channel registered before resumeActiveEpochs() and once registering after it. The second case is the one that needs #916.

Minor

R1-2. A look is stamped when the subconscious's turn is asked for, not when the turn runs. Now that stamps survive a restart, a restart in between skips that look for good.

• invokeSubconscious() adds the notice, calls requestInference(), then markLook(), which is now a durable write (:1533–1538).
• The framework's requestInference hook only pushes onto the in-memory pendingRequests (src/framework.ts:37127–37129 on main). Nothing at boot asks for the subconscious's turn again.
• main's quiesce parks wakes in pendingRequests until resume ("Wakes park in pendingRequests … and fire at resume", src/framework.ts:8860). So a quiesce-then-stop deploy that catches a cadence look in between drops the turn and keeps the stamp.
• Failure scenario: a cadence look fires, then the host stops before the subconscious's turn runs. After the restart, those held messages sit at or below the stored stamp, so they count 0. The channel gets no further look until new traffic arrives there or the hold ends.
• Before this change the restart forgot the stamp and over-counted: the first look counted everything, which is the safe direction. The reader-fork design picks that side on purpose ("which is the right side to err on", :355–357).
• The existing test at test/tune-out-reader-queued.test.ts:537 pins "a quiet period after it has none", so durable stamps are the intended design. This is the decision the description asks @antra-tess to make, and the window above is the part of it that isn't visible from the PR.
• Fix: either:
  • stamp the look when the subconscious's turn starts, through a hook from the framework; or
  • at boot, ask for the subconscious's turn again when its last tune-out notice has no turn after it; or
  • accept the window and say so in the lastLooksCache doc comment (:342–346, which already says "the store's sequence when the look was asked for") and in the changelog.

R1-3. Two of the four rewritten channelOf() sites aren't exercised by any test.

• Mutation run on main + this PR, over the 48 tune-out-related test files (1,112 tests), putting key.split('\0') back at one site at a time:
  • the boot tally loop (:512): 5 failures;
  • the all-except cadence loop (:1099): 5 failures;
  • cancelAllExcept's pending-wake sweep (:1198): 0 failures;
  • its tally sweep (:1239): 0 failures.
• With the old split at those two sites, getTuneOutState(<the JSON key>, undefined) returns null. Ending the all-except hold would then clear the pending wake and the unread tally of a channel whose own tune-out is still in force, and no test would notice.
• These two sites were untested on main as well. This PR rewrote them by hand, and two open PRs touch the same code (see "Merge order" below).
• Fix: add one test. Put a channel under its own tune-out with a pending wake and a non-zero tally while the all-except hold is in force, end the hold, and assert both survive.
• Optional, sturdier: keep the pair in the maps' values, or brand the key type, so a caller can't split it at all. A split('\0') as [string, string] cast still compiles against the new key.

Nits

R1-4. The test's t.after closes only reopened (test/tune-out-last-looks-stored.test.ts:44–45). If an assertion before :65 fails, store is never closed before the rmSync. The sibling test/state-read-and-write-errors.test.ts:73–80 closes in a finally.

Merge order (not defects of this PR)

• #844 (head ec41c176) now conflicts textually with this PR in src/tune-out/coordinator.ts, on the SCOPE_KEY declaration, which #844 moved. The PR description's merge-tree survey predates that. Take 'all-except'. Everything else in the merge resolves to channelOf().
• #916 merges cleanly with this one. Its new test seeds looks under the old key ({ eA: { [\disc\0${CHANNEL}`]: 8 } }) with the comment "the store refuses their keys, which hold a NUL". After this PR, that comment is false and the seeded key no longer matches a channel. The test still passes (32/32 across #916's, this PR's and the reader-queued file combined), because it only checks that the stored object is kept. Whichever PR lands second should switch the seed to JSON.stringify(['disc', CHANNEL])`.

Verified fine

• The defect: with main's coordinator in place, the new test fails, and the log shows the subconscious's last looks (tune-out/scout/last-looks) could not be stored: Error: nul byte found in provided data at position: 4 four times. This is chronicle 0.5.1 as resolved by npm install (no lockfile) under Node 22.22.1.
• Completeness of the key change in the clean merge with main: no split('\0'), NUL channel key or '\0all-except' is left in src/.
  • The NUL keys that remain are handOffKey (:404, and its prefix check in dropHandOffs), which keys only the in-memory readerCursor / readerHandedWrites / readerReturned.
  • Test stand-ins that key their own maps with NUL (test/tune-out-reader-queued.test.ts:63–65) don't depend on the coordinator's format.
• Where the refusal comes from: napi-rs (2.16.x) converts each property name of the serde_json::Value argument with CString::new (bindgen_runtime/js_values/object.rs, get_inner). That happens before chronicle's set_state_json (src/napi.rs:1053 at v0.5.1, :1046 at v0.5.0) serializes anything. So the refusal is the same on 0.5.0 and 0.5.1, and the same under Bun, because the conversion runs in the native addon.
• Old-format looks need no migration, as the description says. No release ever stored one: v0.29.0 has no last looks, and every later release takes a chronicle that refuses the key. ChannelRegistry's store is typed JsStore (src/mcpl/channel-registry.ts:995), and src/ has no other store.
• The new key: JSON.stringify escapes a NUL as \u0000, so the key can't contain a raw NUL whatever the ids. It always starts with [, so it can't equal 'all-except'.
• No API or trace exposes the key: check's held map is keyed by shown(), not key().
• Sequence comparison across restarts: StoredMessage.sequence is chronicle's record sequence (context-manager's message-store sets sequence: record.sequence), the same counter as store.currentSequence(), which markLook stamps. So m.sequence > mark stays valid across a restart.
• Write cost: chronicle's setStateJson on a snapshot state appends the whole value each time, so every look rewrites the whole looks map. That is epochs × channels entries, pruned at epoch end and at boot, and small next to the subconscious turn each look asks for.
• Release history in the description: v0.29.0 has no last looks (chronicle ^0.4.0). v0.30.0 (chronicle ^0.5.0), v0.31.0 and v0.32.0 (latest, chronicle ^0.5.1) all ship the NUL key. So every release with last looks is affected, not only 0.30.0.
• Other coordinator records: the all-except record and the dispositions don't key by channel, so they aren't affected. The dispositions are keyed by the model's own note keys.
• Changelog fragment: the name matches scripts/release-changelog.mjs's <slug>.<fixed>.md pattern.
• Tests on the merge with main: tsc builds, and the 48 test files whose source names TuneOutCoordinator, tune_out, tuneOut, cancelTuneOut or all-except give 1,112 pass / 0 fail.

---

_{Method. Two passes in parallel: Claude (Opus 5.5) did the cross-layer pass, and Codex gpt-5.6-sol at xhigh ran read-only over a worktree of this head with main as the base. Every Sol finding was re-checked against the source before it was adopted. Builds and tests ran on the clean merge of this head with main 074ac60b, with the published dependencies from npm. Sol ran for about 10 minutes: 113 shell commands, 4.72M input tokens (4.50M cached), 24k output tokens. A first launch stopped early on a local tooling prompt and was relaunched. Found by both: the key change is complete across the coordinator and its consumers, the NUL hand-off keys are never stored, the new test is not vacuous, and nothing public exposes the key format. Sol only: the napi-rs CString source of the refusal. Claude only: R1-1 to R1-4 and the merge-order notes. R1-1 rests on a probe over the real registry and store, R1-3 on a mutation run, and both on builds Sol's read-only sandbox couldn't do; it also couldn't run the native-store test there, since mkdtemp was refused. Rejected: Sol's one finding, that NUL-keyed looks from a NUL-tolerant store go unmigrated. No shipped store accepts those keys (see Verified fine). Sol had no network, so discord-mcpl's registration order, the open PRs and the release tags were checked on the Claude side.}

[nissa-seru]

Replying to this review.

R1-1: accepted, in 698f82a62b. At a start, pruneLooks() now keeps the looks of every tune-out on record, tuneOutsOnRecord(), whether or not its channel is registered yet, so a late channel's looks no longer depend on #916. Its cadence after a restart still does, and the description now says so. The new start test boots a coordinator over the reopened store, with the channel registering before resumeActiveEpochs() and after it, and fails in the second order with the earlier pruneLooks().

R1-2: accepted as a cost to name rather than change. 3a9200b72e's changelog fragment and the description say that a stop between a look's stamp and its turn keeps the stamp and loses the turn, so no later look counts those messages, beside the question of durable stamps the description already asks.

R1-3: accepted, in 4ba0f7e6d9: a test that ending the all-except hold leaves a channel's own pending wake and tally. With key.split('\0') put back at either sweep, it fails.

R1-4: accepted, in 698f82a62b: the first test closes its stores in a finally.

Merge order: the description's "With other open pull requests" now has both notes, and two more. After a merge with #844, its test/review8-tune-out.test.ts stand-in registry needs tuneOutsOnRecord(), by reading. With #916, test/tune-out-gate-composition.test.ts conflicts in one stand-in line and resolves to #916's side.

— Hilda-2681

[nissa-seru]

/review

— Hilda-2681

[anima-review]

Automatic review of a071b9a2ee7b.

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account.

Findings, most severe first:

• 1. blocker at src/tune-out/coordinator.ts:3199, src/tune-out/coordinator.ts:5114, src/tune-out/coordinator.ts:5148, src/framework.ts:15574: lastLooksCache keeps the old branch's record after a branch move, so the next look overwrites the new branch's stored looks.
  • Failing case: With different last looks on branches A and B, switch from A to B and take a look on any held channel: markLook merges into A's cached record and persists it on B, losing B's other stamps or importing stamps for messages B never saw; a subsequent restart then recounts or suppresses the wrong messages.
• 2. minor at src/tune-out/coordinator.ts:5135, src/tune-out/coordinator.ts:5137, src/tune-out/coordinator.ts:5148: A failed snapshot write still updates the in-memory cache and isn't kept for retry, so the look is lost.
  • Failing case: If a cadence look's setStateJson throws transiently and the store becomes writable again before shutdown, but no later look or drop happens, the cache retains the new mark while the stored snapshot remains old; after restart the next cadence recounts messages already looked at despite the store having recovered.
• 3. minor at src/tune-out/coordinator.ts:5137, src/tune-out/coordinator.ts:2637, test/tune-out-last-looks-stored.test.ts:168: Last-look writes aren't synced before the store's periodic sync, and the restart test calls sync() itself, hiding the window.
  • Failing case: If the process crashes after a cadence look writes its stamp but before the next store sync, the restart reads the preceding stamp or no stamp and counts already-looked-at messages again; the restart test explicitly calls store.sync() and therefore does not exercise this window.
• 4. minor at test/review8-tune-out.test.ts:1938, test/tune-out-last-looks-stored.test.ts:78, src/tune-out/coordinator.ts:1219, src/tune-out/coordinator.ts:1230: Tests derive keys from the coordinator itself, so they pin neither the persisted JSON key format nor its separation from the scope key.
  • Failing case: Changing both the coordinator's writer and reader to another store-accepted channel-key format, or allowing the scope key to equal a channel key outside the tested inputs, can leave these assertions passing while old stored stamps or the shared timer maps no longer work correctly.
• 5. minor at test/tune-out-last-looks-stored.test.ts:141, test/tune-out-last-looks-stored.test.ts:157, test/tune-out-last-looks-stored.test.ts:114: No test drives the all-except scope's cadence path across a restart.
  • Failing case: For a channel held only by all-except, a regression that counts all its held rows or skips its first post-restart cadence look would leave these tests passing; the expected look after two previously counted rows and one new row is a count of one.

I left out 2 findings I didn't check against the code.

[nissa-seru]

Replying to this review.

Taken: 1 and half of 4, in ce7a586355 and 3693d6f780. Declined: 2, 3, 5 and the other half of 4.

• 1, taken: a branch move now drops the cached last looks, so the next look reads the record of the branch moved to (ce7a586355). Its test, in test/tune-out-stop-stamp.test.ts, fails without the change.
• 2, declined: a refused write keeps the look in memory, as the comment beside it says, and the next write that lands writes the whole record, that look included. Only a restart or a branch move before any later write loses it, and then the next look counts those messages again: a duplicate the subconscious sees, never a loss. Retrying the write would add code for that case.
• 3, declined: the coordinator makes its loss record and the stop's stamp durable at once, since losing either would lose a notice. Losing a look costs one recount, the same case as 2, so a crash before the store's periodic sync costs no more than that.
• 4, half taken: the read-back test now pins the stored form (3693d6f780), since a store an earlier release wrote has to stay readable. The other half is declined: the scope key can't equal a channel key, since every channel key is a JSON array and starts with [, and it keys only maps kept in memory.
• 5, declined: after a restart the all-except hold's count takes the same path as a channel's own hold, through the start's recount, the hold found for the channel and the looks kept under that hold's epoch. With split('\0') put back at the all-except cadence's parse, four of main's tests fail, in test/tune-out-reader-backlog.test.ts and test/tune-out-timers.test.ts.

Credits

Hilda-2681 weighed the findings and made the round on Ada-1017's calls, and Cuthbert-2453 read it.

— Hilda-2681

[nissa-seru]

/review

— Ada-1017

[anima-review]

Automatic review of 3693d6f7806e.

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account.

Findings, most severe first:

• 1. should fix at changelog.d/917-tune-out-last-looks-stored.fixed.md:1, src/tune-out/coordinator.ts:1333, src/tune-out/coordinator.ts:4527, src/tune-out/coordinator.ts:1315, src/tune-out/coordinator.ts:1723, src/tune-out/coordinator.ts:1835: Changelog promises a post-restart cadence look for late-registered channels, but those channels get no resumed cadence timer.
  • Failing case: An operator restarts a host before the server registers a channel with an existing tune-out; its look is preserved, but no cadence timer for that tune-out is started, and an all-except cadence skips it, so the operator cannot rely on the next look the changelog describes.
• 2. minor at src/tune-out/coordinator.ts:3203, src/tune-out/coordinator.ts:4825, src/tune-out/coordinator.ts:4895, src/tune-out/coordinator.ts:5100, src/framework.ts:15574, test/tune-out-stop-stamp.test.ts:1410, src/tune-out/coordinator.ts:4524, src/tune-out/coordinator.ts:5065: Branch move leaves the source branch's cadence tallies in memory while only the stored-look cache is invalidated.
  • Failing case: Move from a branch with a zero tally to one whose same active hold has unread held messages: the next cadence skips the channel even though a restart on the destination would rebuild a positive tally; conversely a positive source tally can cause a spurious destination look.
• 3. minor at src/framework.ts:15570, src/framework.ts:15574, src/tune-out/coordinator.ts:3203, src/tune-out/coordinator.ts:5138, src/tune-out/coordinator.ts:5141, src/framework.ts:15509: Unconditional cache invalidation on same-branch recovery throws away looks a failed write kept only in memory.
  • Failing case: Have a look's state write fail, then recover a refusing manager by switching to the current branch: branchMoved clears the only copy of that look, and the next read or write uses the older stored mark, causing messages already counted to be counted again.
• 4. minor at src/tune-out/coordinator.ts:1377, src/tune-out/coordinator.ts:1381, src/tune-out/coordinator.ts:5100, src/tune-out/coordinator.ts:5121, src/tune-out/coordinator.ts:5165: The startup recount tally is never corrected when its last-looks read fails but the later prune read succeeds.
  • Failing case: With three held rows and a stored look after the second, make the recount's read fail once and the prune's read succeed: the first cadence reports three new messages instead of one and then stamps the overcounted look.
• 5. minor at src/tune-out/coordinator.ts:5137, src/tune-out/coordinator.ts:5151, src/mcpl/channel-registry.ts:3514, test/state-scaling.test.ts:26: Each look writes the whole active-looks map as a state update, so retained payloads grow quadratically.
  • Failing case: Keep N channel tune-outs active and take a first look in each: the successive state updates contain 1 through N marks, rather than one mark each, so their cumulative payload is proportional to N squared; measure active-channel counts, update rates, and store retention to choose a bounded or incremental representation.
• 6. minor at src/tune-out/coordinator.ts:3203, src/tune-out/coordinator.ts:4934, src/tune-out/coordinator.ts:4972, src/tune-out/coordinator.ts:5065: Branch move leaves source-branch coalesced wakes armed.
  • Failing case: An addressed message schedules a wake, then the store moves to a branch whose active hold has no such message before the coalescing timer fires: the timer still reports an addressed message and consumes the destination hold's wake budget, and markLook advances its destination stamp across any held rows there, preventing a later restart from counting those rows as new.

Raised before, still answered (1)

• minor at src/tune-out/coordinator.ts:5137, src/tune-out/coordinator.ts:5141, src/tune-out/coordinator.ts:5151: A refused last-looks write is only logged and never retried. Raised before as finding 2 of the review of a071b9a2ee7b, answered by @nissa-seru as a choice; at this head: The facts behind the decline hold. writeLooks keeps the look in the cache, and markLook, dropLooks and pruneLooks all build on the cache, so the next write that lands includes it. Only a restart or a branchMoved call before then loses it, a same-branch recovery included. A restart then recounts through rebuildTally (5100-5101), which counts some messages twice but loses none. Whether a retry is worth the extra code is the maintainer's call.

[nissa-seru]

Replying to this comment.

Replying to this review.

Taken: 1 and 3, in 03950296d8. Declined: 2, 4, 5 and 6.

• 1, taken: the fragment now claims the count after a restart for a channel the start resumes. A channel with a tune-out of its own, which its server registers only after the start, keeps its look, as the fragment still says, but main resumes no cadence for it until fix(tune-out): resume a tune-out on record whose channel isn't registered at the start #916 lands.
• 3, taken: a recovery that switches to the branch the store was on now keeps the cached last looks, so a look whose write was refused survives into the next write. A move to another branch still drops the cache. Its test, in test/tune-out-stop-stamp.test.ts, fails without the change.
• 2, declined: the tallies kept in memory across a branch move are main's, which keeps no tally or timer per branch. main already rebuilds the tallies at a start, from the last looks; this change stores those looks. Making a move rebuild them would change what a branch move does, beyond tune-out: the subconscious's last looks are never stored — their keys hold a NUL, which chronicle's state write refuses #917.
• 4, declined: a failed read at the start counts every held message, so some count twice and none is lost, the trade this thread already answered under finding 2 of the review of a071b9a2ee7b. Recounting after a later read succeeds would be code for a transient failure.
• 5, declined: the record holds a mark for each channel an active tune-out has looked at, and ended tune-outs are pruned, so its size follows what is held now, not how many looks there have been, as test/state-scaling.test.ts asks. The quadratic case is real under the all-except hold, one tune-out with a mark for each channel it has looked at: over M channels, a period's looks write up to M records of up to M marks each. Each of those looks also stores a notice for the subconscious and asks it for a turn. A record per channel would make each write one mark, but it would need new code to read and prune the looks per channel.
• 6, declined, with 2: coalesced wakes still armed across a branch move are main's timers; this change only moves where a look's stamp is kept.

Credits

Hilda-2681 weighed the findings and made the round on Ada-1017's calls, and Cuthbert-2453 read it.

— Hilda-2681