fix(cli): wait for detached run terminal state - #2136
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe local-control CLI advances to surface version 2. Detached runs expose ChangesCLI surface version and contracts
Interaction-aware phase projection
Root-session watcher termination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as run watch
participant CliRunService
participant SessionTranscript
participant RootSession
participant ProviderRound
CLI->>CliRunService: start watch for run
CliRunService->>SessionTranscript: retrieve pending assistant messages
ProviderRound-->>CliRunService: provider-round completion or failure
CliRunService->>RootSession: inspect root session state
RootSession-->>CliRunService: idle or error
CliRunService-->>CLI: emit terminal result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/main/cli/runService.test.ts (1)
248-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne pending-watcher assertion, defined twice and too weak. Both sites assert that the watcher promise has not settled after a single microtask. The watcher awaits
emitand the event iterator several times per event, so a regression that terminates one tick later still passes. These are the primary regression tests for continued watching.
test/main/cli/runService.test.ts#L248-L260: makeexpectWatcherPendingrace the watcher promise against a scheduled timer instead of awaiting one microtask.test/main/cli/runService.test.ts#L612-L622: replace the inline copy with a call toexpectWatcherPending.🤖 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 `@test/main/cli/runService.test.ts` around lines 248 - 260, In test/main/cli/runService.test.ts:248-260, update expectWatcherPending to race the watcher promise against a scheduled timer so it verifies the promise remains pending through the event-processing turns, rather than checking after one microtask. In test/main/cli/runService.test.ts:612-622, remove the duplicated inline pending assertion and call expectWatcherPending instead.
🤖 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 `@docs/architecture/local-control-plane/spec.md`:
- Line 218: Update the capability-matrix section heading associated with
CLI_SURFACE_V2 from “V1 Capability Matrix” to “V2 Capability Matrix,” leaving
the surrounding content unchanged.
In `@src/main/cli/runService.ts`:
- Around line 478-481: Update the phaseMessages selection in buildSnapshot to
avoid calling getMessages(runId) for the full transcript during generating runs
with pagination. Use an existing bounded projection, such as the latest
assistant message or bounded recent-assistant query, as the source for runPhase
while preserving page.messages for non-generating or non-paginated cases.
---
Nitpick comments:
In `@test/main/cli/runService.test.ts`:
- Around line 248-260: In test/main/cli/runService.test.ts:248-260, update
expectWatcherPending to race the watcher promise against a scheduled timer so it
verifies the promise remains pending through the event-processing turns, rather
than checking after one microtask. In test/main/cli/runService.test.ts:612-622,
remove the duplicated inline pending assertion and call expectWatcherPending
instead.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9331a156-6dbe-465b-b306-71c4c48427c9
📒 Files selected for processing (18)
docs/architecture/local-control-plane/spec.mddocs/guides/cli.mdsrc/cli/format.tssrc/main/agent/deepchat/runtime/sessionUpdates.tssrc/main/app/composition.tssrc/main/cli/index.tssrc/main/cli/routes.tssrc/main/cli/runService.tssrc/main/cli/server.tssrc/main/cli/surface.tssrc/shared/contracts/localControl.tssrc/shared/contracts/routes/ocr.routes.tssrc/shared/contracts/routes/runs.routes.tstest/main/cli/client.test.tstest/main/cli/inputCapabilityServices.test.tstest/main/cli/runService.test.tstest/main/cli/server.test.tstest/main/cli/surface.test.ts
zerob13
reviewed
Aug 11, 2026
zerob13
left a comment
zerob13
left a comment
Collaborator
There was a problem hiding this comment.
The lifecycle fix and phase contract are coherent and CI is green. One added optimization expands startup risk without evidence that the existing session index is insufficient; details are inline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
deepchat run watchexiting after a single provider round while the detached Agent run is still active or waiting for user interaction.idleorerroras the authoritative run terminal state.chat.stream.completedandchat.stream.failed.running | awaiting_interaction | terminalthroughruns.getand run snapshots.Root Cause
chat.stream.completedandchat.stream.faileddescribe a single provider-stream boundary, not the lifecycle of the complete detached run.A tool loop, permission or question interaction, subagent execution, pending-input handoff, or another provider round may still follow. Treating these events as terminal caused
run watchto return success before the owned root Session actually finished.Compatibility
phaseis a required field in the strict public run snapshot schema. The local-control surface version is therefore bumped from1to2so incompatible clients and servers fail closed.The local-control protocol remains version
1, and the existing/v1/...transport paths are unchanged.Test Coverage
Added regression coverage for:
Validation
Fixes #2113
Summary by CodeRabbit
New Features
Improvements
run watchcontinues through provider stream completion and ends when the root run becomes idle or errors.