Repository navigation
fix(ui-bridge): stop a turn or run by a target that is never reused - #1848
Conversation
A session with no question numbers left now fails its test at once instead of waiting for an answer that cannot come. A repeated trust reply is tested to be refused without waiting behind a running turn, which would hold up a cancel on the dispatch thread. The question counter notes why it is a compare-and-swap loop.
A turn number repeats after session.rewind, and a manifest run has none, so a cancel that named a turn could still stop a later turn with the same number, and could never stop a run. Each turn and run now gets a target that is never handed out twice. turn.send and manifest.run return it, the session view carries the target of the turn on screen, and turn.cancel stops the one it names and nothing else. A cancel that still names a turn number is refused rather than read as naming nothing. capabilities.actionTargets advertises cancel as expected_target. The TypeScript client names the target the caller gives, or the one on screen, and names none while a send is unanswered or once the view has ended, where what it shows is stale. cancel() takes an optional target and reports whether the bridge stopped it. A runtime that reports no targets is still accepted: an absent target reads as the idle target 0 and the client sends unnamed cancels to it. The client no longer tracks the largest turn it sent. RPCVIEW-6 states the target rule, including that a target is unique within one bridge process and that a session with a view cannot start a run. RPCVIEW-7 states what the client names. The protocol document gives one return shape for turn.cancel and the capability's shape.
Same behavior: an absent target reads as 0, and a target that is not a number is refused.
|
[puLL-Merge] - brave/bravebot@1848 DescriptionReplaces turn-number cancel targeting (RPCVIEW-6) with a process-unique Possible Issues
ChangesChanges
sequenceDiagram
participant UI as Client (agent-client)
participant B as Bridge
participant R as running::next_target
participant W as Worker
UI->>B: turn.send {session, prompt}
B->>R: next_target()
R-->>B: target N
B->>B: open.running = Running{target N}
B-->>UI: session.view.update {turn, target N, running}
B->>W: spawn turn
B-->>UI: {turn, target N}
Note over UI,B: later / after rewind
UI->>B: turn.cancel {session, target M}
alt "turn" param present
B-->>UI: error bad_request
else M == running.target and not finished
B->>W: cancel.cancel()
B->>B: watches.stop_firing()
B-->>UI: {cancelled: true}
else M stale
B-->>UI: {cancelled: false}
else no target
B->>W: cancel whatever running
B-->>UI: {}
end
W-->>B: turn.error Cancelled
B-->>UI: session.view.update {status: cancelled}
|
netzenbot-reviewer
left a comment
netzenbot-reviewer
left a comment
There was a problem hiding this comment.
Recommendation: comments only
What this pull request does
This replaces the turn number with a new per-process "target" as the way turn.cancel says which turn or manifest run it means. The bridge hands out each target from a global counter that never repeats. turn.send and manifest.run now return it, and the session view carries the target of the turn on screen. turn.cancel takes target, stops only the turn or run with that target, and answers cancelled: true or false. A cancel with no target still stops whatever is running, and one that still sends the old turn parameter gets bad_request. This closes a hole where, after session.rewind, a late cancel for the old turn 3 could stop the new turn 3, and a manifest run had nothing a cancel could name. The advertised capability changes from cancel: expected_turn to cancel: expected_target. The TypeScript client names the target the caller passes, or else the one on screen, and names none while a send is unanswered, after the view has ended, or against a runtime that does not advertise targets. AgentSession.cancel now returns {cancelled: boolean | null} and SendResult gains target. The diff adds Rust tests (rewind, manifest run, view refusal, trust-reply lock, question exhaustion), updates the scenario fixtures, and updates the specs and protocol docs. The "a session with a view cannot start a manifest run" refusal is pinned by a new test, but this diff shows no new production code for it, so it appears to rely on existing behaviour.
How this review reached its recommendation
The review read 54 changed files.
What it checked:
- Written best practices. The changes were compared with 20 rules from this project's best-practice documents (dependencies, paths, shared-implementation, specs, tests, ui, writing).
- Bugs. The changes were read for mistakes that would make the code misbehave or stop it building.
- This project's own criteria. The changes were checked against these questions:
- Whether the change agrees with the project's specs, and whether a change to behaviour a spec describes also updates that spec and its tests.
- Whether untrusted content (text from the web, files or tools) can influence a decision, or be given a better trust label than it came with, anywhere other than the places built for that.
- Whether the tests would fail if the behaviour broke, rather than covering only the allowed case or passing with the bug put back.
- Whether the change does what its title says and nothing more, with no unrelated refactor and no new dependency, permission or network destination the title does not explain.
The checks flagged 2 possible problems. 2 of them went to a second reader, who checked each against the full source code rather than only the diff, and kept 2.
…e one cancel(target) dropped the target against a runtime that does not advertise action targets and sent an unnamed cancel, which stops whatever is running. A caller that named a stale turn could stop the current one. It now throws UnsupportedError and sends nothing.
turn.send accepted any number as a target, including a negative or fractional one, where a view update already required a count. Both paths now refuse it, with a wire test and a scenario that pin the refusal. The spec and protocol text for the cancel's `turn` parameter now state the present rule and its reason rather than what an earlier version did.
netzenbot-reviewer
left a comment
netzenbot-reviewer
left a comment
There was a problem hiding this comment.
Recommendation: approve
What this pull request does
Each turn and each manifest run in the ui-bridge now gets a target number from a process-wide counter that never repeats. turn.send and manifest.run return it, the session view carries the target of the turn on screen, and turn.cancel takes target instead of the old turn number. A cancel naming a target stops only the running turn or run with that target and answers { "cancelled": true|false }; a cancel naming nothing still stops whatever is running and answers {}, and a cancel that still sends turn is refused with bad_request. This fixes a delayed cancel for an earlier turn 3 stopping the new turn 3 after session.rewind reuses the number, and lets a cancel name a manifest run, which has no turn number. The capability advertised in actionTargets changes from expected_turn to expected_target, and the TypeScript client's cancel(target?) now returns { cancelled }, names the on-screen target only when no send is unanswered and the view has not ended, and throws UnsupportedError if given a target by a runtime that does not advertise it. The diff matches the description; it also adds tests for the question-number and repeated trust-reply behaviour, and no terminal or desktop screen changes.
Trying it by hand
The description does not say how to try this change. These steps would:
- Run the automated reproduction:
BRAVEBOT_ALLOW_UNCONFIGURED_BUILD=1 make check-agent-clientand expect every test to pass, including 'after a rewind a turn is numbered again, and a cancel meant for the earlier turn with that number stops nothing'. Also runcargo test -p bravebot-ui-bridge. - To try it by hand, start
bravebot-rpc(the NDJSON stdio bridge) against a configured or stub model and sendagent.info. Expectcapabilities.actionTargets.cancelto beexpected_target. - Send
session.newfor a directory, thenturn.sendwith a prompt. Expect the response to contain bothturnandtarget, and asession.view.startview to carry the sametarget. - While a turn is held, send
turn.cancelwithtargetset to a different number. Expect{ "cancelled": false }and the turn to keep running. Then send it with the real target and expect{ "cancelled": true }and aturn.errorof kind cancelled. - Send
session.rewindwithsteps: 1, thenturn.sendagain. Expect the sameturnnumber as before but a differenttarget, and a cancel naming the earlier target to answer{ "cancelled": false }. - Send
turn.cancelwith the oldturnparameter. Expect abad_requesterror and nothing stopped. Send it with neither parameter and expect{}and the running turn to be cancelled. - Start a manifest run with
manifest.runon a session that has no view, and expect atargetin the response. A cancel naming it should answer{ "cancelled": true }. Check thatmanifest.runon a session aftersession.view.startis refused withbad_request.
How this review reached its recommendation
The review read 9 of the 56 changed files: only those that changed since the bot last reviewed this pull request.
What it checked:
- Written best practices. The changes were compared with 20 rules from this project's best-practice documents (dependencies, paths, shared-implementation, specs, tests, ui, writing).
- Bugs. The changes were read for mistakes that would make the code misbehave or stop it building.
- This project's own criteria. The changes were checked against these questions:
- Whether the change agrees with the project's specs, and whether a change to behaviour a spec describes also updates that spec and its tests.
- Whether untrusted content (text from the web, files or tools) can influence a decision, or be given a better trust label than it came with, anywhere other than the places built for that.
- Whether the tests would fail if the behaviour broke, rather than covering only the allowed case or passing with the bug put back.
- Whether the change does what its title says and nothing more, with no unrelated refactor and no new dependency, permission or network destination the title does not explain.
None of these checks flagged anything, so there was nothing to double-check.
3 participants
Closes #1847
User impact: none for terminal and desktop users, who send one action at a time. A client that connects, drops and sends a cancel late can no longer stop a later turn, and can now stop a manifest run.
The problem
This follows #1783 and its fix in #1784. #1784 let a client say which turn a cancel was for, using the turn number. A turn number repeats: after
session.rewindthe next turn gets a number that was already used. A cancel meant for the earlier turn 3 therefore stops the new turn 3. A manifest run has no turn number, so a cancel could never name one. Clients that reconnect and send actions late, such as the planned mobile client, hit this.Reproduce
The affected front end is any client of
bravebot-rpc(newline-delimited JSON). The terminal and desktop apps do not send late cancels, so there is no screen to walk through. The reproduction is the client test that drives a realbravebot-rpcagainst a scripted model.bravebot-rpc:BRAVEBOT_ALLOW_UNCONFIGURED_BUILD=1 make check-agent-client.Observed: I did not run this against the parent commit. I reproduced the old keying by making each turn's target equal to its turn number in
send_turn. With that change,make check-agent-clientfailed only that test (233 of 234 passed), and the 83bravebot-ui-bridgeunit tests still passed. The new Rust testa_turn_after_a_rewind_has_a_new_target_though_its_number_repeatsalso failed with that change, at the assertion that the two turns' targets differ.Expected: the second turn has the same number and a different target, and a cancel naming the first target answers
{ "cancelled": false }and stops nothing.The fix
Each turn and each manifest run gets a target from a counter that never repeats within a bridge process.
turn.sendandmanifest.runreturn it, and the session view carries the target of the turn on screen.turn.canceltakestarget: it stops the turn or run with that target and nothing else, and answerscancelled: trueorfalse. A cancel with no target stops whatever is running, as before. A cancel that still sends the oldturnparameter is refused, because reading it as naming nothing would stop whatever is running.The TypeScript client names the target the caller gives, or the one on screen. It names none while a send is unanswered or after the view has ended, since the target it shows is then stale. A runtime that does not advertise
cancel: expected_targetreports no targets; the client accepts that and sends unnamed cancels to it. A session with a view cannot start a manifest run, so the view never has to carry a run's target. A target is unique within one bridge process, so a restarted bridge counts again.This also adds tests for the question-number and trust-reply behaviour from #1784: a session with no question numbers left now fails its test at once, and a repeated trust reply is shown not to wait behind a running turn.
Test plan
cargo fmt --all -- --check- passedcargo clippy -p bravebot-ui-bridge --all-targets -- -D warnings- passedcargo test -p bravebot-ui-bridge- 343 passed, 0 failedmake check-agent-client- 234 of 234 passed, including the tests that drive a realbravebot-rpcmake check-spec- exit 0manifest.runfails when that refusal is disabled - observed, then restoredmake check-affected,make check-all; they are slow and fail on unrelated environment problems on this machineSpec and tests
RPCVIEW-6 (targets, the
turnrefusal, one-process uniqueness, runs and views) and RPCVIEW-7 (what the client names) indocs/specs/session-view.mdare updated, with the tests that pin each.ui/docs/phase-0-rpc-protocol.mddocumentstargetonturn.send,manifest.run, the view andturn.cancel.