feat: steer-while-running — inject a user message into an in-flight run (#512) - #570
Conversation
b0f80fe to
c2fcbab
Compare
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review (Spec / Standards / Domain / prior art): requesting changes for reproducible concurrency, lifecycle, protocol-correlation, and TUI correctness issues. I compared the design against current Codex, Goose, Cline, OpenCode, Claude Code changelog evidence, and Aider. The highest-risk cluster is the run/transport ownership around terminal steering; CI's race-test job also timed out with an agent.test process still alive, consistent with the inbox liveness bug called out inline.
| return SteerAccepted, nil | ||
| default: | ||
| // Full slot: supersede — replace the pending steer and report the win. | ||
| <-r.steer.slot |
There was a problem hiding this comment.
Blocking — this supersede transition is not atomic and can deadlock. After the non-blocking send observes a full slot, drainSteer or cancelSteer can consume it before this receive; <-r.steer.slot then waits forever. A concurrent enqueue can also refill between the receive and the following send, blocking that send. The separate done check has the same TOCTOU shape: closeSteer can win after the check and this call can still report accepted for a value no loop will drain. A cap-1 channel does not make this compound replace/close operation linearizable. Please use one mutex around {closed, pending} (or an equivalent single-owner state machine) and add deterministic liveness tests that force drain/close between observation and mutation; -race alone cannot detect a channel-protocol deadlock.
| // permanent is always false here (the ChunkDone StopError path has NO Go error | ||
| // to classify — honest fail-open). | ||
| func (e *Engine) terminateComplete(ctx context.Context, r *Run, sess *session.Session, reason session.StopReason, text string, usage session.Usage, errMsg string) { | ||
| r.closeSteer() |
There was a problem hiding this comment.
Blocking — a steer accepted during a final text response is silently lost on the normal clean-terminal path. finishTurnNoTools goes straight to terminateComplete for meaningful text without another Step 2a pass. If EnqueueSteer accepted while that response was streaming, this closes the inbox with the value still parked; the caller already received accepted, and Service never gets too_late to promote it. That contradicts the next-boundary/never-drop contract. Codex and Goose both re-check pending input before clean exit. Please make the clean-terminal decision atomically observe/consume pending steer and continue the loop, or have terminal close return the pending value into an explicit promotion path.
| // control reader keeps processing frames; the run's terminal EvResult is | ||
| // relayed onto this stream through the shared runRelay, and its outcome is | ||
| // reported back as the steer ack (promoted=true: never a drop). | ||
| go h.drivePromotedSteer(ctx, id, text, promotedRun, rl) |
There was a problem hiding this comment.
Blocking — this goroutine has no owner keeping Converse alive. The primary relayRun returns as soon as the original run's events close; Converse then returns and cancels the same stream context used to start/relay this promoted run. The promoted terminal event and ack (queued only after its relay completes) can therefore race a closed RPC. TestSteer_NoConcurrentSendOnTerminalRace explicitly permits the ack/result to arrive after stream close, so it does not prove AC3.2/AC5.1. runRelay.sendErr is also accessed by both relays without synchronization; serializing only Send does not fix that race. Please make the RPC own a sequential active-run relay/handoff and wait for the promoted run before returning, rather than launching a second unjoined relay.
| // interleaves them onto the stream); a too_late steer the Service promotes | ||
| // drives the fresh follow-up run and reports its terminal outcome back as the | ||
| // steer ack. | ||
| func (h *HarnessServer) readControl(ctx context.Context, id session.SessionID, run *agent.Run, rl *runRelay) { |
There was a problem hiding this comment.
Blocking — all subsequent controls remain bound to the original run. Once a steer is promoted, ResumeApproval, Cancel, and CancelChild still target this captured, terminal handle. A promoted follow-up that asks permission cannot be approved on this stream, and Esc/cancel does not stop its active tool work. The active-run handoff should update the single control target atomically with relay ownership; do not let reader and relay disagree about which run owns the stream.
| // outcome is the closed-enum result (mirrors agent.SteerOutcome). | ||
| SteerOutcome outcome = 1; | ||
| // text echoes the steer text the outcome is about (empty for steer_cancel). | ||
| string text = 2; |
There was a problem hiding this comment.
High — text is not a safe correlation key. Acks and drain echoes can interleave with newer submissions (the relay explicitly allows random ready-case ordering), so the TUI can apply ack(A) after sending A+B, overwrite the newer state, or clear it on echo(A). The same frame can also be routed to a newer run occupying the session registry after a terminal handoff. Current prior art avoids this: Codex carries expectedTurnId + clientUserMessageId; Goose carries expectedRunId + a generated messageId; Cline uses stable pending-prompt IDs. Please add an expected run/generation and client operation/message ID to steer/cancel, echo them in Ack/SteerEcho, and have clients ignore stale responses.
| combined := text | ||
| if m.steer != nil && (m.steer.Phase == steerPending || m.steer.Phase == steerSent) { | ||
| // Merge onto the combined in-flight text (the version the engine last | ||
| // confirmed, or the un-acked pending text); the supersede replaces it. |
There was a problem hiding this comment.
High — edit-back duplicates the instruction instead of replacing it. editBackQueue copies the complete current steer into the textarea but deliberately leaves m.steer.Text unchanged. On resend, text already equals the edited full steer, and this produces old + "\n\n" + edited-old (for example check A becomes check A\n\ncheck A carefully). That contradicts the UI's “resend to replace” contract and sends obsolete guidance to the model. Track that the draft is editing the existing steer and send it as the complete replacement; update the test to assert exact frame equality—the current Contains assertions at steer_test.go:228 mask this bug.
| m.steer.Text = msg.Text | ||
| m.steer.Phase = steerSent | ||
| m.statusMsg = m.deps.Theme.Style("muted").Render("steer queued") | ||
| case client.SteerTooLate: |
There was a problem hiding this comment.
High — this reports successful delivery even when promotion failed. The server emits too_late with promoted=false on routing/run-entry errors, but this branch ignores msg.Promoted and always renders “sent as a follow-up.” That can tell the user an instruction ran when it was dropped due to a lease/liveness/funnel error. Branch on Promoted; for false, preserve the text and show an explicit not-sent/retry state (ideally with a bounded error outcome), rather than advancing to steerPromoted.
| // SteerOutcome (accepted/superseded/too_late) — never an error for the ordinary | ||
| // too-late race (that outcome is what the Service promotes on). | ||
| func (r *Run) EnqueueSteer(text string) (SteerOutcome, error) { | ||
| return r.enqueueSteer(text) |
There was a problem hiding this comment.
Standards violation — normalize producer text at the semantic ingress, not only in the protobuf mapper. This exported engine API accepts arbitrary Go strings and later records/emits the raw value; toProtoSteer(valid(...)) repairs only the client projection. An external engine consumer can therefore persist/replay invalid UTF-8 and send it to a provider while the wire echo differs, violating the repo's recorded == streamed == model-view invariant and its explicit two-layer UTF-8 rule. Apply session.ToValidUTF8 once before the value enters the inbox so history, model view, event, and mapper backstop all agree.
| - **ADR-0114** landing with this plan — the steer contract: engine inbox (single supersedable slot), the drain echo (`EvSteer`), the auto-promote-on-terminal-race rule, the queue-only awaiting behaviour, and the gRPC-only-v1 / HTTP-deferred scope cut. Copy `docs/adr/template.md`. | ||
| - **`docs/usage/http-sse-api.md`** — a note that steer is gRPC-only in v1 (the HTTP run path has no mid-run client→server channel). | ||
| - **`docs/tui.md`** — the steer-mode queue card (pending / sent / promoted) and the capability-driven flip vs the local queue. | ||
| - **`docs/architecture.md`** — the steer inbox + Step 2a seam under the loop section (living "how it works"). |
There was a problem hiding this comment.
The documented cross-cutting deliverable is missing from this PR. docs/architecture.md is not changed even though the loop injection and Converse lifecycle changed; AGENTS.md requires the living architecture doc to move with behavior. Also, the new run-scoped inbox/channels and promoted-run goroutine outlive one call, so AGENTS.md requires entries in ADR 0027 List 1, and the intentionally restart-lost pending steer needs the List 2 fidelity decision. Please update both canonical inventories rather than leaving the lifecycle only in ADR 0114/implementation notes.
|
Thanks for the thorough review — every finding was legitimate, and several were design-level. All 9 are addressed; the rework was then re-reviewed (targeted security+architecture, then a full 3-axis panel) and one panel-caught defect (the TUI #1 — supersede select non-atomic → deadlock. The cap-1-channel inbox was the wrong primitive (a channel can't linearize a compound replace-or-report-closed). Reworked to #2 — clean terminal drops an accepted steer. Confirmed: #3 — promoted run orphaned when Converse returns + unsync'd #4 — controls bound to the original run after promote. Same handoff: #5 — no correlation key. Added #6 — TUI edit-back duplicates steer text. Fixed at root: edit is now cancel-then-recompose — the client holds a queue, collapses to ONE bundle per send (one #7/#8 — UTF-8 only repaired in the mapper. #9 — missing docs. Added the loop section to Panel-caught extra (beyond your 9): the full panel found that the TUI ignored One architectural question your review surfaced but which is bigger than this PR: single-driver-per-session is not enforced across clients (steer/Cancel/Approve are stream-attached, not driver-scoped; run-start is serialized but live signaling isn't). Filed #632 to track enforcing single-driver + explicit frontend handoff (and the observer-can't-see-pending-steer gap). Steer is the forcing case; the enforcement is a follow-up. Gates green across the board: lint 0 · test 155 pkgs/0 FAILs (-race) · generate+api committed · docs matlatl strict clean · site build · ac-trace 27/0 · mecademo. Ready for re-review. |
dcf4c1d to
1a37017
Compare
b1a9bf7 to
0d6f4a0
Compare
Covered in further commits/edits. All comments addressed.
f472f63 to
73dea07
Compare
…-running) Closes #512. Mid-run steer: the operator injects a message into a *live* run rather than waiting for turn-end and submitting a fresh prompt. gRPC-only v1 on the bidi Converse stream (`steer`/`steer_cancel` frames); the TUI is maximally dumb (it sends a steer and renders the authoritative EvSteer echo) — ALL race resolution lives in server+engine plumbing. Engine (engine/agent/steer.go): a Run-scoped, single-slot, append-default mutex inbox. A second steer on the occupied slot APPENDS into the pending bundle (merged with a blank-line separator, drained as ONE user message); supersede/slot_full were dropped through the review rounds. Replacing a pending bundle is the explicit steer_cancel-then-resend. UTF-8-repaired at ingress. In-memory, best-effort: a pending steer is lost with the run (reset-by-design, ADR 0027 rows 57/34). A parked steer blocks a clean terminal (clean-exit continue-run) and both terminate paths drain-then-close, so it is never closed unconsumed. Service (internal/adapter/server/service.go): authorize-first, then live-enqueue or terminal-race promote via StartRunContent (the hardened run-entry funnel, with the promote-grace awaiting the just-terminal run's deregistration). The id lives at this wire-correlation layer only: a per-session FIFO of ordered frame ids, stamped on the drain echo as the watermark (the LATEST contributing send's id — positional, never text-match; pinned by TestLookupSteerMessageIDExactUnderDuplicateTexts). Ids are 64-rune-clamped at track. gRPC wire (contracts/proto + internal/adapter/server/grpc.go): steer / steer_cancel oneof arms, the SteerOutcome closed enum (accepted / appended / retracted / none_pending / too_late), the single-writer streamSender gate for every Send, the sequential active-run handoff (Converse relays a promoted run on the SAME stream; the control target swaps atomically before its relay starts), and the drain-echo watermark stamp. ServerCapabilities.steer = 20 (additive capability bit, computed once in composition). TUI (cmd/mecatui/ui): when the capability is advertised, each enter sends ONLY the new fragment with a fresh client-minted id (engine appends server-side); the ordered queue of sends (id + text) is the single correlation source (no burn maps; the watermark derives from the tail send). The drain echo splits the queue at the watermark — prefix drained (rendered in context at its true stream position), suffix pending. The ack advances only the lifecycle phase (never the queue). `↑` on empty input is cancel-then-recompose (steer_cancel on the watermark id; resend as a fresh fragment under a NEW id). Re-composed fragments render per-part on the card. Capability absent → the #228 local merge-queue, byte-identical. Docs: ADR-0228 (the landed contract), updated acceptance plan + agent-loop.md/IMPLEMENTATION-NOTES.md/tui.md/http-sse-api.md, user-docs/mecatui.md, engine/CHANGELOG.md consolidated, AGENTS.md list not widened (no new loop diagnostics lines). The 14-task orchestration spine is under .claude/plans/steer-while-running/ (left in for the PR's own review audit; superseded by the ADR on merge). Deferred (recorded in ADR-0228): HTTP/SSE steer endpoint, ACP steer, steer-to-child. Co-Authored-By: mecatl <noreply@stacklok.com>
73dea07 to
1c322f5
Compare
The steer-while-running capability (#570) was gRPC-only; the HTTP/SSE tier — Studio's only transport — could not steer. This lands the follow-up ADR 0232 named: - POST /v1/sessions/{id}/steer {text, message_id} -> 200 {outcome: accepted|appended|too_late}. The live-run fast path is extracted from Service.Steer into Service.SteerEnqueue (single-owner); HTTP never promotes — on too_late the caller keeps the text and drives its own follow-up prompt, so never-drop holds at the client. - POST /v1/sessions/{id}/steer-cancel -> {outcome: retracted|none_pending} via the existing Service.CancelSteer. - The EvSteer drain-echo message_id stamp is extracted into one shared Service.stampSteerEcho used by BOTH the gRPC relay and relayRunSSE, so HTTP clients get watermark correlation too. - The HTTP capabilities echo gains the steer bit. Offline httptest coverage for every outcome, the echo correlation, and the capability echo; Service.Steer's promote behavior pinned unchanged by the existing steer suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Steer-while-running (#512)
Inject a user message into an in-flight run — Claude Code's "steer while running" — instead of waiting for turn-end and submitting a fresh prompt. See ADR-0114 + the acceptance plan (27 ACs,
ac-trace27/0).Design principle: the TUI stays maximally dumb — it sends a steer and renders the authoritative echo; all race resolution lives in server + engine plumbing.
Round 2 — addressed Ozz's review (9 findings, all fixed)
JAORMX's panel review requested changes across concurrency, lifecycle, protocol-correlation, and TUI correctness. Each finding was triaged, discussed, and fixed; the rework was independently re-reviewed (security + architecture) — both re-reviews confirm the fixes hold.
selectnon-atomic → deadlock/TOCTOUsync.Mutex+{closed,pending,has}; SUPERSEDE DROPPED — a second enqueue on a full slot returnsslot_full(reject); replace = explicit cancel-then-resend. The non-atomic replace path is deleted, not guarded.engine/agent/steer.gofinishTurnNoToolsdefersterminateCompletewhile a steer is parked; the run extends until it drains at the next Step 2a.engine/agent/loop.go:1396runRelay.sendErrunsync'dConverserelays the promoted run on the SAME stream (onestreamSender) before returning;sendErrsingle-owner; mailbox seal Converse-owned.internal/adapter/server/grpc.goreadControl's control target swaps atomically to the promoted run; one "active run" at a time.message_idon Steer/SteerCancel/SteerAck/SteerEcho (client-minted, server-echoed, client ignores stale). Engine stays text-only; the id lives in a Service FIFO (steerMsgIDs). Noexpected_run_id(wire has no run-id concept).message_id);↑(optimistic, not synchronous) cancels the outstanding bundle and pulls the whole not-yet-drained set into one editable blob; resend = new bundle, fresh id. Burned ids (echo seen) open a fresh draft.cmd/mecatui/uisession.ToValidUTF8at ingress inenqueueSteer— history == echo == model-view by construction; the mapper backstop stays last-resort.engine/agent/steer.godocs/architecture/agent-loop.md), ADR-0027 List 1 row + List 2 fidelity decision (pending steer restart-lost by design), ADR-0114 updated in place.SteerOutcomeenum:SUPERSEDED→SLOT_FULL(harness.proto).message_idadded toSteer,SteerCancel,SteerAck,SteerEcho.How it works (final shape)
A run-scoped mutex inbox (single slot,
slot_full) sits on theRun. A mid-runsteerframe →Service.Steer(authorize → live-enqueue or terminal-promote-via-funnel). The loop drains it at Step 2a (post-tool-results, pre-next-turn) viarecordContinuation, and emits theEvSteerecho (stream-positioned,message_id-tagged) so the client knows where it landed and that it's the one it sent. The TUI shows the steer queued until it lands, then in context — mirrors the #228 queue UX.Deferred (explicit scope cuts)
HTTP/SSE steer, ACP steer, steer-to-child, the interrupt verb, intent-vs-action timing.
Verification
task lint0 ·task test155 pkgs / 0 FAILs (-race, both modules + standalone hygiene) ·task generate+task api:updatecommitted ·task docsmatlatl strict clean ·task site:buildgreen ·task ac-trace-strictsteer plan 27 ACs / 0 failures ·go run ./cmd/mecademofull session. Round-2 rework re-reviewed post-fix by independent security + architecture reviewers.Closes #512