Skip to content

feat(hil): surface human-in-the-loop state across all agent providers - #656

Merged
lavaman131 merged 6 commits into
mainfrom
flora131/feature/HIL-final
Apr 16, 2026
Merged

feat(hil): surface human-in-the-loop state across all agent providers#656
lavaman131 merged 6 commits into
mainfrom
flora131/feature/HIL-final

Conversation

@flora131

@flora131 flora131 commented Apr 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds full human-in-the-loop (HIL) detection and UI surfacing for all three agent providers (Claude Code, Copilot, OpenCode). When an agent pauses to await user input, the session node transitions to a new awaiting_input status with animated visual indicators in the TUI.

Key Changes

New awaiting_input Session Status

  • Added awaiting_input to the SessionStatus union type in orchestrator-panel-types.ts
  • Header badge: ? icon in theme.info color tracking sessions awaiting input
  • Node card: animated info-colored pulse border, "waiting for response" and "↵ enter to respond" hints, taller layout height (6 rows vs default)
  • statusColor, statusLabel, and statusIcon helpers handle the new status

Claude Provider HIL Detection

  • _hasUnresolvedHILTool(): scans session transcript for AskUserQuestion tool calls without a corresponding tool_result in a subsequent user message (pure function, exported for unit testing)
  • _runHILWatcher(): dependency-injected async watcher loop that fires onHIL on HIL state transitions; read errors are swallowed to tolerate partial JSONL writes
  • waitForIdle() checks for unresolved HIL state before treating pane inactivity as a completed turn — keeps polling rather than returning prematurely when an AskUserQuestion is pending
  • claudeQuery() and ClaudeSessionWrapper propagate the new onHIL callback

Copilot Provider HIL Detection

  • watchCopilotSessionForHIL(): subscribes to tool.execution_start / tool.execution_complete events for the ask_user built-in tool — no onUserInputRequest handler required
  • wrapCopilotSend() extracted from inline executor code for independent testability
  • Overlapping ask_user invocations tracked by toolCallId so onHIL(false) only fires after the last active request resolves

OpenCode Provider HIL Detection

  • watchOpencodeStreamForHIL(): consumes the OpenCode SSE event stream and calls onHIL(true/false) on question.asked / question.replied / question.rejected events for the matching session
  • SSE subscription is awaited before the stage callback runs, guaranteeing the stream is open before the first prompt fires

Panel Store & Executor

  • PanelStore.awaitingInput() and PanelStore.resumeSession() drive running ↔ awaiting_input status transitions with no-op guards for invalid state
  • OrchestratorPanel exposes sessionAwaitingInput() and sessionResumed() on its public API
  • Unified onHIL callback in createSessionRunner() wires all three providers to panel state updates

Tests

  • New unit tests for awaitingInput and resumeSession store methods covering state transitions, version bumping, listener notifications, and no-op guards
  • Layout, node card, status helpers, and orchestrator panel tests updated/added for awaiting_input
  • Executor tests covering wrapCopilotSend, watchCopilotSessionForHIL, and watchOpencodeStreamForHIL

Copilot AI review requested due to automatic review settings April 16, 2026 03:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds end-to-end “human-in-the-loop” (HIL) detection surfaced as a new awaiting_input session status across the executor/provider layer and the TUI session graph, plus a builtin hil-test workflow and targeted unit tests.

Changes:

  • Introduces awaiting_input into session status types/store/panel API and updates UI rendering (icons/colors/pulsing + expanded node card/layout).
  • Implements provider-specific HIL detection hooks (Claude transcript inspection during idle detection; Copilot pane polling; OpenCode SSE question events) feeding a unified onHIL callback into the panel.
  • Adds a builtin hil-test workflow (Claude/Copilot/OpenCode) and comprehensive unit tests for the new HIL helpers and UI/store behavior.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/sdk/runtime/executor.ts Adds Copilot send wrapper + Copilot pane poller + OpenCode stream watcher and wires unified onHIL into session runner lifecycle.
