Skip to content

fix(acp): state the reply destination in mid-turn steer prompts - #6633

Open
odedlaz wants to merge 2 commits into
block:mainfrom
odedlaz:bob/acp-native-steer-anchor
Open

fix(acp): state the reply destination in mid-turn steer prompts#6633
odedlaz wants to merge 2 commits into
block:mainfrom
odedlaz:bob/acp-native-steer-anchor

Conversation

@odedlaz

@odedlaz odedlaz commented Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown

buzz-acp has two mid-turn steer transports, and only one told the agent where to reply.

The cancel+merge fallback builds its prompt through queue::format_prompt, which states the thread scope and a --reply-to anchor. The native path hand-assembled its body from framing plus one event block and called neither format_context_hints nor resolve_reply_anchor. With no destination in the prompt, the previous subject's thread stayed the only live one in the agent's context, so a message steered in from thread B got answered in thread A. Regressed in e567491a1 (#1160), which introduced try_native_steer.

This is the default path: --multiple-event-handling defaults to steer, native is tried first, and cancel+merge is the fallback for an unadvertised transport, transport failure, a missing run ID, a rejected cross-adapter outcome, or method-not-found. It is not goose-specific either — _goose/unstable/session/steer and the cross-adapter _session/steering carry the same body, so any adapter advertising _meta.steering.supported is affected, claude-agent-acp included.

The fix

queue::format_native_steer_prompt owns the whole body, and native_steer_framing and format_event_block are now private to queue, so the hand-assembly cannot recur. resolve_reply_anchor absorbed the DM branch format_prompt had inline, so one function holds the rule instead of two copies — the duplication is how the defect happened. Only per-event routing context is added: a steer is a delta into a live turn, so standing context, channel metadata and history stay out.

Two accepted tradeoffs:

Every non-DM native steer is anchored, agent turns included. The path is synchronous on the main event loop and a profile lookup needs an uncached relay query, so turn_is_human_facing sees an unknown sender and treats it as human. The fallback deliberately leaves agent-to-agent steers free to nest; native no longer does. Losing a human's reply destination is the worse failure, and the alternative is a network call on the hot path.

On unresolved channel metadata, native declines instead of guessing. Either guess is wrong somewhere: the non-DM rule forces an anchor onto a top-level DM that should have none, and the DM rule mis-targets in a channel. So DmClassification is a closed Dm | NonDm | Unresolved — gates_as_dm answers for every state, while native_steer_scope yields a NativeSteerScope only for the resolved ones. A decline hands the event to cancel+merge, which resolves the channel again at flush time. If that retry also fails, the fallback keeps its pre-existing non-DM default; that limit predates this PR.

Test enrollment

buzz-acp was compiled by CI and executed by no job — just test-unit names its packages explicitly and nothing runs cargo test --workspace — so the new routing tests would have guarded nothing. This PR adds the package to that recipe and to its scripts/run-tests.sh mirror, enrolling 821 tests, 810 of which predate this PR and have never run in CI.

Both commands clear the BUZZ_* family in a subshell first. config.rs asserts clap defaults through CliArgs::parse_from, which reads #[arg(env)] unconditionally, and a buzz-acp-hosted agent sets exactly those variables.

Verification

just ci exits 0 at c8b98ab92 on the repository's pinned Hermit toolchain, with the buzz-acp step this PR adds running all 821 green. The mobile path filter matches nothing in this diff, so those lanes skip upstream; dart format, flutter analyze and flutter test were run locally and pass. The run-tests.sh arm the gate never reaches was run directly, against a blob differing from this one only by a deleted comment: strip its BUZZ_* postcondition and a leaked variable reaches cargo test with the suite still green.

The four routing tests fail against the pre-fix body shape, and both classification-seam mutants — mapping Unresolved onto a resolved scope, and picking the wrong scope for a resolved one — fail the seam test, which reads the request try_native_steer actually sent. Pre-fix behaviour was observed rather than only inferred: steers delivered mid-turn to a live claude-agent-acp session carried the header, the event block and the closing note, with no [Context], no Thread root and no --reply-to.

Not verified: an end-to-end live steer on a patched build, confirming the posted reply's e tag resolves to the steering thread. That needs a harness running this branch, and the harness is the thing under test.

@odedlaz
odedlaz requested a review from a team as a code owner August 23, 2026 21:25
@odedlaz
odedlaz marked this pull request as draft August 23, 2026 21:29
@odedlaz
odedlaz force-pushed the bob/acp-native-steer-anchor branch from 436e5bd to 37b0a34 Compare August 24, 2026 08:58
odedlaz added 2 commits August 24, 2026 12:52
@odedlaz
fix(acp): state the reply destination in mid-turn steer prompts
fd491d3 
buzz-acp has two mid-turn steer transports. The cancel+merge fallback builds its
prompt through `queue::format_prompt`, so it states the thread scope and a
`--reply-to` anchor. The native path hand-assembled its body from framing plus
one event block and called neither `format_context_hints` nor
`resolve_reply_anchor`, so the agent received no destination at all — leaving the
previous subject's thread as the only live one in its context, and replies landed
there.

That path is the default: `--multiple-event-handling` defaults to `steer` and
native is tried first, with cancel+merge only as the fallback. It affects every
adapter on either transport (`_goose/unstable/session/steer` and the
cross-adapter `_session/steering`), not only goose.

`queue::format_native_steer_prompt` now owns the whole body, and
`native_steer_framing` / `format_event_block` are private so the hand-assembly
this replaced no longer compiles outside the module. `resolve_reply_anchor`
absorbed the DM branch `format_prompt` had inline, so both transports resolve the
anchor through one function. Standing context, channel metadata, profile labels
and conversation history stay omitted: re-sending what the turn usually holds
defeats the delta, and where it may not hold them the hints tell the agent to
fetch.

Two constraints shaped the fix:

- The native path is synchronous on the main event loop and the only producer of
  a profile lookup is an async relay query, so identities are unknown here.
  `turn_is_human_facing` treats an unknown identity as human, which anchors every
  non-DM native steer — including the agent-to-agent ones the fallback leaves
  free to nest. Deliberate: losing a human's reply destination is the worse
  failure of the two.
- Unresolved channel metadata has no safe native reading. The authorization gate
  fails closed (unresolved means DM) so permissive `respond_to` modes cannot be
  exercised in a channel we failed to classify. Native steering cannot pick an
  anchor rule at all: the non-DM rule points a DM reply at its conversation root
  instead of the triggering message and forces an anchor on a top-level DM that
  should have none, the DM rule makes the inverse mistakes in a channel, and an
  anchorless prompt recreates the cross-thread bug above. `classify_dm` resolves
  the channel once because `ChannelInfoResolver` does not cache the unresolved
  case and a second call would pay a second lazy REST fetch on the main loop.

`DmClassification` is a closed `Dm | NonDm | Unresolved`. `gates_as_dm` answers
for every state; `native_steer_scope` returns `Option<NativeSteerScope>`, whose
two variants are the resolved states only. `try_native_steer` declines on `None` before
building a prompt, sending, or withholding. The caller's existing fallback then
issues cancel+merge with the event still queued, and that path resolves the
channel again at flush time — so declining buys a second attempt at definitive
metadata rather than discarding the steer. Production call sites hand the whole
classification to `try_native_steer`; that seam declines or yields a resolved
scope, and `native_steer_prompt_blocks` turns that scope into the formatter's
boolean. No call site names a reading.

Verification beyond CI: the four routing tests were run against a mutant that
restores the pre-fix body shape (framing + event, no `[Context]`) and all four
fail; the omissions test was run against a mutant that enriches the channel
metadata and fails. Mapping `Unresolved` onto a resolved native scope fails the
decline guard, and choosing the wrong scope for a resolved classification fails
it too: the assertions read the request `try_native_steer` actually sent, so the
chain is pinned from classification to anchor rather than from an intermediate
the test supplied. The resolved arms double as the decline's control — they
steer on the same fixture, so a declining `false` is not a pool that could never
have steered. The defect itself was observed first-hand — two native steers delivered to a claude-agent-acp session
mid-turn carried the steer header, the event block and the closing note, and no
`[Context]`, `Thread root` or `--reply-to`.

Co-authored-by: Oded Lazar <olazar@neo.ai>
Signed-off-by: Oded Lazar <olazar@neo.ai>
@odedlaz
test(acp): clear the BUZZ_* family for the buzz-acp gate step
c8b98ab 
Adds buzz-acp to `just test-unit` and its `scripts/run-tests.sh` mirror. Its
tests were compiled by CI and executed by no job, so the routing assertions in
the parent commit would not have guarded anything.

Both steps run the package behind a subshell that unsets every `BUZZ_*` variable
first. `config.rs` asserts clap defaults through `CliArgs::parse_from`, which
reads `#[arg(env)]` unconditionally, and its test module deliberately avoids
`std::env::set_var` to dodge parallelism races — so it assumes a fixed
environment, and inside a buzz-acp-hosted agent that assumption does not hold.
Clearing the whole family rather than a list of names gives those tests a
deterministic no-Buzz-config environment; a name list is a function of the test
set, so it drifts in both directions as tests are added. It does not reproduce
CI's environment: `.github/workflows/ci.yml` sets `BUZZ_TEST_POSTGRES_PASSWORD`
for every job.

The `run-tests.sh` copy asserts the outcome rather than each `unset`, because
`run_test_step` invokes it from an `if` — which suppresses `errexit` for the
whole call, so a variable that cannot be unset would leak into the test
environment and the step would still report success. The `Justfile` copy sits
in an `if` body rather than a condition, so it already aborts on a failed unset.

Scoped to a subshell so the package steps above keep their environment.

Co-authored-by: Oded Lazar <olazar@neo.ai>
Signed-off-by: Oded Lazar <olazar@neo.ai>
@odedlaz
odedlaz force-pushed the bob/acp-native-steer-anchor branch from 37b0a34 to c8b98ab Compare August 24, 2026 09:59
@odedlaz
odedlaz marked this pull request as ready for review August 24, 2026 13:11
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 24, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@odedlaz