feat(server): resolve agent-switch slash commands in RunSession#3782
Conversation
389357f to
d210ce1
Compare
HTTP callers driving the runtime had no way to trigger a mid-session agent switch: only the interactive CLI/TUI runners resolved slash commands via PrepareUserMessage before appending them to the session. Mirror that behaviour in the HTTP session manager. Scoped to agent-switch commands (cmd.Agent != "") so instruction-expansion commands still pass through as literal user text and no existing HTTP caller is silently reinterpreted.
d210ce1 to
29b1599
Compare
Two bugs in applyAgentSwitchCommands surfaced during review: - ResolveCommand ran after SetCurrentAgent, so it looked up the routing command in the target sub-agent's command table. Routing commands typically live only on the source agent, so LookupCommand returned !ok and ResolveCommand returned the raw "/plan args" verbatim, leaking the routing prefix into session history. - In a multi-message batch, a successful switch for msg[i] followed by a failed switch for msg[i+1] left the runtime on the intermediate agent even though the turn was aborted, so future turns started on the wrong agent. Resolve the command before switching (matches pkg/cli/runner.PrepareUser Message ordering), and best-effort restore the pre-batch agent on any SetCurrentAgent error. Tests updated so commandFakeRuntime supports per-agent command tables and per-target-agent SetCurrentAgent failures — the shared map masked the first bug.
Two more issues surfaced during review: - LookupCommand ran against the live runtime, so in a multi-message batch the second command's lookup consulted whichever agent an earlier switch had already moved the runtime to. A batch like [/plan …, /build …] where /build is only declared on root would silently drop the second prefix once /plan moved the runtime to planner. - The rollback SetCurrentAgent call on the error path silently discarded its own error, so a runtime that fails to restore stays on the intermediate agent with no diagnostic. Refactor to two-pass: resolve every user message against the pre-batch agent first, then apply the collected switches. Each message's interpretation is now independent of what earlier messages did to the runtime. On rollback failure, emit a WARN log naming the intermediate agent so operators can spot the stuck-session case.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The implementation is clean and correct. applyAgentSwitchCommands correctly mirrors what the CLI's PrepareUserMessage does:
- Two-pass resolution is sound: all
LookupCommand/ResolveCommandcalls happen against the pre-batch agent before anySetCurrentAgentcall, so every message in a multi-message batch is interpreted against the session's starting agent — not against whatever an earlier message switched to. - Rollback on error is correct:
originalAgentis captured before any mutations; ifSetCurrentAgentfails mid-batch, the runtime is restored, and the WARN log on rollback failure provides observability without crashing. - Message content is mutated only after a successful switch: the
messages[s.idx].Content = s.contentline executes afterSetCurrentAgentsucceeds (or is skipped as a no-op), so history is never partially rewritten on failure. - Scope is intentionally narrow: only commands with
Agentset are resolved; instruction-expansion commands pass through unchanged, preserving backward compatibility for existing HTTP callers. - Test suite is thorough: all critical paths are covered — happy path, idempotent re-assertion, instruction-only commands, unknown commands, non-user roles, error propagation, two-pass lookup independence, and mid-batch rollback.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The two-pass design is sound: all LookupCommand calls happen before any SetCurrentAgent call, correctly scoping each lookup to the pre-batch agent. Rollback on failure and the idempotent skip of same-agent switches are both handled correctly. One low-severity logging accuracy issue was found (inline).
| if err := rt.SetCurrentAgent(ctx, s.target); err != nil { | ||
| if rbErr := rt.SetCurrentAgent(ctx, originalAgent); rbErr != nil { | ||
| slog.WarnContext(ctx, "failed to restore agent after switch error; session may be on wrong agent", | ||
| "original_agent", originalAgent, "stuck_on", s.target, "err", rbErr) |
There was a problem hiding this comment.
[low] stuck_on log field names the attempted target, not the agent the runtime is actually on
When both SetCurrentAgent(s.target) and the rollback SetCurrentAgent(originalAgent) fail, the warning log records "stuck_on", s.target. But s.target is the agent the code tried to switch to — and that call failed, so the runtime almost certainly did not move to s.target. The session is more likely stuck on whatever agent it held before the attempt (could be a prior iteration's result or the original), not the one named in the log. An operator reading the warning to diagnose a stuck session would be misled into looking at the wrong agent.
A one-line fix is to query the runtime's actual current state:
| "original_agent", originalAgent, "stuck_on", s.target, "err", rbErr) | |
| "original_agent", originalAgent, "stuck_on", rt.CurrentAgentName(ctx), "err", rbErr) |
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The new applyAgentSwitchCommands function is well-structured with a clear two-pass design that correctly separates lookup from apply. The idempotence check and rollback logic are present and tested. Two minor low-severity observations are noted inline on newly introduced code.
| if err := rt.SetCurrentAgent(ctx, s.target); err != nil { | ||
| if rbErr := rt.SetCurrentAgent(ctx, originalAgent); rbErr != nil { | ||
| slog.WarnContext(ctx, "failed to restore agent after switch error; session may be on wrong agent", | ||
| "original_agent", originalAgent, "stuck_on", s.target, "err", rbErr) |
There was a problem hiding this comment.
[low] stuck_on log field names the wrong agent when rollback also fails
When SetCurrentAgent(ctx, s.target) fails on line 1567, the runtime was never placed on s.target — the call errored out. If a prior iteration in the same batch had already successfully switched to an intermediate agent T_prev, the runtime is currently on T_prev at the point the rollback attempt is made. If the rollback (SetCurrentAgent(ctx, originalAgent)) also fails, the session is stuck on T_prev — but the log records stuck_on: s.target, which is the agent the failed switch was targeting, not the agent the runtime is actually on.
An operator diagnosing a stuck session from this log would search for the wrong agent. The correct value to surface is the runtime's actual current agent at the time of rollback failure, i.e. the result of a rt.CurrentAgentName(ctx) call at that point.
This only manifests when a batch contains multiple agent-switch commands and both the Nth switch and the rollback fail — an uncommon but valid scenario with multi-message batches.
| "original_agent", originalAgent, "stuck_on", s.target, "err", rbErr) | |
| slog.WarnContext(ctx, "failed to restore agent after switch error; session may be on wrong agent", | |
| "original_agent", originalAgent, "stuck_on", rt.CurrentAgentName(ctx), "err", rbErr) |
| Confidence | Score |
|---|---|
| 🟢 strong | 97/100 |
| return fmt.Errorf("switch agent to %q: %w", s.target, err) | ||
| } | ||
| } | ||
| messages[s.idx].Content = s.content |
There was a problem hiding this comment.
[low] Partial message content mutation if a multi-message batch switch fails mid-way
The second-pass loop rewrites messages[s.idx].Content = s.content immediately after each successful SetCurrentAgent, before the loop finishes. If the loop contains multiple switch entries (a batch of messages each starting with a different slash command) and a later entry's SetCurrentAgent fails, the earlier messages will already have had their slash-command prefix stripped while the later ones retain it. The messages slice is passed by reference, so the caller sees a partially-rewritten slice.
In the current RunSession call-site, this is harmless — the caller discards the slice on error (returns nil, err) and never appends anything to the session. But if retry logic is ever added that re-uses the same messages slice, it will encounter inconsistently-modified content (some prefixes stripped, others intact) with no indication that a partial mutation occurred.
A defensive fix is to stage the content rewrites and apply them only after all SetCurrentAgent calls in the loop have succeeded:
| messages[s.idx].Content = s.content | |
| for _, s := range switches { | |
| if s.target != rt.CurrentAgentName(ctx) { | |
| if err := rt.SetCurrentAgent(ctx, s.target); err != nil { | |
| if rbErr := rt.SetCurrentAgent(ctx, originalAgent); rbErr != nil { | |
| slog.WarnContext(ctx, "failed to restore agent after switch error; session may be on wrong agent", | |
| "original_agent", originalAgent, "stuck_on", s.target, "err", rbErr) | |
| } | |
| return fmt.Errorf("switch agent to %q: %w", s.target, err) | |
| } | |
| } | |
| } | |
| for _, s := range switches { | |
| messages[s.idx].Content = s.content | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 70/100 |
Host applications driving the runtime over HTTP had no way to trigger a mid-session agent switch: only the interactive CLI/TUI runners resolved slash commands (via
PrepareUserMessage) before appending them to the session. Anyone else — REST clients, Electron hosts, custom orchestrators — could only invoke the target agent via thePOST /api/sessions/:id/agent/:agent/:agent_nametwo-segment route, whose semantics only actually take effect on the first turn of a session (the cached runtime silently ignores it on turn 2+).This PR closes that gap on the HTTP side.
SessionManager.RunSessionnow runs each incoming user message throughLookupCommand/ResolveCommandbefore appending, mirroring what the CLI already does:Scope is intentionally narrow: only commands whose
Agentfield is set are resolved here. Instruction-expansion commands (those declaring anInstruction) pass through unchanged, so an existing caller that happens to send text starting with/is not silently reinterpreted server-side. The blast radius of the change is limited to configs that already declaredcommands.<x>.agent: <name>, which today are the tightly-controlled agent-switching commands.Idempotence is a design choice: when the target agent is already current,
SetCurrentAgentis skipped. That lets host applications re-assert the desired agent on every turn (e.g. by always prefixing the outgoing message with/planor/builddepending on a UI toggle) without paying for a no-op switch or tracking per-session applied state client-side.