feat(tasks): live view-mode switching for subagent conversations (v2) - #1313
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b742d9c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| try { | ||
| await tailOutputStream(handle.session.getOutputStream(), ctx, signal); |
There was a problem hiding this comment.
When the selected subagent is running, this calls AgentSession.getOutputStream(), whose implementation in src/agent/session/agent-session.ts:1248-1251 always throws because callers must use sendMessageStream() instead. The broad catch suppresses that error and the finally block prints “Task complete,” so every real running-task view exits immediately and falsely reports completion rather than tailing output.
Useful? React with 👍 / 👎.
| resolve(); | ||
| }); | ||
|
|
||
| process.stdin.on('data', onKeypress); |
There was a problem hiding this comment.
In the persistent-compositor REPL, stdin already has a keypress listener, so adding this independent data listener makes both consumers process every navigation key. For example, pressing Up in this menu also makes the compositor recall the previous user prompt; pressing Enter then queues that hidden prompt before this handler opens the selected task, causing it to be automatically resubmitted after returning. Suspend or replace the existing input surface while the menu owns navigation instead of attaching a competing stdin listener.
Useful? React with 👍 / 👎.
| if (!isRunning) { | ||
| if (ictx) ictx.viewingTaskId = undefined; | ||
| return; |
There was a problem hiding this comment.
For every completed in-memory subagent, this branch returns immediately after rendering a footer that says to press Esc. The slash handler therefore resumes the main REPL prompt at once, and the installed soft-stop callback no longer owns a blocking view; moreover, idle-mode Esc is handled through the input surface's idle-escape path rather than this soft-stop hook. As a result, completed conversations never actually enter the advertised view mode and cannot be dismissed with Esc as instructed.
Useful? React with 👍 / 👎.
…logging Implements the foundation for viewing subagent conversations in the REPL, analogous to Claude Code's /tasks feature. - `src/agent/subagent/log.ts` — SubagentLogWriter + SubagentLogReader. Always-on JSONL per subagent under state/subagent-logs/<session>/<id>.jsonl. Opt-out via AFK_SUBAGENT_LOG=0. - `src/agent/subagent/completed-cache.ts` — Bounded LRU of recently-completed subagent handles for memory-first /tasks:view. - `src/agent/subagent/handle.streaming.ts` — Extracted streaming methods from handle.ts (streamToFinalMessage, runToResult, runInBackground, dispatchStopAndRelease). Drops handle.ts from 417 to ~203 code lines. - `src/cli/output-event-format.ts` — Shared OutputEvent→text formatter, extracted from bgsub.ts and enriched with tool args preview. - `src/cli/commands/interactive/task-view.ts` — Replay renderer for subagent conversations (memory Message[] path + disk OutputEvent path). - `src/cli/slash/commands/tasks.ts` — /tasks (list), /tasks:view <id> (view conversation), /tasks:cancel <id> (cancel running). - `session-types.ts` — Added getHistory() to IAgentSession interface. - `subagent.ts` — Wired SubagentLogWriter + CompletedCache into forkSubagent. - `handle.ts` — Delegates to handle.streaming.ts, added _logWriter field. - `bgsub.ts` — Imports formatDiskEvent from shared module. - `slash/index.ts` — Registers tasksCommands. - `env.ts` — Registers AFK_SUBAGENT_LOG env var. - `paths.ts` — Added subagent-logs path helpers.
…(v2) - Add viewingTaskId?: string to InteractiveCtx (shared.ts) — tracks when REPL is in task-view mode - New task-view-mode.ts: enterTaskViewMode(), exitTaskViewMode(), renderTaskViewHeader(), buildTaskFooterLine() — manages live view UX - Live tailing: streams OutputEvent from handle.session.getOutputStream() for running subagents, updating output in real-time - Esc to return: wires setSoftStopHandler to exitTaskViewMode, restores status line and repaint - Arrow key navigation in /tasks list: ↑/↓ to move cursor, Enter to open task view, Esc to return to main prompt - /tasks:view <id> now enters live view mode via enterTaskViewMode() (replaces page-and-return v1 UX with proper mode switching) - 30 tests passing across tasks.test.ts (13) and task-view.test.ts (17)
24b742d to
db5fe9c
Compare
|
Shadow-verify is complete. Let me compile the final review. PR #1313 Review —
|
S-1 (high/security): Suspend TerminalCompositor before registering process.stdin 'data' listener in /tasks interactive mode. The compositor holds the stdin claim during slash dispatch; a second listener creates the dual-consumer phantom-turn bug (#511 class). Added suspendInput()/resumeInput() around the navigation promise with a cleanup() helper on all exit paths. F2 (high/correctness): Call exitTaskViewMode() in the finally block on natural stream completion. Previously, repaintStatusLine() and FOOTER_RETURN were only emitted on the Esc path, leaving the REPL visually stuck in task-view mode when a subagent finished naturally. S-2 (medium/security): Replace inline JSON.stringify rendering of in-memory history with renderMessagesView() from task-view.ts, which applies CONTENT_PREVIEW_CHARS=200 truncation on tool_use/tool_result content. This also wires the previously dead-code task-view.ts module (resolves finding F5). F4 (medium/correctness): Add .catch(() => resolve()) on the enterTaskViewMode promise chain so a rejection returns to the prompt instead of hanging the REPL indefinitely.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
Prior Automated Round — Finding Dispositions
1 prior automated review (on commit db5fe9c0, 2026-08-27). Current HEAD is a0074940 which explicitly addresses 4 prior blocking findings.
| Prior finding | Disposition |
|---|---|
S-1 (high): stdin dual-consumer — process.stdin.on('data') conflicts with compositor stdin claim |
FIXED — commit a0074940 adds compositor?.suspendInput() before the listener and compositor?.resumeInput() in cleanup() on all exit paths. |
F2 (high): natural stream completion never calls exitTaskViewMode — status line unreset |
FIXED — commit a0074940 adds exitTaskViewMode(entry) in the finally block behind if (!signal.aborted). Wave 1.5 verified the logic is correct (Esc path calls exitTaskViewMode directly, then signal.aborted is true in finally so the block is skipped — no double-call). |
S-2 (medium): in-memory history rendered with JSON.stringify — no truncation |
FIXED — commit a0074940 imports and calls renderMessagesView() from task-view.ts which applies CONTENT_PREVIEW_CHARS=200 truncation. This also wires F5 (dead code). |
F4 (medium): no .catch() on enterTaskViewMode promise chain — REPL hangs on rejection |
FIXED — commit a0074940 adds .catch(() => resolve()). |
F5 (medium, waived): renderMessagesView had zero callers (dead code) |
FIXED as a side-effect of the S-2 fix. |
| F6 (medium, waived): no tests for running path | STILL PRESENT — zero tests for the live tail loop, natural stream completion, or Esc during running. Advisory. |
| F7 (low): no backpressure on live-tail rendering | STILL PRESENT — per-event ctx.out.line() with no batching. Advisory. |
F3 (low): 'data' event vs 'keypress' inconsistency |
STILL PRESENT — advisory, no behavioral impact. |
F1 (low): dead wireEscapeToExit call immediately overwritten for running path |
STILL PRESENT in a different form — see new finding N2 below. Advisory. |
Decision
✅ MERGE (with follow-ups)
Severity arithmetic: 0 critical, 0 high → MERGE (with follow-ups); 2 medium advisory (non-blocking per sweep policy — only critical/high block), 2 low advisory, 1 nit advisory.
Per sweep policy: medium and below are non-blocking. They are listed as follow-ups below with suggested issue rather than fix commits.
Review Summary
PR: feat(tasks): live view-mode switching for subagent conversations (v2)
Reviewed ref: a0074940f98b0feb5ebfce920a33cdca309edb62
Regime: full (+1009/−124, 5 files: 2 new production, 1 new test, 2 modified)
CI: Lint & Build ✅, Test (ubuntu-latest) ✅, Test (macos-latest) ✅, PTY scrollback ✅, Docs ✅, Publish Bundle ✅. All green.
Wave 1.5 citation verification: 3 of 7 high-severity security findings were FABRICATED (premises refuted against a0074940):
- Security finding 1 (manager.get returns snapshot with no session): FABRICATED —
SubagentManager.get()returnsSubagentHandle | undefinedfromactive.get() ?? completed.get()?.handle;SubagentHandleImplcarriesreadonly session: IAgentSession. No crash. - Security findings 2/3/4 (compositor not resumed, viewingTaskId not cleared on abort): FABRICATED —
cleanup()IS called beforeenterTaskViewModedispatch (line 272); Esc handler explicitly callsexitTaskViewMode(entry)which clearsviewingTaskId; the "double-wire window" is synchronous code with no await — events cannot interleave. - Security finding 5 (dual data listeners): DROPPED — insufficient evidence that readline's internal listener survives
suspendInput; the design comment acknowledges the dual-consumer risk and correctly applies the fix.
The 4 prior blocking findings are verifiably fixed. The remaining findings are follow-up hygiene.
🟡 Advisory Findings (non-blocking — suggested as follow-up issues)
N1 · medium · advisory · correctness · task-view-mode.ts:185–188 · ref:a0074940
Disk path installs wireEscapeToExit handler then returns without clearing it.
When the disk fallback path completes (handle is null and disk replay returns), wireEscapeToExit(entry) is called then the function returns early. If the user returns to the task list without pressing Esc, the soft-stop handler installed by wireEscapeToExit remains live. On the next soft-stop event from any context, exitTaskViewMode(entry) fires against the already-exited view, emitting FOOTER_RETURN and calling repaintStatusLine() into whatever screen state is current.
wireEscapeToExit(entry);
if (ictx) ictx.viewingTaskId = undefined;
return; // handler not cleared — stale soft-stop handler survivesSuggested fix (follow-up): Before return, add entry.ctx.setSoftStopHandler?.(null).
N2 · medium · advisory · correctness · tasks.ts:274–280 · ref:a0074940
ictx absent from TaskViewEntry construction in /tasks interactive mode — viewingTaskId tracking inoperative.
The PR's stated new feature ("viewingTaskId tracking") is not wired at the primary call site. Both TaskViewEntry constructions in tasks.ts omit ictx, so enterTaskViewMode always sees ictx === undefined and the if (ictx) ictx.viewingTaskId = id guard never fires. The REPL loop cannot know the user is in task-view mode.
const entry: TaskViewEntry = {
id: selected.id, manager, sessionLabel, ctx,
// ictx omitted — viewingTaskId tracking never activates
};Suggested fix (follow-up): Thread InteractiveCtx into the /tasks and /tasks:view handlers (e.g. via an optional ictx on SlashContext or a module-scope set at bootstrap) and include it in the entry.
N3 · low · advisory · test-coverage · task-view.test.ts · ref:a0074940
Zero tests for the live tail loop (running path), natural stream completion, and Esc-during-running.
The 17 tests cover completed-handle memory path and disk fallback. The F2 fix (finally block on natural completion) is the explicitly stated fix from this PR round but has no test. A stated fix with no test is unverified at the test level (though Wave 1.5 verified the logic is correct).
Suggested fix (follow-up): Add tests: (a) tailOutputStream resolves naturally → assert exitTaskViewMode called, FOOTER_COMPLETE emitted; (b) abort fires → assert exitTaskViewMode NOT called from finally.
N4 · low · advisory · correctness · task-view.ts · ref:a0074940
renderEventsView exported but has zero callers.
task-view.ts exports renderEventsView(header, events, options) but nothing in this PR imports it. It is dead code. The memory path now uses renderMessagesView (S-2 fix), but renderEventsView was presumably intended as the disk-event counterpart and was never wired.
Suggested fix (follow-up): Either wire renderEventsView in replayDiskEvents (replacing the inline loop) or remove it until it has a defined call site.
N5 · nit · advisory · correctness · tasks.ts:280 · ref:a0074940
Blank .catch(() => resolve()) discards programming errors silently.
The F4 fix prevents hangs but makes any unexpected throw from enterTaskViewMode indistinguishable from success. A brief console/ctx error log before resolving would surface bugs during development.
void enterTaskViewMode(entry).then(resolve).catch(() => resolve());
// All errors silently resolve — no diagnostic signalSuggested fix: .catch((e) => { ctx.out.error?.(task view error: ${String(e)}); resolve(); })
Spec-Compliance
Stated intent: PR #1313 title+body — spec-compliance assessed.
| Requirement | Status |
|---|---|
viewingTaskId?: string on InteractiveCtx |
✅ Field added, optional, additive |
enterTaskViewMode / exitTaskViewMode — new file |
✅ Implemented in task-view-mode.ts |
Live tailing via handle.session.getOutputStream() |
✅ tailOutputStream streams AsyncIterable<OutputEvent> |
Esc to return via ctx.setSoftStopHandler |
✅ Wired in running-path override |
Arrow-key navigation in /tasks list |
✅ \x1b[A/\x1b[B + cursor state |
| S-1 fix: suspendInput/resumeInput around stdin listener | ✅ Confirmed at ref |
| F2 fix: exitTaskViewMode in finally block on natural completion | ✅ Confirmed at ref |
| S-2 fix: memory path uses renderMessagesView (truncated) | ✅ Confirmed at ref |
| F4 fix: .catch() on enterTaskViewMode dispatch | ✅ Confirmed at ref |
viewingTaskId tracking at /tasks call site |
ictx absent from entry (N2, advisory) |
| 30 tests passing | ✅ CI green |
Dimensions With No Issues
- Security — after Wave 1.5 drops fabricated findings: compositor suspend/resume is correctly symmetric; no new trust boundaries or injection surfaces;
ctx.out.line()renders text, no injection risk. - API-compat —
viewingTaskId?: stringis additive optional;formatHandleLinesignature change is module-private (zero external importers confirmed); new exports intask-view-mode.tsandtask-view.tsare net-new with no breaking changes. - Perf-observability —
renderList()only redraws on recognized arrow keys (if/else-if chain), not on every keystroke; clearScreen + redraw on arrow key is standard TUI practice for short lists.
What Was Not Checked
- Citations verified against branch HEAD
a0074940f98b0feb5ebfce920a33cdca309edb62via GitHub API file reads and Wave 1.5 inline verification. 3 of 7 proposed high-severity security findings were refuted and dropped. - Stated intent: PR #1313 title+body — spec-compliance assessed.
- Not checked: runtime test execution — static review only. CI shows all checks passing.
- Not checked: Telegram/daemon surface paths into
enterTaskViewMode—setSoftStopHandleris absent on those surfaces so the Esc path is a no-op; the live-tail loop runs to natural completion only. - Not checked: whether
suspendInput()inTerminalCompositorpauses the readlineInterface's internal'data'listener (insufficient evidence to assert either way). - Not checked: compositor re-arm state after task-view exit (pre-existing surface concern, not introduced by this PR).
1 participant
Summary
Implements v2 live view-mode switching for the
/tasksfeature, stacking on #1312 (the v1 always-on JSONL logging + page-and-return viewer).What's new in v2
1.
viewingTaskIdonInteractiveCtxAdded
viewingTaskId?: stringtoInteractiveCtx(shared.ts). When set, the REPL is in task-view mode tracking the subagent being viewed.2. Live view-mode switching (
task-view-mode.ts— new file)enterTaskViewMode()— clears screen, renders conversation history, starts live tailing for running subagents, wires Esc to exitexitTaskViewMode()— clearsviewingTaskId, repaint status line, emits return noticerenderTaskViewHeader()— builds status header with ID, type, and colored status badgebuildTaskFooterLine()— footer message indicating running vs completed state + Esc hint3. Live tailing
For running subagents,
enterTaskViewModesubscribes tohandle.session.getOutputStream()(anAsyncIterable<OutputEvent>) and appends new events in real-time viaformatOutputEvent. Stops when the stream is exhausted or Esc is pressed.4. Esc to return
Wired via
ctx.setSoftStopHandler— the existing REPL surface seam. Esc during any view (running or completed) callsexitTaskViewModeand clears the handler.5. Arrow-key navigation in
/taskslist/tasksnow shows a cursor-navigable list:enterTaskViewModesetSoftStopHandleris not available (non-TTY surfaces)Files changed
src/cli/commands/interactive/shared.tsviewingTaskId?: stringtoInteractiveCtxsrc/cli/commands/interactive/task-view-mode.tssrc/cli/commands/interactive/task-view.test.tssrc/cli/slash/commands/tasks.ts/tasks(cursor nav) and/tasks:view(live mode)Gates
pnpm lint✓pnpm audit:filesize:check✓ (all files < 350 code lines)pnpm scan:env:check✓pnpm exec vitest run src/cli/slash/commands/tasks.test.ts src/cli/commands/interactive/task-view.test.ts✓ (30 tests passing)Stack
Stacks on #1312 (v1: always-on JSONL logging, page-and-return /tasks:view).