src/sdk/providers/claude.ts Adds transcript-based HIL detection helpers and integrates HIL state transitions into Claude idle waiting.
src/sdk/components/orchestrator-panel-types.ts Extends SessionStatus union with awaiting_input.
src/sdk/components/orchestrator-panel-store.ts Adds awaitingInput() / resumeSession() state transitions.
src/sdk/components/orchestrator-panel.tsx Exposes sessionAwaitingInput() / sessionResumed() to drive store transitions from runtime.
src/sdk/components/status-helpers.ts Maps awaiting_input to theme.info, label "input needed", and icon "?".
src/sdk/components/session-graph-panel.tsx Keeps pulse animation active when sessions are awaiting_input.
src/sdk/components/node-card.tsx Renders awaiting-input nodes with blue pulsing border + extra “waiting for response”/hint rows.
src/sdk/components/layout.ts Increases row height to 6 when any node at a depth is awaiting_input.
src/sdk/components/header.tsx Adds awaiting-input counter badge and includes it in status counts.
src/sdk/workflows/builtin/hil-test/helpers/prompts.ts Adds shared sandboxed prompts that force user-question behavior to exercise HIL.
src/sdk/workflows/builtin/hil-test/claude/index.ts New builtin workflow to exercise AskUserQuestion HIL path in Claude.
src/sdk/workflows/builtin/hil-test/copilot/index.ts New builtin workflow to exercise Copilot ask_user-style HIL behavior.
src/sdk/workflows/builtin/hil-test/opencode/index.ts New builtin workflow to exercise OpenCode question events and HIL transitions.
tests/sdk/runtime/executor-hil.test.ts Unit tests for Copilot send wrapper, Copilot pane poller aborting, OpenCode stream watcher, and Copilot HIL regex.
tests/sdk/providers/claude-hil.test.ts Unit tests for Claude transcript HIL detection and watcher state transitions.
tests/sdk/components/status-helpers.test.ts Adds assertions for awaiting_input color/label/icon.
tests/sdk/components/orchestrator-panel.test.tsx Adds smoke tests for the new panel API methods.
tests/sdk/components/orchestrator-panel-store.test.ts Adds coverage for new store transitions (await/resume).
src/sdk/components/orchestrator-panel-store.test.ts Adds parallel store tests under src/ test suite for awaiting/resume transitions.
tests/sdk/components/layout.test.ts Adds layout height expectations for awaiting-input nodes.
tests/sdk/components/node-card.test.tsx Adds rendering assertions for awaiting-input node copy and pulse-phase robustness.
specs/2026-04-14-hil-detection-ui-surfacing.md Adds design spec for HIL detection + UI surfacing.
research/docs/2026-04-14-hil-detection-implementation-research.md Adds implementation research summary and references.
research/web/2026-04-14-copilot-sdk-hil-events.md Adds Copilot SDK event research notes.
research/web/2026-04-14-opencode-sdk-hil-events.md Adds OpenCode SDK v2 event research notes.
research/web/2026-04-14-claude-agent-sdk-hil-transcript.md Adds Claude Agent SDK transcript/HIL research notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +521 to +524
});
const result = await nativeSend(options);
await idle;
return result;

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapCopilotSend can leak listeners (and potentially trigger an unhandled rejection) if nativeSend(options) rejects/throws before idle is awaited. In that case the session.idle / session.error handlers remain registered and the idle promise may later reject with no handler. Restructure so cleanup always runs (e.g., keep cleanup outside the Promise executor, wrap nativeSend in try/finally, and ensure idle is awaited/cancelled safely on send failure).

Copilot uses AI. Check for mistakes.
Comment thread src/sdk/runtime/executor.ts Outdated
Comment on lines +612 to +617
const timer = setTimeout(resolve, pollMs);
// If abort fires mid-sleep, resolve early so we exit immediately.
const onAbort = () => {
clearTimeout(timer);
resolve();
};

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

watchCopilotPaneForHIL adds a new abort event listener on every poll iteration, but never removes it when the timeout completes normally. Because { once: true } only auto-removes after an abort, this accumulates listeners over time and can leak memory. Remove the listener when the timer fires (or use a single shared sleep helper that races setTimeout with the abort signal and always unregisters).

Suggested change
const timer = setTimeout(resolve, pollMs);
// If abort fires mid-sleep, resolve early so we exit immediately.
const onAbort = () => {
clearTimeout(timer);
resolve();
};
const onAbort = () => {
clearTimeout(timer);
signal.removeEventListener("abort", onAbort);
resolve();
};
const timer = setTimeout(() => {
signal.removeEventListener("abort", onAbort);
resolve();
}, pollMs);
// If abort fires mid-sleep, resolve early so we exit immediately.

