Skip to content

fix(*): answer a waiting question with a message typed into the composer - #1048

Merged
0xKT merged 2 commits into
mainfrom
fix/answer-pending-question-from-composer
Oct 24, 2026
Merged

0xKT merged 2 commits into
mainfrom
fix/answer-pending-question-from-composer

Conversation

@silverLXT

@silverLXT silverLXT commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

While an ask_user question waits, the page still sends text typed into the main composer as a mid-turn inject (turn.send with busy: "inject"). Injects are drained only at the top of a loop iteration, which the turn never reaches while it is blocked inside ask_user. So the question ran out its budget as unanswered, the tool returned "user did not answer; proceed with best judgment", the agent went on with its own recommended option, and the person's reply was merged afterwards as a side note.

The channel intake already checks the question broker before injecting (gateway_commands.py, question_broker.pending_req(cid) -> reply). turn.send now does the same:

  • when a question is pending on the lane and the send is an inject without attachments, the text answers the question (question_broker.reply(lane, content)), and nothing is injected or queued;
  • the text is still a message the person sent, so it is announced with message.injected and every window draws it as a mid-turn bubble;
  • the prompt the question drew did not answer, so the broker sends it clarify.closed (QuestionBroker.reply(..., close=True)) and the sheet closes;
  • the result is {turn_id: <the running turn's id>, accepted: true, naming: false}, the same shape as before;
  • with no question pending, or with attachments, the send is handled exactly as before.

register_turn_methods takes the broker and the umbrella registration passes the one it already has; bootstrap.py already hands it in on both paths, so no client change is needed.

To reproduce on main: on the web page, ask "Before you start, ask me which of two greetings to use, then print it." When the question sheet appears, type the answer into the main composer and press Enter. The message is shown as added to the turn, the sheet stays open, and after the budget the agent proceeds on its recommended option.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

On Windows 11, Python 3.13:

  • pytest tests/test_rpc_turn_send.py tests/test_question_broker.py: 117 passed (follow-up commit included).

  • New test_a_message_typed_while_a_question_waits_answers_it fails on main (the text is injected and the question stays pending) and passes here; test_with_no_question_waiting_a_typed_message_is_still_an_inject pins the unchanged path. The first now also observes the message.injected frame and one clarify.closed for the pending request; test_an_answer_from_elsewhere_closes_the_surface_that_asked covers the broker flag.

  • On the web page (headless browser), the typed answer shows as a bubble and the prompt closes.

  • ruff check and ruff format --check on the changed files: clean.

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

While a question is pending, any composer text without attachments is taken as its answer, as the channel intake already does; a reply that does not fit the question reaches the agent as that answer and it can ask again. Rollback: revert the commit.

Generated with Claude Code

While an ask_user question waits, the page still sends text typed into the
main composer as a mid-turn inject. Injects are read only between model
calls, which the turn never reaches while it is blocked on the question, so
the question ran out its budget as unanswered and the agent went on with
its own recommendation; the person's answer arrived afterwards as a side
note. The channel intake already checks the question broker first; turn.send
now does the same: when a question is pending on the lane, an inject without
attachments answers it, and nothing is injected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: close the rendered clarify prompt when the composer resolves its pending question.

I found one user-visible lifecycle defect in the new answer path, marked inline. I covered AGENTS.md/CLAUDE.md and the Runtime/Web context rules, the complete diff, registration and callers, QuestionBroker and channel-path parity, Web/TUI consumers, relevant history, backward compatibility, and whether the tests were weakened (they were not). Verification: uv run pytest tests/test_rpc_turn_send.py tests/test_question_broker.py -q passed all 116 tests after uv sync --all-packages; git diff --check passed. The added test proves that the broker receives the text and no inject is submitted, but does not observe the outstanding clarify surface, which is where the defect remains.

Comment thread raven/rpc/methods/turn.py Resolved
@LivXue
LivXue requested a review from 0xKT October 22, 2026 06:59
@LivXue

LivXue commented Oct 22, 2026

Copy link
Copy Markdown
Member

Windows compatibility review: no additional Windows-specific issue found in the inspected scope.

Scope: all 3 changed files at head d8b33a69d0384903aa940162dbb18ac2553a00a2, against base/merge base c42988358efa3932c336d036b96da4b954d81d80. Read the complete local diff, registration/bootstrap and channel callers, QuestionBroker, relevant Web/TUI consumers, tests, and all existing discussion pages. The new route resolves an asyncio future; attachment/path handling and dependencies are unchanged.

Native Windows verification (Python 3.13.12, isolated detached checkout and uv environment):

  • uv sync --frozen --python 3.13 --extra dev: succeeded.
  • uv run --frozen --no-sync pytest tests/test_rpc_turn_send.py tests/test_question_broker.py -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\a130fd4dd4344beabbc69e870abc412a\checks\1048-main\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\a130fd4dd4344beabbc69e870abc412a\checks\1048-main\junit.xml: 116 passed.
  • git diff --check c42988358efa3932c336d036b96da4b954d81d80..d8b33a69d0384903aa940162dbb18ac2553a00a2: exit 0.

Code-reading limitation: no browser/TUI end-to-end or live-provider run. gloryfromca's existing unresolved clarify-dialog lifecycle finding remains relevant on Windows; the passing backend tests do not clear it. This comment adds no duplicate finding and makes no full-compatibility or merge claim.

When a message typed into the composer answers a waiting ask_user question,
nothing told the page. The prompt the question drew drops itself only when
it is the one answering, so it stayed on screen offering choices that went
nowhere; and since no turn or inject was made of the text, no frame carried
it and the person's own message never appeared in the transcript.

QuestionBroker.reply takes close=True for an answer that came from
elsewhere, and sends the same clarify.closed an unanswered question gets.
turn.send uses it, and announces the text with message.injected, the frame
every window already draws a mid-turn message from.

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

The new revision fixes the outstanding prompt lifecycle defect: composer-originated answers explicitly close the rendered question, remain visible through the existing message.injected path, and leave ordinary prompt-originated answers unchanged. I reviewed the full diff and the delta from the prior head, repo rules and Runtime/Web context, broker and registration callers, channel parity, frontend consumers, history, compatibility, and test strength. I also replied to the author and resolved my settled thread. Verification: uv run pytest tests/test_rpc_turn_send.py tests/test_question_broker.py tests/test_rpc_registration.py -q passed all 127 tests; git diff --check passed.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review at head 723ac91e6863db8da5abb77ddc160dce27ef6c1a, against base c42988358efa3932c336d036b96da4b954d81d80.

Inspected the complete five-file diff, RPC registration/bootstrap callers, question-broker routing, Web/TUI clarify-close consumers, tests, and existing discussion. No additional Windows compatibility issue was found in this scope.

  • Native Windows test evidence: all 127 tests passed across test_rpc_turn_send.py, test_question_broker.py, and test_rpc_registration.py on Python 3.13.12. The updated tests verify one ID-matched clarify.closed, the message.injected text frame, no extra submitted turn, and the ordinary inject path when no question is pending. This confirms the backend fix for the previously reported prompt lifecycle issue, already resolved in that discussion.
  • Code inspection: this change introduces no platform-specific path, shell, process, or dependency operation. The close notification uses the existing async delivery path; the inspected Web/TUI consumers identify the question by request ID.
  • Source verification: used an isolated checkout outside the active workspace, matched all 4,660 source files and Git modes/tree to this exact head, rechecked the five changed files after tests, and ran git diff --check successfully.

Coverage limits: UTF-8 mode was enabled; legacy Windows code pages and Python 3.12 were not tested. These are backend tests, without a live browser/TUI/provider session or the full repository suite; they do not establish complete Windows compatibility.

Exact verification commands and results

Each Python invocation used an isolated uv environment and temporary profile/Raven/OAuth/temp directories. UTF-8 mode was enabled and imports were asserted to belong to the selected checkout.

$run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1292e0f475b0450bb12d681f2a4fa415'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'

# 1048-question: 127 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1292e0f475b0450bb12d681f2a4fa415\pr1048\head-tree'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1292e0f475b0450bb12d681f2a4fa415\pr1048\head-tree' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_rpc_turn_send.py' 'tests/test_question_broker.py' 'tests/test_rpc_registration.py' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1292e0f475b0450bb12d681f2a4fa415\checks\1048-question\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1292e0f475b0450bb12d681f2a4fa415\checks\1048-question\junit.xml'
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check c42988358efa3932c336d036b96da4b954d81d80..5289c2ccbc3804ff264799d0958925deffa0b8d5 # exit 0

@0xKT
0xKT merged commit ed5fbe5 into main Oct 24, 2026
46 of 48 checks passed
@0xKT
0xKT deleted the fix/answer-pending-question-from-composer branch October 24, 2026 02:08
LivXue added a commit that referenced this pull request Nov 1, 2026
## Summary

Pressing stop while an `ask_user` question is on screen did not stop the
turn. `QuestionBroker.await_question` caught `CancelledError` together
with
`TimeoutError` and returned the question's default, so the cancellation
ended
inside the broker: the tool reported "user did not answer; proceed with
best
judgment", the model went on with its own recommendation or asked again,
and
`turn.cancel` waited for that whole continued turn while the page
already
showed the turn as stopped. Reproduced on the web page with a headless
browser: after stop the same turn still ran a 45 s command and finished
with
an answer, and a prompt that asks again when unanswered put a second
question
on screen.

The turn now ends where it was stopped:

- `await_question` re-raises `CancelledError` when a cancellation of the
calling task arrives while the wait is pending. The count is snapshotted
  before the wait because `Task.cancelling()` is cumulative: a task that
swallowed an earlier cancellation must not turn someone else's cancelled
  future into its own. The prompt is still closed with `clarify.closed`.
- A pending future cancelled by anyone else still resolves to the
default.
The ACP elicitor already turns a cancelled task into a cancelled form,
so
  its behavior is unchanged; its docstrings and the contract papers now
  describe the new exit.
- A stint's first round could leave `StintDriver.start` without a
receipt
  while its approval was on screen (a Stop, or `load_playbook`'s 120 s
ceiling). The record was left `running`, and a later request took it up
through `resume` without asking. `start` now claims the record and
starts a
stamp-only beat before the approval, and closes the record as stopped on
  any such exit before a round is opened.
- The close reads, checks and writes the record under one
`write_transaction`, so a `pause`, `stop`, `extend` or claim that landed
during the question survives, and a record another process or host
claimed
  is left to it. A cleanup that cannot read or write is logged and the
cancellation still propagates; only a cancellation or a timeout is
blamed
  on the approval.
- `StintStore.touch` moves only `touched_at_ms` under the write lock and
compares the holder's pid and host, so the beat cannot undo a concurrent
stop or pause and cannot keep another host's claim alive. `extend`
refuses
a first round that never opened, and says whether starting it again asks
for approval; `resume` asks before taking one up and closes it on
refusal
or cancellation. A first round paused for the hourly dispatch budget
stops
the pre-approval beat, so `holding()` is false as soon as the record is
  paused.

Review follow-ups on the final head, one commit each:

- A close that loses the race to a round another caller already opened
no
longer stops the beat table's current entry. It stops only the beat this
caller started, and only when no round opened, so a live round keeps the
  heartbeat that keeps a second host off its checkout.
- The close now also asks the caller's own record whether round one
opened.
  `_submit` opens the round on the object before writing it, so a failed
write no longer records `stopped` for a round that is running (which is
not
  `unfinished` and would let a second stint open on the same project).
- `resume` closes a never-opened first round on "not approved" only. A
delegation pause or a validation refusal leaves the record `interrupted`
and retryable instead of terminal; an unanswered question (a timeout, or
one the broker could not deliver) fails safe to `_NOT_APPROVED` and
closes
  like any other unanswered ask.
- The dispatch budget is charged after the first round's confirmation,
so a
  budget pause on round one follows an approval. The record now carries
`first_round_approved`, and `resume` takes that round up without asking
a
second time. Records written before the field read as not approved and
are
  asked again.
- `resume` claims the stint only when it is going to ask. A resume that
asks
nobody has nothing to keep alive while it preflights, and a stop during
that window puts the record back where the resume found it: a pause
stays
a pause, and a stale `running` becomes `interrupted`, so the next resume
  is not refused for the five-minute stale window.

Follow-ups after the second review round:

- The stopped-preflight restore re-reads and writes under the same
  transaction as the first-round close, and leaves alone a record whose
  round opened or that another process has moved or claimed.
- The transient-refusal comment now names the refusals that actually
reach
  that branch, and says where an unanswered question lands.
- The open-round write re-reads the record and writes it under one
`write_transaction`. A `stop` or `pause` that landed while the approval
  question was on screen therefore survives the round being recorded:
`advance` ends or parks the stint after that round instead of the record
  being revived to `running`. A record that cannot be read is replaced
  rather than turning a dispatched round into an error the turn reads.
- A new end-to-end test stops a lane while the real broker holds its
  question. It drives the real scheduler, lane, `QuestionBroker` and
`AskUserTool` under a fake `run_turn` -- the seam where the loop runs a
tool -- and fails if the broker answers the cancellation with the
default.

## Type

- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

- [x] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

Native Windows 11, Python 3.12.13, `PYTHONUTF8=1`, isolated
`USERPROFILE`/`TEMP`, exact-head worktree, `uv run --frozen --no-sync`:

```
pytest tests/test_playbook_stint.py tests/test_stint_record.py
-> 186 passed, 9 failed

pytest tests/test_playbook_stint.py tests/test_stint_record.py \
  tests/test_question_broker.py tests/test_ask_user_tool.py \
  tests/test_rpc_turn_cancel.py tests/test_spine_scheduler.py \
  tests/test_acp_elicitation.py tests/test_acp_ask_user.py \
  tests/test_acp_questions.py tests/test_cli_playbook_commands.py \
  tests/test_rpc_playbooks.py tests/test_a2a_asking.py \
  tests/test_agents_oncall_flow_escalation_tools.py \
  tests/test_subagent_dag_core.py tests/test_rpc_spine.py
-> 1200 passed, 18 failed, 15 skipped
```

All 18 failures are environment-only and have the same IDs at the merge
base: 8 Windows symlink privilege (`WinError 1314`), 1 `os.kill` on a
dead
pid, 2 path/fixture failures in `test_rpc_playbooks`, and 7 POSIX
reference
paths in `test_subagent_dag_core`. The broader run was made on the final
tree.

The six new regression tests fail on the head before their fixes and
pass
here: dropping the broker's re-raise fails the end-to-end stop test,
withdrawing the open-round record write fails the two out-of-band
stop/pause tests, withdrawing the stopped-preflight restore fails both
stopped-resume tests, and withdrawing the open-round read fallback fails
the
unreadable-record test. Reverting the earlier `_close_first_round`
change
still fails the two tests that cover the beat and the failed-write
close.

`ruff check` and `ruff format --check` pass on the touched files, and
the
repository's commit-message lint passes over the branch. Internal
contract
docs (`CONTEXT.md`, `docs/Proactivity-Implementation.md`, and the ACP
elicitation design spec) are updated; no user-facing screenshots are
needed.


## Risk

- [x] Security impact considered -- no new external input or permission
  surface; the change only lets a stopped turn's cancellation reach the
  turn.
- [x] Backward compatibility considered -- `first_round_approved` is an
  additive record field; older builds drop unknown keys and read it as
false, so they ask once more rather than run unapproved. The broker's
new
  raise only reaches callers whose task was cancelled, and those callers
were audited. The new record writes preserve every field they re-read,
so
  older builds read the same file.
- [x] Rollback path is clear -- revert the follow-up commits or the
branch;
  records carrying the new field remain readable.

## Related Issues

#1048 (independent; no dependency)

---------

Co-authored-by: silverLXT <xiaotian.luo@evermind.ai>
Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
Co-authored-by: Dizhan Xue <xuedizhan17@mails.ucas.ac.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants