feat(cli): attach remote-control to current TUI#3931
Conversation
3bac087 to
fc1692b
Compare
11df467 to
6ce88d6
Compare
6ce88d6 to
caea747
Compare
fc1692b to
593cf8d
Compare
3c828e9 to
214c1ed
Compare
593cf8d to
cf5f16e
Compare
214c1ed to
f98ee57
Compare
cf5f16e to
8b62124
Compare
f98ee57 to
f37a27c
Compare
wenshao
left a comment
There was a problem hiding this comment.
Findings on lines outside the PR diff (could not be posted inline):
[Critical] config.test.ts:1672,1677,1690,1704 — tsc type errors: 'mcpServers' is possibly 'undefined' (TS18048) and Object is possibly 'undefined' (TS2532). Add optional chaining or non-null assertions.
[Critical] RemoteControlServer.ts:455-466 — Optimistic command/ack. set_model, set_permission_mode, and interrupt handlers send command/ack immediately after writing to the input file, before the TUI processes the command. If the TUI rejects the change, the remote client already believes it succeeded. Move ack emission to the TUI side after successful execution.
[Critical] TuiRemoteBridge.ts — New 215-line file has no test file. Core orchestrator (bridge/watcher/registry/server coordination) has zero test coverage for start/shutdown/error paths.
[Critical] TuiRemoteControlManager.ts:166-244 — Manager class start/stop/getStatus/shutdown not tested. Only helper classes are covered.
[Suggestion] config.test.ts — Missing test for --remote-control + --input-format stream-json mutual exclusion (config.ts line 657).
[Suggestion] remoteControlCommand.test.ts — Missing error path tests for parseRemoteControlArgs (--help, unknown actions, unknown options, missing values).
[Suggestion] AppContainer.tsx:1021-1151 — setControlHandler logic (interrupt, model switch, permission mode switch, error handling) has no test coverage.
[Suggestion] TuiSessionRegistry.ts — handleOutputMessage: generic system messages incorrectly set state to 'working' with no recovery path. createSession always sets 'idle' regardless of prior state.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| async shutdown(): Promise<void> { | ||
| try { | ||
| await this.dualOutputBridge.shutdown(); |
There was a problem hiding this comment.
[Critical] shutdown() may skip server.stop() if earlier steps throw. The single try/catch short-circuits: if dualOutputBridge.shutdown() or registry.checkForOutput() throws, server.stop() is never called, leaving the HTTP server running with a port leak.
| await this.dualOutputBridge.shutdown(); | |
| async shutdown(): Promise<void> { | |
| try { await this.dualOutputBridge.shutdown(); } catch (e) { | |
| debugLogger.debug('TUI remote-control dual-output shutdown error:', e); | |
| } | |
| try { await this.registry.checkForOutput(); } catch (e) { | |
| debugLogger.debug('TUI remote-control output check error:', e); | |
| } | |
| try { await this.server.stop(); } catch (e) { | |
| debugLogger.debug('TUI remote-control server stop error:', e); | |
| } | |
| this.remoteInputWatcher.shutdown(); | |
| this.registry.shutdown(); | |
| rmSync(this.tmpDir, { recursive: true, force: true }); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.append('error', { message: error.message }); | ||
| this.reading = false; | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
[Critical] pendingOutput is not cleared on output file truncation. When currentSize < this.bytesRead, bytesRead is reset to 0 but pendingOutput retains stale data. Compare with RemoteInputWatcher which correctly clears pendingInput in the same scenario. A stale partial line prepended to re-read content produces malformed JSON.
| }); | |
| if (currentSize < this.bytesRead) { | |
| this.bytesRead = 0; | |
| this.pendingOutput = ''; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| version?: string; | ||
| } | ||
|
|
||
| export class CompositeDualOutputBridge implements DualOutputBridgeLike { |
There was a problem hiding this comment.
[Suggestion] Dead code: CompositeDualOutputBridge and CompositeRemoteInputController are defined and exported but never used anywhere in the codebase. The Mutable* variants in TuiRemoteControlManager.ts provide the same fan-out capability with dynamic add/remove. Consider removing these unused classes.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| export class TuiRemoteControlManager { | ||
| private bridge: TuiRemoteBridge | null = null; |
There was a problem hiding this comment.
[Suggestion] TOCTOU race in start(): the guard if (this.bridge && this.info) is checked before await bridge.start(). Two concurrent calls could both pass the guard, creating duplicate servers. Add a double-check after the await.
| private bridge: TuiRemoteBridge | null = null; | |
| const info = await bridge.start(); | |
| if (this.bridge) { | |
| await bridge.shutdown(); | |
| return { info: this.info!, alreadyStarted: true }; | |
| } | |
| this.bridge = bridge; | |
| this.info = info; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.writeInputCommand({ type: 'interrupt', request_id: requestId }); | ||
| this.setState('interrupted'); | ||
| return requestId; | ||
| } |
There was a problem hiding this comment.
[Suggestion] permissionMode is not validated in session/create path. RemoteControlServer.handleSetPermissionMode correctly validates against ['default', 'plan', 'auto-edit', 'yolo'], but createSession passes the raw value through. Add the same allow-list check for defense in depth.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| getContextUsage(sessionId: string, _showDetails: boolean = false): string { | ||
| this.assertSession(sessionId); | ||
| const requestId = randomUUID(); | ||
| this.append('control/response', { |
There was a problem hiding this comment.
[Suggestion] getContextUsage() returns normally (triggering command/ack) but simultaneously appends a control/response error event. This sends conflicting signals to the client. Either throw an error so the caller can route it, or remove the command/ack and rely solely on control/response.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| : `Failed to parse TUI output line: ${trimmed}`, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] lastError is never populated in setState(). All 9 call sites pass only the state argument; the lastError parameter is never used. Parse result event subtypes (e.g., subtype === 'error') and pass error messages so remote clients see error context in session/state snapshots.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| controller.setSubmitFn(this.submitFn); | ||
| } | ||
| if (this.confirmationHandler) { | ||
| controller.setConfirmationHandler(this.confirmationHandler); |
There was a problem hiding this comment.
[Suggestion] removeController cleanup clears confirmationHandler and controlHandler but not submitFn. If a removed controller is later reused, it retains a stale submitFn closure. Add controller.setSubmitFn(() => false) for symmetry.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| private readonly options: { | ||
| version?: string; | ||
| dualOutput: MutableDualOutputBridge; | ||
| remoteInput: MutableRemoteInputController; |
There was a problem hiding this comment.
[Suggestion] Temp directory leak on TuiRemoteBridge constructor failure. new TuiRemoteBridge(...) is called before the try block; if mkdtempSync creates the dir but the constructor subsequently throws (e.g., assertHostAllowed), the temp dir is never cleaned up. Move the constructor call inside the try block.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
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. |
Summary
This is PR 3 of the 3-PR stacked remote-control implementation. It is stacked on #3930, which is stacked on #3929.
Stack split:
qwen remote-controlworker server and mobile/browser protocol.qwen --remote-controland/remote-controlattach the live Ink TUI to the same remote-control server without spawning a child session.This PR reuses the existing dual-output and remote-input primitives for current-session control. It adds
TuiRemoteBridge,TuiRemoteControlManager, a single-session TUI registry, dynamic dual-output/input providers, composite dual-output/input contexts, remote control command handling in the TUI, CLI attach flags,/remote-controlslash command support, LAN URL printing for attach mode, and partial-line buffering for remote-input/dual-output JSONL.Mobile/current TUI behavior
/remote-control,/remote-control start,/remote-control status, and/remote-control stopare available inside the TUI./remote-control --allow-lanandqwen --remote-control --remote-control-allow-lanbind to0.0.0.0when no explicit host is provided, so phones on the same trusted LAN can connect.control_request/control_response; either local TUI or remote UI can resolve first.Validation
cd packages/cli && npm run typecheckcd packages/cli && npx vitest run src/config/config.test.ts src/remoteControl/EventLog.test.ts src/remoteControl/PairingManager.test.ts src/remoteControl/RemoteSessionRunner.test.ts src/remoteControl/RemoteControlServer.test.ts src/remoteControl/TuiSessionRegistry.test.ts src/remoteControl/TuiRemoteControlManager.test.ts src/remoteInput/RemoteInputWatcher.test.ts src/ui/commands/remoteControlCommand.test.ts src/services/BuiltinCommandLoader.test.ts src/nonInteractive/control/ControlDispatcher.test.ts src/nonInteractive/session.test.tscd packages/cli && npx eslint src/gemini.tsx src/remoteControl/TuiRemoteControlManager.ts src/remoteControl/TuiRemoteControlManager.test.ts src/remoteControl/TuiSessionRegistry.ts src/remoteControl/index.ts src/services/BuiltinCommandLoader.ts src/ui/AppContainer.tsx src/ui/commands/remoteControlCommand.ts src/ui/commands/remoteControlCommand.test.ts src/ui/commands/types.ts src/ui/hooks/slashCommandProcessor.ts --max-warnings 0npm run buildgit diff --check feat/remote-control-worker...feat/remote-control-tui-attach