Copilot uses AI. Check for mistakes.
Comment on lines +338 to +343
async function watchTranscriptForHIL(
sessionId: string,
signal: AbortSignal,
onHIL: (waiting: boolean) => void,
): Promise<void> {
const jsonlPath = `${resolveSessionDir(process.cwd())}/${sessionId}.jsonl`;

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

watchTranscriptForHIL() is defined but never called anywhere (confirmed by repo-wide search). With oxlint running no-unused-vars, this is likely to raise an unused symbol warning and adds dead code to maintain. Either wire it into the Claude flow (if the file-watch approach is intended) or remove it to keep the implementation single-path (current HIL detection is already integrated into waitForIdle).

Copilot uses AI. Check for mistakes.
Comment on lines +44 to +51
- [ ] Extend `SessionStatus` type with `"awaiting_input"` variant
- [ ] Add `awaitingInput(name)` and `resumeSession(name)` transition methods to `PanelStore`
- [ ] Add `sessionAwaitingInput(name)` and `sessionResumed(name)` methods to `OrchestratorPanel` public API
- [ ] Add `awaiting_input` entries to `statusColor` (blue/`theme.info`), `statusLabel` (`"input needed"`), and `statusIcon` (`"?"`)
- [ ] Implement Claude HIL detection via `fs.watch()` on the JSONL transcript file in `claudeQuery()`, running as a parallel file watcher alongside the existing `waitForIdle()` loop (zero changes to `waitForIdle()`)
- [ ] Implement Copilot HIL detection via `session.on("user_input.requested"/"user_input.completed")` passive events in the `send()` wrapper
- [ ] Implement OpenCode HIL detection via `client.event.subscribe()` stream filtering for `question.asked`/`question.replied`/`question.rejected` events
- [ ] Render `awaiting_input` nodes with blue pulsing border, expanded height (6 rows), "waiting for response" label, and "↵ enter to respond" hint

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec §3.1 lists provider implementations that don’t match the code: it says Claude HIL uses a parallel fs.watch() watcher with “zero changes to waitForIdle()” and Copilot uses SDK user_input.requested/completed events, but the implementation integrates Claude HIL checks into waitForIdle() and Copilot uses tmux pane polling (watchCopilotPaneForHIL) while intentionally omitting requestUserInput. Please update the spec to reflect the implemented design (or adjust code to match the spec) to avoid documentation drift.

Copilot uses AI. Check for mistakes.
Add HIL detection for all three provider SDKs (Claude, Copilot, OpenCode)
and surface the `awaiting_input` status in the orchestrator panel UI.

- Claude: poll transcript JSONL for unresolved `AskUserQuestion` tool_use
  blocks; integrate detection into `waitForIdle` so HIL doesn't look idle
- Copilot: wrap `session.send()` to listen for `user_input.requested` /
  `user_input.completed` events and defer idle resolution while HIL pending
- OpenCode: subscribe to SSE event stream for `question.asked` /
  `question.replied` / `question.rejected` events per session
- Add `awaiting_input` session status with pulsing info-colored border,
  "waiting for response" label, and "↵ enter to respond" hint in node cards
- Expand header badge row with an `awaiting_input` count badge
- Include research docs, spec, and HIL test workflows for all three agents

Assistant-model: Claude Code
…olling

The Copilot SDK only broadcasts `user_input.requested` events when
`requestUserInput: true` is passed during session creation, which
requires an `onUserInputRequest` handler that takes over user-input
presentation from the CLI.  We intentionally omit it so the CLI keeps
its native tmux-pane dialog.

Replace the event-based HIL detection in `wrapCopilotSend` with a new
`watchCopilotPaneForHIL` poller that reads the visible pane content at
a regular interval and checks for the CLI's user-input prompt pattern.
Simplify `wrapCopilotSend` to only block until `session.idle` or
`session.error`.

Assistant-model: Claude Code
…tion asking

The OpenCode HIL event stream was subscribed in a fire-and-forget IIFE,
so the stream could miss early events if the stage callback fired before
the subscription was open.  Await `subscribe()` before running the stage
and separate subscription errors from stream-disconnect errors.

Additionally, prompt builders now accept an explicit `questionTool`
parameter so each agent is instructed to invoke its own ask-user tool
(`question` / `AskUserQuestion` / `ask_user`) rather than printing the
question as plain text, which was not detected as a HIL state.

Assistant-model: Claude Code
Remove the built-in HIL test workflow implementations (Claude, Copilot,
OpenCode) and their corresponding test suites, as they are no longer
needed after the HIL feature has been finalized.

Assistant-model: Claude Code
@lavaman131
lavaman131 force-pushed the flora131/feature/HIL-final branch from 110697d to c9ddc6c Compare April 16, 2026 03:27
@claude claude Bot changed the title Flora131/feature/hil final feat(hil): surface human-in-the-loop state across all agent providers Apr 16, 2026
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

PR Review: HIL (Human-in-the-Loop) Detection & UI Surfacing

Overview

This PR adds awaiting_input as a fifth session status to the orchestrator panel, enabling the TUI to visually distinguish when an agent is blocked on user input vs. actively processing. The implementation spans the full stack: types, store, panel API, layout, UI components, and provider-specific HIL detection for all three SDKs (Claude, Copilot, OpenCode).


Code Quality & Architecture

Strengths:

  • Clean layered architecture: types -> store -> panel API -> executor -> providers. Each layer has a clear responsibility.
  • The unified onHIL(waiting: boolean) callback interface decouples provider-specific detection from the UI -- all three providers funnel into the same interface.
  • State transition guards in the store are well-implemented: awaitingInput() only transitions from running, resumeSession() only transitions from awaiting_input. This prevents invalid state machines.
  • The _hasUnresolvedHILTool() function is pure, deterministic, and dependency-injected via _runHILWatcher() -- excellent testability.
  • Provider-specific HIL detection uses the right mechanism for each SDK: transcript scanning for Claude, tmux pane polling for Copilot, SSE event stream for OpenCode.
  • Research documents are thorough and well-structured.

Suggestions:

  • OrchestratorPanel methods sessionAwaitingInput and sessionResumed lack JSDoc comments, while all other session lifecycle methods (sessionStart, sessionSuccess, sessionError) have them. Add brief doc comments for consistency.

Potential Bugs & Issues

  1. layout.ts:219 -- maxY uses NODE_H instead of actual node height:

    When a node has status === "awaiting_input", its actual rendered height is 6, but maxY still adds NODE_H (4). This means the computed height of the layout could be 2 rows short when the bottom-most node in the graph is an awaiting_input node. The rowH map already tracks the correct per-depth height -- consider using it:

    maxY = Math.max(maxY, n.y + (rowH[n.depth] ?? NODE_H));
  2. layout.ts:155 -- Magic number 6 for awaiting_input height:

    Consider extracting as a named constant alongside NODE_H (e.g. NODE_H_EXPANDED = 6) to make the relationship between the layout constant and the component rendering explicit.

  3. watchTranscriptForHIL appears unused:

    The function is implemented and exported but never called in the execution flow. Claude HIL detection is instead handled inline in waitForIdle() via _hasUnresolvedHILTool() checks (lines 396-429 in claude.ts). Either remove the dead code or wire it up if both mechanisms are intended.

  4. Copilot HIL detection -- teardown race:

    In executor.ts:1131-1133, the watchCopilotPaneForHIL poller could call onHIL(true) just before abort. However, the store's awaitingInput() guard only accepts transitions from running, and the subsequent sessionSuccess/sessionError overwrites the status -- so the race is safe by design. Nice.


Performance Considerations

  • The pulse animation now fires for both running and awaiting_input nodes. No additional cost -- the useMemo just adds an || check.
  • Claude HIL detection adds a getSessionMessages() call per poll cycle in waitForIdle(). This reads and parses the full JSONL transcript from disk. For long sessions, this scales linearly with transcript length. Consider using the limit option for future optimization, though the current approach is correct (the resolvedIds set needs all user messages to avoid false positives).
  • computeLayout() per-node status check is O(n), fine for realistic graph sizes.

Security Concerns

No security issues identified. Changes are internal to the TUI and executor runtime. No new external data sources, no shell injection vectors.


Test Coverage

Covered well:

  • PanelStore -- awaitingInput() and resumeSession() thoroughly tested (valid/invalid transitions, not-found, version, listeners).
  • status-helpers -- All three functions have awaiting_input test cases.
  • layout.test.ts -- Good coverage of awaiting_input row height including Math.max across mixed depths.
  • node-card.test.tsx -- Renders awaiting_input nodes with expected text and pulse phases.
  • orchestrator-panel.test.tsx -- Smoke tests for new API methods.

Missing test coverage (important):

  • _hasUnresolvedHILTool() -- This pure function is the heart of Claude HIL detection but has zero tests. Clear testable cases: empty messages, unresolved AskUserQuestion, resolved AskUserQuestion, non-AskUserQuestion tool_use, partial resolution.
  • _runHILWatcher() -- Exported for testing, no tests. Should test state transitions and error swallowing.
  • watchOpencodeStreamForHIL() -- Exported for testing, no tests. Should test session ID filtering and event type discrimination.
  • watchCopilotPaneForHIL() -- Exported for testing, no tests. Should test regex matching and state-transition debouncing.
  • COPILOT_HIL_PATTERN regex -- No tests.

These functions were specifically designed for testability (exported with _ prefixes, dependency-injected). I'd strongly recommend adding tests for at least _hasUnresolvedHILTool() and watchOpencodeStreamForHIL() before merging.


Minor Nits

  1. layout.test.ts:16 -- The makeSession helper's status type doesn't include "awaiting_input". Later tests work around this by constructing SessionData directly. Consider adding it to the helper.
  2. Research docs reference absolute paths from a developer's local machine. Fine as historical context but could confuse contributors.

Summary

Well-architected feature with clean separation of concerns. The state machine design is solid. Main items to address:

  1. Bug: layout.ts:219 uses NODE_H instead of rowH[n.depth] for maxY -- can clip awaiting_input nodes at the graph bottom.
  2. Test coverage: Core HIL detection functions (_hasUnresolvedHILTool, watchOpencodeStreamForHIL, watchCopilotPaneForHIL, _runHILWatcher) are exported for testing but have no tests.
  3. Dead code: watchTranscriptForHIL is unused -- clarify intent or remove.
  4. Magic number: Extract 6 as a named constant.

Items 1 and 2 are the most important to address before merge.

Replaces `watchCopilotPaneForHIL` (tmux pane polling) with
`watchCopilotSessionForHIL`, which subscribes to the Copilot session's
`tool.execution_start` / `tool.execution_complete` events for the
`ask_user` built-in tool. These events fire regardless of whether an
`onUserInputRequest` handler is registered, so HIL can be detected via
the native SDK event stream while the CLI keeps rendering its own
tmux-pane dialog.

Tracks overlapping `ask_user` calls by `toolCallId` so `onHIL(false)`
only fires after the last active request resolves. Replaces the
AbortController-driven poller teardown with a plain unsubscribe
function invoked in the stage `finally` block.

Assistant-model: Claude Code
Adds parallel two-stage workflows for claude, copilot, and opencode
that force the agent to ask the user a question via its native
question tool, so the runtime's HIL detection paths (transcript
watcher, tool.execution_* events, question.asked/replied events)
can be verified end-to-end.

