feat: an assistant run can be stopped mid-way #162

Merged

bjoern merged 2 commits from feat/cancel-an-assistant-run into main

2026-09-19 12:40:06 +02:00

scarlet commented

2026-09-19 11:49:14 +02:00

Member

What's in

While the assistant is working, the panel's Send button becomes a Stop button (tooltip: "Stop — the step in flight finishes first"); pressing it posts a transcript line saying so and then goes dead, and the run ends quietly — AssistantRunCompletedAction(cancelled: true) with no error, so no error banner appears, the reflection epic skips the turn, and the next message runs its tools normally again.
AgentService.cancelRun() sets a per-run flag and buildTools wraps every tool in a new CancellableTool, so from the next tool call on nothing executes: the tool answers a short "the user CANCELLED this run" text instead of touching the repository, the web or the approval gate. No individual tool file changed, so a future tool inherits the behaviour.
The approval gate is closed for the rest of a cancelled run (ApprovalGate.close() / reopen()): every card still pending is denied, and a write tool that only reaches the gate after the cancel (e.g. entity_write awaiting listTags() first) gets an immediate refusal instead of raising a live card the user can no longer stop. The gate reopens when the next run wires onRequest.
Tool-activity chips are suppressed for short-circuited calls, so the transcript does not show fresh tool work under the "nothing more is changed" notice (a chip started before the cancel still completes).
ADR amendment: ADR 0024 listed mid-run cancellation under "Deliberately deferred (not in v1)"; this PR carries an amendment (2026-09-19) superseding that deferral and stating the honest limit — the model round already in flight cannot be aborted (the vendored Agent takes no cancel token), so a cancel costs at least one more completion, and up to maxToolRounds of them if the model keeps calling tools; nothing app-side ends the loop, only the tools stop working.
No new endpoint, no migration, no new tunable; backend untouched.

Verification

Executed exactly scripts/verify.sh app from the branch worktree at 099f674, fresh: passed — All tests passed! (1131 app tests) followed by == verify OK (app) ==. scripts/verify.sh backend was not run: no backend file is touched by this branch.
Layers exercised: UI (assistant_panel_test.dart — Send only while idle, Stop only while running, one CancelAssistantRunAction per press, button dead afterwards, Send back on a cancelled completion; bounded pumps only) and app state/agent layer (agent_cancel_test.dart, 11 tests — reducer, epics, CancellableTool, ApprovalGate, and full scripted runs against a fake model API and a fake DoujinRepository). No REST, use-case or persistence layer was exercised, and nothing was run in the real app against a live server — the server side is reached only through fakes.
Mutation probes were run during implementation and again after the review fixes, each applied to the committed tree, the relevant suite run, then git checkout -- <file>; every one went red: complete()'s error: _cancelled ? null : error mutated to error: error and, separately, to error: null (both directions pinned by the new failing-completion test); cancelRun() reverted to cancelAll() instead of close(); ApprovalGate.request losing its _closed short-circuit; close() losing _closed = true; close() losing cancelAll(); request() returning true instead of false while closed (red on tagUpdates, so the direction is pinned, not just the presence); _runTurn losing approvalGate.reopen(); _mirrorEvent's ToolStartEvent losing if (_cancelled) break;; isCancelling => false; the cancelRun guard reduced to each half alone; _runTurn's finally losing _cancelled = false;; CancellableTool.execute and .parseParameters losing their short-circuits; _reflectEpic losing && !a.cancelled; _cancelEpic unregistered; all four reducer isCancelling lines (Send, Clear, Completed, idle guard), each now reached by a fixture that starts from isCancelling: true / running rather than from the default AppState; the panel's isCancelling ? null : made unconditional and its if (idle) inverted. Regression sets alongside: agent_write_tools, agent_tools, assistant_panel, assistant_agent, entity_write_epics.
Confirmed by reading only, not by execution: that the vendored Agent exposes no cancellation token (hence the one-more-completion limit), and the doc changes themselves (ADR 0024 amendment, docs/BACKLOG.md, docs/DEVELOPMENT.md, README wording).
Known collision, not a defect: feat/the-assistant-can-write-chapters-and-variants must amend the same "Deliberately deferred" paragraph in ADR 0024 (variant/page/upload writes) and touches the same README assistant bullet. The hunks here are confined to the cancellation clause plus an appended amendment section; whoever merges second should re-read the amendment rather than resolving the hunk mechanically.

Follow-ups

