Repository navigation
fix(tune-out): the default channel skips channels that are tuned out - #863
nissa-seru wants to merge 2 commits into
Conversation
Anarchid
left a comment
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at ed4ddb7 · 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: COMMENT (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at ed4ddb7 against main e85c57d (merge base 59d6095)
This is round 1. We haven't reviewed this PR before, so there are no earlier findings to give dispositions for.
Verdict
COMMENT: the core fix is correct, but four minor issues should be fixed or answered before merge. The default is now worked out when it's read instead of when a message is accepted, so whether a message counts follows its channel's tune-out at that moment. That removes all three failures the description reports, and the red-first claim holds exactly. The PR also merges cleanly onto today's main, which has moved 54 commits past the merge base, and passes there. What's left is at the edges of the new model. Two lifecycle paths break invariants the PR itself states: a channel withdrawn and re-added while the coalescer holds a message, and a server removed while one of its channels is tuned out. The changelog also says one thing about the cancel wake that the code doesn't do, and the fall-through to older channels has no bound, which the tests (two channels, one server, fresh messages) don't exercise.
Blockers
None.
Major
None.
Minor
R1-1. A coalesced message held while its channel is withdrawn and then re-added still sets the default.
src/mcpl/channel-registry.ts:1926-1927 lets noteInbound run whenever this.channels.has(incomingKey). A channels/changed that removes X and then adds X back puts a new entry under the same key (registerEntry), so the check passes for a message accepted for the old entry. The same closure already checks the entry itself before marking the channel open ("the entry is the one the message was accepted for", ending at :1960). The default check doesn't.
Scenario (reproduced): take the setup of the PR's own test at test/channel-lifecycle-order.test.ts:1516, with other:z as the default and a coalesced m1 on test:x held. Insert handleChanged('test', { added: [descriptor('x', true)] }) after the removal. Then getDefaultPublishChannel() returns test:x and incomingFor() gives m1, so other:z is displaced. The changelog, the commit message and the comment at :1922-1925 all say a message admitted after its channel was withdrawn "sets nothing".
Fix: require this.channels.get(incomingKey) === incomingEntry in that condition, and add the re-add variant of the test at :1516.
R1-2. A removed server's tuned-out channel becomes the default again when its tune-out ends.
retireInbound (channel-registry.ts:2284-2292) keeps the newest entry of all as gone, even when that entry's channel is tuned out. removeServer deliberately leaves desired state in place, the coordinator has no removal hook, and cancelTuneOut (:3263) doesn't require the channel to be registered. So the expiry timer (src/tune-out/coordinator.ts:178-191) still ends the tune-out, and from then on defaultInbound stops skipping the gone entry.
Scenario (reproduced on this PR merged into main): accept a message on a:general, then one on b:noisy, and tune out b:noisy. The default is a:general, correctly. Then removeServer('b'), and the default is still a:general. When the tune-out's duration ends, the default becomes b:noisy, and channel_publish with no channelId is refused: "The channel this would default to, "noisy", isn't registered now, so nothing was posted." Before the cancel, the same call posted to a:general. This breaks the invariant test/shared-channel-id.test.ts:622 names ("none of a removed server's channels becomes the default again"). The new cancel wake also names b:noisy (coordinator.ts:462). A wake that names a server-qualified channel that isn't registered gets no fallback (framework.ts:20812), so the resident's cancel turn can't put its plain speech anywhere. A channels/changed withdrawal of a tuned-out channel that holds the newest message takes the same path (I read this one but didn't run it). There keepMessage is true, so after the cancel, incomingFor also names a message on the withdrawn channel.
Fix: in retireInbound, drop a retired entry whose channel is tuned out instead of keeping it as gone: it can only become the default again through exactly this resurrection. In cancel, pass the channel to requestInference only when it's still registered (isDeclaredChannel), so the wake keeps its old fallback otherwise. The PR lists "a server removed during a tune-out" under "Not verified", and this is the test that case needs.
R1-3. The changelog's claim about a cancel wake "taken into a turn already open" isn't what the code does.
changelog.d/tune-out-keeps-default-channel.changed.md:27-28 says that in that case "it fares as a channel message's wake does". The commit message says "an unaddressed channel message's wake". Neither is quite right. Only a turn's opening start reads the trigger's channel; every other start keeps its locus (framework.ts:20847), and only arrivals can move it. The cancel's dump row is system: true (coordinator.ts:447). isConversationalInjection rejects system rows (framework.ts:414), so repinChannel (:18223-18224) never turns the dump into an arrival. Taken into an open turn, the cancel wake therefore never moves that turn's locus. That holds even where the turn has already posted in the cancelled channel, which is exactly where an unaddressed follow-up would re-pin it (:18233). This isn't a regression, since on main the wake named no channel at all. But users read the changelog.
Fix: say "taken into a turn already open, it leaves that turn's locus where it is" in both texts. If the intent really is to behave like a channel message, carry the cancelled channel as an arrival.
R1-4. Nothing bounds how far the default falls back, and the tests don't exercise it.
Tuning out the channel that spoke last now moves the default to whichever channel spoke before it, however long ago and on whatever server (defaultInbound, :2294-2302). A heartbeat tick's, reminder's or skip_reply self-wake's plain speech then goes there. context.incoming and a refusal's reaction point at that older message, which can be hours old and in a different place. Before, a refusal's reaction marked the newest message. The resident is told where it's going (announceLocusIfChanged, framework.ts:18169, and the tool description), so this is a design point rather than a defect. Still, every new test uses two channels on one server with messages seconds old.
Fix: at minimum, say in the changelog that the default may now be an older message on another channel or server. Consider an age bound, or no fall-back across servers. Add one test where the fall-back target is on a second server, so the behaviour is pinned down deliberately.
Verified fine
- It runs against today's
main, not just the merge base. The PR's CI ran on a merge withf14e7fbc, before #860 landed. Merged ontoe85c57dbit merges textually clean, and no reference to the removeddefaultPublish*fields is left insrc/ortest/.tsc(TypeScript 7.0.2) exits 0 with context-manager 0.23.1, membrane 0.8.0 and chronicle 0.4.0. The full suite on Node 22 has no failures. One harness artifact on my side: 12 files that loadquickjs.wasmran before I'd copied it intodist/. With it in place they passed 616/616. - Red-first. With the merge base's
framework.ts,channel-registry.tsandcoordinator.tsunder the PR's tests, 6 of the 8 new tests fail. Tests 5 and 8 pass, exactly as the description says. - Every old reader and writer of the default is converted in the merged tree:
markAccepted,getDefaultPublishChannel,incomingFor,buildChannelContext,globalFallbackFor, the publish resolver, thechannels/changedremoval,removeServerandstopAll. Outside the registry only two readers remain, both callingincomingForwhen a refusal happens (reactToRefusalatframework.ts:10890, and the failure log at:28173), and both read it fresh. incomingForcan't pair one channel with another channel's message.globalFallbackForreturns onlynullorgetDefaultPublishChannel(), which is the samedefaultInbound()entry the message comes from.- "A notice such as a join doesn't set it" is true, and was already true on
main:chat:systemis inNON_CONVERSATIONAL_TAGS(src/mcpl/tags.ts), andmarkAcceptedfilters on it. - Cost:
getTuneOutStateis one map lookup, sodefaultInbound()is linear in the channels that have spoken. The map is bounded by live channels plus at most one retainedgoneentry, which the next noted message clears. - Cancel wake in a batch: an addressed wake still outranks it, since it competes as an ambient channel wake by event time. Its channel and server travel as a pair, and
source: 'tune-out'isn't a framework wake source, as before. The fourthrequestInferenceargument is optional, and the coordinator's other two callers (the subconscious's turns) correctly pass none. - A channel the resident closed (one of the "Not verified" items) is still eligible as the default; this PR doesn't change that. It matches
channel_close's own description, "You can still speak there" (channel-registry.ts:556). - Fixture:
CHANNELSdefaults tonoisy, and a command line with no@labelpicks the first channel, so the other six test files that usetune-out-mcpl-server.mjsare unaffected, and they pass. - With #861: its head is now
3a24c472, not thed1ff4245the description cites. A merge with this PR still conflicts in exactly the same two comment hunks (the turn-locus comment inframework.tsandInferenceRequest.channelId's doc), with no signature conflict.
Method. Two independent passes, merged after re-verifying each claim. Codex gpt-5.6-sol at xhigh reasoning effort reviewed in a read-only sandbox with no network, against both the PR head and the PR merged onto current main: 10 min 22 s, 114 shell commands, 7.70 M input tokens (7.42 M cached), 32.7 k output (19.0 k reasoning). It couldn't run the end-to-end tests because the sandbox has no writable temp directory. Claude ran the network and runtime checks: the fetch of current main and #861, the merge onto main, tsc, the full suite, the red-first run and the registry probes. Sol only: R1-1, which Claude reproduced with a probe, and R1-3. Sol filed R1-3 as a major routing defect; Claude downgraded it to a changelog-accuracy issue because the behaviour matches main and the commit message's stated intent. Claude only: R1-2 (reproduced with a probe) and R1-4. Both, independently: the conversion of every reader and writer, the notice claim, the cost and bounds of the scan, the fixture's backward compatibility, and the #861 conflicts at its current head. Rejected: Sol's point that latestInbound is keyed with the NUL-joined lifecycleKey rather than destinationKey. A collision needs a NUL inside an operator-configured server id, and the key matches the desiredStates key that tune-out state already uses.
A tune-out diverts a channel's messages away from the agent: they're stored stamped and never shown to it or wake it. But the channel registry kept one default, the channel, message and thread of the most recent message it accepted, and set it on acceptance, before the framework decided to divert the message. So a diverted message became both the default channel and the message the agent is taken to be replying to. A turn with no channel of its own, such as a heartbeat tick or a reminder, then spoke into the tuned-out channel, after a message the agent was never shown, and the channel's server was told that message as context.incoming. Tuning out the channel that had spoken last left the default there too. The registry now keeps each channel's latest accepted message (latestInbound, in the order they were accepted) and works the default out when it's read: the newest on a channel that isn't tuned out now. That reads the same state the framework's diversion reads (TuneOutCoordinator.onIncoming diverts exactly when getTuneOutState is set), at the moment the default is used. So whether a message counts follows its channel's tune-out as it stands then, and one accepted just before a tune-out begins or ends gets the same answer as one accepted after, however long a coalescer holds its batch before delivering it. Once a tune-out ends, its channel's latest message counts again, the one it diverted included; the cancel's dump has shown its text, unless backlogCap is 0 or the window has no room for a line, and an image or audio it carried goes to the subconscious's report. With every channel that has spoken tuned out, there's no default, and channel_publish with no channelId is refused as having nothing to default to. A channel that's gone, its server removed (removeServer) or the channel withdrawn through channels/changed, keeps its latest message only when it's the default as it stands, marked gone so the next noted message drops it, and loses it at once otherwise (retireInbound). So the default's channel is kept as the default's ref until the next message, as main's one slot was, and is the default while no newer message counts, a newer one on a channel that's tuned out not counting, and a call that would default to it is refused as the default gone; a server's removal drops its message and thread, and a withdrawal keeps them, as on main. A refusal, not a move to an older channel, keeps what the agent's send does from turning on a diverted message it was never shown. A tuned-out channel is never the default, so one that goes is dropped and can't become the default when its tune-out ends, which a tune-out's expiry still does after its server is gone. A message the coalescer admits after its channel was withdrawn sets nothing, as one admitted after its server's removal already didn't, nor one admitted after its channel was registered anew meanwhile, withdrawn or not: the entry must be the one the message was accepted for, as the open-marking beside it already requires. A withdrawal of several channels retires them together, against the default as it stood before any of them went. The default can be an older message on another channel or another server, when every newer one is on a channel that's tuned out. The agent's wake when a tune-out ends named no channel, so its turn took the default channel, the tuned-out one only when the latest message to set it had been posted there. The cancel's wake now names the channel the tune-out ended, on its server, as a channel message's wake does, so a turn it starts alone, which has just been given that channel's backlog, speaks there. Merged with other wakes, it fares as an unaddressed channel message's wake does. Taken into a turn already open, its dump leaves that turn's locus where it is, since a system row never re-pins a turn; the subconscious's note on that channel, when there is one, is a conversational row and can, as an unaddressed message in a channel the turn has sent into does. While the registry holds no entry for the channel, as after its server is removed, the wake names no channel, as before this change. requestInference, the coordinator's hook, takes the channel as an optional fourth argument. channel_publish's description, and its channelId parameter's, said the default is the channel of the most recent message received on any channel, whether or not the agent has been shown it. They now say on a channel that isn't tuned out, and that a notice such as a join, which already didn't, doesn't set it either. The doc comments that say what sets the default say the same. Tests that set the registry's three default fields by hand now set a channel's latest message through noteInbound. The tune-out fixture registers a channel per label in CHANNELS, and a command line's kind can name one (ambient@other), so a test has a channel the agent talks in beside the one it tunes out. With CHANNELS unset it registers #noisy alone, as before. The tune-out tests' stand-in registries answer getDescriptor as a registry does for a registered channel, since the cancel's wake now asks it. The new tests drive the default through channel_publish with no channelId and a skip_reply self-wake's plain speech, not a heartbeat tick's plain speech, so they hold whether or not a heartbeat turn keeps the fallback. Review findings this answers: R1-1, a message the coalescer admits after its channel was registered anew; R1-2, a tuned-out channel that goes, and the cancel wake for a channel gone from the registry; R1-3, the cancel wake taken into a turn already open, in the changelog and this message; R1-4, how far the default falls back, in the changelog and a test.
ed4ddb7 to
e0aa4e5
Compare
|
@Anarchid All four R1 findings are answered in R1-1, fixed. The condition that lets the coalescer's acceptance set the default now also requires R1-2, fixed. One rule changed beside R1, and R1-2's fix holds under it. A channel that was the default when it went is kept, gone, until the next message comes. It stays the default while no newer message counts, even when a newer message sits on a tuned-out channel, and a call that would default to it is refused. At R1-3, fixed in text. The changelog now says that, taken into a turn already open, the cancel's dump "leaves that turn's locus where it is, while the subconscious's note on that channel, when there is one, can move it, as an unaddressed message there can." The commit message says so too. R1-4, a changelog sentence and a test, with no bound. The changelog now says the default "may be an older message, on another channel or another server". All the runs here were on Linux with Node 24.22.1, context-manager 0.23.1, membrane 0.8.0, chronicle 0.4.0 and mcpl-core 0.4.0, in a clean checkout whose files are — Daniel-2612 |
main's supplied-text-in-context tests build a TuneOutCoordinator over stand-in registries. The cancel's wake now asks the registry for its channel's descriptor, so those stand-ins answer getDescriptor as a registry does for a registered channel, as the tune-out tests' stand-ins on this branch do.
|
We're closing this pull request.
The runs were on Linux with Node 24.22.1, with Hamish-2699 ran the tests, and Daniel-2612 wrote this comment. — Daniel-2612 |
While a channel is tuned out, it's now never the residents' default channel, and no message on it is the message they're taken to be replying to, whether the tune-out diverted that message or it came before the tune-out began.
channel_publishwith nochannelIdposts where the latest message on a channel that isn't tuned out came in, and in locus mode askip_replyself-wake's plain speech goes there too. When a tune-out ends, its channel's messages count again, and the resident's wake for that end names the channel while the registry still holds it. This changeschannel_publish's description and itschannelIdparameter's description, which every resident with that tool is sent.Problem
On
59d609586e, themaincommit this change starts from,ChannelRegistrykeeps one default: the channel, message and thread of the latest message it accepted. It sets them inmarkAccepted, before the framework decides to divert the message. These runs used a realAgentFrameworkwith a subconscious, its heartbeat, and two channels, #other and #noisy. The resident first answered a message, E1, in #other:incomingFornamed the diverted message. In a control case in the same probe run, with nothing diverted, the tick spoke in #other.59d609586eit names no channel (src/tune-out/coordinator.ts:462). After a diverted message, with the default on that message, its turn spoke into #noisy.incomingForstayed N0.Changes
src/mcpl/channel-registry.ts: the registry keeps each channel's latest accepted message (latestInbound), with a number (seq) recording the order they were accepted in. It works the default out when the default is read: the newest message on a channel that has no tune-out state then.getDefaultPublishChannel,incomingFor,buildChannelContext,globalFallbackForand the publish resolver all read it this way.channels/changed, is kept, gone, until the next message comes. It stays the default while no newer message counts, even when a newer message sits on a channel that's tuned out. A removed server's channel stays it with no message behind it, soincomingFornames none. Any other channel that goes is dropped then, a tuned-out one included, so it doesn't become the default when its tune-out ends.src/tune-out/coordinator.tsandsrc/framework.ts: the wake when a tune-out ends names its channel and server while the registry has a descriptor for that channel, as a channel message's wake does, so a turn that wake starts alone puts its plain speech there. When the channel is gone from the registry, as after its server is removed, the wake names no channel.channel_publish: its description and itschannelIdparameter's description now say the default is the channel of the most recent incoming message "on a channel that isn't tuned out", and that a notice such as a join doesn't set it. A resident whose host runs this change is sent these new texts in the tool definitions of its requests, so a provider's prompt cache that holds the old texts doesn't match them.getDescriptor.changelog.d/tune-out-keeps-default-channel.changed.md.Tests
test/tune-out-default-channel.test.tsis new, with 10 tests, on a realAgentFrameworkwith a subconscious and the tune-out test server, in locus mode. Where a test has the resident callchannel_publishwith nochannelId, it does so in a heartbeat tick's turn.channel_publishwith nochannelIdposts in that message's channel.skip_replyself-wake's plain speech goes where the resident last heard a message.channel_publishwith nochannelIdposts where it heard a message before that, and that channel counts again once the tune-out ends.channel_publishwith nochannelIdposts.In
test/shared-channel-id.test.tsandtest/channel-lifecycle-order.test.ts, nine new tests cover a channel that goes:The existing removal tests in those files assert what they did before.
On Linux with Node 24.22.1, context-manager 0.23.1, membrane 0.8.0, chronicle 0.4.0 and mcpl-core 0.4.0, in a clean checkout whose files are
e0aa4e5f43's:tscexits 0, and again with59d609586e'ssrc/framework.ts,src/mcpl/channel-registry.tsandsrc/tune-out/coordinator.tsin place.npm testruns 8,539 tests, of which 8,477 pass, none fail and 62 are skipped.59d609586e, 7 of the 10 tests intune-out-default-channelfail. The three that pass there are the test of a diverted message counting once its tune-out ends, the test of the wake when a tune-out ends after a diverted message, and the test of that wake after its channel's server was removed.With #861
@antra-tess's open #861, at
3a24c4728f, changes the framework so that a heartbeat turn's plain speech is held as drafts. Merged with this change (e0aa4e5f43), it conflicts in two comments, the turn-locus comment insrc/framework.tsandInferenceRequest.channelId's doc insrc/types/agent.ts, where both changes edit the same comment lines. No merged checkout was run at this head.Not verified
incomingFor's message through a refusal.changelog.d/tune-out-keeps-default-channel.changed.md.Hamish-2699 wrote the change, and Ada-1017 made its design calls. Desmond-2579, Amos-2081 and two internal reviews read earlier versions, and Alfred-2570 read the changed tests and changelog. Duncan-2410 read the round that answers @Anarchid's review, and an internal review read an earlier version of it. Daniel-2612 wrote this description.