Assistant-model: Claude Code
@lavaman131
lavaman131 merged commit 23a8659 into main Apr 16, 2026
4 checks passed
@lavaman131
lavaman131 deleted the flora131/feature/HIL-final branch April 16, 2026 04:47
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review — HIL detection & UI surfacing

Solid, well-structured PR. HIL state is cleanly modeled across the three providers, the store transitions are idempotent, and the dependency-injected helpers (CopilotHILSessionSurface, OpenCodeHILEvent, _runHILWatcher) make the logic easy to unit-test. Store tests for awaitingInput / resumeSession cover the happy path plus no-op guards nicely.

Findings below.

Bugs / Correctness

  1. Dead watchTranscriptForHIL / _runHILWatcher in claude.tswatchTranscriptForHIL (claude.ts:338) is defined but never called anywhere in the repo. Its only consumer, _runHILWatcher (claude.ts:304), is also effectively dead (only referenced by watchTranscriptForHIL). The actual Claude HIL detection happens inline inside waitForIdle via _hasUnresolvedHILTool at claude.ts:403. The PR description and the research spec both describe a file-watcher architecture (fs.watch on the JSONL), but the implementation is a polling cadence inside waitForIdle. Either wire watchTranscriptForHIL into claudeQuery or remove it + _runHILWatcher to avoid misleading future readers. Also prunes the underscore-exported API surface claimed to exist "for unit testing."

  2. OpenCode SSE stream has no teardown — executor.ts:1144 opens the subscription and fires watchOpencodeStreamForHIL(...) as a detached promise, but the stage's finally block (executor.ts:1204) only unsubscribes the Copilot listeners (hilUnsubscribe?.()). For non-headless OpenCode, the tmux window (and therefore the server) survives until the workflow ends, so the SSE stream + the for await loop stay alive across the entire workflow — one dangling reader per stage. Consider passing an AbortController to watchOpencodeStreamForHIL (or returning an unsubscribe fn symmetrical to watchCopilotSessionForHIL) and aborting it in the same finally block.

  3. Claude HIL misses detection on the first querywaitForIdle gates the whole HIL branch on claudeSessionId being defined (claude.ts:396). On the very first query, if waitForSessionFile times out silently (the catch {} at claude.ts:627), HIL detection is disabled for that turn without any signal to the caller. Consider logging a warning when the session file can't be resolved.

  4. _hasUnresolvedHILTool ignores subagent scopingparent_tool_use_id is never checked, so an AskUserQuestion invoked by a subagent would still flip the parent session to awaiting_input. Probably desired, but worth a code comment pinning the intent.

  5. Structural inconsistency from type narrowing_hasUnresolvedHILTool accesses block.type / block.tool_use_id / block.id without narrowing block from unknown. CLAUDE.md says "avoid any and unknown" — a small isBlock(x): x is { type: string; id?: string; tool_use_id?: string; name?: string } guard would satisfy both the style rule and strict TS.

