Repository navigation
fix(channels): /think executes for real; commands resolve the conversation agent - #7159
DaBlitzStein wants to merge 7 commits into
Conversation
…e conversation agent Two defects behind slash commands confirming without acting (librefang#7140): 1. /think was a stub — stored nothing, never affected the model. Now: per-agent extended-thinking preference stored on the bridge adapter and applied as the per-turn thinking override through send_message_with_blocks_and_sender_thinking (new KernelApi method + kernel impl). 2. Command routing divergence — /new /reset /reboot /compact resolved the target agent via the router chain only, while the chat path also consults the conversation override (librefang#5671), sticky holder (librefang#5323), per-peer binding, and instance default. A divergent resolution reset a different agent than the one holding the conversation. resolve_for_command now mirrors the chat chain layer for layer. 551 channels tests pass.
houko
left a comment
There was a problem hiding this comment.
Blocking compile failure: crates/librefang-api/src/channel_bridge.rs constructs KernelBridgeAdapter without the required thinking_prefs field in two test fixtures (currently around lines 3348 and 3395). This causes Quality, Unit, Ubuntu, and aarch64 to fail before the behavior can be evaluated. Please initialize the field consistently with the surrounding adapter fixtures, then rebase/update onto current main so the already-fixed dependency security failure is no longer inherited.
houko
left a comment
There was a problem hiding this comment.
Two blockers remain on the current tip.
-
thinking_prefs is keyed only by agent_id, while the user-facing result says the preference applies to this chat. One agent can serve multiple conversations. A /think command in one conversation therefore changes thinking and cost behavior for every other conversation routed to the same agent. Please key this preference by the canonical conversation identity and pass that identity through set_thinking and the send path. Add a regression with two conversations sharing one agent.
-
resolve_for_command does not actually reproduce build_thread_key. The normal path adds peer_id from sender_user_id(message). The command path builds channel, account_id, chat_id, and thread only. Existing sticky claims are therefore stored under a different ThreadKey in group conversations and current_holder will miss them. Please share the canonical key builder or pass the already-built key into command dispatch. Add a regression where a group-thread claim includes peer_id and /new or /reset resolves the holder.
These are correctness issues in the two behaviors this PR claims to fix, so I do not recommend merging the current tip.
…n streaming send The /agent command only wrote the router user-default, never the per-conversation binding that resolve_for_command consults first, so the ack claimed the new agent while dispatch kept the old one. Persist the selection via set_conversation_binding keyed by (account, platform_id). The primary streaming send path never consulted per-agent thinking prefs, so /think was inert on streamed chats. Read the pref and pass it through. Regression tests cover the binding side effect, the adapter write-through, and the streaming-path override.
|
The assertion is The deeper issue is that the assertion doesn't test what the test is named for. Its own comment concedes this: "What matters here is that the stream started and ended with a reported failure — the pref read and the thinking-aware kernel entry were both reached." But Asserting the actual contract would both fix the failure and make the test meaningful: that the preference set by Two notes while you're in here:
Separately: #7701 currently fails to compile against Not pushing anything to your branch. |
`streaming_send_applies_thinking_pref_set_by_think_command` asserted `status.is_err()` on the reasoning that "the provider-less test kernel fails the agent loop at the driver boundary". That is a property of whichever driver `MockKernelBuilder` seeds, not of the adapter under test, and the harness now resolves the loop successfully — so the assertion failed with `expected provider-boundary failure, got: Ok(())` on both the Unit (lib+bin) and Ubuntu shard 1/4 lanes. Assert the adapter-side contract instead. After the streaming send, the pref must still be readable under `agent_id.0.to_string()` — the exact key `set_thinking` writes and both send paths (`channel_bridge.rs:849` streaming, `:915` non-streaming) look up. A writer/reader key disagreement is what makes `/think` silently inert, which is the regression this PR fixes and which no assertion on the loop's terminal status can observe. The stream still has to start and the status oneshot still has to resolve rather than hang or drop; both remain covered by the surrounding `expect`s.
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.
…ecovered and security work
Editors
- Running-agent editor mounts AgentManifestForm in edit mode: 11 editable fields become ~40, with unknown fields preserved through extras.
- Channels get a UI at last: PUT /api/agents/{id}/channels had shipped without a single client.
- Tools tab reaches parity with Skills: a visible Customize button, per-tool assignment from Available, and MCP grants that actually apply.
- Agent-type editor reuses the same AgentManifestForm plus a Channels picker: 7 fields become 41.
Recovered work
- The layered command resolver from librefang#7159, dropped by the post-sync merge, leaving a comment that promised behaviour the code no longer had.
- workflow_runs.total_steps persistence from the closed librefang#6504, so a run reloaded after a restart no longer reports "step X of 0".
Fixes
- Agent-type saves stop destroying every field the flat JSON does not carry, and channels/routing survive the round-trip.
- The TUI model-routing editor persists instead of silently discarding input.
- /think scopes to the conversation it was typed in rather than every conversation of the agent.
- Ephemeral spawn: cost caps evaluate against the billed agent's own quota, workers run incognito, agent-type fallback cannot borrow another agent's identity, base_url and api_key_env are checked against operator configuration, and system_prompt passes the taint check.
- Skillhub degrades gracefully now that its API is gone, and the endpoint is configurable.
- A broken audit chain is diagnosable and recoverable without discarding history.
- Kernel unit tests resolve an explicit stub driver instead of depending on no driver being installed, which made the suite spawn the real claude CLI and bill for it.
Naming
- Agent templates are agent types: /api/agent-types is canonical, /api/templates stays as a deprecated alias with its original operation ids.
|
Status note, for visibility rather than action: the per-conversation scope for Waiting on this PR:
For the record on the current behaviour: both upstream implementations are inert. |
…rt it The `/think` regression this PR fixes is a key-drift bug: `set_thinking` writes the per-agent preference into `thinking_prefs`, and the send paths read it back, but the map key was derived independently at all three sites. If a read site ever derived a different key the toggle would go silently inert, and the guard test could not see it — its final assertion re-derived `agent_id.0.to_string()` itself, so it pinned a literal the test controlled rather than the lookup the send path performs. Derive the key once in `KernelBridgeAdapter::thinking_pref_key` and route the write through `store_thinking_pref` and both reads through `thinking_override_for`, so write and read cannot drift apart by construction. `streaming_send_applies_thinking_pref_set_by_think_command` now asserts through `thinking_override_for` — the very accessor `send_message_streaming_with_sender_status` calls — and adds a negative case pinning that an agent which never used `/think` still gets `None` rather than inheriting another agent's toggle. No behaviour change: the key expression is identical, only its definition site moved.
|
What was actually wrong. That distinction matters here, because the bug this PR fixes is a key-drift bug. What changed (
No behaviour change: the key expression is byte-identical, only its definition site moved. Verification (
On ordering. Fixing #7159 first and rebasing #7701 on top is the right sequence, since #7701 builds on the |
…-origin # Conflicts: # crates/librefang-channels/src/bridge.rs
houko
left a comment
There was a problem hiding this comment.
The /think preference is described to the user as scoped to “this chat”, but it is stored and read only by agent_id (crates/librefang-api/src/channel_bridge.rs:624-627, 717-739). A single agent commonly serves multiple Telegram chats and can also serve other channel instances, so /think on in one group or DM changes the reasoning mode for every subsequent turn routed to that agent, including turns from other users and channels. This also means /think off from any one chat disables it for all of them. Please scope the preference by the actual channel session/conversation identity (at least channel/account/chat plus agent), pass that scope through set_thinking, and add a regression test with two conversations routed to the same agent proving that toggling one does not affect the other.
|
Superseded — all three halves of this branch are on The scoping fix the review asked for. The regression test with two conversations routed to the same agent, also asked for by name:
Closing rather than rebasing: there is nothing here upstream does not already do, and the version upstream landed is the scoped one the review called for rather than the agent-keyed one this branch shipped. |
Fixes the two root causes behind Telegram slash commands confirming without acting (issue #7140).
1.
/thinkwas a stubset_thinkingstored nothing and never affected the model. Now:KernelBridgeAdapter(keyed by agent id)send_message_with_blocks_and_sender_thinking(KernelApi method + kernel impl; default ignores the override)2. Command routing divergence
/new/reset/reboot/compactresolved the target agent via the router chain only, while the regular chat path (resolve_or_fallback) also consults:/agentoverride (RFC: rethink channel inbound routing — HITL vs AITL topology, channel-instance binding, per-conversation /agent override #5671)[[bindings]]match[[sidecar_channels]] agentA divergent resolution reset a different agent than the one holding the conversation — a real reset, invisible to the user.
resolve_for_commandnow mirrors the chat chain layer for layer, in the same precedence order.Not in this PR
/goalhonesty fix (fire-and-forget start) — depends on the extended goal-runner signature from feat(goals): autonomous goal runner with loop engineering, /goal CLI, TUI, and channels #6505, tracked separately.Verification
cargo check --workspace --libcleancargo test -p librefang-channels --lib: 551 passed, 0 failedCloses #7140
Follow-up commits (same issue #7140)
18ef5fd:
/agentnow also persists the per-conversation binding (the ack claimed the new agent while dispatch kept the old one —set_conversation_bindinghad zero production callers), and the streaming send path now applies per-agent/thinkpreferences (previously only the non-streaming fallback honored them). Regression tests assert the binding side effect and the streaming-path override.