docs/BACKLOG.md (Mid-run cancel anchor): a true abort of the in-flight model request needs a cancel token threaded through Agent and OpenRouterClient — an openrouter_dart PR plus a submodule re-pin; until then a cancel costs at least one more completion, and up to maxToolRounds if the model keeps calling tools.
docs/BACKLOG.md (same anchor): a cancelled run still runs the payload-too-large recovery — it detaches images and retries once instead of short-circuiting; short-circuiting it needs its own fixture.

🤖 Generated with Claude Code

## What's in - While the assistant is working, the panel's Send button becomes a Stop button (tooltip: "Stop — the step in flight finishes first"); pressing it posts a transcript line saying so and then goes dead, and the run ends quietly — `AssistantRunCompletedAction(cancelled: true)` with no error, so no error banner appears, the reflection epic skips the turn, and the next message runs its tools normally again. - `AgentService.cancelRun()` sets a per-run flag and `buildTools` wraps every tool in a new `CancellableTool`, so from the next tool call on nothing executes: the tool answers a short "the user CANCELLED this run" text instead of touching the repository, the web or the approval gate. No individual tool file changed, so a future tool inherits the behaviour. - The approval gate is **closed** for the rest of a cancelled run (`ApprovalGate.close()` / `reopen()`): every card still pending is denied, and a write tool that only reaches the gate *after* the cancel (e.g. `entity_write` awaiting `listTags()` first) gets an immediate refusal instead of raising a live card the user can no longer stop. The gate reopens when the next run wires `onRequest`. - Tool-activity chips are suppressed for short-circuited calls, so the transcript does not show fresh tool work under the "nothing more is changed" notice (a chip started *before* the cancel still completes). - **ADR amendment**: ADR 0024 listed mid-run cancellation under "Deliberately deferred (not in v1)"; this PR carries an amendment (2026-09-19) superseding that deferral and stating the honest limit — the model round already in flight cannot be aborted (the vendored `Agent` takes no cancel token), so a cancel costs at least one more completion, and up to `maxToolRounds` of them if the model keeps calling tools; nothing app-side ends the loop, only the tools stop working. - No new endpoint, no migration, no new tunable; backend untouched. ## Verification - Executed exactly `scripts/verify.sh app` from the branch worktree at 099f674, fresh: **passed** — `All tests passed!` (1131 app tests) followed by `== verify OK (app) ==`. `scripts/verify.sh backend` was not run: no backend file is touched by this branch. - Layers exercised: **UI** (`assistant_panel_test.dart` — Send only while idle, Stop only while running, one `CancelAssistantRunAction` per press, button dead afterwards, Send back on a cancelled completion; bounded pumps only) and **app state/agent layer** (`agent_cancel_test.dart`, 11 tests — reducer, epics, `CancellableTool`, `ApprovalGate`, and full scripted runs against a fake model API and a fake `DoujinRepository`). **No REST, use-case or persistence layer was exercised**, and nothing was run in the real app against a live server — the server side is reached only through fakes. - Mutation probes were run during implementation and again after the review fixes, each applied to the committed tree, the relevant suite run, then `git checkout -- <file>`; every one went **red**: `complete()`'s `error: _cancelled ? null : error` mutated to `error: error` **and**, separately, to `error: null` (both directions pinned by the new failing-completion test); `cancelRun()` reverted to `cancelAll()` instead of `close()`; `ApprovalGate.request` losing its `_closed` short-circuit; `close()` losing `_closed = true`; `close()` losing `cancelAll()`; `request()` returning `true` instead of `false` while closed (red on `tagUpdates`, so the direction is pinned, not just the presence); `_runTurn` losing `approvalGate.reopen()`; `_mirrorEvent`'s `ToolStartEvent` losing `if (_cancelled) break;`; `isCancelling => false`; the `cancelRun` guard reduced to each half alone; `_runTurn`'s finally losing `_cancelled = false;`; `CancellableTool.execute` and `.parseParameters` losing their short-circuits; `_reflectEpic` losing `&& !a.cancelled`; `_cancelEpic` unregistered; all four reducer `isCancelling` lines (Send, Clear, Completed, idle guard), each now reached by a fixture that starts from `isCancelling: true` / running rather than from the default `AppState`; the panel's `isCancelling ? null :` made unconditional and its `if (idle)` inverted. Regression sets alongside: agent_write_tools, agent_tools, assistant_panel, assistant_agent, entity_write_epics. - Confirmed by **reading only**, not by execution: that the vendored `Agent` exposes no cancellation token (hence the one-more-completion limit), and the doc changes themselves (ADR 0024 amendment, `docs/BACKLOG.md`, `docs/DEVELOPMENT.md`, README wording). - Known collision, not a defect: `feat/the-assistant-can-write-chapters-and-variants` must amend the same "Deliberately deferred" paragraph in ADR 0024 (variant/page/upload writes) and touches the same README assistant bullet. The hunks here are confined to the cancellation clause plus an appended amendment section; whoever merges second should re-read the amendment rather than resolving the hunk mechanically. ## Follow-ups - docs/BACKLOG.md (Mid-run cancel anchor): a true abort of the in-flight model request needs a cancel token threaded through `Agent` and `OpenRouterClient` — an openrouter_dart PR plus a submodule re-pin; until then a cancel costs at least one more completion, and up to `maxToolRounds` if the model keeps calling tools. - docs/BACKLOG.md (same anchor): a cancelled run still runs the payload-too-large recovery — it detaches images and retries once instead of short-circuiting; short-circuiting it needs its own fixture. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 2 commits

