fix: mark interrupted tool calls as cancelled, not failed - #35
Conversation
#completeActiveTools() was closing orphaned tool calls with
{ phase: "completed", success: false } when a turn ended mid-execution.
The web UI's MessageRow rendered that as an "edit failed — no error
message" banner — a misleading error for what was a user-initiated
interruption.
Switch to { phase: "cancelled", reason: "interrupted" }, consistent with
the restart-recovery path in resume-reconcile.ts which already used this
pattern. The web UI's PhaseBadge and ToolStateBody already handle the
cancelled phase correctly.
Adds tool-interrupt-state.test.ts (17 tests) verifying the invariant
and including a regression test that the old completed/false state would
have triggered the error banner.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe daemon now marks interrupted open tool calls as ChangesInterrupted tool terminal state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 58.34% 59.69% +1.35%
==========================================
Files 46 47 +1
Lines 6943 7255 +312
==========================================
+ Hits 4051 4331 +280
- Misses 2892 2924 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tests/tool-interrupt-state.test.ts (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment overstates the banner condition.
The doc says the banner shows when
phase === "completed" && success === false && no output, but neither the mirroredtriggersErrorBannernorMessageRow.tsxgates on output —outputis only used for the displayed text, not the predicate. Drop the&& no outputto avoid implying a third condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/tool-interrupt-state.test.ts` at line 42, The test doc comment overstates the error banner predicate by adding a no-output requirement that is not used anywhere else. Update the comment in tool-interrupt-state.test.ts so it matches the actual logic in triggersErrorBanner and MessageRow.tsx: the banner appears when phase is "completed" and success is false, without mentioning output as a condition.
36-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTests mirror the logic instead of exercising it.
interruptToolandtriggersErrorBannerre-implement the state transition from#completeActiveToolsand the banner predicate fromMessageRow.tsx. A regression in the actualsession.ts(e.g. reverting tocompleted/false) would leave these tests green, so the "invariant" they protect isn't actually anchored to production code. Consider importing/extracting the real transition (a small exported helper used by both#completeActiveToolsand the test) so the test fails if the daemon's behavior drifts. ThereconcileResumedMessageblock below is the right pattern — it imports and runs the real function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/tool-interrupt-state.test.ts` around lines 36 - 50, The test helpers `interruptTool` and `triggersErrorBanner` are duplicating production logic instead of validating it, so changes in `session.ts` or `MessageRow.tsx` could slip through unnoticed. Refactor the state transition and banner predicate into a small exported helper used by `#completeActiveTools` and `MessageRow.tsx`, then update `tool-interrupt-state.test.ts` to import and exercise that real helper instead of re-implementing the behavior; follow the same pattern already used by `reconcileResumedMessage`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tests/tool-interrupt-state.test.ts`:
- Line 42: The test doc comment overstates the error banner predicate by adding
a no-output requirement that is not used anywhere else. Update the comment in
tool-interrupt-state.test.ts so it matches the actual logic in
triggersErrorBanner and MessageRow.tsx: the banner appears when phase is
"completed" and success is false, without mentioning output as a condition.
- Around line 36-50: The test helpers `interruptTool` and `triggersErrorBanner`
are duplicating production logic instead of validating it, so changes in
`session.ts` or `MessageRow.tsx` could slip through unnoticed. Refactor the
state transition and banner predicate into a small exported helper used by
`#completeActiveTools` and `MessageRow.tsx`, then update
`tool-interrupt-state.test.ts` to import and exercise that real helper instead
of re-implementing the behavior; follow the same pattern already used by
`reconcileResumedMessage`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dd7d611-373c-4e84-bef2-98508eedd961
📒 Files selected for processing (2)
src/daemon/session.tssrc/tests/tool-interrupt-state.test.ts
#completeActiveTools() loop body is unreachable without a live SDK turn, giving the patch 0% coverage. Extract the per-tool logic into a TypeScript-private (not JS `#` private) _applyInterruptedStateToTool() method so tests can exercise it directly via `as unknown as SessionInternal` without mocking the SDK. Adds four new Session-level tests in tool-interrupt-state.test.ts that call the extracted method with a real Session, seeded scrollback, and captured broadcast — covering the actual changed code in session.ts: - delta broadcast carries cancelled/interrupted state - scrollback replay reflects the updated state after the call - no-op when msgId not in scrollback (no crash) - broadcast state does not trigger the web UI error banner Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tests/tool-interrupt-state.test.ts`:
- Around line 303-325: The scrollback test is currently relying on
restoreScrollback to pre-convert the tool call state, which hides the behavior
being tested. Update this test (and the one below) to seed the stored message in
the executing phase directly in the session scrollback, then call
_applyInterruptedStateToTool on SessionInternal and verify the replayed message
changes to cancelled/interrupted. Use makeToolCallMessage, restoreScrollback,
and _applyInterruptedStateToTool to locate the relevant setup and keep the
assertion focused on the scrollback replay result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74482546-a684-4596-97b2-d403d07905e7
📒 Files selected for processing (2)
src/daemon/session.tssrc/tests/tool-interrupt-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/daemon/session.ts
| it("updates the scrollback entry so replay shows the cancelled state", () => { | ||
| const session = makeSession(); | ||
| const msgId = randomUUID(); | ||
| const toolMsg = makeToolCallMessage({ phase: "executing" }, msgId); | ||
| (toolMsg as SessionMessage & { sessionId: string }).sessionId = session.id; | ||
|
|
||
| session.restoreScrollback([toolMsg]); | ||
|
|
||
| (session as unknown as SessionInternal)._applyInterruptedStateToTool(msgId); | ||
|
|
||
| // Re-attach to trigger a scrollback replay — the replayed message must | ||
| // show the updated (cancelled) state, not the original (executing) state. | ||
| const { client, received } = makeClient("replay-client"); | ||
| session.attach(client); | ||
|
|
||
| const replay = received.find((m) => m.type === "scrollback.replay") as | ||
| | { messages: SessionMessage[] } | ||
| | undefined; | ||
| expect(replay).toBeDefined(); | ||
| const updated = replay!.messages.find((m) => m.messageId === msgId); | ||
| expect(updated?.tool?.state.phase).toBe("cancelled"); | ||
| expect((updated?.tool?.state as ToolCancelledState).reason).toBe("interrupted"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'resume-reconcile.ts' --exec cat -n {}Repository: saucam/codeoid
Length of output: 2577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test and the session implementation around the relevant methods.
ast-grep outline src/tests/tool-interrupt-state.test.ts --view expanded || true
printf '\n--- session.ts outline ---\n'
ast-grep outline src/session.ts --view expanded || true
printf '\n--- relevant snippets ---\n'
rg -n "restoreScrollback|_applyInterruptedStateToTool|replay" src/tests/tool-interrupt-state.test.ts src/session.ts src/**/*.tsRepository: saucam/codeoid
Length of output: 11138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/tests/tool-interrupt-state.test.ts --view expanded || true
printf '\n--- session.ts outline ---\n'
ast-grep outline src/session.ts --view expanded || true
printf '\n--- relevant snippets ---\n'
rg -n "restoreScrollback|_applyInterruptedStateToTool|replay" src/tests/tool-interrupt-state.test.ts src/session.ts src/**/*.tsRepository: saucam/codeoid
Length of output: 11138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/tests/tool-interrupt-state.test.ts --view expanded || true
printf '\n--- session.ts outline ---\n'
ast-grep outline src/session.ts --view expanded || true
printf '\n--- relevant snippets ---\n'
rg -n "restoreScrollback|_applyInterruptedStateToTool|replay" src/tests/tool-interrupt-state.test.ts src/session.ts src/**/*.tsRepository: saucam/codeoid
Length of output: 11138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'session.ts' --exec sh -c 'printf "\nFILE: %s\n" "$1"; cat -n "$1" | sed -n "1,260p"' sh {}Repository: saucam/codeoid
Length of output: 12680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact restoreScrollback and _applyInterruptedStateToTool implementations.
sed -n '1440,1495p' src/daemon/session.ts
printf '\n---\n'
sed -n '2528,2565p' src/daemon/session.ts
printf '\n--- test excerpt ---\n'
sed -n '296,360p' src/tests/tool-interrupt-state.test.tsRepository: saucam/codeoid
Length of output: 6382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact restoreScrollback and _applyInterruptedStateToTool implementations.
sed -n '1440,1495p' src/daemon/session.ts
printf '\n---\n'
sed -n '2528,2565p' src/daemon/session.ts
printf '\n--- test excerpt ---\n'
sed -n '296,360p' src/tests/tool-interrupt-state.test.tsRepository: saucam/codeoid
Length of output: 6382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact restoreScrollback and _applyInterruptedStateToTool implementations.
sed -n '1440,1495p' src/daemon/session.ts
printf '\n---\n'
sed -n '2528,2565p' src/daemon/session.ts
printf '\n--- test excerpt ---\n'
sed -n '296,360p' src/tests/tool-interrupt-state.test.tsRepository: saucam/codeoid
Length of output: 6382
Seed the scrollback entry without resume reconciliation restoreScrollback already rewrites executing tool calls to cancelled/interrupted, so this test can pass before _applyInterruptedStateToTool runs. Seed the stored message directly in executing so the scrollback update is exercised; the same applies to the test below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/tool-interrupt-state.test.ts` around lines 303 - 325, The
scrollback test is currently relying on restoreScrollback to pre-convert the
tool call state, which hides the behavior being tested. Update this test (and
the one below) to seed the stored message in the executing phase directly in the
session scrollback, then call _applyInterruptedStateToTool on SessionInternal
and verify the replayed message changes to cancelled/interrupted. Use
makeToolCallMessage, restoreScrollback, and _applyInterruptedStateToTool to
locate the relevant setup and keep the assertion focused on the scrollback
replay result.
Patch release rolling up six fixes/doc changes since v0.1.1: - file explorer switches workdir on session change (#37) - interrupted tool calls marked cancelled, not failed (#35) - writeBatch guarded against in-flight streams; no TUI double-print (#33) - TUI reconnects + re-mints token on JWT expiry (#34) - Telegram /attach disconnects old session before switching (#29) - richer README badges (#27) Bumps package.json to 0.1.2 so the release.yml tag check passes, and backfills the previously-undocumented 0.1.1 entry in the CHANGELOG. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
#completeActiveTools()insession.tsclosed orphaned tool calls with{ phase: "completed", success: false }whenever a turn ended while a tool was still executing (user hit Stop, error, or turn end beforetool_resultarrived)MessageRow.tsxrenderscompleted && success === falseas an "edit failed — no error message" error banner — a misleading error for what was actually a user-initiated interruption{ phase: "cancelled", reason: "interrupted" }instead, which is already handled correctly byPhaseBadge(shows a danger badge) andToolStateBody("cancelled — interrupted")Why this is the right state
resume-reconcile.tsalready usescancelled/interruptedfor the identical scenario on daemon restart. TheToolCancelledStatetype is defined in the protocol for exactly this purpose. This change makes turn-end interruption consistent with restart recovery.Test plan
src/tests/tool-interrupt-state.test.ts— 17 new tests:cancelled/interruptedstate transition mirrors what#completeActiveTools()now producescompleted/falsestate would trigger the error banner (the old bug)cancelledstate does not trigger the error bannerreconcileResumedMessage— both paths now produce identical terminal state🤖 Generated with Claude Code
Summary by CodeRabbit