Conversation
Generated-by: Codex
There was a problem hiding this comment.
The change adds a private handoff on the existing PTY: Host-scoped control and interaction state, a fenced Unix input queue, a private output collector, and a Desktop card that publishes only explicitly selected observations. I checked the 52-file diff, protocol/IPC and lifecycle paths, and visually inspected all three supplied screenshots. The merge-tree and diff whitespace check are clean; the local test build and six focused suites passed (185 passed, 4 skipped). I did not independently run the real SSH/model journey or a macOS Desktop smoke test.
The current-head test check is failing at the Windows skip inventory. This is a merge blocker even though the focused runtime tests pass. Please update the generated inventory and rerun the full check.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed head 4256bfc43e998cebe17af9327bbf712fcb1aa1c4, including the new terminal feedback/reconnect flow, Host outcome protocol and lifecycle, tool discovery, live-turn replay, focused tests, and the four changed screenshots. The earlier Windows skip-inventory failure is fixed: npm run windows:inventory -- --check now passes (106 declarations). This head is not ready to merge.
[P1] The required test check still fails. scripts/windows-package-source-closure.test.mjs:137-144 derives the Windows-branching source closure and requires exact equality with the recovery workflow's path filter. packages/runtime-host/src/server/runtime-resource-coordinator.ts:305-308 is now reachable in that closure, but .github/workflows/windows-recovery.yml:93-95 omits it. CI run 36214738106 fails in Release contracts at that exact assertion; I reproduced the same failure locally with Node 24. Add this path to the filter and rerun the current-head check. The PR also currently conflicts with main in docs/windows-test-inventory.md, so its merged result needs revalidation.
The current-head CI build and the local Windows inventory check pass. Local focused Host/runtime/UI/desktop tests passed (152 tests); git diff --check passed. The red gate skipped typecheck, affected workspace tests, and Desktop E2E. I did not independently run the macOS SSH/model journey or cross-platform packaging. No schema or migration files changed.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Adds an end-to-end terminal-handoff path: WriteStdin gains a discoverable handoff action (advertised by Bash only when a Desktop human surface is registered), a new runtime.resource.handoff control operation with strict codecs, coordinator-owned fencing of queued agent writes via input epochs, a private PTY display collector that keeps post-handoff output out of the model-visible parser, masked-input card reusing ChatComposer, and computer-use capture fencing while a private surface is visible. The issue (no safe way for an agent to let a human type a password into a live PTY; base WriteStdin/Bash had no such path or guidance) is real, and the layering is right: authority lives in the Runtime Host, input bytes never enter interactions/journals, unknown delivery is wedged rather than replayed. Tests are strong and non-vacuous (epoch-fence stale-write rejection, dedup, controller expiry, in-flight capture drain barrier are all pinned by assertions that fail under revert). Scope is large but mostly load-bearing; the splittable fat is the committed real-provider harness and PR screenshots.
Findings
- [P1] CI
testjob is red on the head SHA —test fail 2m32s(onlywindows_recoverypasses). The PR body claims all suites green locally, so something differs in CI (fast failure suggests typecheck/lint/unit, not the opt-in journey). A +3468-line credential-handling feature must not merge with the required suite failing; identify and fix the failing job first. - [P2] Stale private-surface flag fences all computer-use when
readyfails or the handoff closes mid-mount — apps/desktop/src/main/runtime-host-shell-runs-ipc-main.ts:108-119 setssetPrivateTerminalSurface(sender.id, …, true)and drains captures beforecontrolTerminalHandoff(input); if that call rejects (controller conflict, surface gone, or the 30s readiness deadline racing the card mount) or returnsclosedbecause the child exited betweenlookupandready, the flag is never cleared and the renderer'sfailed()path sends norelease. Until the user closes the tab or clicks Reconnect,hasPrivateTerminalSurface()stays true and every desktop computer-use call throws (apps/desktop/src/main/runtime-host-native-capabilities.ts:222-225) although nothing private is shown. Fail-closed, but over-broad; clear the flag when the server call does not confirmready. - [P2] Input fence depends on undocumented node-pty internals and an unverified non-blocking invariant — packages/runtime/src/pty-process-driver.ts:155-175 reads
(this.pty as IPty & { fd?: number }).fdandwriteSyncs to it, assuming O_NONBLOCK ("A synchronous nonblocking write…", line 163). Nothing verifies that; if the fd is blocking under a backpressured/unresponsive child,writeSyncblocks the Runtime Host event loop synchronously — the 5sAbortSignal.timeoutguards indrainInput(line 140) cannot interrupt a blocked sync write, wedging all sessions on the host. PR verification ran only on macOS. Verify the flag at construction (fall back to unfencedpty.writeotherwise) and ensure Linux CI exercises pty-process-driver.test.ts. - [P3] All server-side handoff failures are swallowed into a generic conflict with no logging — packages/runtime-host/src/server/runtime-resource-coordinator.ts:613-619 maps every error (collector snapshot failure, admission error, genuine bug) to
operation_conflicteven though the operation spec declaresinternal_failure. In a credential path this makes production incidents undiagnosable; log before mapping. - [P3] Committed real-provider harness hard-codes a proprietary endpoint — scripts/terminal-handoff/README.md (+ real-model.mjs, 437 lines) requires
gpt-5.6-terraand a custom base URL; unrunnable for other contributors and will rot. Five PR-review screenshots under docs/images/pr/ are review artifacts, not durable docs. Consider generalizing the harness or keeping it and the images out of tree; the deterministic suites already carry the regression weight.
Verdict
needs-changes — direction and tests are solid, but the failing CI gate plus the stale capture-fence flag and unverified non-blocking writeSync assumption must be resolved before merge.
|
Review follow-up after rebasing onto current main (head c7f5473):
Validation: full build:test; 281 focused terminal/interaction/runtime tests; Windows source-closure and inventory checks; Astryx inventory; renderer architecture; protocol epoch 196 -> 197. Update for the raw-fd lifecycle review (fix 39601af, current head 1ced798 after merging main and refreshing generated inventories):
Validation: @maka/runtime full suite (3665 passed, 14 existing skips); post-merge build:test and 43 focused Runtime/Desktop lifecycle tests; protocol epoch 197 -> 198; real PTY backpressure ordering; node-pty exit lifecycle; deterministic Driver fd-reuse regression. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head and its changes since 4256bfc4. I found no substantiated P0–P3 issue in this pass. The previous Windows source-closure failure is addressed by including runtime-resource-coordinator.ts in windows-recovery.yml; the current-head test, windows_acp, and windows_recovery checks pass. The previous unbounded terminal-handoff retention is now limited to 128 terminal states (runtime-resource-coordinator.ts:705-714), with request-ID lookup replacing the linear scan (:512-516); release, replacement, and drain remove the corresponding records. The existing PTY input fence and unknown-delivery no-replay behavior remain in place. The five supplied screenshots, including the exited-process state, were inspected. The 65-file PR diff is clean and static merge with current main 0fd75408 has no conflict. I did not independently execute the real-model SSH journey, cross-platform packaging, or a >128-handoff eviction test; the last behavior is supported by code inspection rather than a dedicated regression test. No schema or migration change was found.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head 4adb5b1f (a different model lineage from the parallel review). The two previously raised issues, the Windows closure path and the 128-entry retention cap, are fixed. Several properties hold:
- private input doesn't reach the transcript, Interaction answers, logs or other sessions except via the P1 below;
- unknown-delivery input isn't replayed;
- only resumed or closed handoffs are evicted at the cap;
- model writes are fenced by both epoch/phase and
inputOpen; - the workflow change only adds a
pathsfilter, with no trigger or permission change.
P1: private input can be written to an unrelated file or socket that reuses the PTY fd (packages/runtime/src/pty-process-driver.ts:113-125, :160-199).
- Why. The new input queue writes with
writeSync(fd, …)on the raw Unix fd and is gated only onexited/disposed. node-pty closes the fd and disables its ownwriteas soon as it reads EIO, which can be well beforeexitis emitted. The repo's own node-pty patch comment calls out exactly this fd-reuse risk, and this path bypasses that protection. - Failure scenarios.
- The child closes its terminal but hasn't exited yet.
- Password bytes are still queued at the 5 s drain timeout when ssh exits.
- In either case the remaining bytes go to whatever the process opened next on that fd number, such as the session DB, a log or another client's socket.
- Reproduced on Linux. In a PoC, a child closes its stdio but keeps running, and a newly opened file receives the same fd. After the write, the file contains
SECRET-PASSWORD\r. Calling node-pty directly with base behaviour leaves the file empty. - Suggested fix. Stop flushing (and drop the queue) as soon as node-pty reports EIO or its data stream ends, not only on
onExit. Alternatively,dupthe fd for the write path and close it at the same moment node-pty closes its own. - A related regression.
:183-187now treats any non-EAGAIN/EINTR write error as an integrity failure and kills the process, where base only logged and dropped.
P2: after Resume, leaving the card makes the terminal result unshareable (runtime-resource-coordinator.ts:568-573, terminal-handoff-panel.tsx:136-140, workbar-surface.tsx:522-525).
- Why. Switching tabs or collapsing the right panel after Resume unmounts the card, which sends
release. In the resumed phase the Host then deletes the handoff. - Effect. On return the user only sees the "output is private" banner, with no way to view or share the private output, and the model waits indefinitely.
- Evidence. This is derived from the code; it wasn't clicked through in the UI. No test covers it.
P3:
- Protocol replies are dropped for good in private mode (
shell-run-manager.ts:649,:980). A later full-screen program such as vim, run by the model in the same shell, may hang. - One apply failure can affect every session. A throwing handoff
applypoisons the interaction coordinator (new hunk around:1712-1722). ThestopBackgroundTaskit calls isn't caught, so one failed stop affects every session on the Host. - Control isn't bound to the requesting Desktop. Another client of the same owner holding the grant, including a remote owner, can take over while the card is inactive. That's acceptable, but it should be documented.
- Resume admission isn't queued. It's checked before the resource queue, so Resume can still be admitted behind an earlier unknown-delivery input. The current UI prevents this.
Verified:
pty-process-driver2/2 passes.- The coordinator, interaction, shell-run-manager and UI suites pass (168 pass, 4 skipped).
- Two files couldn't load in this environment (a missing
@ai-sdk/provider).
Not verified: renderer tests, E2E, real SSH on macOS, or the EIO-vs-exit ordering on macOS.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; the P1 was reproduced locally and checked against the code, but please verify before acting.
…rminal-handoff # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/app-shell-session-events.ts # docs/astryx-surface-file-inventory.md # packages/runtime-host/src/protocol/index.ts
There was a problem hiding this comment.
Reviewed current head 1ced798. The new fd identity check does not close the private-input leak: on Linux, separate PTY masters can have the same dev/ino/mode/rdev tuple, so a retired descriptor reused by another PTY passes the guard. The earlier Resume → switch card → return → Share failure also remains. I would not merge until both paths are fixed and covered by regressions. The current-head test, windows_acp, and windows_recovery checks pass; a fresh merge-tree against main is clean. I did not run the full suite or a packaged Desktop session locally; I reproduced the PTY identity/receipt mechanism with a Linux openpty probe.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| disposed = true; displayEpoch.current++; clearTimeout(timer); | ||
| if (input.current) input.current.value = ''; | ||
| setPrivateInput(''); setState(undefined); setConnected(false); setConfirmed(false); | ||
| void terminal.handoff!({ ...identity, action: 'release' }).catch(() => {}); |
There was a problem hiding this comment.
P2: This cleanup still sends release whenever the card becomes inactive/unmounts. After Resume, the Host release path discards the resumed handoff (runtime-resource-coordinator.ts:568-573); returning to the card cannot observe or Share the private output (the Host now reports that the original handoff is no longer live). The new commits do not change this path. Please retain the resumed handoff across temporary card switches and test Resume → switch away → return → Share.
There was a problem hiding this comment.
The loss of the live private display is fixed. release now removes only the controller identity; the Host retains a live resumed handoff until the original process exits. Returning can reclaim and observe that same terminal. The Host regression also checks that the retention cap does not evict live resumed terminals.
The latest real Desktop/SSH/model run passed Resume → collapse/expand → switch tabs/return → reload, followed by two more turns in the original shell with its marker and working directory preserved. The input card stays gone after completion.
The interaction has also changed: manual sharing was removed, so there is no longer a Share action to validate. That is not a solution to model result observation: the agent still cannot read private output, and this remains an explicit merge-blocking acceptance gap. The refreshed PR screenshots show the current behavior. Could you recheck the retention fix and whether this simpler interaction is the right direction?
Astro-Han
left a comment
There was a problem hiding this comment.
A second, independent review of head 1ced798e (Claude lineage). It adds to the hqhq1025 review 5334255449 above and does not repeat its findings.
It independently confirms the P1. On Linux, every /dev/ptmx master has the same fstat fields. In our run, two unrelated PTYs both had ino 89, rdev 1282, dev 5 and mode 8630. So the captureUnixFd/ownsUnixFd fingerprint (pty-process-driver.ts:234-261) cannot tell them apart.
A local proof of concept follows the path from EIO to onExit:
- A's PTY hits EIO.
- The Host opens a new PTY for another session.
- The new PTY gets the same fd number.
writePrivatePtyInput(shell-run-manager.ts:687-693) passes the check.
The other shell then receives the secret as keystrokes (B-GOT:[SECRET-PASSWORD]) and echoes it into that session's transcript and model context.
- Suggested fix: in the existing node-pty patch, set a "closed" flag synchronously before the fd is closed, or go back to node-pty's own write stream.
- Suggested test: the regression test should use two real PTYs. The new
pty-process-driver-write-lifecycle.test.tsreuses the fd with a plain file, which can't catch this case.
Additional P3: a normal exit can be recorded as an integrity failure. Between the fd closing and its reuse, fstat returns EBADF. ownsUnixFd then returns false and failWrites (pty-process-driver.ts:172-201) calls handleIntegrityFailure (shell-run-manager.ts:1020-1021). That window should be treated as "input closed".
Still open from the previous head:
- The P2 is unchanged: the handoff is dropped once the card goes inactive after Resume.
- These P3s from our earlier review are unfixed:
- protocol replies are swallowed in private mode;
- an
applythrow poisons the coordinator; - control is not bound to the requesting Desktop;
- there is a race between
canAnswerandapply.
Merge conflict check: the merge b87ec54b3 resolves its conflicts correctly, and RUNTIME_HOST_COMPATIBILITY_EPOCH → 198 is correct against main, which is at 197.
Tests: the pty-process-driver tests pass 3/3, and shell-run-manager passes 63 with 4 skipped. Not run: coordinator, renderer, E2E, and the macOS PTY fingerprint.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
…rfaces Cover Linux PTY-to-PTY descriptor reuse, recheck queued Resume authority, isolate terminal apply failures, and preserve private review across card transitions. Record the complete real-model Desktop interaction journey. Generated-by: Codex
…ate drafts Remove the redundant confirmation checkbox after product-contract ablation. Reuse input-group and eye controls for local draft reveal, keep transport and draft guards, and restore live handoffs before manual terminals after reload. Record all 23 real-model UI steps. Generated-by: Codex
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head 18400ac5a18a9dc4b7343dfdc72684408deb131a, focusing on the changes since 1ced798e and the two previously blocking findings. I found no additional substantiated P0–P3 issue in this pass; this is a COMMENTED review, not approval.
The PTY-to-PTY fd-reuse leak is addressed at its ownership boundary. packages/runtime/src/pty-process-driver.ts:69-77,163-189 now calls the pinned node-pty instance's synchronous writer rather than writing a captured raw fd. patches/node-pty+1.2.0-beta.15.patch disposes queued writes on the PTY's exit, stream error, or socket close, and makaWriteSync refuses writes after that fence. The real-PTY regression in packages/runtime/src/__tests__/pty-process-driver-write-lifecycle.test.ts forces backpressure, closes the first PTY before child exit, reuses its fd for a second PTY on Linux, and checks that queued private input never reaches the second PTY. This covers the prior fstat-based false ownership proof.
The lost review/share surface after switching cards is also addressed. packages/runtime-host/src/server/runtime-resource-coordinator.ts:585-590,721-727 releases only the controller while retaining a live resumed handoff; runtime-resource-coordinator.test.ts:277-295,415-466 exercises release, reclaim, observation/sharing, and retention beyond 128 live handoffs. The revised Resume boundary rechecks controller identity and unknown input delivery at runtime-resource-coordinator.ts:408-423. The extra renderer changes provide a second-handoff refresh, local draft reveal, and handoff-first recovery; I found no confirmed regression in the paths inspected.
Node 24 clean install and build:test passed. Six focused suites passed: 136 tests, 4 platform skips, 0 failures, including the real PTY regression. Current-head test, windows_acp, and windows_recovery checks are green. The PR diff passes git diff --check and merges cleanly in a local merge-tree against fresh main de4fc5ff95b1f8034ca00b448984b31ca711dce1. I visually checked all five updated screenshots. The PR introduces no schema or migration changes.
Not verified here: packaged Desktop, real SSH/model end-to-end, native macOS PTY behavior, Windows ConPTY handoff, and remote-host transport. The current change remains a large feature and needs human merge judgment.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
A second, independent review of head 18400ac5 (Claude lineage), alongside the hqhq1025 review 5335117041.
The previous P1 is fixed; we checked it independently. Writes now go through the patched node-pty makaWriteSync, which refuses to write once _socket.destroyed or _writeStream._disposed is set. Instrumenting a real node-pty shows destroyed is already true when _destroy starts, and the fd is closed inside _destroy, so the check always runs before the fd can close. We walked every close path:
- EIO on read;
- macOS EOF and timeout;
onexit,destroy(),kill()and_close();- the native
close(master)on spawn failure.
All are covered. Re-running our earlier fd-reuse PoC against the new build, the second PTY still receives the same fd number but gets nothing. The patch applies cleanly to a pristine node-pty@1.2.0-beta.15 with patch-package --error-on-fail.
The previous P2 is fixed on the Host side. release only clears the controller (runtime-resource-coordinator.ts:585-590), and live resumed state is kept until process exit (:690-727).
P2 (new, reproduced on a real PTY): terminal queries in the replayed output are answered again into the private prompt (shell-run-manager.ts:654-666).
- At handoff,
collector.accept(live.rawBuffer)replays the recent output into the new private parser. - The guard
!live.privateTerminalis meant to ignore replies produced by that replay. - The parser processes the replay asynchronously, and
live.privateTerminalis assigned on the very next line. The guard has therefore already stopped applying by the time the replay is parsed.
So any device-status or cursor query still in the last 16k characters of output gets a fresh reply that is written to the PTY. Such queries are common from fish, zsh themes and vim. The reply lands at the password prompt the user is about to type into. Our PoC shows a second ESC[0n reaching the child's stdin after preparePtyHandoff.
Suggested fix: wait for the replay to finish parsing, for example via the collector's snapshot barrier, before letting replies through. Please also add a regression test that replays a buffer containing a DSR query.
P3:
- Between EIO and exit, the first write is dropped silently but still reported as queued. A second write or a protocol reply then becomes an integrity failure (
pty-process-driver.ts:122,shell-run-manager.ts:431-437,:991). - A failed recheck inside Resume kills the terminal the user just logged into, while the resume answer is already saved (
runtime-resource-coordinator.ts:471-479). This happens, for example, when the card is released right after the click. The persisted record then says the terminal resumed when it was actually killed. resizestill acts on the stored fd without the closed check, and this predates the PR. A PoC shows it resizing another PTY that reused the fd. No input leaks.
Tests:
- Runtime PTY driver: 3/3.
shell-run-manager: 63 passed, 4 skipped.- Runtime-host coordinator: 55/55.
- Desktop main handoff: 19/19.
- The renderer typechecks.
Not run: macOS or Windows hardware, and Desktop end to end.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; the replay path was checked against the code, but please verify before acting.
| onProtocolReply: (data) => { | ||
| // Ignore replay while constructing the private parser. Subsequent | ||
| // device/status replies stay within the PTY, never in model output. | ||
| if (!live.privateTerminal || live.driverExit || live.termination || live.integrityFailure) |
There was a problem hiding this comment.
P2: collector.accept(live.rawBuffer) parses asynchronously, and live.privateTerminal is set on the next line. By the time the replayed output is parsed, this guard no longer applies, so old DSR or cursor queries in the replay get fresh replies written into the PTY, landing at the password prompt. Please wait for the replay to finish parsing before letting replies through.
There was a problem hiding this comment.
Confirmed by checking the current code: this remains open. collector.accept(live.rawBuffer) is still followed immediately by assigning live.privateTerminal, so the callback guard does not cover asynchronous parsing of the replay. The existing live private-mode DSR test only verifies responses to new queries after Resume; it does not establish that replayed queries are suppressed.
I have added this explicitly to the PR's remaining-work checklist and am not marking it fixed. The correction needs a replay-completion barrier while preserving the routing/recording boundary for newly arriving private bytes, plus a regression proving that an old DSR query does not inject a second response into the password prompt. The refreshed screenshots and successful normal SSH journey do not cover this edge case.
Retire the completed input card, prioritize one actionable status and collapse technical details. Remove sharing through the renderer, Host protocol and Runtime, retaining output isolation and documenting the unresolved result-observation boundary. Verify two subsequent real-provider turns reuse the authenticated shell. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
|
@hqhq1025 @Astro-Han, I have refreshed the PR description and screenshots to show the current interaction, and replied directly to the four still-open inline threads. This is a preliminary PR for design and interaction review. My input-card proposal in #5309 has not yet received a reply in the issue. I do not want to treat implementation progress as agreement on that change to the user journey. Could you first assess whether the approach and the screenshots make sense? If so, I will continue developing the PR and resolving the remaining gaps before requesting merge. Current review status:
The earlier Windows inventory/source-closure problems were fixed and confirmed in the later reviews; I am not extrapolating older green CI to the new screenshot commit. The latest owned-writer change also avoids the Driver's raw-fd assumption; the pinned Unix node-pty implementation sets O_NONBLOCK, and availability requires its owned writer. This does not replace the remaining platform acceptance. Latest real App run: 24 captured steps, 14 model requests, two subsequent turns in the same SSH shell without another password, and no generated credential found in the scanned model/event/log/file surfaces. This review pass also reran 49 focused Desktop/Host/driver tests plus the isolated apply-failure regression on macOS. Automatic post-handoff result observation is still unresolved. I removed the manual sharing flow to simplify the interaction, but kept private output isolated: the real delayed-echo experiment showed why opening output on Resume is unsafe. The PR is open for review, with the replay, closed-input and Resume issues explicitly unchecked. Please assess the proposed direction rather than treating these screenshots as a request for merge approval. This update was prepared with Codex; the claims above distinguish implemented fixes, prior independent review, current local validation and outstanding work. |
hqhq1025
left a comment
There was a problem hiding this comment.
This head simplifies the private-input card, removes the manual-sharing path, and preserves private terminal output after Resume. The PR correctly states that automatic post-handoff observation remains unimplemented and that the interaction still needs product agreement; it is not merge-ready. The previous PTY fd-reuse and card-release fixes remain in place. One previously reported P2 remains reachable on this head (inline): replaying historical terminal output into the new private parser can send fresh terminal-protocol replies into the live password prompt.
I reviewed the 29-file increment and the affected controller/runtime paths, inspected the waiting/retry/resumed screenshots, and ran Node 24 build:test plus focused Desktop/Host/Runtime suites (146 passed, 4 platform skips). Current-head windows_acp, windows_recovery, and label checks pass; hosted test was still running when I submitted this review. The branch has no schema/migration change and a clean static merge/diff-check against main. I did not run the macOS Electron + real SSH/model journey or independently reproduce the replay-query PoC on this head; the finding follows from unchanged asynchronous collector control flow and the prior real-PTY reproduction. The new focused tests do not cover replayed protocol queries.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| this.handleIntegrityFailure(live, new Error('Private terminal display failed')), | ||
| }); | ||
| collector.accept(live.rawBuffer); | ||
| live.privateTerminal = { collector, inputOpen: true }; |
There was a problem hiding this comment.
[P2] Do not enable protocol replies until the historical buffer has finished parsing. collector.accept(live.rawBuffer) only queues an asynchronous parse (pty-screen-collector.ts:118-157), but live.privateTerminal is assigned immediately afterward. When the replay contains an old DSR/cursor query, its delayed onProtocolReply sees a truthy live.privateTerminal and calls live.driver.write(data) into the current password/verification prompt. The new head leaves this ordering unchanged, so the prior real-PTY reproduction still applies. Fence replies for the replay generation or await a parse cut before enabling the reply path, and add a regression with an old query in rawBuffer plus a live private prompt.
Summary
Refs #5309. This is a preliminary PR for design and interaction feedback, not a merge-ready implementation.
I proposed a compact input card bound to the original terminal in this issue comment. The issue author has not yet replied to that proposal in the issue, so I do not consider the adjustment from direct terminal input to an input card agreed upon. This initial implementation makes the proposed approach concrete and reviewable in a real Desktop flow.
@hqhq1025 @Astro-Han, could you first review whether the proposed approach and the interaction below are reasonable for the issue? In particular, does a familiar private input field alongside the original terminal, followed by one explicit completion action, provide the right experience? If the direction looks reasonable, I will continue developing the PR and resolving the remaining correctness and acceptance gaps before requesting merge.
The proposed flow is:
WriteStdinhandoff action and requests human input for its exact live PTY. No chat-keyword detection or new standalone tool family is involved.The Runtime process/resource and Interaction lifecycles remain authoritative. Agent writes are fenced during human input; uncertain delivery is never replayed and blocks completion. Workbar owns surface registration, and restored cards wait for that registration and the authoritative phase. Existing permissions remain in force. Supported scope is macOS/Linux Desktop.
The major functional gap is still automatic post-handoff result observation. The agent can continue executing in the original shell, but cannot currently read its private output. Making output public on Resume leaked delayed credentials in a real-PTY experiment, and literal-input filtering failed cursor-edit/encoded-output cases. Manual sharing has been removed; neither connection reuse nor these screenshots establish full issue acceptance.
Design and privacy boundary · Reproducible real-provider harness
Interaction screenshots
Fresh captures from the actual macOS Electron + Runtime Host + OpenSSH +
gpt-5.6-terrajourney on 2026-09-28, using the same viewport. These replace the older screenshots. The selected frames below come from a 24-step capture. Dark solid regions mask intentionally echoed test credentials in the screenshots only; they are not part of the product UI. The input-reveal example uses harmlessreview-demotext.1. Agent requests private input in the original terminal.
2. An incorrect SSH password produces one actionable error and allows retry.
3. After the explicit completion click, the input card and its actions disappear.
Additional states: baseline, details, reveal, completion, later-turn reuse and exit
Normal terminal before handoff
Connection details, expanded on demand
Local draft reveal
Authentication fixture completed; one completion action is available
The fixture output is masked because it deliberately echoes test credentials. This UI state is not machine-verified authentication success.
Third conversation turn, still using the original authenticated SSH connection
The private terminal output is masked in this image. The test asserted
FOLLOWUP3:original-shell:/tmpin the actual terminal and verified that no new input card appeared. The agent explicitly reports that it cannot observe that private result.Original process exited
Remaining work before merge
Verification
fullIssueAcceptance: false.AI use
Codex implemented, tested and documented the change. Real-provider acceptance used gpt-5.6-terra. Commits include
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?