fix(acp): retry channel turns that leave a mention unanswered - #3648
Open
wpfleger96 wants to merge 4 commits into
Open
fix(acp): retry channel turns that leave a mention unanswered#3648wpfleger96 wants to merge 4 commits into
wpfleger96 wants to merge 4 commits into
Conversation
Both launch paths default to `buzz_acp=info` — the binary's own `EnvFilter` fallback and the `RUST_LOG` Desktop hands its managed child. `EnvFilter` matches a directive against a target by path segment, so bare targets like `pool::prompt` and `acp::stream` match nothing and are dropped: turn start, turn stop, and every wire line are invisible in a default deployment, and a turn that goes wrong leaves no trace at all. Prefixing the targets rather than widening the filter keeps the fix in one crate — the alternative requires the two filter strings to be kept in step across the binary and Desktop forever. A test walks the crate source and fails on any target the shipped filter would swallow. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A turn that streams no assistant text and completes no tool call still reports `end_turn`, which the harness cannot distinguish from a real answer. The mention is dequeued as handled and the user is left waiting on a reply that will never come, with nothing to retry. `AcpClient` now records whether a turn produced anything, cleared at the top of every `session/prompt`. A channel turn that ends `end_turn` with the flag still clear is reported as a failure, so the batch flows through the existing backoff and dead-letter path instead of vanishing. Only successful work counts. buzz-agent reports a rejected tool call as `completed` with `rawOutput.isError` set, so counting bare `completed` would mask the exact turn this detects. Whitespace-only chunks are formatting and thought chunks are reasoning the user never sees, so neither qualifies. Heartbeats are exempt: they are self-prompts with no waiting user and nothing to requeue. The failure is application-class (`AgentError`) rather than a protocol error — the stdio pipe is intact and the process is healthy, so the agent returns to the pool instead of being respawned against the crash circuit. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The first cut of dead-turn detection asked "did this turn produce any output at all", which is both too broad and too narrow. Too broad: `PromptSource::Channel` covers every matched batch, not just mentions. Under `--subscribe all` or a `require_mention: false` rule an agent watches traffic nobody addressed to it, and base_prompt.md tells it to stay silent when it has nothing to add. Retrying that turned correct restraint into ten retries and a user-visible failure notice. `MatchedRule` now carries `requires_response` — the rule required a mention, or the event p-tags the agent — and it rides through `QueuedEvent`, `BatchEvent` and `FlushBatch` so classification can ask whether anyone was actually owed an answer. Too narrow: an event injected mid-turn by a native steer is not in the batch, and the ack used to drop it outright. If the turn then went silent, that mention was gone with no retry — the same disappearance this change exists to close, one layer down. A `Success` ack now moves the event into a delivered-steer ledger keyed by the turn's output epoch, and the turn's terminal classification settles it: output after the delivery retires it, silence releases it to the queue front for an ordinary, fully-budgeted retry. The per-turn boolean became that monotonic epoch counter because a flag cannot tell "spoke, then went silent on the new message" from "answered the steer". Expiry and panic recovery settle the ledger too, so a delivered event can never outlive its turn. Two smaller corrections. The ACP schema lets the initial `tool_call` carry `status: "completed"`, so a connector that sends no follow-up `tool_call_update` was being read as dead after doing real work; one shared predicate now covers both events. And the control-signal race branch, which synthesizes its own `EndTurn`, routes through the same classifier rather than relying on a polling invariant that lives in another function — the invariant holds today, but one inserted `.await` would silently turn that branch into a bypass. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Two ways an event could still be lost after the dead-turn retry landed. A steer ack and its turn's result travel on different channels, and the main loop's biased select polls results first, so a genuinely-sent Success can be processed after the turn it belongs to has terminated. Settling only the delivered ledger left that event withheld with no turn left to judge it, and the late ack then moved it into a ledger nothing would ever resolve — silent loss with an extra step. Terminal classification now settles both tables: delivered entries against the turn's final output epoch, still-withheld entries released as delivery-unknown. The ack path reports whether the event was still pending, so a late ack of any shape is a total no-op — no second copy, no deadline extension for an unrelated turn, and no cancel+merge fallback aimed at a turn that already ended. The cost is at most one visible duplicate when the watcher lagged a real Success, the same duplicate-over-silent-loss trade startedNewTurn already accepts. The turn-ID-bound alternative was not taken: it threads a turn id through the ack path to buy strictness this policy gets from ordering alone. Retry serialization dropped cancelled_events. A merged re-prompt carries the events an interrupted turn was already working on in that bucket, and both requeue paths iterated only batch.events — so a mention that was interrupted and then died in the merged retry was gone with no error anywhere. Both now share one reinsertion helper that restores the whole batch. Cancelled events are normalised into ordinary queued events rather than returned to the cancelled side table: that table is exempt from retry_after in the cancelled-only flush fallback, so restoring there would let a throttled batch re-flush immediately and spin, and the merge annotation frames in-progress interrupted work, of which a failed turn has none. The ack arm's match moved out of the select! into apply_steer_ack so the "no spurious fallback signal" half of the contract is directly testable; the logic is unchanged. Existing steer-ledger tests changed by function rename only — resolve_delivered_steers became settle_turn_steers, since it no longer settles just the ledger and no caller should be able to settle half the state. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
tlongwell-block
added a commit
that referenced
this pull request
Jul 31, 2026
…3763) ## Why A Buzz agent's assistant text and reasoning are never shown to anyone — only what it posts through the CLI. A turn that runs fifteen tool calls and never publishes is a silent failure: the requester waits on a result that was produced and thrown away. This adds an optional reminder at the end-of-turn gate, off by default. Tyler asked for it in buzz-mesh; plan iterated to **9.5/10 with @wren** (Minimalness 9.7, Elegance 9.5, Correctness 9.3). ## What `BUZZ_AGENT_REQUIRE_REPLY=1` (default off, per-agent opt-in). A turn about to end with no recognized attempt to post gets a reminder and is rerolled. **At most two, then the turn ends regardless** — the guard catches accidental omission, it does not compel speech. The reminder text explicitly licenses silence so it cannot fight the base prompt's "silence is usually correct." **This is not a new MCP hook.** `RunCtx::run` *is* the turn, so the two per-turn locals need no plumbing, and every tool call already passes through it with arguments visible. The objection is appended at the existing `_Stop` gate and rides `push_hook_outputs_as_tool_results`, so the model receives it as a lower-trust tool result with `{hook, server, text}` attribution. No new trust path, no new lifecycle event, no dev-mcp or CLI protocol change. Earlier revisions of this plan needed four crates (a `_UserPromptSubmit` hook, a marker file, a `buzz-cli` change, dev-mcp state). Tyler pointed out the agent already knows both facts; that deleted all of it. Net runtime change is ~35 lines in `agent.rs` + ~4 in `config.rs`. ### Recognition contract A registered non-hook tool whose qualified name ends in `__shell`, whose `command` argument contains `messages send` or `reactions add`. - **The `__` separator is exact, not approximate.** Given `has()` + `!is_hook()`, `ends_with("__shell")` is *provably equivalent* to a bare name of `shell`: registration forbids `__` in server and bare names (`mcp.rs:227,268`) and qnames are `{server}__{bare}`, so a trailing `__shell` could only straddle the separator if the bare name began with `_` — which `is_hook` excludes. Without the separator, `powershell` and `noshell` would match. - **Reads the structured `command` field**, not serialized arguments, so a `description` that quotes a send cannot disarm the guard, and a non-string `command` is rejected rather than coerced. - **Detects an attempt, not a successful publish.** A failed send already returns non-zero exit and error JSON — louder than this reminder. The variable is named `buzz_reply_call_seen` so the code can't pretend otherwise. - **Checked after the per-turn tool-call cap**, since a discarded call never ran. - `messages send` also covers `messages send-diff`. Reactions count because the base prompt directs agents to react rather than post a bare acknowledgement. **Known limits, both deliberate and documented:** a command assembled at runtime (`$CMD`) or hidden in a wrapper script is missed; text that merely quotes a send (`echo "buzz messages send"`) matches. Missing a real post is the expensive direction and substring matching is the forgiving one there. Neither edge is pinned by a test, so the matcher stays free to improve. ### Budget Reminders share `BUZZ_AGENT_STOP_MAX_REJECTIONS`, the existing outer cap on every end-turn objection. Default 3 fits both; at 1 only one fits; at 0 the guard is off with the hooks. A round carrying both a hook objection and a reminder costs one rejection and delivers both texts. An independent budget would either violate that bound or need a second arbitration rule. ## Prior art - **#3467** (closed) built the same detector one layer up in `buzz-acp` for a different remedy. None of its symbols are on main — this borrows its permission to be coarse, but reads structured data that ACP didn't have. - **#3648** (open) detects turns with *no output at all*; a turn with fifteen tool calls and no post counts as output there, so it does not cover this case. - **#3741** (merged) is mesh-only. ## Testing **14 new tests.** 4 unit tests on the matcher; 10 integration tests through the ACP wire harness: off by default, `=0` still off, opted-in silent → exactly 2 reminders then `end_turn`, registered `fake__shell` send → 0 reminders, hallucinated `fake__shell` → still reminded, publish call truncated past the 64-call cap → still reminded, budget 1 → 1 reminder, budget 0 → off, combined `_Stop` hook objection + reminder → one round both texts and after 2 reminders the hook objection continues alone, unparseable `=true` → startup error naming the key. **10 mutation checks, each breaking a specific named test** — neutralize the nag cap, stop sharing the budget, neutralize `buzz_reply_call_seen`, drop `has`/`is_hook`, ignore the flag, drop the `__`, drop `reactions add`, read serialized args, move detection before truncation. `tests/bin/fake_mcp.rs` gains `FAKE_MCP_SHELL_TOOL=1`: it previously exposed no tool with a bare name of `shell`, so the satisfied-guard path was untestable. Full `cargo test -p buzz-agent` green at 9e0ae1f; clippy `-D warnings` and `cargo fmt --check` clean. **Unrelated flake found:** `cancelled_turn_with_usage_emits_notification_before_response` (`tests/fake_llm.rs`) is timing-sensitive. Under 10 loaded cores it fails **2/20 on this branch and 1/20 at unmodified `origin/main@02be413`** — pre-existing, not caused by this change (which is inert without the env var). Flagging so it isn't misattributed to the next PR that's open when CI hits it. ## Docs `crates/buzz-agent/README.md` is the primary home (env var, recognition contract, limits, budget interaction). `docs/MCP_DRIVEN_HOOKS.md` gets a short cross-reference explaining this is *not* a hook — otherwise readers hunt for a `_ReplyGuard` tool that doesn't exist. --------- Signed-off-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com> Signed-off-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@buzz.block.builderlab.xyz> Co-authored-by: Dawn (sprout agent) <c6237ef84fa537c78dcee78efd2d4e59f728859c7f194da42ac51ededfa0be05@sprout-oss.stage.blox.sqprod.co> Co-authored-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@buzz.block.builderlab.xyz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A mention can produce a turn that ends
end_turnhaving streamed no assistant text and completed no tool call. Nothing is posted to the channel, but the harness sees a normal successful stop — so the mention is dequeued as handled, the retry machinery never engages, and the user waits on a reply that will never arrive. Two observed turns failed this way; one of them died on its first tool call being rejected, got the error back, and then ended the turn silently.Diagnosing it after the fact was impossible because the turn lifecycle logging is invisible in every default deployment.
Log targets
Both launch paths default the log filter to
buzz_acp=info— the binary's ownEnvFilterfallback incrates/buzz-acp/src/lib.rs, and theRUST_LOGDesktop hands its managed child.EnvFiltermatches a directive against a target by path segment, so bare targets likepool::promptandacp::streammatch nothing and are dropped. Turn start, turn stop, and every wire line never reach the log.All 19 custom targets in the crate are now prefixed with
buzz_acp::. Prefixing rather than widening the filter keeps the fix inside one crate — the alternative requires two filter strings, in two repos, to be kept in step forever. Neither filter string is touched. A test recurses the crate source and fails on any target the shipped filter would swallow.What makes a turn dead
A turn is dead when it ends
end_turnon a channel batch that contained at least one event the agent was expected to answer, and produced no output at all. Such a turn is reported as a failure and its batch flows through the existing backoff and dead-letter path (MAX_RETRIES) instead of vanishing.Silence is only a failure when someone was owed an answer.
PromptSource::Channelcovers every matched batch, not just mentions — under--subscribe allor arequire_mention: falserule the agent watches traffic nobody addressed to it, andbase_prompt.mdinstructs it to stay quiet when it has nothing to add.MatchedRuletherefore carriesrequires_response(the rule required a mention, or the event p-tags the agent) throughQueuedEvent→BatchEvent→FlushBatch, and passive batches are never retried.What counts as output is deliberately narrow:
agent_message_chunk, non-blankagent_message_chunk, whitespace onlytool_call/tool_call_updatecompleted, no error flagcompleted+rawOutput.isErrorcompletedwould mask the exact turn this detectsfailed/in_progressagent_thought_chunkHeartbeats are exempt — they are self-prompts with no waiting user and no batch to requeue. Every non-
EndTurnstop is already classified by its own arm and is left alone. Both completion paths — the natural one and the synthesizedEndTurnin the control-signal race — share one classifier, so they cannot drift.Mid-turn steered events
An event injected into a running turn by a native steer is not part of the batch, and a
Successack used to drop it outright. If the turn then went silent, that mention disappeared with no retry — the same failure, one layer down.AcpClientnow tracks a monotonic output epoch instead of a boolean, because a flag cannot distinguish "spoke, then went silent on the new message" from "answered the steer". ASuccessack parks the event in a delivered-steer ledger tagged with the epoch at acceptance; the turn's terminal classification settles it. Output after delivery retires the event; silence releases it to the queue front for an ordinary, fully-budgeted retry. Passive steered events are retired unanswered, same contract as passive batches. In-flight expiry and panic recovery settle the ledger as unanswered, so a delivered event can never outlive its turn.Acks that lose the race with their own turn
The ack watcher and the prompt result travel on different channels, and the main loop's
biased select!polls results first, so a genuinely-sentSuccesscan be processed after the turn it belongs to has terminated. Settling only the delivered ledger would leave that event withheld with no turn left to judge it, and the late ack would then move it into a ledger nothing ever resolves.EventQueue::settle_turn_steerstherefore settles both steer tables at terminal classification: delivered entries against the turn's final output epoch, still-withheld entries released as delivery-unknown, all in arrival order at the queue front.record_delivered_steerandrelease_native_steerreport whether the event was still pending, andapply_steer_ackhangs every effect off that answer — so a late ack of any shape queues no second copy, extends no unrelated turn's in-flight deadline, and fires no cancel+merge fallback at a turn that has ended.An unacked withheld event is released even when it is passive, unlike a delivered passive event: the latter is known to have reached the agent, so silence answers it, while an unacked one may never have arrived at all.
Retry preserves the whole batch
A steer or interrupt cancel puts the events an agent was already working on into
FlushBatch::cancelled_events, which the next flush merges into an annotated re-prompt. Both retry paths —requeueandrequeue_preserve_timestamps— iterated onlyevents, so when a merged re-prompt then failed, the interrupted bucket was discarded. For a response-required mention that is the same silent loss, reached through the retry machinery meant to prevent it.Both now share one reinsertion helper that restores every event with its own
prompt_tag,received_atandrequires_response, cancelled first since they arrived earlier, under the same per-channel cap. Cancelled events are normalised into ordinary queued events rather than returned to the cancelled side table: that table is exempt fromretry_afterin the cancelled-only flush fallback, so restoring them there would let a throttled batch re-flush immediately and spin, and the merge annotation frames work an agent was interrupted part-way through — after a failed turn there is none.Tradeoffs
A turn that emits only thought chunks and then stops is retried rather than accepted. That costs a bounded retry, which is the cheaper side of the trade against silently dropping a user's mention.
A steer acked
startedNewTurn— delivered into a fresh, detached turn — reports the current turn's epoch. If that detached work produces nothing observable before the awaited response lands, the event is redelivered. The same applies to aSuccesswhose ack the watcher delivers after its turn has already settled. A visible duplicate is the right side to err on against a silent loss.