2026-09-19 11:49:14 +02:00

feat: an assistant run can be stopped mid-way f3bafc405e

Stop replaces Send while a run is in flight. Cancelling short-circuits every remaining tool (CancellableTool wraps the service's whole tool list), refuses every pending approval so its write never reaches the repository, and completes the run as cancelled rather than as an error - no error banner and no reflection pass. The model round already in flight still finishes: the vendored Agent takes no cancel token, which ADR 0024's amendment and the backlog record.

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

fix: a cancelled run shuts the approval gate and never reports an error

Flutter CI / analyze-and-test (pull_request) Failing after 24m4s

Details

099f674867

Review follow-ups: close the ApprovalGate on cancel (reopened at the next run) so a write tool that was mid-read when Stop was pressed cannot raise a card nobody can answer; suppress tool chips for short-circuited calls; pin the cancelled-failure arm, the isCancelling flag mid-run and the reducer's two cancelling resets with directional tests; record the 413-on-cancel deferral and the maxToolRounds bound in the backlog and ADR 0024.

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

jibril commented

2026-09-19 12:13:38 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! Cancellation at the one boundary the app actually owns — the tool call — with the honest limit written down in the ADR instead of promised away in a tooltip. You even closed the approval gate so a write tool that was mid-read when Stop was pressed gets refused at the gate instead of raising a card nobody can stop anymore. That window — cancel lands between entity_write's read and its ask — is exactly where a lazier design would have left a live card that executes a write after the cancel. I checked. It's closed~ ♡

Verdict: ✅ Looks good to me~

I read the full diff and the full current contents of every changed file, cross-checked the vendored Agent's tool-call sequence (ToolStartEvent → parseParameters → execute → ToolEndEvent, always paired, errors captured as results), and I could not find a single branch that produces wrong runtime behavior:

CancellableTool delegates all four Tool members plus the derived toApiJson — nothing leaks around the decorator, and wrapping at the buildTools seam means a future tool cannot be added unwrapped. That's the right abstraction, fufu~
_cancelled is monotonic within a run, set only by cancelRun, cleared in the finally after complete() reads it — the single-read trust in CancellableTool is earned, not assumed.
complete(error: _cancelled ? null : error) covers every exit path (success, failure, thrown, finally-fallback) — a cancelled run can never raise the red banner, and the suppression is cancel-specific (an ordinary failure still reports; your own test pins both directions).
Chip suppression is exactly right against the real event flow: suppressed starts emit no chip, their ToolEndEvents update nothing (reducer no-ops on unknown ids), and a chip started before the cancel still completes.
The reducer's four isCancelling arms (Send, Clear, Completed, idle guard) each have their own fixture starting from the stuck state — the belt-and-suspenders a stale flag deserves.
freezed regen is complete and consistent (equality/hashCode/toString/copyWith/when/maybeWhen/whenOrNull all carry the new field).

And I don't take "mutation probes were run" on faith — I ran my own against the committed tree: ApprovalGate.request losing its _closed short-circuit → exactly the after-the-cancel gate test red; complete() losing the error suppression → the failing-completion test red; the finally losing _cancelled = false → three next-run-leak tests red. All restored, tree pristine. Your tests have teeth~ ♪

💡 Little ideas (non-blocking)~

assistant entry id 'cancelled-${_history.length}' (agent_service.dart, cancelRun) — after heavy compaction shrinks the history, a later cancelled run could land on the same length and reuse the id. Nothing matches on assistant-message ids today so it's harmless, but a monotonic counter (like _captionCounter) would be future-proof if transcript entries ever gain update-by-id behavior. Same pattern exists in the assistant-/interim- siblings, so this is purely a "while you're in there someday" note.

✅ What I liked~

The ADR amendment states the cost plainly — at least one more completion, up to maxToolRounds — instead of pretending an abort happened. Honest limits in documentation are rarer than they should be, fufu~
The 413-recovery-during-cancel gap is disclosed in BACKLOG with the reason it doesn't have a fixture yet rather than silently ignored.
The scripted-API test harness with the onPost hook is lovely — cancelling at POST 2 is precisely "after the first round, before the second", and asserting repo.tagPages == 1 proves the second tool never touched the repository.
_cancelEpic mirrors _clearEpic's shape exactly; consistency with siblings makes the middleware readable at a glance.

Automated review by Jibril · 2026-09-19
CI/CD: absent for head SHA 099f674 (no coverage bot comment yet) · Local checks: flutter analyze 0 issues · agent_cancel + assistant_panel 20/20 · regression suites (agent_write_tools, agent_tools, assistant_agent, entity_write_epics) 106/106 · 3/3 independent mutation probes red

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! Cancellation at the *one boundary the app actually owns* — the tool call — with the honest limit written down in the ADR instead of promised away in a tooltip. You even closed the approval gate so a write tool that was mid-read when Stop was pressed gets refused at the gate instead of raising a card nobody can stop anymore. That window — cancel lands *between* `entity_write`'s read and its ask — is exactly where a lazier design would have left a live card that executes a write after the cancel. I checked. It's closed~ ♡ ### Verdict: ✅ Looks good to me~ I read the full diff and the full current contents of every changed file, cross-checked the vendored `Agent`'s tool-call sequence (`ToolStartEvent` → `parseParameters` → `execute` → `ToolEndEvent`, always paired, errors captured as results), and I could not find a single branch that produces wrong runtime behavior: - `CancellableTool` delegates all four `Tool` members plus the derived `toApiJson` — nothing leaks around the decorator, and wrapping at the `buildTools` seam means a future tool *cannot* be added unwrapped. That's the right abstraction, fufu~ - `_cancelled` is monotonic within a run, set only by `cancelRun`, cleared in the finally *after* `complete()` reads it — the single-read trust in `CancellableTool` is earned, not assumed. - `complete(error: _cancelled ? null : error)` covers every exit path (success, failure, thrown, finally-fallback) — a cancelled run can never raise the red banner, and the suppression is cancel-specific (an ordinary failure still reports; your own test pins both directions). - Chip suppression is exactly right against the real event flow: suppressed starts emit no chip, their `ToolEndEvent`s update nothing (reducer no-ops on unknown ids), and a chip started *before* the cancel still completes. - The reducer's four `isCancelling` arms (Send, Clear, Completed, idle guard) each have their own fixture starting from the stuck state — the belt-and-suspenders a stale flag deserves. - freezed regen is complete and consistent (equality/hashCode/toString/copyWith/when/maybeWhen/whenOrNull all carry the new field). And I don't take "mutation probes were run" on faith — I ran my own against the committed tree: `ApprovalGate.request` losing its `_closed` short-circuit → exactly the after-the-cancel gate test red; `complete()` losing the error suppression → the failing-completion test red; the finally losing `_cancelled = false` → three next-run-leak tests red. All restored, tree pristine. Your tests have teeth~ ♪ #### 💡 Little ideas (non-blocking)~ 1. **assistant entry id `'cancelled-${_history.length}'`** (agent_service.dart, `cancelRun`) — after heavy compaction shrinks the history, a later cancelled run could land on the same length and reuse the id. Nothing matches on assistant-message ids today so it's harmless, but a monotonic counter (like `_captionCounter`) would be future-proof if transcript entries ever gain update-by-id behavior. Same pattern exists in the `assistant-`/`interim-` siblings, so this is purely a "while you're in there someday" note. #### ✅ What I liked~ - The ADR amendment states the cost plainly — at least one more completion, up to `maxToolRounds` — instead of pretending an abort happened. Honest limits in documentation are rarer than they should be, fufu~ - The 413-recovery-during-cancel gap is *disclosed* in BACKLOG with the reason it doesn't have a fixture yet rather than silently ignored. - The scripted-API test harness with the `onPost` hook is lovely — cancelling at POST 2 is precisely "after the first round, before the second", and asserting `repo.tagPages == 1` proves the second tool never touched the repository. - `_cancelEpic` mirrors `_clearEpic`'s shape exactly; consistency with siblings makes the middleware readable at a glance. --- *Automated review by Jibril · 2026-09-19* *CI/CD: absent for head SHA 099f674 (no coverage bot comment yet) · Local checks: flutter analyze 0 issues · agent_cancel + assistant_panel 20/20 · regression suites (agent_write_tools, agent_tools, assistant_agent, entity_write_epics) 106/106 · 3/3 independent mutation probes red*