Repository navigation
refactor(api): one session-id resolver for channel resets, and an honest /new count - #7701
Conversation
|
All four red lanes (
#7159 is itself red right now ( Flagging rather than acting on it, since sequencing two of your PRs is your call. Nothing pushed to either branch. |
…ragment The fragment referenced #7701 (a separate, unmerged PR reporting the same symptom) instead of this PR's own number, so the release flow's generated-line dedup would never match it and #7705 would appear twice in the release notes. Also rewrap several newly-added or touched comment blocks to one sentence per line per the repo's prose convention.
#7705) * fix(channels): derive the reset commands' session scope from one place `/new`, `/reboot` and `/compact` each re-derived the `(channel, chat_id)` pair that the kernel turns into a `SessionId` via `SessionId::for_sender_scope`, rather than reusing what the inbound message path derived. The two disagreed in two independent ways. `build_sender_context` falls back to the metadata-carried sender id when `sender.platform_id` is empty; the command arms passed `None`. The Telegram adapter is exactly the case the inbound fallback exists for — its comment names it — so inbound messages landed in `for_channel(agent, "telegram:<user_id>")` while `/new` cleared `for_channel(agent, "telegram")`. The reset was honest about an empty session and the conversation the user could see kept every message. `build_sender_context` also sanitizes a channel name that collides with the kernel's reserved system channels so a `Custom("cron")` adapter cannot address the internal cron session; the command arms used the raw name. Both producers now call one `session_scope` helper, following the same single-source-of-truth pattern as `SessionId::for_sender_scope` and `compose_sender_scope` on the kernel side. `handle_command` takes the metadata-derived sender id as an argument because it only ever received `&ChannelUser`, which is what made the drift possible in the first place; every call site already holds the `ChannelMessage`. Two regression tests pin the invariant at the boundary: the command scope must be byte-identical to the inbound scope for the empty-`platform_id` Telegram shape, and a reserved `Custom` channel name must come back rewritten. Reverting `session_scope` to the old command-path derivation fails both. * fix(channels): reference the merged PR in the reset-scope changelog fragment The fragment referenced #7701 (a separate, unmerged PR reporting the same symptom) instead of this PR's own number, so the release flow's generated-line dedup would never match it and #7705 would appear twice in the release notes. Also rewrap several newly-added or touched comment blocks to one sentence per line per the repo's prose convention.
The librefang#7701/librefang#7159 heads were rebased onto a main that replaced the thread_ownership registry param with a derived sender_user_id in handle_command; the conflict resolutions mixed the two APIs. Rebuild the command path on the new API (BindingContext-based resolution) and keep the workflow-run owner threading on run_workflow_text.
932b006 to
da80a17
Compare
64574ac to
cedd338
Compare
… too The channel reset helpers only reset the channel-derived sid, but the message path can resolve the conversation to the agent's canonical session (entry.session_id) — e.g. when the sender context falls past the channel branch. /new then acked success on a dead session while the visible conversation kept its history (observed live on proteo with deannatroi: derived session 0 messages, canonical 198). Reset both sessions when they differ, and report the cleared message count in the ack so a silent no-op is diagnosable from the chat itself. Regression test: reset_channel_session_clears_derived_and_canonical.
cedd338 to
b60b66b
Compare
houko
left a comment
There was a problem hiding this comment.
The diagnosis and the fix are right — a reset that acks against a session holding nothing while the visible conversation keeps 198 messages is exactly the bug worth chasing, and the regression test seeds both sids and asserts both are cleared.
One thing is now wrong, though, and it is in the sentence the user reads.
reset_channel_session still ends with:
Ok(format!(
"Session reset for this {channel} chat ({cleared} messages cleared). Other surfaces untouched."
))That claim was true when the reset touched only the channel-derived sid.
It is not true after this change: canonical_session_besides returns entry.session_id, which is precisely the session the WebUI chat and the routing chains resolve to — the doc comment on the new helper says so.
So /new in Telegram now clears the WebUI conversation as well, and tells the user it did not.
That is the more consequential half of the change, and it is the half the ack hides.
Either the wording should say what actually happens ("this chat and the agent's main session"), or — if clearing the canonical session from a channel is meant to be scoped rather than global — the reset needs to be narrower than ResetScope::Session(csid).
The same applies to /reboot and /compact, which now reach the canonical session too; whatever those two ack should agree with whatever /new ends up saying.
Smaller note on the count: cleared sums the canonical session's messages and then the derived session's, so a user who has 198 in one and 3 in the other is told "201 messages cleared" for what they experience as one conversation.
Reporting the count for the session the reply is being sent into, or naming both numbers, matches what they can actually verify.
houko
left a comment
There was a problem hiding this comment.
Well-diagnosed bug, and the regression test with the seeded canonical/derived pair is exactly right. But "reset both" is a superset of the fix, and it takes out a surface the ack promises not to touch.
Blocking
1. /new in a channel now wipes the WebUI conversation, while still saying it didn't.
The new helper's own doc comment names who owns the canonical session:
the agent's canonical session (
entry.session_id) wins whenever the sender context falls past the channel branch — the WebUI chat, and some routing chains.
So when the two differ, entry.session_id is not a dead session — it is the session the WebUI chat is using. Resetting it unconditionally from a Telegram /new destroys that conversation, and the reply still reads:
"Session reset for this {channel} chat ({cleared} messages cleared). Other surfaces untouched."which is now false in precisely the case this change was written for. The proteo symptom was "we reset the wrong session"; the fix applied is "reset both", which trades a silent no-op for silent collateral damage — arguably the worse of the two, since the no-op was recoverable.
What the command should do is reset the session the conversation actually resolved to. The message path already has that resolution order (the comment describes it); factoring it into one function and calling it from both the message path and the reset path fixes the divergence at its source and keeps the ack honest. Failing that, reset both but change the wording — an operator needs to know the WebUI history went with it.
2. /reboot and /compact have the same blast radius and no wording change at all.
Both now act on the canonical session first with no ack update. /compact in particular is destructive-ish and silent about it.
3. The canonical operation aborts the primary one on failure.
if let Some(csid) = self.canonical_session_besides(agent_id, sid) {
self.kernel.compact_agent_session_with_id(agent_id, Some(csid), true).await
.map_err(|e| format!("{e}"))?;
}
self.kernel.compact_agent_session_with_id(agent_id, Some(sid), true).await…The ? on the secondary target runs before the primary one is touched, so a failure compacting the WebUI session means the user's own channel chat is never compacted and they get an error for something they did not ask about. Same shape in reboot_channel_session. If both targets stay, the secondary should be best-effort (log and continue), or the primary should go first.
Non-blocking
if let Ok(Some(s)) = self.kernel.memory_substrate().get_session(sid)folds a lookup error into "zero messages", so a substrate failure reports0 messages clearednext to a successful-looking ack. Worth distinguishing, given the whole point of the count is to make a no-op visible.- The
clearedtotal sums two sessions into one number, so "2 messages cleared" cannot tell the user that one came from a surface they were not looking at. If both sessions keep getting reset, reporting them separately would make the blast radius self-evident. - The helper name
canonical_session_besidesreads as "besides" = "other than", which is right, butcanonical_session_if_differentsays it without the second parse. Minor.
| .await | ||
| .map_err(|e| format!("{e}"))?; | ||
| let cleared = match cleared { | ||
| Some(n) => format!("{n} messages cleared"), |
There was a problem hiding this comment.
"1 messages cleared" — this string is user-facing in every channel chat, and the new test at line 3728 pins the ungrammatical form.
format!("{n} message{} cleared", if n == 1 { "" } else { "s" }).
There was a problem hiding this comment.
Fixed in 342d24a.
crates/librefang-api/src/channel_bridge.rs:1958 now branches: let message = if cleared == 1 { "message" } else { "messages" };, used in the format string at line 1965.
reset_channel_session_leaves_the_canonical_session_untouched (channel_bridge.rs:3732) asserts the reply contains "1 message cleared".
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn reset_channel_session_leaves_the_canonical_session_untouched() { |
There was a problem hiding this comment.
Neither of these two tests exercises the only input class whose resolved session id this diff changes.
"telegram" is not a reserved name, so SessionId::for_sender_scope(agent, "telegram", Some("chat-42")) — what origin/main computed here — and channel_session_id(agent, "telegram", Some("chat-42"), false) are the same value.
The session identity these tests assert therefore held before the change as well; the only assertion origin/main fails is reply.contains("1 messages cleared"), which is the ack wording, not the resolution.
The case that actually differs is a reserved channel name going through KernelBridgeAdapter::channel_session, and nothing in this diff covers it.
The kernel-side test pins channel_session_id against resolve_dispatch_session_id directly and never touches the adapter.
Seeding ext-cron:chat-7, calling reset_channel_session(agent, "cron", Some("chat-7")), and asserting the internal SessionId::for_channel(agent, "cron") session is untouched would cover the change and guard the reserved-name path at the adapter boundary.
There was a problem hiding this comment.
Fixed in 342d24a.
Added reset_channel_session_scopes_a_reserved_name_away_from_the_internal_session at crates/librefang-api/src/channel_bridge.rs:3762.
It seeds an ext-cron:chat-7 session and the internal SessionId::for_channel(agent, "cron") session with distinct message counts, calls reset_channel_session(agent, "cron", Some("chat-7"), false) through the adapter, and asserts the external session is cleared while the internal one keeps its message, covering the reserved-name path at the adapter boundary as suggested.
| The reset commands used to re-derive the session id themselves, which is only correct for as long as two independently written derivations agree; when they drifted apart the command acked success against a session holding no messages while the visible history survived untouched. | ||
| Both ends now call the kernel's `channel_session_id`, which also carries the reserved-channel guard that a hand-rolled derivation could forget. | ||
| The `/new` ack reports how many messages were cleared, so a no-op is visible from the chat itself, and says so explicitly when the count could not be read rather than printing zero for a lookup that failed. | ||
| (#7701) (@DaBlitzStein) |
There was a problem hiding this comment.
(#7701) (@DaBlitzStein) should close the previous sentence rather than stand on its own line.
cargo xtask collect-fragments indents every continuation line by two spaces, so this assembles into the release body with an orphan attribution line hanging under the bullet.
changelog.d/README.md ("ending with (#PR) (@your-github-login)") and its worked example both put it inline, as do the neighbouring fragments — see changelog.d/fixed/6943-workflow-create-persistence-failure.md.
There was a problem hiding this comment.
|
One more, posted here rather than inline because the line is outside this diff.
Not currently exploitable: |
…ession id
`resolve_attachment_session_id` was the fourth inline
`for_sender_scope(agent, channel, chat)` mirror, and the only one of the
four without the reserved-name guard: an external sender whose channel
happens to carry a reserved system name ("cron") would have landed the
attachment on the internal system session id. Not exploitable through
current callers — request_sender_context sanitizes at construction —
but the guard this PR adds to the other three sites exists precisely so
that drift class cannot reopen at one of them.
The channel branch now delegates to
`LibreFangKernel::channel_session_id(agent, ctx.channel, ctx.chat_id,
ctx.is_internal_system)`, the single resolver the dispatch paths and the
/new /reboot /compact handlers already name, so all four sites share one
guard. Behaviour is unchanged for every non-reserved channel name.
The new test pins the guard at this site: an external "cron" sender
resolves to a different id than the unguarded `for_sender_scope` would
produce. Red by construction — before this change the function returned
exactly that unguarded value.
|
The fourth site is migrated at
Pinned with a test: Verification on the pushed tip: all five |
… how reserved names resolve The 09-07 review found the /new count had three defects and the reserved name assumption was invisible to the compiler: - The count was read before the reset, outside the agent and session locks the delete takes, from a second full row decode. reset_session now returns the number of messages it cleared, read under the same lock from the row the ownership check already loaded — one read, and an inbound turn can no longer make the ack under-report. - The count included the messages inject_reset_prompt pushes into the fresh session: a second /new with a configured reset prompt reported "1 messages cleared" on a true no-op. The count is now taken from the pre-wipe row, before any injection. - A per-agent count broke the ack dedup in apply_channel_reset — a broadcast /new produced one near-identical line per agent with no way to tell whose number was whose. The ack now carries the agent's name, so every reply is self-identifying. And one message is "1 message". - is_internal_system was hardcoded in the adapter on a doc-comment claim. It is now threaded through ChannelBridgeHandle, so each caller states how a reserved channel name resolves instead of the adapter assuming external ingress. The reserved-name path — the only input class whose resolved id the resolver change affects — is pinned by an adapter test: seeding an external ext-cron chat and the internal cron session, running the reset externally, and asserting the internal session survives. The changelog fragment closes its last sentence with the attribution instead of leaving it on an orphan line, per the README's worked example.
|
All eight 09-07 inline findings addressed at The title and body now describe the diff. You were right that the resolution is byte-identical for every reachable caller — The count is now read by the reset itself. The broadcast reply is self-identifying. The ack carries the agent's manifest name, so the dedup one level up in
The reserved-name path is pinned at the adapter. The fragment closes its previous sentence with the attribution, per the README's worked example. Verification on the pushed head: |
|
All eight inline findings from the 09-07 sweep are now answered in their own threads, each with the commit, the resulting file:line and the test name. Seven were addressed in I had replied to earlier rounds in this thread rather than on your line comments, which left those threads unanswered and unresolvable on your side even where the code was already fixed. That was my mistake, and it applies to #7749, #7781, #7973 and #7974 as well — I am working through those the same way. Ready for another look whenever you have time. |
| // `inject_reset_prompt` can add anything to the fresh session — | ||
| // a second `/new` with a configured reset prompt reports 0, not | ||
| // the injected messages (#7701 review). | ||
| let cleared = old_session.as_ref().map(|s| s.messages.len()).unwrap_or(0); |
There was a problem hiding this comment.
Re-review of the 09-07 finding "the count includes the messages reset_one_session injects": not fixed, and the scenario I gave reproduces verbatim on this head.
Moving the count ahead of inject_reset_prompt only excludes what this reset injects. It does not exclude what the previous reset injected, and those are exactly the messages the row holds on a second /new.
inject_reset_prompt pushes Message::system(...) into the fresh session (session_ops.rs:962 for System-position injections, session_ops.rs:964-971 for the legacy reset_prompt, session_ops.rs:975 for AfterReset), and that session is persisted at the same sid. The next /new reads that row at session_ops.rs:445, and line 459 counts it.
Concrete scenario, unchanged from the last round — config.toml has [session] reset_prompt = "Fresh start.":
- Telegram chat with 12 messages, user types
/new→ "12 messages cleared", one system message injected into the fresh session. - User types
/newagain, having said nothing in between →old_session.messages.len() == 1→ "1 message cleared".
A true no-op still acks as a successful reset; the grammar fix only made it read better. With two System / AfterReset injections configured the second /new reports 3.
The changelog fragment states the opposite as a shipped property — "a configured reset prompt cannot be counted as history" (changelog.d/fixed/7701-channel-reset-session-resolution.md:4) — so this reaches the release notes as a claim the code does not hold up.
Of the two fixes suggested last round, only the first one closes it: count what the user can see, not what the row holds.
| let cleared = old_session.as_ref().map(|s| s.messages.len()).unwrap_or(0); | |
| // Counted from the row already read under the lock, and restricted to | |
| // non-system messages: `inject_reset_prompt` seeds the fresh session | |
| // with `Message::system` entries, so counting the whole row makes the | |
| // *next* `/new` report the previous reset's injections as cleared | |
| // history — a true no-op acking as a successful reset (#7701 review). | |
| let cleared = old_session | |
| .as_ref() | |
| .map(|s| { | |
| s.messages | |
| .iter() | |
| .filter(|m| m.role != librefang_types::message::Role::System) | |
| .count() | |
| }) | |
| .unwrap_or(0); |
There was a problem hiding this comment.
Fixed exactly as suggested — the count now filters Role::System rather than moving the read point, since moving it only excluded what the current reset injects, not what a previous reset left behind.
crates/librefang-kernel/src/kernel/session_ops.rs (reset_one_session, and the identical bug I found in the sibling reset_all_sessions, introduced by the same commit and previously untested): both now filter old_session.messages by Role::System before counting.
Tests second_new_with_reset_prompt_reports_zero_cleared (per-session scope) and second_agent_wide_reset_with_reset_prompt_reports_zero_cleared_and_skips_summary (agent-wide scope), both in crates/librefang-kernel/tests/session_reset_scope_test.rs. Both configure [session] reset_prompt and reset twice with no user activity in between, then assert the second reset reports 0. Demonstrated red against the code this comment describes (old_session.messages.len(), unfiltered) before applying the fix, green after.
One thing this fix does NOT make structural, said plainly rather than implied: the filter works because Role::System and "message this diff injected" happen to coincide today — verified that inject_reset_prompt is the only production writer of a persisted Role::System message, across all four InjectionPosition phases. If something else ever persists a genuine Role::System message as real conversation history, this count would silently under-report again, and nothing here would catch it. The comment above the fix says this now.
While in there I also found the unfiltered count leaking into the >= 2 gate that decides whether to save a session summary — a no-op reset holding 2+ injected system messages still triggered an aux-LLM summary call over the reset prompt as if it were conversation. Fixed in the same pass, reusing the same non-system count for that gate.
| .id; | ||
| let external = | ||
| LibreFangKernel::channel_session_id(assistant, "cron", Some("chat-7"), false); | ||
| assert_ne!( |
There was a problem hiding this comment.
The test asked for is here, but it does not pin the guard — every assertion still passes with resolve_scope_channel removed from channel_session_id.
external on line 3778 is computed by calling LibreFangKernel::channel_session_id itself, the same function under test. Drop the guard and both the seed and the reset target move together to for_sender_scope(agent, "cron", Some("chat-7")), so:
- the premise
assert_ne!(external, SessionId::for_channel(assistant, "cron"))still holds —for_sender_scopecomposes"cron:chat-7"before hashing (librefang-types/src/agent.rs:371), so it is a different id fromfor_channel(agent, "cron")regardless of the guard; ext_after.messages.is_empty()still holds — the reset cleared whatever id the test seeded;internal_after.messages.len() == 1still holds, for the same reason the premise does.
So the only input class this diff changes is still uncovered. The assertion that discriminates is the one the sibling attachment test at routes/agents/mod.rs:2024 already makes: compare against the unguarded derivation.
| assert_ne!( | |
| let external = | |
| LibreFangKernel::channel_session_id(assistant, "cron", Some("chat-7"), false); | |
| let unguarded = SessionId::for_sender_scope(assistant, "cron", Some("chat-7")); | |
| assert_ne!( | |
| external, unguarded, | |
| "test premise: the reserved-name guard must rewrite 'cron' to 'ext-cron' \ | |
| before derivation — without it this test proves nothing" | |
| ); | |
| assert_ne!( | |
| external, | |
| SessionId::for_channel(assistant, "cron"), | |
| "test premise: external 'cron' must be ext-scoped, not the internal session id" | |
| ); |
Seeding unguarded too and asserting it survives the reset would make the failure mode unmistakable, but the premise assertion above is enough to fail the test if the guard is ever dropped.
There was a problem hiding this comment.
Added the unguarded-derivation comparison, exactly as suggested.
One correction to how I'd describe what's new here: reset_channel_session_scopes_a_reserved_name_away_from_the_internal_session already existed at this head, and the guard it exercises (resolve_scope_channel inside channel_session_id) was already correct and already applied — verified by reading 342d24a8d's version of both files. What was missing was a premise assertion that would fail if the guard were ever removed; without it, the test passed for a reason unrelated to the guard (for_sender_scope composing "cron:chat-7" is already a different id from for_channel's "cron"-only formula, guard or no guard).
Demonstrated: removed resolve_scope_channel from channel_session_id, ran this test alone, got the new assertion failing on two identical SessionIds. Restored, test passes.
| @@ -0,0 +1,4 @@ | |||
| `/new`, `/reboot` and `/compact` typed in a channel chat now resolve their target session through the same function the inbound message path uses, so they clear the conversation the user is actually looking at. | |||
| The reset commands used to re-derive the session id themselves, which is only correct for as long as two independently written derivations agree; when they drifted apart the command acked success against a session holding no messages while the visible history survived untouched. | |||
There was a problem hiding this comment.
The title and PR body were rewritten as asked, but the changelog fragment was not, and this file is what ends up in the GitHub release body verbatim — which is the half of that finding I was actually worried about ("so reviewers and the release notes are not told a live bug was fixed").
Lines 1-2 as written:
- "now resolve their target session through the same function the inbound message path uses, so they clear the conversation the user is actually looking at" — implies they previously did not. On this base they did:
session_scope(librefang-channels/src/bridge.rs:2902) already returnssanitize_channel_name(channel_type_str(channel)), andresolve_scope_channel("ext-cron", false)is the identity, so the new derivation is byte-identical for every reachable caller. The PR body now says exactly this; the fragment still says the opposite. - "when they drifted apart the command acked success against a session holding no messages while the visible history survived untouched" — that drift is already closed on
origin/main, by thesender_user_idthreading into the command arms and theSenderContextthreading through broadcast dispatch. Nothing in this diff closes it.
Line 4 also asserts "a configured reset prompt cannot be counted as history", which does not hold — see the session_ops.rs:459 thread.
What this diff actually delivers and can be claimed without qualification: one resolver instead of four hand-rolled mirrors; the reserved-name guard applied at the attachment site (resolve_attachment_session_id), which is a real behaviour change; the count sourced from the reset under its own lock; the agent name in the ack. That is a changed/ entry, not a fixed/ one — the only user-visible bug this closes is the attachment landing in a different session than the message for a reserved-named channel.
Please rewrite against the pushed head and move the file to changelog.d/changed/ (or keep the attachment fix in fixed/ and put the refactor in changed/).
There was a problem hiding this comment.
Rewrote the fragment and moved it to changelog.d/changed/.
On the first pass I made the same mistake this thread is about, one level down: the third sentence claimed the attachment-resolver change closed a live bug ("an external sender ... could have its attachment land on the internal system session"). A second review pass caught it — enumerated all four production callers of resolve_attachment_session_id, and none of them reach the resolver with an unsanitized reserved channel name: two go through request_sender_context, which sanitizes at construction time; one passes sender_context = None; one sets use_canonical_session: true, which skips the channel branch entirely. Same shape as the three reset commands: the fix makes the drift class structurally impossible, it does not close a bug anyone could hit. The fragment now says that explicitly, matching the standard already applied to the reset commands.
python3 scripts/check-changelog-attribution.py --staged passes on the current file.
| /// | ||
| /// #7701 is what disagreement looks like, and it has now cost two rounds: the channels half of the pair drifted first (`session_scope` in `librefang-channels::bridge` is the mirror of this function, and its doc-comment carries that half of the story), and the kernel half re-inlined `for_sender_scope(agent, resolve_scope_channel(..), chat)` at three sites that each had to remember the reserved-name guard. | ||
| /// A reset that derives its own id "succeeds" against an empty session while the conversation the user can see keeps every message — the failure is silent on both ends, because the ack is generated from the reset that did nothing. | ||
| pub fn channel_session_id( |
There was a problem hiding this comment.
New, and not blocking — flagging it because the doc-comment above claims this is the "single source of truth for the channel branch of session resolution" and one production derivation is still outside it.
mirror_channel_send_to_session derives its target with a bare SessionId::for_sender_scope(owner, channel, Some(recipient)) (crates/librefang-runtime/src/tool_runner/channel.rs:76), with channel coming straight off the channel_send tool's arguments and no reserved-name guard.
Scenario: an operator runs a ChannelType::Custom("cron") adapter. The inbound path scopes that conversation to ext-cron:<recipient> (session_scope → sanitize_channel_name), and after this PR /new and the attachment writer agree on the same id. The channel_send mirror still writes to cron:<recipient>, so the mirrored outbound message lands in a session nothing else reads, and /new cannot clear it.
librefang-runtime cannot call into librefang-kernel (circular), but it already depends on librefang-channels, so librefang_channels::types::sanitize_channel_name(channel) is available in that file and is the same rewrite resolve_scope_channel applies for an external caller. Different crate, so per the repo's deferral bar I'm not asking for it here — but either fix it or drop "single source of truth" to "single source of truth for the kernel-side dispatch and reset paths", so the next reader does not trust a guarantee that has a hole in it.
There was a problem hiding this comment.
Confirmed the gap — mirror_channel_send_to_session (crates/librefang-runtime/src/tool_runner/channel.rs:76) does derive with a bare for_sender_scope, no guard, and librefang-runtime does depend on librefang-channels without importing sanitize_channel_name in that file.
Narrowed the doc-comment as suggested: "single source of truth for the kernel-side dispatch and reset paths" instead of the unqualified claim, and named the gap explicitly.
Didn't fix channel.rs:76 itself — that's librefang-runtime, outside this PR's crate. It now has its own issue, #8243.
…count A second /new (or agent-wide reset) with a configured session.reset_prompt counted the previous reset's injected system message as cleared history, so a true no-op acked as a successful reset. Both reset_one_session and reset_all_sessions now filter Role::System out of the pre-wipe count.
The premise assertion compared the guarded derivation only against for_channel(agent, "cron"), which differs from for_sender_scope's "cron:chat-7" composition regardless of the guard - the test passed even with resolve_scope_channel removed from channel_session_id. Compare against the unguarded for_sender_scope derivation directly, which channel_session_id would produce verbatim if the guard were dropped.
The doc-comment claimed this was the single source of truth for every channel-scoped SessionId derivation. mirror_channel_send_to_session in librefang-runtime still derives its target with a bare for_sender_scope, no reserved-name guard, and cannot be migrated here without crossing the runtime -> kernel dependency direction. Narrow the claim to the kernel-side dispatch and reset paths and name the gap so the next reader does not trust a guarantee that has a hole in it.
A session holding only a previous reset's injected system messages (reset_prompt plus an AfterReset context_injection) still cleared the raw messages.len() >= 2 gate in reset_one_session and reset_all_sessions, so a true no-op reset still spawned an aux-LLM summary call over the reset prompt and injections, rendered into the transcript as if they were conversation. Both gates now reuse the non-system count already computed for the cleared total. Also covers the untested sibling: reset_all_sessions had the same injected-system-message miscount as reset_one_session (both introduced by the same earlier commit), with no regression test. The new test exercises ResetScope::Agent end to end: two resets, asserting the second reports 0 cleared and never re-summarises the first reset's injections.
…ment overstated reachability The fragment said an external sender on a reserved channel name could have its attachment land on the internal system session before this diff. Every production caller of resolve_attachment_session_id either passes no sender context, requests the canonical session, or builds the context through request_sender_context, which sanitizes the channel name at construction time - so the guarded and unguarded derivations were byte-identical for every reachable caller, same as the three reset commands. Migrating the attachment resolver closes the drift class, not a live bug; the fragment now says so.
|
Every one of the 12 review threads on this PR now has an inline reply, and the branch is at Each reply states what changed and where, the commit it landed in, and the literal failure from reverting the production block and running the test against it — a regression test that stays green with the fix removed guards nothing, so that cycle was run rather than assumed. Where a finding was not acted on, the reply says so and gives the reason instead of leaving the thread unanswered. Flagging it here because the review is still recorded as requesting changes, and a push does not retract that on its own. Ready for another look whenever it suits you. |
houko
left a comment
There was a problem hiding this comment.
LGTM. Both blocking findings are fixed, and the second one is fixed in the way that actually matters.
The test that could not fail. reset_channel_session_scopes_a_reserved_name_away_from_the_internal_session now compares external against the unguarded derivation:
let unguarded = SessionId::for_sender_scope(assistant, "cron", Some("chat-7"));
assert_ne!(external, unguarded, "test premise: the reserved-name guard must rewrite 'cron' to 'ext-cron' …");That is the discriminating premise. The old assert_ne!(external, for_channel(assistant, "cron")) passed with the guard removed, because for_sender_scope composes "cron:chat-7" before hashing and is a different id from for_channel's "cron"-only formula either way. Deleting resolve_scope_channel from channel_session_id now fails this test on its first assertion, which is what it is for. The premise comment says so explicitly, so the next person cannot quietly weaken it back.
The input class the earlier tests missed. Covered — "cron" with an external (is_internal_system = false) caller, seeded on both sides, asserting the internal for_channel(assistant, "cron") session keeps its message while the ext-cron one is emptied. That is the only input class whose resolved id this diff changes, and it now has a test.
The third point (mirror_channel_send_to_session deriving outside the single source of truth) I raised as non-blocking, and it is resolved the right way for a non-blocking one: the doc-comment no longer claims to be the source of truth for every derivation, it names the one production site that is still outside it, and it records that the guard is reachable from librefang-runtime via librefang_channels::types::sanitize_channel_name — so whoever migrates it does not have to rediscover that the circular-dependency objection applies to the kernel constants and not to the channels crate. That is the same conclusion #7995 reached from the other direction, which is a good sign.
All checks green; BLOCKED is the stale CHANGES_REQUESTED this approval clears.
Process note: #8228 landed a large rewrite of crates/librefang-channels/src/bridge.rs after this PR's CI ran, and crates/librefang-kernel/src/kernel/messaging.rs is also touched by the still-open #8213. Git reports mergeable, but the main ruleset has no strict status-check policy, so I am updating the branch to re-run CI against the current base rather than merging on a green that predates #8228.
What this diff does
Four things, none of them a live-bug fix — the symptom originally quoted in this PR (derived 0 messages, canonical 198) was the fall-past-the-channel-branch case, which
origin/mainalready closed by threading aSenderContextthrough broadcast dispatch. What remains is making the drift class structurally impossible and the/newack trustworthy. houko's review asked for exactly this reframing, and the title now matches it.1 — One session-id resolver for the channel reset commands.
/new,/rebootand/compactderive their target sid throughLibreFangKernel::channel_session_id, the same function every dispatch resolver takes for channel traffic, instead of re-deriving it by hand.is_internal_systemis threaded through theChannelBridgeHandletrait so the caller states how a reserved channel name resolves rather than the adapter assuming it — these handlers are methods on a public trait, and "reachable only from external ingress" was true but invisible to the compiler. For every caller that exists today the derivation is byte-identical (session_scopealready sanitizes, andsanitize_channel_nameis idempotent); the value is that the fourth hand-rolled mirror cannot reappear, and theresolve_attachment_session_idsite — the fourth inlinefor_sender_scopecopy, the one without the reserved-name guard — is migrated to the same resolver.2 —
/newreports a count worth trusting. The number comes fromreset_sessionitself: read under the same agent and session lock the delete takes, so an inbound turn cannot make the ack under-report, and taken from the pre-wipe row beforeinject_reset_promptcan add anything — a second/newwith a configured reset prompt reports 0, not the injected messages. The ack carries the agent's name, so a broadcast/newreply is self-identifying (the dedup one level up only collapses byte-identical strings; per-agent counts made every line differ with no way to tell whose number was whose). And one message is "1 message", not "1 messages".3 — The reserved-name path is the only input class whose resolved id the resolver change touches, and it is now pinned by an adapter test. Seeding
ext-cron:chat-7and the internalSessionId::for_channel(agent, "cron")session, calling the reset externally, and asserting the internal session survives covers the guard at the adapter boundary — the kernel-side test pinschannel_session_idagainstresolve_dispatch_session_iddirectly and never touches the adapter.4 — Changelog fragment closes its last sentence with the attribution instead of leaving it on an orphan line, per the README's worked example.
Verification
On the pushed head:
cargo checkof kernel, channels and api; the three adapter tests by name (reset_channel_session_leaves_the_canonical_session_untouched,reboot_and_compact_channel_session_leave_the_canonical_session_untouched,reset_channel_session_scopes_a_reserved_name_away_from_the_internal_session); the channels bridge tests against the updated trait;cargo clippy -p librefang-api -p librefang-channels --all-targets -- -D warnings.Review trail
9d32f4afb): reset only the session the conversation resolved to, through the factored resolver. This sweep's finding that the resolution is identity for every reachable caller is correct and is now what the title and this body say.is_internal_system, grammar, missing reserved-name coverage, fragment line) are the changes in item 2 and 3 above.