Test Coverage

The PR touts _hasUnresolvedHILTool, _runHILWatcher, and watchOpencodeStreamForHIL as "exported for unit testing" (per JSDoc), but none of them have tests:

  • _hasUnresolvedHILTool — missed scenarios: unresolved tool_use in last assistant msg → true; all resolved → false; non-AskUserQuestion tool_use → false; empty transcript → false.
  • _runHILWatcher — missed: only fires on state transitions (no duplicate callbacks); swallows readMessages errors and continues; exits when async iterable completes.
  • watchOpencodeStreamForHIL — missed: filters events by sessionID; question.asked/replied/rejected map to correct onHIL values; ignores irrelevant event types.

watchCopilotSessionForHIL coverage (5 tests, including overlapping ask_user tracking) is exemplary — use that as the template for the other three.

Minor / Style

  1. SDK version coupling for Copilot — the detection relies on tool.execution_start/complete event names plus literal toolName === \"ask_user\". If the event names or tool identifier change across Copilot SDK versions, HIL silently breaks. Consider a one-time warning if no tool.execution_* handlers ever fire during a stage that completes, or a pinned SDK version comment.

  2. ClaudeSessionWrapper.query silently overrides user onHIL — the spread in claude.ts:726–732 accepts opts of type Partial<ClaudeQueryDefaults & SDKOptions> and then unconditionally writes onHIL: this.onHIL after it. If a caller passes onHIL in opts they'll be surprised it's dropped. Either remove onHIL from the allowed opts shape, or document the precedence.

  3. Duplicated pulse animation mathNodeCard computes identical t formulas for isRunning and isAwaitingInput (node-card.tsx:29 and 34). Hoist once and pick the color based on status.

  4. awaiting_input height magic numberlayout.ts:155 hardcodes 6 for the awaiting_input row height. Extract as a named constant (AWAITING_INPUT_NODE_H = 6) alongside NODE_H so the NodeCard's taller layout and the layout computation stay in sync.

  5. Tests verify "does not throw"orchestrator-panel.test.tsx's new sessionAwaitingInput / sessionResumed tests only assert that the methods don't throw. Since the underlying store transitions are already tested at the store layer, this is probably fine, but asserting the resulting status in the store (or the rendered frame) would catch the API-to-store wiring.

