fix(acp): report a turn that produced no assistant text as empty, not ok - #4648
fix(acp): report a turn that produced no assistant text as empty, not ok#4648MahdiHedhli wants to merge 2 commits into
empty, not ok#4648Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51381b8542
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Used for slash-command pass-through: ACP connectors detect commands via | ||
| /// the **first** block's text starting with `/`, so the harness sends | ||
| /// `["/cmd args", "<buzz context>"]` instead of one wrapped block. | ||
| /// Whether the most recent turn emitted any non-whitespace assistant text. |
There was a problem hiding this comment.
Move the new getter and its documentation outside the existing rustdoc block: as written, Rust associates the preceding “Like session_prompt_with_idle_timeout … separate text content block” description with last_turn_emitted_text, while session_prompt_blocks_with_idle_timeout loses its documentation entirely. This produces misleading generated API docs and leaves a public method undocumented.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7cab7e4 — good catch, and it was a real docs regression rather than a style nit.
The getter was inserted between session_prompt_blocks_with_idle_timeout and its rustdoc, so the "Like session_prompt_with_idle_timeout…" description bound to the getter and the prompt-block method was left undocumented. Moved the getter and its own doc comment above the block, so each description now attaches to the item it describes.
No behaviour change; cargo test -p buzz-acp --lib still 669 passing and cargo fmt --check clean.
…ot `ok`
A CLI runtime that refuses to start a turn generally ends its ACP session
cleanly without emitting any `agent_message_chunk`. Expired or missing
credentials, an untrusted working directory, and a missing provider config all
behave this way. That is protocol-legal, so the harness sees
`StopReason::EndTurn`, records `outcome="ok"`, posts nothing, and logs nothing
above DEBUG. The agent appears healthy and idle while every turn is silently
lost.
Observed with codex-acp: with a working directory outside a trusted project the
CLI refuses ("Not inside a trusted directory"), the turn returns in ~4s having
emitted nothing, and the harness reports success. With the identical config
inside a trusted repository the same prompt takes ~70s and replies. Both were
logged as `outcome="ok"`, which is the only reason this took so long to find.
Track whether a turn emitted non-whitespace assistant text, label such turns
`empty`, and log them at WARN rather than DEBUG — the default deployment level
is exactly where this needs to surface. Whitespace-only chunks do not count, so
a runtime that emits a stray newline before refusing still reports `empty`.
No protocol change, no behaviour change for turns that produce output.
Signed-off-by: Mahdi Hedhli <16087011+MahdiHedhli@users.noreply.github.com>
51381b8 to
6dfa5b9
Compare
The new getter was inserted between `session_prompt_blocks_with_idle_timeout` and its documentation, so rustdoc attached that description to the getter and left the prompt-block method undocumented. Move the getter and its own doc comment above the block instead. No behaviour change. Signed-off-by: Mahdi Hedhli <16087011+MahdiHedhli@users.noreply.github.com>
1 participant
Fixes #4646.
Problem
A CLI runtime that refuses to start a turn generally ends its ACP session cleanly without emitting any
agent_message_chunk.buzz-acpmaps every clean session end tooutcome="ok", posts nothing, and logs nothing aboveDEBUG. The agent looks healthy and idle while every turn is silently discarded.Expired credentials, an untrusted working directory, and a missing provider config all produce this identical signature.
Evidence
Observed with
codex-acp1.1.7 /codex-cli0.146.0 on a headless relay seat whoseWorkingDirectorywas a service-user home rather than a trusted project:~3.8s elapsed, nothing posted, no warning at any level. The cause was visible only outside the harness:
With the identical configuration inside a trusted repository the same prompt took ~70s and replied normally. Both were logged
outcome="ok"— which is the only reason this took as long as it did to find.Change
AcpClienttracks a per-turnturn_emitted_textflag, reset at eachsession/promptand set fromagent_message_chunk. Per-turn rather than per-session, because a runtime that refuses one prompt may serve the next — staleness would mask a recurring refusal.PromptOutcome::Ok(_)with no emitted text is labelledemptyinstead ofok.WARNrather thanDEBUG, naming the three things worth checking (credentials, working directory, provider config). A default deployment is not atDEBUG, which is exactly when this needs to be visible.No protocol change. No behaviour change for turns that produce output. The label flows through the existing
outcomefield, so existing turn-metric consumers get the distinction without changes.Verification
cargo test -p buzz-acp --lib— 669 passed, 0 failedcargo fmt -p buzz-acp -- --checkcleanmainand re-verified after rebase, sincemainhad changed both touched filesNot included
Deliberately left out, happy to add if wanted:
emptyturns for one agent — a single empty turn can be legitimate, a run of them is not.codex-acpshould surface the CLI's refusal as an ACP error rather than an empty successful turn. This harness-side fix is still worth having as defence in depth, since it covers every runtime rather than one.