fix: resolve four reported bugs (#6423, #6442, #6443, #6444)#6449
Conversation
- #6443 (security): reject cross-account (cross-tenant) channel_send — the account_id tool parameter was accepted from the model unvalidated, so on a multi-tenant daemon an agent (via hallucination or prompt injection) could dispatch content into another tenant's chat; the kernel now stamps the turn's originating account into the manifest and the in-process tool rejects a mismatched explicit account_id on the same channel, mirroring the #6117 recipient guard (in-process path; the /mcp bridge is flagged as a parallel surface in the PR body). - #6442: derive the fallback agent.toml path from the agent UUID instead of the literal "agent" when the DB has no source_toml_path, so agents whose display name is entirely non-ASCII no longer collapse onto the shared workspaces/agents/agent/agent.toml and auto-discovery finds the real directory. - #6444: set metadata["was_mentioned"] and detect 1:1 DMs (m.direct / joined-member count) in the Matrix sidecar adapter, so the default mention_only policy no longer silently drops all Matrix inbound. - #6423: resolve the model context window provider-aware via a new ModelCatalog::find_model_for_manifest that reconciles the openrouter/ prefix mismatch, so OpenRouter agents stop showing the 8192 unknown-model denominator (all eight provider-blind lookup sites routed through it). Verified: cargo check --lib --tests and clippy -D warnings on librefang-runtime / librefang-kernel / librefang-types, scoped cargo test (runtime channel_send + find_model_for_manifest, kernel non_ascii_fallback), and sdk/python matrix adapter tests (120 passed).
github-actions
Bot
added
size/L
…y lookup global_thinking_backfill_allowed() called the provider-blind find_model(), the same lookup this PR's #6423 fix replaced everywhere else in agent_execution.rs and messaging.rs. A bare OpenRouter manifest model (no `openrouter/` prefix) missed the prefixed catalog entry and fell back to the permissive "unknown model" default, silently backfilling global [thinking] config onto a model that does not support it. Route this call through find_model_for_manifest(provider, model) like the other eight call sites, and add a regression case alongside the existing #6398 test.
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Reviewed the diff against CLAUDE.md (worktree/hook rules, deterministic-prompt-ordering, serde-default gotcha, conventional commits, prose wrapping) plus general correctness. Verified cargo check -p librefang-kernel -p librefang-runtime -p librefang-types --lib --tests and cargo clippy -p librefang-kernel --all-targets -- -D warnings clean in a fresh worktree on this branch.
One thing I fixed directly and pushed (58a4684): manifest_helpers::global_thinking_backfill_allowed still called the provider-blind find_model(model) — the exact lookup this PR's #6423 fix replaced at its other eight call sites in agent_execution.rs / messaging.rs. A bare OpenRouter manifest model would miss the prefixed catalog entry here too and fall back to the permissive "unknown model" default, silently backfilling [thinking] config onto a model that doesn't support it. Routed it through find_model_for_manifest(provider, model) like the rest, and added a regression case next to the existing #6398 test.
Two inline comments below on things I'm not confident are worth fixing unilaterally (both same-bug-class as things this PR does fix, but touch code this PR doesn't, so I've left them for your call rather than expanding the diff).
Also worth a quick look while you're in this area, though I couldn't leave it as a line comment since these files aren't part of the diff: find_model_for_manifest reconciles the OpenRouter prefix mismatch for context-window/tool-support/thinking-backfill lookups, but a few other provider-blind find_model(&model) calls resolving reasoning_echo_policy from a (provider, model) pair look like the same gap — they discard the provider and would miss a bare OpenRouter model too: kernel/session_ops.rs:1204 and kernel/triggers_and_workflow.rs:75 (both discard resolution.resolved's (String, String) provider via .map(|(_, m)| ...)). kernel/handles/catalog_query.rs:25 and :38 (lookup_reasoning_echo_policy / lookup_supports_vision) have the same symptom but are behind the CatalogQuery trait shared with librefang-kernel-handle / drivers, so fixing those would mean a cross-crate signature change rather than a local one. Not blocking — just noting the fix may be more incomplete than the PR description's "all eight" implies, since these are a different symptom (reasoning-echo-policy / vision, not context window) that share the same root cause.
Generated by Claude Code
| None | ||
| } | ||
|
|
||
| /// Resolve a catalog entry for a manifest's `(provider, model)` pair, |
There was a problem hiding this comment.
Minor, repo-rule nit: CLAUDE.md's prose-wrapping rule says doc comments should break only at sentence boundaries, not at a column width — "one sentence = one line, regardless of length." This doc comment (and quite a few others this PR adds — e.g. the channel_send account-guard comment block in tool_runner/channel.rs:245-261, the #6442 fallback comments in agent_state.rs/boot.rs, several in matrix.py) hard-wraps mid-sentence at ~80 cols instead, e.g.:
/// Resolve a catalog entry for a manifest's `(provider, model)` pair,
/// reconciling the provider-prefix mismatch that OpenRouter (#6423) — and
/// any provider whose live catalog ids are stored as `{provider}/{raw_id}`
/// — exhibits.
That's one sentence split across 4 lines. I recognize this matches the pre-existing style already used nearby (e.g. the #6117 comments this PR extends), so it may just be established practice in this crate rather than an oversight — flagging in case it's actually intended to be enforced going forward, since the rule is explicit about applying to "any paragraph an agent is already touching." Not blocking either way.
Generated by Claude Code
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod non_ascii_fallback_tests { |
There was a problem hiding this comment.
These regression tests cover the three fallback sites this PR fixes (agent_state.rs, boot.rs), but there's a fourth call site in this same file that looks like the same collision class and wasn't converted: backfill_workspace_dir (workspace_setup.rs:366-367), a few hundred lines up in this file:
let safe_hand = safe_path_component(hid, "hand");
let safe_role = safe_path_component(role, "agent");If hand_role (read from a tag, potentially free text) sanitizes to empty, every such hand agent collapses onto the shared literal "agent" fallback — the same bug as #6442. hid is probably already a slug/id, so less likely to be non-ASCII, but role looks like it could plausibly be operator-supplied. Flagging in case it was simply missed rather than deliberately out of scope; not necessarily blocking.
Generated by Claude Code
- #6423 completeness: global_thinking_backfill_allowed still called provider-blind find_model, so an OpenRouter agent whose manifest model is stored bare (dashboard set_agent_model) missed the prefixed catalog entry, returned None → unwrap_or(true), and backfilled the global [thinking] config onto a non-reasoning model — the exact #6398 parameter-error regression the gate prevents; the helper now takes provider and routes through find_model_for_manifest, with a regression test. - #6443 WhatsApp-style adapters: the cross-account guard used an exact sender_channel/channel string match, so it no-opped on adapters that embed the conversation id in the channel (WhatsApp gateway stamps sender_channel="whatsapp:<jid>" per #5227) while channel_send targets the bare "whatsapp"; the guard now compares the base channel type via a channel_base helper, with a regression test. - #6443 metadata contract: the sender_account_id metadata key is now a shared librefang_types::agent::SENDER_ACCOUNT_ID_METADATA_KEY constant used at both kernel stamp sites and both runtime read sites, so a typo cannot silently disable the security guard. - #6444 restart persistence: the Matrix DM cache was in-memory only, so a daemon restart (which resumes /sync from the persisted since_token, whose incremental response omits m.direct for unchanged rooms) started empty and dropped every established DM under MentionOnly; the producer now seeds the DM registry via an explicit m.direct account-data GET at startup. - #6444 m.direct semantics: _update_dm_rooms_from_account_data now FULL-REPLACES the m.direct-derived DM set (it is a complete-state event) instead of add-only, so a room removed from the registry stops being treated as a DM; the member-count-derived DM set is tracked separately and FIFO-bounded like the sibling caches. Verified: cargo check --lib --tests + clippy -D warnings on librefang-runtime / librefang-kernel / librefang-types (clean), scoped cargo test (kernel global_thinking_backfill + non_ascii_fallback: 4 passed; runtime channel_send + find_model_for_manifest: 24 passed), and sdk/python matrix adapter tests (124 passed).
github-actions
Bot
added
size/XL
…ugh the /mcp bridge (#6443) (#6455) #6449 fixed the cross-account (cross-tenant) channel_send guard on the in-process agent-loop path and flagged the /mcp bridge (subprocess claude-code driver) as an unchanged parallel surface pending a maintainer call. This lands that parity, mirroring how #6125 threaded the #6117 peer scope through the same chain: - CompletionRequest gains sender_account_id — the bot account / tenant the inbound turn arrived on. Out-of-band callers (cron, triggers, compaction, routing probes) leave it None via the struct's existing Default. - Both agent loops (run_agent_loop and the streaming variant) populate it from the manifest metadata stamp the kernel already writes (SENDER_ACCOUNT_ID_METADATA_KEY, #6449). - The claude-code driver's write_mcp_config forwards it on the bridge connection as X-LibreFang-Current-Account-Id, next to the existing X-LibreFang-Current-Peer-Jid / -Channel / -Chat-Id headers; empty or absent values emit no header. - mcp_http reads the header back and routes tool execution through execute_tool_with_sender_account, so tool_channel_send's existing #6443 guard rejects an explicit account_id that differs from the turn's account on the same channel. Clients that omit the header run unguarded exactly as before. Tests: - claude_code::tests — the peer-scope header tests now cover the account header (emitted when present, omitted when absent). - api_integration_test::test_mcp_http_channel_send_cross_account_guard_uses_account_header — end-to-end /mcp tools/call: mismatched account blocked, matching account passes, missing header no-ops. Verified: cargo check --workspace --lib, cargo clippy --workspace --all-targets -D warnings (clean), cargo fmt, scoped cargo test (llm-drivers mcp_config: 5 passed; api channel_send guards: 2 passed; runtime channel_send: 23 passed). Co-authored-by: Evan <[email protected]>
2 participants
Summary
Fixes four open bug reports, verified present on
main:channel_sendaccount_idaccepted unvalidated → cross-tenant dispatchsafe_path_component()collapses any all-non-ASCII agent name to a shared fallback TOML pathis_group=trueand never setswas_mentioned, somention_onlydrops all inboundEach was cross-checked against the current code before fixing; #6423 in particular is fixed differently from the reporter's suggestion (see below).
#6443 — cross-account (cross-tenant)
channel_sendguardaccount_idselects which registered bot instance a send routes through. It was extracted from the model's tool input and passed straight to the kernel send path with no validation. On a multi-tenant daemon (several bot accounts, each with its owndefault_agentserving a different customer) an agent induced via hallucination or prompt injection could pass another tenant'saccount_idand dispatch content into that customer's chat. The #6117 guard does not cover this — it only comparesrecipientwithin the same channel and runs beforeaccount_idis even parsed.Changes:
SenderContext.account_id, already populated from the inbound adapter config name) intomanifest.metadata["sender_account_id"], next to the existingsender_chat_idstamp — bothmessaging.rsandagent_execution.rssites.channel_send.execute_toolkeeps its historical positional signature (external API tool-run routes and ~57 unit tests untouched) and forwardsNoneto a newexecute_tool_with_sender_account; only the in-process agent loop threads the stamped account.tool_channel_sendrejects an explicitaccount_idthat differs from the turn's account on the same channel, mirroring the bug(security/channels): channel_send can dispatch to a different recipient on the same channel (cross-chat message leak) #6117 recipient guard's explicit-value-only scoping. Auto-filled sends carry noaccount_id; out-of-band callers (cron / triggers / API) leave the accountNoneand the guard no-ops.DeferredToolExecutiongains anaccount_idfield (serde-default, back-compatible with persisted rows) so an approved, deferredchannel_sendstill enforces the guard.Question for maintainer —
/mcpbridge parity. #6117 fixed the in-process path and then the/mcpbridge (subprocessclaude-codedriver) in a separate commit. This PR fixes the in-process path for #6443 the same way. The/mcpbridge path (network.rsreadsx-librefang-current-*headers forwarded byclaude_code.rs::write_mcp_config) would need acurrent-account-idheader threaded throughlibrefang-llm-drivers+librefang-apifor full parity — a different crate/domain than the rest of this PR. Should I extend it here, or land it as a follow-up mirroring how #6117 was split? Flagged rather than silently deferred.#6442 — non-ASCII agent name TOML fallback collision
safe_path_component()strips every non-ASCII character. An agent whose display name is entirely Cyrillic / CJK / accented-Latin sanitizes to the empty string, and the three fallback sites passed the literal"agent", so every such agent collapsed ontoworkspaces/agents/agent/agent.toml: TOML edits were silently ignored and two such agents overwrote each other. The spawn-time resolver (resolve_workspace_dir) already uses the agent's UUID as the fallback, so the fallback sites disagreed with the directory spawn actually created.Fix: the three fallback sites (
boot.rsre-sync,persist_manifest_to_disk,reload_agent_from_disk) now passagent_id.to_string()as the fallback, matchingresolve_workspace_dir. Distinct agents never collide, and the fallback resolves the same directory spawn created (issue's suggested fix #2).#6444 — Matrix sidecar DM / mention detection
The adapter hardcoded
is_group=Truefor every room and never setmetadata["was_mentioned"]— the only mention signal the group gate reads. Under the defaultGroupPolicy::MentionOnly, 1:1 DMs and explicit @-mentions were both silently dropped (OB-06 group_gating_skip), so the bot appeared dead.Fix (both in
matrix.py):m.directaccount data or its/syncsummary reports two joined members. The verdict is cached (_dm_rooms) because the summary is sent lazily; a later summary-less sync does not flip a known DM back to group.was_mentioned— set toTruewhen the bot's ownMATRIX_USER_IDis in the MSC3952m.mentionslist, parity with the Discord / Feishu adapters.#6423 — OpenRouter context-window resolution
The symptom (8192 denominator) is real, but the reporter's diagnosis and suggested fix were both off:
openrouter/{raw_id}, but the dashboard model-picker (set_agent_model) strips the prefix before persisting, so the manifest holds a bareraw_id.find_model_for_provider(provider, model)still requires an exactidmatch, so it would also miss the prefixed catalog id.resolve_model_metadatais not a drop-in — it has zero production callers and its prefix-stripping does not cover theopenrouter/slash form.Fix: a new
ModelCatalog::find_model_for_manifest(provider, model)resolves in order — exact(provider, model), then re-qualified(provider, "{provider}/{model}"), then provider-blindfind_model(legacy fallback, so nothing that resolved before regresses). All eight provider-blindfind_model(&manifest.model.model)lookup sites in the kernel — context report, liveContextBudget(this is not merely a display bug), compaction, and tool-support detection — now route through it.Verification
cargo check -p librefang-runtime -p librefang-kernel -p librefang-types --lib --tests— clean (run natively in this worktree's isolatedtarget/, no contention with the shared tree).cargo clippyon the touched crates — zero warnings.librefang-kernel workspace_setup::non_ascii_fallback_tests— collision + spawn-parity for bug(kernel/agents): safe_path_component() collapses any all-non-ASCII agent name to the same fallback TOML path, breaking auto-discovery #6442.librefang-runtime tool_runner::tests— fivechannel_sendaccount-guard cases for bug(security/channels): channel_send account_id accepted unvalidated — cross-tenant dispatch across bot accounts #6443 (mismatch blocked, match/no-account/cross-channel/out-of-band pass).librefang-runtime model_catalog::tests::test_find_model_for_manifest_reconciles_openrouter_prefixfor OpenRouter-specific runtime context-window issue #6423.sdk/python tests/test_matrix_adapter.py— six cases for bug(channels/matrix-sidecar): every room is_group=true and was_mentioned never set — mention_only drops all matrix inbound, mentions included #6444 (was_mentionedset / not-set, DM via summary +m.direct, group via summary, cached verdict); 120 passed.Out-of-scope follow-ups
/mcpbridge parity for bug(security/channels): channel_send account_id accepted unvalidated — cross-tenant dispatch across bot accounts #6443 — flagged above, pending maintainer call.