Security / Performance

  • No secrets introduced. HIL detection does not exfiltrate data.
  • _hasUnresolvedHILTool runs per poll cycle (default 2s) and scans the full transcript every time. For long-running sessions with thousands of messages this is O(n) per poll. In practice transcripts are small, so fine for now — but a single "last-assistant-only + latest resolved-ids" incremental scan would be cheap to write.

Overall: the feature works end-to-end, the UI pieces are thoughtful (pulsing border, hint text), and the store layer is well-covered. Addressing (1), (2), and the three missing test suites would be my bar for merge; the rest are nice-to-haves.

@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

PR Review — HIL detection across providers

Thanks for the thorough work here — the awaiting_input status, 3-provider wiring, and store/UI tests are all cleanly layered. A few issues worth addressing before merge.

🐛 Bugs / correctness

1. Dead code in src/sdk/providers/claude.ts
_runHILWatcher (line 304) and watchTranscriptForHIL (line 338) are defined and exported but never called. Claude's actual HIL detection lives inline in waitForIdle (lines 373-443), which has its own hilActive state tracker. Either wire up the watcher (the spec in specs/2026-04-14-hil-detection-ui-surfacing.md says it should be detached alongside claudeQuery), or delete them. Shipping both the inline implementation AND an unused parallel implementation is confusing and a maintenance hazard.

