Repository navigation
fix(tune-out): entering a tune-out keeps the subconscious's window anchor at an all-except hold in force - #933
Conversation
enter() set the subconscious's window anchor to the oldest start among the tune-outs on record and its own, and left out an all-except hold in force, which the start's resume counts and enterAllExcept's own start stands for. Entering a channel's tune-out while an older hold was in force moved the anchor past the hold's start, though the comment above the reduce said never to move it forward there, and the hold's earlier rows left the subconscious's window. main has the same gap. The reduce now starts from the older of the new tune-out's start and the hold's, and its comment says what it keeps. Test: a hold entered at sequence 3, then a channel's tune-out at 20, over the real registry and store, leave the anchor at 3 both times. The test file's coordinator takes the sequence as an optional argument for it. Before this change the second anchor was 20.
|
/review |
|
Automatic review of No findings. |
e2dc0af to
65e58aa
Compare
|
/review — Hilda-2681 |
|
Automatic review of About these findingsThe 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:
|
The comment restated what the reduce does: the anchor never moves past the start of an epoch still in force, a tune-out's or the all-except hold's. The reason for counting the hold is in the previous commit's message, so the code carries no comment there.
|
— Hilda-2681 |
|
/review — Hilda-2681 |
|
Automatic review of No findings. |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at 3be2d81 · 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: APPROVE (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 3be2d81 against fix/tune-out-resume-late-channels 9704133
Round 1. There is no earlier review of ours on this PR, so there are no earlier findings to give dispositions for.
Verdict: APPROVE, one minor
The fix is correct and complete for the coordinator it targets. Before it, enter() was the only one of the coordinator's three anchor sites that ignored the all-except hold. resumeActiveEpochs() and enterAllExcept() already counted both kinds of epoch, so all three now agree. The bug is real in effect, not just in the recorded numbers. In context-manager 0.24.2, which this branch resolves to, WindowedPassthroughStrategy.setAnchor assigns whatever it's given (src/strategies/windowed-passthrough.ts:277-282), and the window drops every row below the anchor (:348-350). So on the base, the hold's held rows between its start and the later tune-out's start left the subconscious's view. The one thing worth acting on is in the test harness and concerns #844 rather than this branch. #844 already fixes the same bug its own way, and it changes the anchor hook's shape. When the two meet, the test file merges cleanly and then fails at runtime, because the harness erases the hook's type.
Blockers
None.
Major
None.
Minor
R1-1. The new test records the anchor hook through hooks as never, so #844's hook change breaks it at runtime, not at build.
test/tune-out-resume-late-channels.test.ts:54 records setSubconsciousAnchor: (sequence: number) => { anchors.push(sequence); }, and :59 passes the hooks as never. #844 (ec41c17601) changes the hook to (start: SubconsciousWindowStart, others?) => void (src/tune-out/coordinator.ts:752 there). It also updates every anchor recorder it can see to start.sequence: test/all-except-unit.test.ts:71 and test/tune-out-cancel-order.test.ts:80 there, which read (sequence: number) => … push(sequence) on this branch at :56 and :82. This file is new in #916, so #844 can't update it.
- Failure scenario:
git merge-treeof #844 with this head gives one conflict, inenter(), as the description says. It resolves to #844'sanchorSubconscious(). This test file merges with no conflict. After the merge,anchorsholds{ sequence: 3, epochIds: [...] }objects. The new test'sassert.deepEqual(anchors, [3, 3])fails, and so does #916's third test withArray(3).fill(7).tscreports neither, because of the cast. I couldn't run the merged tree: #844 needs@animalabs/chronicle^0.6.0, which isn't on npm yet (npm installfails with ETARGET). This conclusion comes from reading the merged sources, not from running them. - Fix: type the one hook the test reads, and leave the rest of the stub as it is:
setSubconsciousAnchor: ((start) => { anchors.push(start); }) satisfies TuneOutFrameworkHooks['setSubconsciousAnchor'],
TuneOutFrameworkHooksis exported fromsrc/tune-out/coordinator.ts:247. I checked this at this head: it compiles. Against a stand-in with #844's object shape,tscrejects thepushwith TS2345, so the merge fails at build and names the line. Asatisfies Partial<TuneOutFrameworkHooks>on the whole stub doesn't work: it also flags the stub's deliberate looseness (addMessagereturns nothing,Rowisn'tStoredMessage). Also add a sentence to "With #844": the conflict resolves to #844's side, because itsactiveStarts()(:1079-1084there) already includes the hold. After that, both anchor tests in this file recordstart.sequence.
Not counted as a finding: the reduce's seed, scope ? Math.min(params.startedAtSequence, scope.params.startedAtSequence) : params.startedAtSequence, is now the third separate copy of "the oldest start in force". #844 gathers them into activeStarts(), and this line is dropped when #844 lands, so it isn't worth refactoring here.
Verified fine
- The fix itself.
scopeis read atsrc/tune-out/coordinator.ts:649, and nothing between there and the reduce at:676-680can end or change the hold.readOptionsis pure.clearTimers,enterTuneOutandarmExpiryact on the one channel, and everything runs synchronously, so no expiry callback can run in between. - The other anchor sites. There are only three
setSubconsciousAnchorcalls insrc/.resumeActiveEpochs()counts the hold while it's in force (:463-484).enterAllExcept()reduces overlistActive()(:1028). The reverse ordering, a hold entered after a tune-out, is already covered by #916's third test (test/tune-out-resume-late-channels.test.ts:117-135). - Exits don't re-anchor, by design. A channel's expiry goes through
cancel(), and the hold's goes throughcancelAllExcept(). Neither moves the anchor, so ending either older epoch leaves the window start where it was until the strategy re-anchors or the next entry does. - A hold past its end time but not yet released still counts.
getAllExcept()doesn't look at the clock, andholdFor()(:943-951) still holds traffic for such a hold, so counting it in the reduce matches what's actually held. - No NaN risk from older records.
AllExceptParams.startedAtSequencehas been written since the all-except scope was introduced (e3fbc64b), so no hold on record lacks it. - Red first, reproduced. I ran a fresh
npm installon Node 22.22.1 / npm 10.9.4, andnpx tsc --noEmit,npm run buildandnpm run pretesteach exited 0 at this head.- At this head,
tune-out-resume-late-channelspasses 4/4. - With the base's
coordinator.tsbuilt in place of this head's, it passes 3 and fails 1: the new test, with[3, 20]where[3, 3]was expected. - With
48f26ce56c'scoordinator.ts(main at the branch point), 0/4 pass. - The 46 test files that name
TuneOutCoordinator,tune_out,tuneOut,cancelTuneOutorall-exceptpass 1,106 of 1,106 at this head.
- At this head,
mainhas the same bug.074ac60be0has the pre-fix reduce atsrc/tune-out/coordinator.ts:664-668, as the description says.- The description's citations hold. Line numbers at
9704133265, at this head and at074ac60be0all match. The textual merge results with #844 matchgit merge-tree. CI shows 12/12 checks green at3be2d81a76. - The changelog fragment follows
changelog.d/README.mdfor both its name (<slug>.fixed.md) and its form (bullets, opening with the audience), and it describes the ordering the fix covers. - The harness change keeps the other tests as they were.
coordinatorOn's new fourth parameter defaults to the old() => 9, and the three existing tests don't pass it.
Method: two independent passes, merged after re-checking every claim. Codex gpt-5.6-sol at xhigh ran read-only on a detached checkout of the head: 482 s, 112 commands, about 4.11M input tokens (3.97M cached) and 19.4k output. It reported no findings. Its verified-fine list matched the Claude pass point for point, each pass reaching it independently: the three anchor sites, the timing of the scope read, startedAtSequence always being present, main having the same bug, and #844's activeStarts() subsuming the fix. I re-checked each of its citations against the committed head and the context-manager 0.24.2 source. Found only by Claude: R1-1 (the hook-shape change in #844 and the as never harness), the red/green and subset runs, and the CI and description checks. Sol couldn't run the tests (its sandbox's temp directory is read-only), and it had no network for the PR body or CI. Neither pass could build #844 merged with this head, because #844's chronicle dependency isn't published.
coordinatorOn hands the coordinator its hooks as never, so a change to the setSubconsciousAnchor hook's shape would leave the recorder compiling and the anchor tests failing only at run time. The recorder now satisfies TuneOutFrameworkHooks['setSubconsciousAnchor'], so such a change fails the build at this line instead. The rest of the stub stays loose, as before.
|
Replying to this review.
— Hilda-2681 |
7a77862
into
anima-research:fix/tune-out-resume-late-channels
On top of #916, entering a channel's tune-out while an older all-except hold is in force now leaves the subconscious's window anchor at the hold's start. In this pull request's new test, a hold entered at sequence 3 and then a channel's tune-out entered at 20 give the anchors
[3, 20]with #916's coordinator at9704133265, and[3, 3]at this head.main's coordinator at48f26ce56cgives[3, 20]too.@tel-0s, this pull request goes into #916's branch,
fix/tune-out-resume-late-channels, and adds three commits to it. We're asking you to merge it there, so the anchor fix lands with #916's fix.Problem
At
9704133265,enter()sets the subconscious's window anchor with these lines (src/tune-out/coordinator.ts:678–682):With #916's coordinator in place of this head's, the new test, "a tune-out entered while an older all-except hold is in force leaves the subconscious's window start at the hold's start", fails at its last assertion: the anchors are
[3, 20]where[3, 3]was expected. The hold's start is 3, and the second anchor is the new tune-out's start, 20.Changes
enter()'s reduce (src/tune-out/coordinator.ts:676–679) starts fromscope ? Math.min(params.startedAtSequence, scope.params.startedAtSequence) : params.startedAtSequence, wherescopeisthis.getAllExcept(), whichenter()already reads (:649), in65e58aa35a. In3be2d81a76, the comment above the reduce goes, since it only restated the reduce.65e58aa35a's message says why the hold counts.test/tune-out-resume-late-channels.test.tsgains that test, over the realChannelRegistryand aJsStore. ItscoordinatorOntakes what the coordinator'scurrentSequencehook answers as an optional fourth argument, whose default is the() => 9the hook had.3edc77a5e1,satisfies TuneOutFrameworkHooks['setSubconsciousAnchor']. The harness hands the coordinator its hooksas never, so a change to that hook's shape left the anchor tests compiling and failing only at run time; now it fails the build at the recorder. The rest of the stub stays loose, as before.changelog.d/tune-out-anchor-hold-in-force.fixed.md: "Hosts with a subconscious: entering a channel's tune-out while an older all-except hold is in force no longer moves the subconscious's window anchor past the hold's start."docs/dendrite.md's "Tune-out without a reader" paragraph aroundtuneOut.enabled. This pull request carried it until November 5.mainhas since rewritten that paragraph inf75c791a13, which removestuneOut.enabled, so the commit is dropped and this pull request no longer touchesdocs/dendrite.md.With #844
@tel-0s, by
git merge-treeon November 6 at 04:31 UTC, #844 (ec41c17601) merges with #916's head with no conflicts. With this head in place of #916's, it conflicts insrc/tune-out/coordinator.ts, in one block:enter()'s window anchor, where #844 callsanchorSubconscious()and this head has the reduce under Changes. That block resolves to #844's side, since itsactiveStarts()(src/tune-out/coordinator.ts:1079–1084atec41c17601) already counts the hold. ItssetSubconsciousAnchorpasses aSubconsciousWindowStartobject (:100,:752there) where this branch's passes a number, so after the merge this file's anchor recorder fails the build until it recordsstart.sequence. #967 and #976, which go into #844's branch, likewise merge with #916's head with no conflicts and conflict with this head insrc/tune-out/coordinator.ts. Each of the other 43 pull requests open then merges with this head as it does with #916's.Tests
At a clean checkout of
65e58aa35a, on Linux with Node 24.22.2 and npm 11.19.1, afternpm installin a fresh copy with nopackage-lock.jsonornode_modules.npm ls --depth=0gives@animalabs/chronicle0.5.1,@animalabs/context-manager0.24.2,@animalabs/mcpl-core0.4.0,@animalabs/membrane0.8.0,@babel/parser7.29.13,@modelcontextprotocol/client2.4.0,@modelcontextprotocol/server2.4.0,@types/node22.20.5,@types/ws8.18.2,chokidar4.0.3,discord.js14.28.1,tsx4.23.15,typescript7.0.2 andws8.22.1.npx tsc --noEmit,npm run buildandnpm run pretesteach exited 0. Each test file ran alone, with the flagsnpm testgivesnodeand the TAP reporter:TuneOutCoordinator,tune_out,tuneOut,cancelTuneOutorall-except: 1,106 of 1,106 tests pass, among themtune-out-resume-late-channels's 4.src/tune-out/coordinator.ts(9704133265) built in place of this head's:tune-out-resume-late-channelspasses 3 and fails 1, the new test, at its last assertion, with[3, 20]where[3, 3]was expected.main'ssrc/tune-out/coordinator.ts(48f26ce56c) built in its place: all 4 fail, fix(tune-out): resume a tune-out on record whose channel isn't registered at the start #916's three and the new test, which fails at the same assertion with[3, 20].This repository's CI ran on this pull request at its head,
3edc77a5e1, and its 12 checks pass. Among them, Build & Test (ubuntu-latest, Node 22.12.0) runs the whole suite withnpm test.At
3be2d81a76, which only removes the comment, with the same install:npx tsc --noEmit,npm run buildandnpm run pretesteach exited 0, andtune-out-resume-late-channelspasses 4 of 4.At
3edc77a5e1, with the same install:npx tsc --noEmitexits 0, andtune-out-resume-late-channelspasses 4 of 4. With the hook's declared type swapped for an object shape like #844's,{ sequence: number; epochIds: string[] },tscfails attest/tune-out-resume-late-channels.test.ts(54,55)with TS2345. With the same swap and the file as it was at3be2d81a76,tscreports nothing in the test file.Not verified
code_execution's tests under Bun.setSubconsciousAnchor, not what the window then holds.mainafter48f26ce56c. No run here used a latermain's coordinator. Atmain's074ac60be0,enter()sets the anchor with the lines Problem quotes (src/tune-out/coordinator.ts:664–668).changelog.d/tune-out-anchor-hold-in-force.fixed.md.Hilda-2681 made this change and ran it red first. The anchor fix first went into an earlier change of ours for #900, which Crispin-2406 read. Naomi-2422 read this pull request. Fern-2426 ran our internal review of #916. Daniel-2612 repeated the runs at a clean checkout and wrote this description. Hilda-2681 reran them at
65e58aa35aand updated the description when the second commit was dropped.