feat(cli): add remote-control foundation#3929
Closed
chiga0 wants to merge 2 commits into
Closed
Conversation
This was referenced May 7, 2026
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
c2731ad to
ca5b8e0
Compare
ca5b8e0 to
267c3d2
Compare
This was referenced May 11, 2026
9d30917 to
8c15016
Compare
22 tasks
Collaborator
Author
|
Closing for now — deprioritized while the daemon PRs (#3889 et al.) land. The remote-control surface will be reconsidered after the daemon layer stabilizes; reopening this stack would need a rebase on the new daemon foundation anyway. Branch preserved for later resurrection. |
wenshao
requested changes
May 15, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] The PR introduces per-turn AbortController in session.ts and passes it to runNonInteractive() in packages/cli/src/nonInteractiveCli.ts, but runNonInteractive() was not updated to support per-turn abort. When abortController.signal.aborted is detected at lines 719 and 988, it calls handleCancellationError() which triggers process.exit() — killing the entire process. This contradicts the PR's goal of "separating session-lifetime abort from per-turn abort."
The notification drain loop at nonInteractiveCli.ts:835 correctly returns gracefully on abort — this is the pattern that should be applied to the main event loop and final holdback loop for per-turn abort. The test at session.test.ts:294 passes because runNonInteractive is mocked to resolve on abort, hiding the real behavior.
Fix options: Add an isPerTurnAbort parameter to runNonInteractive to control whether abort → exit or abort → return; or use AbortSignal.any() to merge session and turn signals so that only the session-level abort triggers process.exit().
[Suggestion] processPendingWork() loop at line 510 only checks session-level abortController.signal.aborted. After per-turn interrupt, monitor notifications continue being dequeued and processed. Consider whether a brief guard or check on currentTurnAbortController is needed.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.abortController.abort(); | ||
| // Do not create a new AbortController to prevent listener leaks. | ||
| // Subsequent queries will check signal.aborted and fail immediately. | ||
| this.currentTurnAbortController?.abort(); |
Collaborator
There was a problem hiding this comment.
[Critical] this.currentTurnAbortController?.abort() silently no-ops when currentTurnAbortController is null (session idle). Only [Session] Interrupt requested is logged with no indication the abort was ineffective. The client receives a success response with zero effect.
Suggested change
| this.currentTurnAbortController?.abort(); | |
| if (this.currentTurnAbortController) { | |
| this.currentTurnAbortController.abort(); | |
| this.dispatcher?.abortOutgoingRequests('Interrupted'); | |
| } else { | |
| debugLogger.warn('[Session] No active turn to interrupt'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -443,17 +443,23 @@ class Session { | |||
| await this.waitForInitialization(); | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] processUserMessage() awaits waitForInitialization() before creating currentTurnAbortController at line 446. On the first turn, waitForInitialization() yields, creating a window where handleInterrupt() sees currentTurnAbortController === null and discards the interrupt. Move the AbortController creation before the await to close the gap.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| try { | ||
| await runNonInteractive( | ||
| this.config, | ||
| this.settings, |
Collaborator
There was a problem hiding this comment.
[Suggestion] processMonitorNotification() has try/finally with no catch block, while processUserMessage() catches and logs errors. If runNonInteractive throws, the error propagates to processPendingWork's outer catch without the specific context. Add a catch block for consistent error logging:
Suggested change
| this.settings, | |
| } catch (error) { | |
| debugLogger.error('[Session] Monitor notification error:', error); | |
| } finally { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } catch (error) { | ||
| debugLogger.error('[Session] Query execution error:', error); | ||
| } finally { | ||
| if (this.currentTurnAbortController === turnAbortController) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] All errors in processUserMessage() are logged at debugLogger.error level as [Session] Query execution error. After this PR, per-turn cancellations from handleInterrupt() are expected behavior, not errors. This will produce noisy error logs for every normal interrupt. Distinguish cancellation from real failures:
Suggested change
| if (this.currentTurnAbortController === turnAbortController) { | |
| } catch (error) { | |
| if (turnAbortController.signal.aborted) { | |
| debugLogger.debug('[Session] Query cancelled:', (error as Error)?.message); | |
| } else { | |
| debugLogger.error('[Session] Query execution error:', error); | |
| } | |
| } finally { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
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
This is PR 1 of a 3-PR stacked remote-control implementation.
Stack split:
/remote-controlslash command support.This PR intentionally keeps the runtime surface small: it does not start a network server yet. The key behavior change is separating session lifetime abort from current-turn abort, so remote interrupt can cancel one turn without poisoning the whole stream-json session.
The full design is included at
docs/design/remote-control/remote-control-technical-design.md. The doc now also includes the current TUI/remote-controlcommand design, LAN attach behavior, post-implementation audit notes, protocol evolution, durable replay, JSONL partial-line handling, mobile UX, and test strategy.Source-backed design notes
dual-outputas a local sidecar primitive rather than renaming it into remote-control.Validation
cd packages/cli && npm run typecheckcd packages/cli && npx vitest run src/nonInteractive/control/ControlDispatcher.test.ts src/nonInteractive/session.test.tsgit diff --check origin/main...feat/remote-control