feat: interactive PTY mode for Claude Code#882
Open
junmo-kim wants to merge 24 commits into
Open
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Agent-terminal subscribe bypasses session namespace checks —
agent-terminal:subscribejoinsagent-session:<sessionId>and replaysgetAgentTerminalReplay(sessionId)before validating that the requested session belongs to the authenticated namespace. The existingterminal:createpath does this check, and the CLI output path broadcasts future PTY bytes to the room name alone, so a client with any valid token can subscribe to a guessed/known session id in another namespace and receive buffered/future agent terminal output. Evidence:hub/src/socket/handlers/terminal.ts:249. - [Minor] Hidden agent terminals still subscribe and enable PTY streaming —
AgentTerminalViewconnects on mount even whenvisibleis false, and the hook subscribes immediately on connect. BecauseSessionChatmounts this component hidden for every active PTY session, merely opening the chat flipsagentTerminalActiveon in the CLI and streams the high-frequency raw TUI path even when the user never opens the terminal. Evidence:web/src/components/AgentTerminal/AgentTerminalView.tsx:111.
Summary
Review mode: initial. Two issues found in the latest diff: missing namespace authorization on agent-terminal subscription and a hidden-view subscription that defeats the intended viewer-gated PTY streaming.
Testing
Not run (automation).
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Deterministic terminal IDs make multiple viewers steal the same shell —
web/src/routes/sessions/terminal.tsx:191now usesterm-${sessionId}for every browser/tab/device viewing a session. The hub registry treats a reused terminal ID from another socket as a stale reconnect and removes the first socket's entry, then the CLITerminalManager.create()sees the same ID and reuses the existing PTY instead of spawning an independent shell. Result: opening the terminal for the same session from a second device reroutes output to the second viewer and leaves the first viewer unable to write/resize reliably. Evidence:web/src/routes/sessions/terminal.tsx:191, related registry behaviorhub/src/socket/terminalRegistry.ts:27.
Summary
Review mode: follow-up after new commits. Prior namespace/subscription concerns appear addressed, but the full latest diff still introduces a regression in the existing remote terminal route: terminal IDs are no longer per viewer.
Testing
Not run (automation). Missing coverage for two simultaneous /sessions/:id/terminal sockets using the same session.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] First-message verifier can re-submit before the PTY driver has submitted —
cli/src/claude/claudePtyLauncher.ts:421startsensureFirstMessageDelivered()insidenextMessage(), before control returns torunAgentPtyand beforerunAgentPtyruns its ownwaitForInputReady()+submitMessage()path. On a slow--resumereplay, that verifier can hit its 5s timeout while the normal submit is still waiting for the prompt, write the prompt directly throughptyControls, and then the driver sends the same text again once ready. That can duplicate the first user message or inject it into the replaying TUI.
Suggested fix:Then wire// RunAgentPtyOpts onMessageSubmitted?: (message: string) => void | Promise<void> // after the normal driver submit path await submitMessage(next.message) await opts.onMessageSubmitted?.(next.message)
claudePtyLauncherto callensureFirstMessageDelivered()from that post-submit hook for only the first submitted message.
Summary
- Review mode: follow-up after new commits. The previous terminal-id finding is addressed. The latest full diff still has a PTY resume-path regression around first-message verification timing.
Testing
- Not run (automation). Suggested: add a PTY launcher/driver test where
nextMessage()returns,runAgentPtyremains inwaitForInputReady()for more than 5s, and the verifier must not write keys until after the driver has submitted.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Multiline prompts are submitted as multiple PTY commands — PTY mode writes the queued web composer text directly to the terminal and then appends Enter. Web messages can be multiline already (normal composer text, batched queued messages joined with
\n, and attachment prompts formatted as@path\n\ntext), so every embedded newline is interpreted by Claude's interactive TUI as Enter before the final submit. The first line runs by itself and the remaining lines can become separate slash commands/prompts or get typed while Claude is already responding. Evidence:cli/src/agent/runAgentPty.ts:323submitsnext.messagethrough the raw PTY path;cli/src/claude/runClaude.tsformats attachments into multiline text before queueing.
Suggested fix:Add a PTY driver test that sendsconst submitMessage = async (message: string): Promise<void> => { const payload = message.includes('\n') ? `\x1b[200~${message}\x1b[201~` : message let echoed = false for (let attempt = 0; attempt < 3 && !echoed; attempt++) { const before = lastOutputAt manager.write(payload) // existing echo wait... } await sleep(150) manager.write('\r') }
line 1\nline 2and asserts the embedded newline is bracketed-paste escaped, with only the final\rsubmitting.
Summary
- Review mode: follow-up after new commits. The previous first-message verifier race is addressed, but the latest full diff still has a PTY input regression for multiline web prompts/attachments.
Testing
- Not run (automation). Suggested: add the multiline PTY submit test above and cover an attachment-formatted prompt path.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] First-message repair still splits multiline prompts — the primary PTY submit path now wraps multiline input in bracketed paste, but the resume/drop repair path re-types the first prompt with raw
sendKeys(trimmed). If the first submitted message is multiline (composer newline, batched queue, or attachment prompt) and the initial submit is swallowed by a slow--resume, the repair turns embedded newlines into Enter keystrokes, so Claude receives separate prompts/commands instead of the original message. Evidence:cli/src/claude/claudePtyLauncher.ts:256.
Suggested fix:const repairPayload = text.includes('\n') ? `\x1b[200~${text}\x1b[201~` : text this.ptyControls.sendKeys(repairPayload) await this.sleep(200) this.ptyControls.sendKeys('\r')
Summary
- Review mode: follow-up after new commits. The previous primary submit regression is fixed, but the first-message delivery repair path still needs the same multiline handling.
Testing
- Not run (automation). Suggested: add a launcher/repair test where the first submitted message is multiline and transcript confirmation stays false, then assert the retry writes bracketed paste plus one final
\r.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Local handoff now starts PTY for every Claude session —
runLocalRemoteLoopnow choosesptywhenever arunPtylauncher is registered, and Claude always registers one. That changes the existing localhapiflow: pressing space to switch from local mode no longer enters the normal SDK remote launcher unless the session explicitly opted into PTY. It also bypasses the PR's stated opt-in behavior for PTY mode. Evidence:cli/src/agent/loopBase.ts:50.
Suggested fix:const remoteMode: SessionMode = opts.startingMode === 'pty' ? 'pty' : 'remote' // ... mode = remoteMode opts.session.onModeChange(mode === 'pty' ? 'remote' : mode)
Summary
- Review mode: follow-up after new commits. The previous multiline repair finding is fixed. One remaining regression: default Claude local-to-remote handoff now routes through PTY because PTY availability is treated as opt-in.
Testing
- Not run (automation). Suggested: add a
runLocalRemoteLooptest wherestartingMode: 'local'andrunPtyis provided; after local returnsswitch, assertrunRemoteis called. Add a second test forstartingMode: 'pty'to keep the opt-in PTY path covered.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] PTY spawn failures are swallowed —
runAgentPtyonly logsonErrorand returns whenmanager.isRunningis false, soClaudePtyLaunchersees a successfulawait claudePty(...), consumes flags, and loops back into another launch instead of surfacingPTY process erroror ending the session. This affects real failures such asclaudenot being installed or Bun failing to attach a terminal. Evidence:cli/src/agent/runAgentPty.ts:271.
Suggested fix:let spawnError: Error | null = null manager.spawn({ // ... onError: (error) => { spawnError = error logger.debug(`${debugPrefix} PTY error: ${error.message}`, error) }, }) if (!manager.isRunning) { throw spawnError ?? new Error(`Failed to spawn ${opts.command} PTY`) }
Questions
- None.
Summary
- Review mode: follow-up after new commits. The previous handoff regression is fixed. One remaining issue: PTY launch failures currently look like successful returns to the caller, which can leave runner-spawned PTY sessions retrying without a useful user-visible failure.
Testing
- Not run (automation). Suggested: add a
runAgentPtytest where the mockedAgentPtyManager.spawncallsonErrorand leavesisRunning=false, assertingrunAgentPtyrejects; add aClaudePtyLaunchertest asserting the rejection emitsPTY process errorinstead of looping.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] PTY startup failures are classified as recoverable after spawn —
runAgentPtycallsonReady()beforewaitForInputReady(), butClaudePtyLaunchertreats that callback as “reached a ready prompt” and resets the immediate-failure counter. Ifclaudespawns and exits before rendering a prompt (bad config, invalid args, auth failure, etc.),reachedReadyis already true, so the new give-up/backoff path is bypassed and the launcher can keep respawning forever. Evidence:cli/src/agent/runAgentPty.ts:299.
Suggested fix:await waitForInputReady() if (!manager.isRunning || signal?.aborted) { throw new Error(`${opts.command} PTY exited before becoming ready`) } opts.onReady()
Questions
- None.
Summary
- Review mode: follow-up after new commits. The previous spawn-error finding is addressed, but the follow-up retry classifier still marks a PTY as ready immediately after spawn instead of after the prompt is actually usable.
Testing
- Not run (automation). Suggested: add a
runAgentPtytest where the mocked PTY exits before any ready output and assertonReadyis not called; add aClaudePtyLaunchertest that repeated pre-ready exits reachgive-up.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] PTY sessions lose their terminal affordance after a local handoff. The new metadata field is documented and consumed as the original launch mode (
metadata.startingMode === 'pty'gates the agent-terminal button), butsetControlledByUser()rewrites it every time the collaboration mode changes. In the PTY loop, leaving PTY for local callssetControlledByUser(..., 'local'), and the next handoff back to PTY reports external mode'remote', so this line persistsstartingMode: 'remote'for a still-PTY-backed session. After the SSE update,SessionChatno longer renders the terminal toggle even though the PTY is running. Evidence:cli/src/agent/runnerLifecycle.ts:149, consumed atweb/src/components/SessionChat.tsx:315.
Suggested fix:export function setControlledByUser( session: ApiSessionClient, mode: 'local' | 'remote', launchMode?: 'local' | 'remote' | 'pty' ): void { session.updateAgentState((currentState) => ({ ...currentState, controlledByUser: mode === 'local', startingMode: launchMode ?? currentState.startingMode ?? mode })) session.updateMetadata((metadata) => ({ ...metadata, startingMode: launchMode ?? metadata.startingMode ?? mode })) }
Questions
- None.
Summary
- Review mode: follow-up after new commits. The previous startup-failure finding is addressed. I found one remaining regression in how PTY launch identity is persisted across local/remote handoff.
Testing
- Not run (automation). Suggested: add a loop/session lifecycle test for a
pty -> local -> ptyswitch and assert metadata/agentStatestartingModeremainspty.
HAPI Bot
There was a problem hiding this comment.
Findings
- No issues found with confidence >= 80% in the current diff.
Questions
- None.
Summary
- Review mode: follow-up after new commits. I re-reviewed the full current diff, including the PTY launch path, metadata/start mode persistence, terminal socket authorization/replay, permission hook bridge, session scanning, spawn/reopen flow, and the follow-up changes since the prior bot review. The prior
startingModehandoff regression appears addressed by preservingptyacross local/remote transitions and adding coverage for that lifecycle.
Testing
- Not run (automation). Residual risk: PTY behavior depends on Claude's live TUI/hooks, so the main uncovered risk is end-to-end behavior with a real Claude CLI across spawn, remote chat input, terminal toggle, local handoff, and reopen.
HAPI Bot
junmo-kim
force-pushed
the
feat/interactive-pty
branch
from
c1995cf to
f2865d1
Compare
There was a problem hiding this comment.
Findings
- No issues found with confidence >= 80% in the current diff.
Questions
- None.
Summary
- Review mode: follow-up after new commits. I re-reviewed the full current diff and used the prior HAPI Bot review plus the compare diff only as follow-up context. I did not find any correctness, security, regression, data-loss, performance, or maintainability issue introduced by the added/modified lines with enough confidence to report. Residual risk: PTY behavior still depends on live Claude CLI/TUI hooks and reconnect behavior that is hard to fully prove from static review.
Testing
- Not run (automation):
bunis not installed in this runner (/bin/bash: bun: command not found), sobun typecheckandbun run test -- --runInBandcould not execute.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Strip dangerous Claude skip flag for every PTY launch —
parseClaudeStartOptionsonly removes--dangerously-skip-permissionswhenpermissionMode === 'bypassPermissions', sohapi --hapi-starting-mode pty --permission-mode default --dangerously-skip-permissionsstill forwards the raw Claude flag while HAPI records/defaults todefault. In PTY mode this bypasses the PreToolUse hook installed inrunClaude.ts, so permission requests and AskUserQuestion no longer reach the web approval/question bridge. Evidence:cli/src/commands/claude.ts:82.
Suggested fix:if (options.startingMode === 'pty') { for (let i = unknownArgs.length - 1; i >= 0; i--) { if (unknownArgs[i] === '--dangerously-skip-permissions') { unknownArgs.splice(i, 1) } } }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff, with prior bot review and compare diff as context only. One PTY permission-mode regression remains: an explicit permission mode plus Claude's dangerous skip flag can disable the hook path this PR relies on.
Testing
- Not run (automation):
bunis not installed in this runner, sobun typecheck/bun run testcould not execute.
HAPI Bot
Pull the CLI arg parsing out of claudeCommand.run into a pure function so the option handling is unit-testable. No behavior change.
…trol axis startingMode conflated two orthogonal axes: control mode (local/remote — who drives the session) and launch kind (PTY — how the agent process is run). Only claude (and the upcoming agy) can launch a PTY, yet every remote flavor's options carried 'pty' and had to collapse it back to 'remote', duplicating the same ternary across runGemini/runOpencode/runPi. Split the launch kind into a separate `interactive` flag: - Shared RemoteAgentCommandOptions.startingMode is back to 'local' | 'remote'; '--hapi-starting-mode pty' now sets `interactive` instead. Non-PTY flavors (gemini/opencode/pi) no longer mention pty at all and revert to their upstream form. - claude maps `interactive` to the runtime SessionMode 'pty' at the loop boundary, so the dispatcher (loopBase), persistence (agentState/metadata startingMode) and the web agent-terminal gate are unchanged — pty still lives in the runtime/dispatch layer where it belongs. No behavior change: PTY sessions launch, persist and resume exactly as before.
…util Move the AskUserQuestion / request_user_input answer-to-input builders (and the question tool-name predicates) out of the SDK permission handler into a standalone util. No behavior change — the SDK handler imports them — so the PTY permission bridge can reuse them without pulling in the SDK handler's dependencies.
The standalone terminal's quick-input keys (Esc/arrows/Ctrl/Alt) and their sticky-modifier dispatch were inline in terminal.tsx. Extract them to a shared QuickKeys module — a useQuickKeyInput hook (modifier state + dispatcher) and a QuickKeyRows presentational component — so other terminal views can reuse them without duplication. Behavior-preserving: the raw onData path stays ungated and shares the modifier state with the buttons, exactly as before.
Hoist the launch/ready/backoff/give-up respawn loop out of the Claude PTY launcher into a RemoteLauncherBase.runRespawnLoop template method, so a second PTY flavor can reuse it instead of duplicating the loop. No behavior change.
… approvals Launch claude inside a PTY and drive it from the web: chat input, in-place /model and /effort changes (auto-confirming claude's "Switch model?" dialog), and --resume. Tool approvals are bridged to the web instead of the SDK's canUseTool (which a PTY agent doesn't have): a PreToolUse hook forwards each tool call to the runner, which surfaces it in the existing web approval modal (reusing state.requests + the permission RPC — no web changes) and returns allow/deny to claude. - generateHookSettings registers the PreToolUse hook (PTY only) with a generous timeout so the blocking hook survives a slow phone approval. - sessionHookForwarder branches on the stdin hook_event_name: PreToolUse posts to a new endpoint and echoes claude's hookSpecificOutput decision on stdout; SessionStart keeps its fire-and-forget behavior. - startHookServer gains a /hook/pre-tool-use endpoint that awaits the decision. - PtyPermissionHandler reuses BasePermissionHandler. Read-only tools auto-allow; bypassPermissions allows all; everything else asks the web. "Allow for session" is honored, including Bash's command-qualified form (Bash(<cmd>) / Bash(<prefix>:*)). Question tools (AskUserQuestion / request_user_input) are routed to the web too: the picked answers are injected back via the tool's updatedInput so claude echoes them instead of prompting in its TUI. Decisions are always allow/deny (never ask, which would fall back to the TUI prompt) and fail closed on errors. A default-mode PTY session therefore prompts for permission — and asks its questions — in the chat like the SDK path; the explicit YOLO toggle still opts into --yolo.
On a turn-stop RPC, write the Esc interrupt key into the live PTY (the claude TUI's own interrupt) instead of killing the process, when PTY controls are active. Falls back to aborting the controller when they are not.
The agent-terminal view was output-only, so a remote viewer could not navigate TUI screens the structured chat composer cannot express — escape a /usage screen, answer a /model dialog, or send Ctrl-C. Add an agent-terminal:input path (web emit -> hub relay -> CLI -> agent PTY) so the xterm onData and a shared QuickKey bar drive the live TUI. The CLI now stores the launcher's sendKeys control, which setAgentTerminalControls previously dropped. The relay reuses the same authorization guard as resize (authorized viewer of an active session in the namespace). The chat composer remains the primary way to send messages.
- Pipe stdin for local Claude processes so chat messages go directly to the running session instead of triggering a mode switch - Add stdin message dedup filter in local launcher scanner callback (guards against user messages without a `message` payload) - Restore lost messages to queue on claudeRemote launch failure - Export extractRawUserTextContent for reuse in dedup logic
junmo-kim
force-pushed
the
feat/interactive-pty
branch
from
5781777 to
d0d6cc6
Compare
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff, using the prior bot review and compare diff as context. The prior PTY permission-strip finding is addressed on this head; I did not find any remaining high-confidence issues introduced by the diff.
Testing
- Not run (automation):
bunis not installed in this runner, sobun typecheck/bun run testcould not execute.
HAPI Bot
On abort, send Esc to interrupt the claude TUI, then (after a 150 ms delay so the TUI finishes restoring its prompt) Ctrl-U to clear the input line and reset the message queue. Emit an abort-restore session event so the web composer can recover the aborted prompt text.
Consume the abort-restore session event: read the last user message text and surface it back in the composer via the existing sendError path, so an aborted prompt is not lost. Skip the signal in the chat timeline since it is a composer side-effect, not a visible event.
There was a problem hiding this comment.
Findings
- [Major] Abort restore can replay an unrelated historical prompt —
handleAbortRequestnow emitsabort-restoreeven when the PTY driver has not submitted a prompt in the current turn. The web consumer then walks backward and restores the last user message before that event, so an abort during idle/startup/no-submission paths can put an old prompt back into an empty composer and invite an accidental resend. Evidence:cli/src/claude/claudePtyLauncher.ts:332,web/src/components/SessionChat.tsx:290.
Suggested fix:Then have the web restoreprivate promptToRestoreOnAbort: string | null = null // in onMessageSubmitted this.promptToRestoreOnAbort = message // in handleAbortRequest, after queue.reset() const prompt = this.promptToRestoreOnAbort this.promptToRestoreOnAbort = null if (prompt) { this.session.client.sendSessionEvent({ type: 'abort-restore', text: prompt }) }
msg.content.textfrom the event instead of scanning historical user turns.
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff, using the prior bot review and compare diff as context. I found one high-confidence regression in the follow-up abort restore behavior.
Testing
- Not run (review automation): per PR-review security rules, I did not execute code from the PR.
HAPI Bot
| this.session.queue.reset() | ||
| // Signal the web composer to restore the aborted prompt text. | ||
| // The web side reads the last user message from normalizedMessages. | ||
| this.session.client.sendSessionEvent({ type: 'abort-restore' }) |
There was a problem hiding this comment.
[Major] Abort restore can replay an unrelated historical prompt
This emits abort-restore even when no prompt was submitted for the turn being aborted. The web handler does not receive the aborted text; it scans backward and restores the last historical user message before the event, so aborting during idle/startup/no-submission paths can repopulate an empty composer with an old prompt and make it easy to resend the wrong thing.
Suggested fix:
private promptToRestoreOnAbort: string | null = null
// in onMessageSubmitted
this.promptToRestoreOnAbort = message
// in handleAbortRequest, after queue.reset()
const prompt = this.promptToRestoreOnAbort
this.promptToRestoreOnAbort = null
if (prompt) {
this.session.client.sendSessionEvent({ type: 'abort-restore', text: prompt })
}Then have the web restore msg.content.text from the event instead of scanning historical user turns.
abort-restore previously fired unconditionally and the web rebuilt the text by scanning backward for the last user message, so aborting during idle/startup/no-submission could replay an unrelated historical prompt into an empty composer and invite an accidental resend. Track the submitted prompt on the launcher (set on submit, cleared when the turn goes idle) and carry it on the abort-restore event itself. The event is emitted only when a prompt was actually in flight, and the web restores that exact text instead of guessing from history.
There was a problem hiding this comment.
Findings
- [Minor] Plain Bash session allow is recorded but never honored —
rememberBashPermission('Bash')storesBashinsessionAllowedTools, but the fast path handlestoolName === 'Bash'separately and only checks literal/prefix command sets. If the hub/API sends a plainallowTools: ['Bash'], later Bash calls still prompt instead of using the recorded session allow. Evidence:cli/src/claude/utils/ptyPermissionHandler.ts:98,cli/src/claude/utils/ptyPermissionHandler.ts:241.
Suggested fix:if (toolName === 'Bash') { if (this.sessionAllowedTools.has('Bash')) { return Promise.resolve({ permissionDecision: 'allow' }); } const command = (input as { command?: string } | null)?.command; if (command && this.isBashCommandAllowed(command)) { return Promise.resolve({ permissionDecision: 'allow' }); } }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff, using the previous bot review and compare diff as context. The previous abort-restore finding appears addressed; I found one remaining PTY permission-state inconsistency.
Testing
- Not run (review automation): per PR-review security rules, I did not execute code from the PR.
HAPI Bot
| const mode = this.options.getPermissionMode(); | ||
|
|
||
| // 1. Already allowed for the session via a prior approval. | ||
| if (toolName === 'Bash') { |
There was a problem hiding this comment.
[Minor] Plain Bash session allow is recorded but never honored
rememberBashPermission('Bash') stores a name-level Bash allow in sessionAllowedTools, but this branch handles toolName === 'Bash' separately and never checks that set. If the hub/API sends allowTools: ['Bash'], subsequent Bash calls still prompt instead of honoring the recorded session allow.
Suggested fix:
if (toolName === 'Bash') {
if (this.sessionAllowedTools.has('Bash')) {
return Promise.resolve({ permissionDecision: 'allow' });
}
const command = (input as { command?: string } | null)?.command;
if (command && this.isBashCommandAllowed(command)) {
return Promise.resolve({ permissionDecision: 'allow' });
}
}A plain "Bash" entry in allowTools is stored in sessionAllowedTools as a name-level allow, but the Bash fast path only consulted the per-command literal/prefix sets — so a session-wide Bash approval was recorded and then ignored, re-prompting on every command. Check sessionAllowedTools for "Bash" before falling back to the per-command match.
There was a problem hiding this comment.
Findings
- [Major] PTY launch drops runner-provided Claude OAuth token — runner-spawned Claude sessions pass per-session auth via
CLAUDE_CODE_OAUTH_TOKEN, but PTY mode now deletes everyCLAUDE_CODE_*variable before spawning the realclaudeprocess. For users relying on that token instead of a local Claude login, the HAPI child starts but the PTY child is unauthenticated, so the new opt-in mode fails or prompts for login instead of starting the session. Evidence:cli/src/claude/claudePty.ts:82with runner contextcli/src/runner/run.ts:398.
Suggested fix:function claudeInheritedEnvKeys(): string[] { return Object.keys(process.env).filter( (k) => k === 'CLAUDECODE' || (k.startsWith('CLAUDE_CODE_') && k !== 'CLAUDE_CODE_OAUTH_TOKEN') ) }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff after the latest commit; the previous Bash session-allow finding is fixed. One remaining PTY auth regression found.
Testing
- Not run (review automation): per PR-review security rules, I did not execute code from the PR.
HAPI Bot
| }, | ||
| // Drop inherited CLAUDECODE / CLAUDE_CODE_* so claude saves its | ||
| // transcript (see claudeInheritedEnvKeys). | ||
| unsetEnv: claudeInheritedEnvKeys(), |
There was a problem hiding this comment.
[Major] PTY launch drops runner-provided Claude OAuth token
Runner-spawned Claude sessions inject auth as CLAUDE_CODE_OAUTH_TOKEN, but this PTY path deletes every CLAUDE_CODE_* key from the child environment. That means users who depend on the runner token path get an unauthenticated interactive claude process instead of a started PTY session.
Suggested fix:
function claudeInheritedEnvKeys(): string[] {
return Object.keys(process.env).filter(
(k) => k === 'CLAUDECODE' || (k.startsWith('CLAUDE_CODE_') && k !== 'CLAUDE_CODE_OAUTH_TOKEN')
)
}PTY mode unset every inherited CLAUDECODE / CLAUDE_CODE_* var before spawning claude so the child saves its transcript. That also dropped CLAUDE_CODE_OAUTH_TOKEN, which the runner uses to pass per-session auth, leaving runner-spawned PTY sessions unauthenticated (login prompt instead of starting). Exclude the token from the unset list.
There was a problem hiding this comment.
Findings
- [Major] PTY spawn returns before Claude is actually ready — the new PTY path tries to surface launch failures by waiting after
rpcGateway.spawnSession(), but it only waits forwaitForSessionActive()athub/src/sync/syncEngine.ts:764.activeis driven bysession-alive, and every CLI session emits that fromAgentSessionBaseconstruction before the PTY launcher has spawned Claude or reachedonReady()(cli/src/agent/sessionBase.ts:78). The PTY launcher currently sends only a chatreadyevent when the prompt is usable (cli/src/claude/claudePtyLauncher.ts:414), so failures like missingclaude, auth failure, or early PTY exit can still make/api/machines/:id/spawnreturn success and drop the user into an empty/failing terminal instead of reporting the spawn error.
Suggested fix:// cli/src/claude/claudePtyLauncher.ts onReady: () => { reachedReady = true this.session.client.emitSessionReady() this.session.client.sendSessionEvent({ type: 'ready' }) } // hub/src/sync/syncEngine.ts if (result.type === 'success' && startingMode === 'pty') { const readyResult = await this.waitForSessionReady(result.sessionId) if (readyResult !== 'ready') { return { type: 'error', message: readyResult === 'ended' ? 'Session ended before Claude PTY became ready' : 'Session spawned but failed to become ready' } } }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff; the prior OAuth-token finding is fixed in the latest commit. One remaining readiness regression found in the PTY spawn flow.
Testing
- Not run (review automation).
HAPI Bot
| // failed PTY spawn surfaces as an error instead of an empty terminal. Other | ||
| // start modes return as soon as the spawn RPC succeeds (legacy behavior). | ||
| if (result.type === 'success' && startingMode === 'pty') { | ||
| const becameActive = await this.waitForSessionActive(result.sessionId) |
There was a problem hiding this comment.
[Major] PTY spawn returns before Claude is actually ready
This wait does not prove the PTY is attached or usable. waitForSessionActive() observes session-alive, and the CLI emits that from AgentSessionBase construction before claudePty reaches onReady(). The PR’s new failure handling can therefore still return success for a PTY session that later fails to spawn or exits before the prompt.
Suggested fix:
// cli/src/claude/claudePtyLauncher.ts
onReady: () => {
reachedReady = true
this.session.client.emitSessionReady()
this.session.client.sendSessionEvent({ type: 'ready' })
}
// hub/src/sync/syncEngine.ts
if (result.type === 'success' && startingMode === 'pty') {
const readyResult = await this.waitForSessionReady(result.sessionId)
if (readyResult !== 'ready') {
return {
type: 'error',
message: readyResult === 'ended'
? 'Session ended before Claude PTY became ready'
: 'Session spawned but failed to become ready'
}
}
}The pty spawn flow only waited for waitForSessionActive, but `active` is driven by session-alive, which every CLI session emits at construction — before the PTY launcher has spawned claude or reached a usable prompt. So a missing claude binary, auth failure, or early PTY exit could still make the spawn RPC return success and drop the user into an empty terminal. Emit session-ready from the launcher's onReady, and have the spawn flow wait for it (mirroring the resume path): a non-ready outcome now returns a spawn error instead of a false success.
There was a problem hiding this comment.
Findings
- [Major] PTY mode is never forwarded from the spawn route - the web client sends
startingMode: "pty"andSpawnSessionRequestSchemaaccepts it, but this positional call passesparsed.data.startingModeintoSyncEngine.spawnSession'sserviceTierparameter instead of itsstartingModeparameter (hub/src/web/routes/machines.ts:55). That meansSyncEngineseesstartingMode === undefined, skips the PTY readiness wait, andrpcGateway.spawnSession()sendsserviceTier: "pty"with nostartingMode; the runner then defaults to--hapi-starting-mode remote, so the new checkbox silently launches the SDK remote path instead of PTY.
Suggested fix:const result = await engine.spawnSession( machineId, parsed.data.directory, parsed.data.agent, parsed.data.model, parsed.data.modelReasoningEffort, parsed.data.yolo, parsed.data.sessionType, parsed.data.worktreeName, undefined, parsed.data.effort, undefined, undefined, parsed.data.startingMode )
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff; the prior readiness issue is addressed, but the public new-session route still drops the new PTY mode before it reaches the runner.
Testing
- Not run (review automation). Add a route-level spawn test that posts
{ startingMode: "pty" }and assertsSyncEngine.spawnSessionreceives it in thestartingModeslot.
HAPI Bot
| parsed.data.effort | ||
| parsed.data.effort, | ||
| undefined, | ||
| parsed.data.startingMode |
There was a problem hiding this comment.
[Major] PTY mode is never forwarded from the spawn route
The route added parsed.data.startingMode, but SyncEngine.spawnSession has two parameters after effort: permissionMode, serviceTier, then startingMode. With the current call this value lands in serviceTier, so SyncEngine sees startingMode === undefined, skips the PTY readiness wait, and the runner receives no startingMode and defaults to remote mode. The new checkbox therefore silently launches the SDK remote path instead of PTY.
Suggested fix:
const result = await engine.spawnSession(
machineId,
parsed.data.directory,
parsed.data.agent,
parsed.data.model,
parsed.data.modelReasoningEffort,
parsed.data.yolo,
parsed.data.sessionType,
parsed.data.worktreeName,
undefined,
parsed.data.effort,
undefined,
undefined,
parsed.data.startingMode
)The spawn route passed parsed.data.startingMode positionally into spawnSession's serviceTier slot, leaving the real startingMode parameter undefined. SyncEngine then skipped the PTY readiness wait and the runner defaulted to --hapi-starting-mode remote, so the web PTY checkbox silently launched the SDK remote path. Both values are strings, so the slot mismatch type-checked and stayed latent. Pass undefined for serviceTier and startingMode in its own (13th) slot.
There was a problem hiding this comment.
Findings
- [Major] Queued PTY prompts can be submitted before Claude is idle —
promptSeenis latched after the first prompt marker, so the laterwaitForInputReady()calls only require 500ms of output silence before writing the next queued message. During a normal long response, any quiet gap can satisfy that condition even though Claude has not rendered the next input prompt yet, so queued messages can be typed into a busy TUI and get dropped, appended to the wrong input, or interrupt the current turn. Evidencecli/src/agent/runAgentPty.ts:344.
Suggested fix:let inputReady = false const waitForInputReady = async (timeoutMs = 20000): Promise<void> => { const start = Date.now() while (Date.now() - start < timeoutMs) { if (signal?.aborted || !manager.isRunning) return const idle = Date.now() - lastOutputAt if (hasMarkers) { if (inputReady && idle >= idleReadyMs) return } else if (sawOutput && idle >= idleReadyMs) { return } await sleep(80) } } // when a prompt/idle marker is seen: inputReady = true // immediately before/after submitting a prompt: inputReady = false await submitMessage(next.message) setThinking(true)
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff. The previous
machines.tsforwarding issue is fixed; one PTY queue-ordering issue remains.
Testing
- Not run (review automation). Add a
runAgentPtytest with two queued messages where the first turn emits non-prompt output and then goes quiet; assert the second message is not written until a later prompt/idle marker arrives.
HAPI Bot
|
|
||
| // Queue semantics: wait until output goes idle (agent back at the | ||
| // prompt) before sending the next queued message. | ||
| await waitForInputReady() |
There was a problem hiding this comment.
[Major] Queued PTY prompts can be submitted before Claude is idle
promptSeen is set once when the first prompt marker appears and is never cleared. After the first submit, this waitForInputReady() call therefore only waits for idleReadyMs of output silence before sending the next queued message. A long Claude response can have a 500ms quiet gap before the next input prompt renders, so queued messages can be typed into a busy TUI and get dropped, appended to the wrong input, or interrupt the current turn.
Suggested fix:
let inputReady = false
const waitForInputReady = async (timeoutMs = 20000): Promise<void> => {
const start = Date.now()
while (Date.now() - start < timeoutMs) {
if (signal?.aborted || !manager.isRunning) return
const idle = Date.now() - lastOutputAt
if (hasMarkers) {
if (inputReady && idle >= idleReadyMs) return
} else if (sawOutput && idle >= idleReadyMs) {
return
}
await sleep(80)
}
}
// when a prompt/idle marker is seen:
inputReady = true
// immediately before/after submitting a prompt:
inputReady = false
await submitMessage(next.message)
setThinking(true)waitForInputReady latched promptSeen permanently, so after the first prompt every later wait returned on just output silence (~500ms). A quiet gap mid-response then satisfied it, letting a queued message be typed into a busy TUI — dropped, appended to the wrong input, or interrupting the turn. Replace the latched check with a re-armable inputReady flag: set by a prompt/idle marker (or the idle watchdog when an idle marker is missed), cleared on a busy marker and on each submit. A queued message now waits for a fresh prompt instead of any silence gap.
handleAbortRequest reset the queue without firing onBatchConsumed, so messages queued behind the aborted turn were cleared on the CLI but left invoked_at=null on the hub: they showed as permanently "queued" in the web and got re-delivered to the fresh prompt by seq-backfill on reconnect, defeating the abort. Collect their localIds and emit them consumed before the reset so the hub clears them.
resumeSession waited only for waitForSessionActive (session-alive), with the ready gate scoped to Cursor ACP. A PTY resume that never reached a usable claude prompt (auth failure, bad --resume, early exit) therefore returned success and dropped the user into a black terminal — the same gap the spawn path already closed. Add the pty session-ready wait to the resume path.
There was a problem hiding this comment.
Findings
- [Major] PTY queue readiness timeout still submits into a busy turn —
waitForInputReady()falls out after 20s without telling the caller whether a fresh prompt/idle marker was seen, and the caller then submits the queued message anyway. A normal Claude turn can run longer than 20s while continuing to emit busy output, so the second queued prompt is typed into the active TUI instead of waiting for the next input prompt. Evidencecli/src/agent/runAgentPty.ts:358.
Suggested fix:const waitForInputReady = async (timeoutMs = 20000): Promise<boolean> => { const start = Date.now() while (Date.now() - start < timeoutMs) { if (signal?.aborted || !manager.isRunning) return false const idle = Date.now() - lastOutputAt if (hasMarkers) { if (inputReady && idle >= idleReadyMs) return true } else if (sawOutput && idle >= idleReadyMs) { return true } await sleep(80) } return false } const ready = await waitForInputReady() if (!ready || !manager.isRunning || signal?.aborted) { break }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff. The previous prompt-marker latch issue was partially addressed, but the remaining timeout path still allows queued PTY messages to be submitted before Claude returns to an input prompt.
Testing
- Not run (review automation). Add a
runAgentPtytest where a second queued message waits longer thantimeoutMswhile busy output continues and assert it is not written until a later idle/prompt marker.
HAPI Bot
|
|
||
| // Queue semantics: wait until output goes idle (agent back at the | ||
| // prompt) before sending the next queued message. | ||
| await waitForInputReady() |
There was a problem hiding this comment.
[Major] waitForInputReady() currently returns void both when the prompt is actually ready and when the 20s timeout expires. Because this call is followed by submitMessage(), any queued prompt can still be written into a long-running Claude turn after the timeout, before a fresh prompt/idle marker arrives.\n\nSuggested fix:\nts\nconst ready = await waitForInputReady()\nif (!ready || !manager.isRunning || signal?.aborted) {\n break\n}\n\nThen make waitForInputReady() return true only when the readiness condition is met and false on timeout/abort/exit.
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
1 participant
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.
What
Adds an opt-in interactive PTY mode for Claude Code. Instead of spawning the agent headlessly over the SDK, the runner launches
claudeinside a real PTY and the web UI shows both a live terminal view and the usual structured chat — driven from the same session.Enable it per-session with the new "PTY mode" checkbox in the new-session form (Claude only; off by default).
Why
A real PTY runs Claude Code through its interactive entrypoint, so the agent behaves exactly as it does in a local terminal (TUI, slash commands, model switching) while remote users keep the structured chat they already rely on. The live terminal makes long-running or interactive turns observable from the phone.
There's also a usage-accounting reason to prefer the interactive entrypoint. Anthropic now meters interactive Claude Code separately from programmatic Agent-SDK use: subscription usage limits "stay reserved for interactive use of Claude Code," whereas "Claude Agent SDK and
claude -pusage no longer counts toward your Claude plan's usage limits" and instead draws on a separate Agent-SDK monthly credit or pay-as-you-go API rates (policy). HAPI is a remote control over a Claude Code session a person is actively driving, not a headless automation product — in this mode it is essentially no different from an SSH client: a transport that relays an interactive terminal to and from the user. Launching the genuine interactive entrypoint (PTY) instead of the SDK simply keeps that usage categorized as the interactive session it really is.How it works
claudeinteractive entrypoint in a PTY and drives it from chat — the first message, in-place/modeland/effortswitches, and--resume. First-run folder-trust is pre-accepted in a disposableCLAUDE_CONFIG_DIR, so the user's own config is never touched.canUseTool, which a PTY agent doesn't have — so a PreToolUse hook forwards each tool call to the runner, which surfaces it in the existing web approval modal and returns the decision to claude. Read-only tools auto-allow; everything else asks the web. The decision is always allow/deny (neverask, which would drop back to the TUI) and fails closed on errors, so the flow never silently auto-runs or hangs.AskUserQuestionis answered in the chat the same way.Screenshots
Testing
cli/web(vitest),hub/shared(bun test): full suites green; TypeScript strict clean across all packages.Scope
Claude-only and opt-in. The driver is written generically so other PTY-based flavors can follow without reworking it. No schema migrations, and no change to existing (non-PTY) sessions. A second flavor (Antigravity CLI) will follow on the same driver in a separate PR.