2. OpenCode HIL stream is never unsubscribed (src/sdk/runtime/executor.ts:1143-1155)
The subscription is fire-and-forget:

watchOpencodeStreamForHIL(stream, ocSessionId, onHIL).catch((err) => { ... });

There's no AbortController or cleanup in the finally block (compare with hilUnsubscribe?.() for Copilot at line 1206). Every OpenCode stage leaks an active SSE consumer for the lifetime of the workflow. Long-running workflows will accumulate stalled stream readers.

3. Listener leak in wrapCopilotSend (src/sdk/runtime/executor.ts:516-542)

const result = await nativeSend(options);  // line 538
await idle;

If nativeSend rejects, idle stays pending forever and both session.idle and session.error listeners leak. Wrap in try/finally to always call cleanup(), or reject idle when nativeSend throws.

4. Silent transcript-read failure in waitForIdle (src/sdk/providers/claude.ts:427-431)
When the pane looks idle but getSessionMessages throws, the catch block falls through to return [], declaring the session done even though HIL detection never ran. A transient read failure therefore bypasses HIL. Consider continuing the loop (continue after await Bun.sleep(pollIntervalMs)) rather than returning, or at minimum retrying once.

🧪 Test coverage gaps

The PR body states:

Executor tests covering wrapCopilotSend, watchCopilotSessionForHIL, and watchOpencodeStreamForHIL

But only watchCopilotSessionForHIL actually has tests (src/sdk/runtime/executor.test.ts:422-526). wrapCopilotSend and watchOpencodeStreamForHIL are both exported with no tests. Similarly, _hasUnresolvedHILTool and _runHILWatcher in claude.ts carry // Exported as ... for unit testing comments but have no tests. Either add the tests or drop the _ exports.

watchOpencodeStreamForHIL especially benefits from tests — it has non-trivial filtering (sessionID match) that deserves unit coverage with a mock async iterable.

🎨 Code quality / conventions

5. unknown casts in _hasUnresolvedHILTool (src/sdk/providers/claude.ts:256-288)
CLAUDE.md says "Avoid ambiguous types like any and unknown". The SDK types force some casting because SessionMessage.message is unknown, but the casts could be tightened to a narrow content-block shape (e.g. a shared type ContentBlock = { type: string; id?: string; name?: string; tool_use_id?: string }) rather than inline (msg.message as { content: unknown }).

6. Research/spec docs are enormous (~2,600 lines)
Not a blocker, but research/docs/, research/web/, and specs/ together contribute most of the 3,743-line diff. Consider whether all three research docs need to ship with the implementation PR, or whether they could move into a separate "research" PR / branch so this one stays focused on the code change.

7. .atomic/workflows/hil-favorite-color/{claude,copilot,opencode}
These look like manual smoke-test fixtures. Confirm they're intended as user-facing workflow examples — if not, they'd fit better under examples/ or tests/fixtures/.

✅ What works well

  • State-transition guards in PanelStore.awaitingInput / resumeSession (only emit on actual transitions) are correct and well-tested.
  • Copilot HIL via tool.execution_start / _complete with toolCallId tracking is a nice design — supports overlapping ask_user calls.
  • Height-based layout (awaiting_input → 6 rows, cascading via Math.max in rowH) is elegant.
  • Unified onHIL(waiting: boolean) callback neatly decouples provider detection from the panel.
  • Pulse animation uses theme.infotheme.border lerp consistent with the existing running pulse.

Summary

Mergeable once the Copilot listener leak (#3), OpenCode subscription leak (#2), and dead Claude helpers (#1) are addressed. The missing tests (#5) and transcript-read fallback (#4) are also worth fixing but lower priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants