feat(cli): 2.E — ink streaming TUI (live node status + token stream + cost)#46
Merged
Conversation
…xt pickup 2.E PR #45 merged. Provider/key commands (the `relavium provider` registry + API keys in the OS keychain via @napi-rs/keyring, resolved keychain → RELAVIUM_<PROVIDER>_API_KEY env var → error) are shipped. No new ADR (secrets.enc deferred past v1.0; @napi-rs/keyring pre-authorized by ADR-0019). - phase-2-cli.md: 2.C heading + status header + Remaining-build-order status line all marked ✅ Done (PR #45); the build-order table drops the 2.C row, renumbers (2.E now #1), and flips 2.C → ✓ in the MCP/chat blocker cells; the gate-closing backbone shrinks to `2.E → 2.G → 2.I → 2.L` (2.C joins 2.K + 2.H as done). - current.md: 2.C added to the Landed list (behind ADR-0019 + ADR-0006); next pickup → 2.E; date bump. - CLAUDE.md + README.md: status paragraphs updated (2.C landed; next pickup 2.E). Refs: phase-2-cli.md 2.C, ADR-0019, ADR-0006 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… cost) The third RunRenderer over the one event bus (alongside --json NDJSON and the plain line renderer), so the surfaces never fork (ADR-0047 "renderer, not a fork"). On an interactive TTY `relavium run` now renders a live view; it degrades to the plain renderer under --no-color / no-TTY / CI=true and to NDJSON under --json. Architecture (framework-free cores per ADR-0047 — the logic is unit-tested without a TTY/React): - render/tui/run-view-model.ts — PURE reducer RunEvent[] → RunViewState: per-node status, the active node's bounded token buffer, cost accumulation, sequenceNumber GAP DETECTION (detect + warn; the live stream is no-drop, so a gap signals a defect — a durable-state resync is deferred to the 2.I read side), terminal summary. Every event is reduced (none dropped); only the displayed tail is bounded. - render/tui/format.ts — pure presentational helpers (spinner frames, status glyph/color, micro-cents→USD per database-schema.md, duration, tokens); color-agnostic (the component applies color, honoring --no-color). - render/tui/final-summary.ts — pure plain-text persistent summary written after unmount (scrollback-safe). - render/tui/run-store.ts — ink-free external store: lifecycle events repaint immediately, agent:token / cost:updated bursts COALESCE to the next frame (throttle) — no flood, no drop. Unit-tested directly. - render/tui/RunApp.tsx — thin ink/React projection via useSyncExternalStore (the surface's only JSX). - render/tui/ink-renderer.ts — thin mount wrapper: the ~12.5fps frame loop (throttle + spinner), and finalize() unmounts (restoring the terminal) then writes the persistent summary. exitOnCtrlC:false so the run.ts SIGINT handler keeps driving the cooperative cancel. - render/renderer.ts — RunRenderer gains an optional finalize?() (awaited by run.ts after the loop, even on a throw); json/plain omit it. Shared by 2.G / 2.M. - render/select.ts — selectRenderer() routes by detectOutputMode (the previously-unused 2.F seam): tui / json / plain. run.ts wires it + awaits renderer.finalize?() in the loop's finally. Deps (ADR-0047, §9a cooling window): ink ^6.8.0 + react ^19.2.7 (+ @types/react devDep), pinned in the catalog, confined to apps/cli. ink 6 is the major honoring BOTH the tech-stack React 19 pin AND the Node ≥20.11 floor (ink 7 needs Node 22; ink 5 wants React 18). apps/cli/tsconfig gains jsx: react-jsx + .tsx. tsup compiles .tsx and (by default) externalizes deps — the inline-vs-external set is finalized at 2.L. Full gate green (incl. build); 173 cli tests (+32: reducer, format, final-summary, store, select); format clean; engine-deps within allowlists; ink/react confined to apps/cli (seam holds); packages/core untouched; Leakwatch 0. Secrets: the TUI renders engine-masked RunEvents and never unwraps them (tool input is already sanitized by the engine; only toolId is shown). Refs: ADR-0047, phase-2-cli.md 2.E, sse-event-schema.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the multi-agent adversarial review (1 high, 3 medium, 7 low, 14 nit confirmed). Acted on the real ones; by-design nits skipped (see below). Tests (the high/medium cluster — the reducer is the ADR-0047 "tested core"): - reducer: the 8 previously-unreduced event cases now have tests — run:timeout (summary+warning), node:skipped, budget:warning/budget:paused, human_gate:paused/resumed, media_job:submitted, agent:file_patch_proposed (singular/plural); plus the parallel agent:token active-switch branch, the MAX_WARNINGS bound, node:completed cost snapshot, and the new backward/empty-summary/token-upsert paths. - ink-renderer: a `mount` injection seam lets a test drive finalize WITHOUT a real ink mount — asserts unmount→waitUntilExit→summary ordering, idempotency, and the writeSummary injection. (+ projection.test, final-summary "run ended" default, run.test finalize-wiring + teardown-error suppression.) 195 cli tests (was 173). Code: - run.ts: a renderer finalize() error is now caught + logged to stderr so it never MASKS the run's real outcome/error (was: a finally-block throw replaced the original). selectRenderer is injectable (test seam). - run-view-model.ts: trackSeq also flags a BACKWARD/duplicate sequenceNumber (keeps the high-water mark); agent:token defensively upserts its node so a token before node:started still surfaces; tool_result omits the ": " suffix for an empty summary. - run-store.ts: the whole high-frequency append family (agent:tool_call/tool_result/file_patch_proposed) coalesces to the next frame, not just token/cost — a tool-heavy loop repaints once per frame. - ink-renderer.ts: pin ink's internal maxFps to the store's frame cadence so the two can't drift. - RunApp.tsx: the pure projection helpers (colorProps, nodeSuffix) move to projection.ts (unit-tested). - commands.md: corrected — --no-color KEEPS the TUI and suppresses ANSI (it does not swap renderers; only no-TTY/CI falls back to the plain renderer, --json to NDJSON). Skipped (by design / not defects): parallel interleaved-token buffer reset (documented 2.E limitation; acceptance is sequential), index-as-key on ink text leaves (harmless, no state/focus), schema.parse on test doubles (no drift found), the inline-vs-external bundling + ink-mount E2E (deferred to 2.L / manual acceptance), the 76-char subject of 3defc92 (already committed; this subject is ≤72). Full gate green (incl. build); format clean; Leakwatch 0; seam holds; packages/core untouched. Refs: ADR-0047, phase-2-cli.md 2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er, polish Three independent reviews (all merge-ready, 0 blockers). Acted on the valid findings: - [Med] --no-color no longer leaks ANSI: the active-token + tool lines hardcoded `dimColor` (itself an ESC[2m SGR) with no gate, contradicting the "suppresses ANSI color" contract this very change documented. Added dimProps(color) (gated like colorProps) and routed all dim through it; + a unit test. The documented contract is "no color/dim SGR" — ink's inherent cursor control on a TTY is out of scope. - [Med] Documented WHY Ctrl-C still cancels under the TUI: RunApp uses no useInput, so ink stays in cooked mode and the kernel keeps delivering SIGINT to run.ts (a real-TTY cancel check remains a manual acceptance). - [Low] run.ts now registers the SIGINT cancel handler BEFORE constructing the renderer, so a renderer-build throw can't leave a live engine with no cooperative-cancel handler (finalize is `renderer?.finalize?.()`). - [Low] Narrow-terminal / newline-free streams: the active-token, tool, and warning lines render with `wrap="truncate-end"` so one logical line can't wrap to dozens of rows; the 6-line cap is now the named MAX_ACTIVE_TOKEN_LINES (was a magic number in RunApp). - [Low] clip() truncates on code points ([...str]) — never splits a surrogate pair. - [Low] final-summary reuses projection.nodeSuffix (one source of truth, was a partial duplicate). - [Low] reducer test for the run:timeout → terminal run:failed override (errorCode 'run_timeout' wins). - phase-2-cli.md §2.E: as-built reconcile note — gap handling is detect-and-warn (resync deferred to 2.I); --no-color keeps the TUI (color-suppressed); the single active-token region is a documented fan-out limit. Skipped (verified correct / by design): the mutable store binding (correct for useSyncExternalStore), unref + final frame (finalize flushes), formatCostUsd float range (display-only), teardown stack under --verbose (a swallowed diagnostic; message suffices), no real-TTY RunApp render test (cursor-control ANSI is inherent; the contract is asserted at the color/dim-prop gate). Full gate green (incl. build); 197 cli tests; format clean; Leakwatch 0; seam holds; packages/core untouched. Refs: ADR-0047, phase-2-cli.md 2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
More reviews will be available in 37 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a complete Ink-based streaming TUI renderer for the CLI ChangesInk TUI Renderer (Milestone 2.E)
Native AbortController and Abort-Aware Error Handling
Configuration, Dependencies, and Documentation
Sequence DiagramsequenceDiagram
participant CLI as runCommand
participant SR as selectRenderer
participant IR as createInkRenderer
participant RS as RunStore
participant RA as RunApp (Ink)
participant FS as renderFinalSummary
CLI->>SR: io, global<br/>(TTY, JSON, color, CI)
SR->>SR: detectOutputMode
alt tui mode
SR->>IR: color flag
IR->>RS: createRunStore(color)
IR->>IR: setInterval(tick, FRAME_MS).unref()
IR->>RA: render via Ink
else json/plain mode
SR-->>SR: JSON or plain renderer
end
SR-->>CLI: RunRenderer {onEvent, finalize}
loop run event stream
CLI->>IR: renderer.onEvent(event)
IR->>RS: store.apply(event)
RS->>RS: reduceRunEvent
alt high-frequency
RS->>RS: set dirty flag
else terminal/status
RS->>RS: flush()
end
end
loop every FRAME_MS
RS->>RS: tick()
RS->>RA: notify (re-render)
end
CLI->>IR: await renderer.finalize()
IR->>IR: clearInterval, store.flush()
IR->>RA: unmount()
IR->>IR: await waitUntilExit()
IR->>RS: store.summaryText()
RS->>FS: renderFinalSummary(state)
FS-->>IR: plain-text summary
IR->>CLI: write summary → stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the interactive ink TUI renderer for relavium run (workstream 2.E), introducing a framework-free view model, an external store with repaint throttling to coalesce high-frequency events, and presentational formatters. The CLI command routing is updated to select the TUI on interactive TTYs, falling back to plain or NDJSON renderers otherwise. The code review feedback suggests defaulting the initial sequence number to 0 to catch missing initial events, guarding the duration formatter against negative inputs, and correcting a misleading comment in RunApp.tsx regarding the footer contents.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+128
to
+152
| const last = state.lastSequenceNumber; | ||
| if (last !== undefined && seq > last + 1) { | ||
| return { | ||
| lastSequenceNumber: seq, | ||
| gapDetected: true, | ||
| warnings: pushBounded( | ||
| state.warnings, | ||
| `event gap: #${last} → #${seq} (some events were not observed)`, | ||
| MAX_WARNINGS, | ||
| ), | ||
| }; | ||
| } | ||
| if (last !== undefined && seq <= last) { | ||
| // Backward / duplicate: keep the high-water mark as `lastSequenceNumber` so a later genuine forward gap | ||
| // is still measured against it (advancing to the lower `seq` would mask the missing range). | ||
| return { | ||
| lastSequenceNumber: last, | ||
| gapDetected: true, | ||
| warnings: pushBounded( | ||
| state.warnings, | ||
| `event out of order: #${seq} after #${last}`, | ||
| MAX_WARNINGS, | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The current gap detection logic does not detect gaps on the very first event of the run because state.lastSequenceNumber starts as undefined. By defaulting last to 0 when lastSequenceNumber is undefined, we can robustly detect if the stream missed the initial events (e.g., starting at sequence number 2 or higher). This also allows us to simplify the code by removing the redundant last !== undefined checks.
const last = state.lastSequenceNumber ?? 0;
if (seq > last + 1) {
return {
lastSequenceNumber: seq,
gapDetected: true,
warnings: pushBounded(
state.warnings,
`event gap: #${last} → #${seq} (some events were not observed)`,
MAX_WARNINGS,
),
};
}
if (state.lastSequenceNumber !== undefined && seq <= last) {
// Backward / duplicate: keep the high-water mark as `lastSequenceNumber` so a later genuine forward gap
// is still measured against it (advancing to the lower `seq` would mask the missing range).
return {
lastSequenceNumber: last,
gapDetected: true,
warnings: pushBounded(
state.warnings,
`event out of order: #${seq} after #${last}`,
MAX_WARNINGS,
),
};
}
Comment on lines
+71
to
+82
| export function formatDuration(ms: number): string { | ||
| if (ms < 1000) { | ||
| return `${Math.round(ms)}ms`; | ||
| } | ||
| const totalSeconds = ms / 1000; | ||
| if (totalSeconds < 60) { | ||
| return `${totalSeconds.toFixed(1)}s`; | ||
| } | ||
| const minutes = Math.floor(totalSeconds / 60); | ||
| const seconds = Math.round(totalSeconds % 60); | ||
| return `${minutes}m${String(seconds).padStart(2, '0')}s`; | ||
| } |
There was a problem hiding this comment.
To prevent unexpected formatting behavior (such as negative minutes or seconds) in case of clock skew or negative duration inputs, it is safer to guard the input ms value to be at least 0 using Math.max(0, ms).
Suggested change
| export function formatDuration(ms: number): string { | |
| if (ms < 1000) { | |
| return `${Math.round(ms)}ms`; | |
| } | |
| const totalSeconds = ms / 1000; | |
| if (totalSeconds < 60) { | |
| return `${totalSeconds.toFixed(1)}s`; | |
| } | |
| const minutes = Math.floor(totalSeconds / 60); | |
| const seconds = Math.round(totalSeconds % 60); | |
| return `${minutes}m${String(seconds).padStart(2, '0')}s`; | |
| } | |
| export function formatDuration(ms: number): string { | |
| const safeMs = Math.max(0, ms); | |
| if (safeMs < 1000) { | |
| return `\${Math.round(safeMs)}ms`; | |
| } | |
| const totalSeconds = safeMs / 1000; | |
| if (totalSeconds < 60) { | |
| return `\${totalSeconds.toFixed(1)}s`; | |
| } | |
| const minutes = Math.floor(totalSeconds / 60); | |
| const seconds = Math.round(totalSeconds % 60); | |
| return `\${minutes}m\${String(seconds).padStart(2, '0')}s`; | |
| } |
Comment on lines
+89
to
+90
| {/* Running cost / duration footer */} | ||
| <Box marginTop={1}> |
There was a problem hiding this comment.
The comment mentions a "duration" footer, but the actual implementation only renders the cumulative cost and final total tokens. If running duration is not intended to be displayed in the live TUI, the comment should be updated to avoid confusion.
Suggested change
| {/* Running cost / duration footer */} | |
| <Box marginTop={1}> | |
| {/* Running cost / tokens footer */} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/cli/src/render/tui/format.test.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a minute-boundary regression case for
formatDuration.Please cover the rounding-carry edge (e.g.,
119_600ms -> 2m00s) som60sregressions are caught.✅ Test addition
describe('formatDuration', () => { it('formats sub-second, second, and minute scales', () => { expect(formatDuration(0)).toBe('0ms'); expect(formatDuration(420)).toBe('420ms'); expect(formatDuration(3200)).toBe('3.2s'); expect(formatDuration(64_000)).toBe('1m04s'); + expect(formatDuration(119_600)).toBe('2m00s'); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/tui/format.test.ts` around lines 22 - 29, Add a regression test case within the existing formatDuration test to verify minute-boundary rounding behavior. Specifically, add an expect statement that tests a millisecond value that rounds up and carries over to create an additional minute (such as 119_600ms which should format to '2m00s') to ensure the formatDuration function properly handles the carry-over logic and does not produce invalid output like 'm60s'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/commands/run.ts`:
- Around line 135-139: The renderer selection via selectRenderer in the try
block can throw an error, but if it does, the handle from engine.start() will
continue running in the background while the error propagates. Add a catch block
after the renderer assignment in the selectRenderer call to catch any renderer
construction failures, cancel the handle to stop the background run before the
error unwinds, and then rethrow the error to maintain proper error propagation.
Apply the same fix to the other locations mentioned (lines 150-152).
In `@apps/cli/src/render/tui/format.ts`:
- Around line 79-81: The formatDuration function can produce invalid time
formats like "1m60s" because the Math.round operation on the seconds value
(totalSeconds % 60) can result in 60 when rounding up from 59.5 or higher. After
calculating the seconds variable, add a check to handle the edge case where
seconds equals 60: increment the minutes variable by 1 and set seconds to 0.
This ensures the function always returns valid time formats like "2m00s" instead
of "1m60s".
In `@apps/cli/src/render/tui/ink-renderer.ts`:
- Around line 51-70: The frame interval created with setInterval is started
before the mount(store) call, and if mount throws an error during the
construction of createInkRenderer, the interval will never be cleaned up since
the renderer instance is never returned and finalize is never called. Wrap the
mount(store) call in a try-catch block that clears the frame interval using
clearInterval(frame) in the catch handler before rethrowing the error, ensuring
the interval is always cleaned up whether mount succeeds or fails.
---
Nitpick comments:
In `@apps/cli/src/render/tui/format.test.ts`:
- Around line 22-29: Add a regression test case within the existing
formatDuration test to verify minute-boundary rounding behavior. Specifically,
add an expect statement that tests a millisecond value that rounds up and
carries over to create an additional minute (such as 119_600ms which should
format to '2m00s') to ensure the formatDuration function properly handles the
carry-over logic and does not produce invalid output like 'm60s'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44c56327-49a0-49bb-99f7-dadc270d1716
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
CLAUDE.mdREADME.mdapps/cli/package.jsonapps/cli/src/commands/run.test.tsapps/cli/src/commands/run.tsapps/cli/src/render/renderer.tsapps/cli/src/render/select.test.tsapps/cli/src/render/select.tsapps/cli/src/render/tui/RunApp.tsxapps/cli/src/render/tui/final-summary.test.tsapps/cli/src/render/tui/final-summary.tsapps/cli/src/render/tui/format.test.tsapps/cli/src/render/tui/format.tsapps/cli/src/render/tui/ink-renderer.test.tsapps/cli/src/render/tui/ink-renderer.tsapps/cli/src/render/tui/projection.test.tsapps/cli/src/render/tui/projection.tsapps/cli/src/render/tui/run-store.test.tsapps/cli/src/render/tui/run-store.tsapps/cli/src/render/tui/run-view-model.test.tsapps/cli/src/render/tui/run-view-model.tsapps/cli/tsconfig.jsondocs/reference/cli/commands.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mdpnpm-workspace.yaml
…cleanups Inline + SonarQube findings; verified each, fixed the valid ones: - [bug] formatDuration could emit "1m60s": Math.round on the seconds can yield 60 (59.5–59.999s rounds up). Now carries into the next minute → "2m00s"; also clamps a negative duration (clock skew) to 0. + 2 tests. - [robustness] run.ts cancels the still-live engine run on an ABNORMAL unwind (renderer construction threw, or the event stream rejected) — `if (outcome === undefined) handle.cancel()` in the finally — so a run can't keep executing unsupervised while the error propagates (cancel is idempotent + safe). - [robustness] ink-renderer clears the frame-loop interval if `mount()` throws during construction (finalize would never run to clear it otherwise). - [Sonar] ink-renderer: dropped the `void` operator (block-body arrow instead). final-summary: lifted the two nested template literals into locals. RunApp: props marked `Readonly<>`; fixed the "duration footer" comment (the footer shows cost + total tokens, not a running duration). run-view-model: reduced reduceRunEvent cognitive complexity by extracting `reduceAgentToken` + a DRY `attemptPatch` helper (also de-duplicates the node:started/node:failed attempt spread). Tests: `toHaveLength` over `.length).toBe`. Skipped (verified not-applicable): - Gap detection on the FIRST event (defaulting last→0): would false-positive on a stream whose first sequenceNumber is 0, AND on a resumed run whose event stream legitimately starts mid-sequence (checkpoint resume). The first observed event must establish the baseline; we can't know the expected start. - Array-index React keys on the active-token / tool / warning lines: ink renders to a string frame (not the DOM); these are stateless `<Text>` leaves in fixed-size trailing windows with no identity/focus/state to preserve across reorders, so the index-key reconciliation hazards the rule targets don't apply. (Happy to add monotonic line ids if a clean Sonar dashboard is wanted — it would churn the reducer for no runtime gain.) Full gate green (incl. build); 199 cli tests; format clean; Leakwatch 0; seam holds; packages/core untouched. Refs: ADR-0047, phase-2-cli.md 2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/render/tui/run-view-model.ts (1)
198-219: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not apply duplicate or out-of-order events after detecting them.
trackSeqrecords duplicate/out-of-order sequence anomalies, but this reducer still applies the event to the view state. A duplicateagent:tokencan append the same token twice, and a stale terminal event can overwritesummaryafter a newer terminal state.Suggested shape for avoiding stale mutations
export function reduceRunEvent(state: RunViewState, event: RunEvent): RunViewState { - const base: RunViewState = { ...state, ...trackSeq(state, event) }; + const seq = trackSeq(state, event); + const base: RunViewState = { ...state, ...seq.patch }; + + if (!seq.applyEvent) { + return base; + } switch (event.type) {Then have
trackSeqreturnapplyEvent: falseforevent.sequenceNumber <= state.lastSequenceNumber, while still returning the warning/gap patch.Also applies to: 326-365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/tui/run-view-model.ts` around lines 198 - 219, The reduceRunEvent function currently applies all events to the state even if they are duplicates or out-of-order, which can cause duplicate tokens or stale state overwrites. Modify the trackSeq function to return an object containing both the tracking patch and an applyEvent boolean flag, setting applyEvent to false when event.sequenceNumber is less than or equal to state.lastSequenceNumber. Then in the reduceRunEvent function, check this applyEvent flag and return the base state without applying the event if it is false. This will prevent duplicate agent:token events from appending tokens twice and prevent stale terminal events from overwriting the summary field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/cli/src/render/tui/run-view-model.ts`:
- Around line 198-219: The reduceRunEvent function currently applies all events
to the state even if they are duplicates or out-of-order, which can cause
duplicate tokens or stale state overwrites. Modify the trackSeq function to
return an object containing both the tracking patch and an applyEvent boolean
flag, setting applyEvent to false when event.sequenceNumber is less than or
equal to state.lastSequenceNumber. Then in the reduceRunEvent function, check
this applyEvent flag and return the base state without applying the event if it
is false. This will prevent duplicate agent:token events from appending tokens
twice and prevent stale terminal events from overwriting the summary field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8d21725-be93-41a7-9e6d-736e9bff58c4
📒 Files selected for processing (8)
apps/cli/src/commands/run.tsapps/cli/src/render/tui/RunApp.tsxapps/cli/src/render/tui/final-summary.tsapps/cli/src/render/tui/format.test.tsapps/cli/src/render/tui/format.tsapps/cli/src/render/tui/ink-renderer.tsapps/cli/src/render/tui/run-view-model.test.tsapps/cli/src/render/tui/run-view-model.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/cli/src/render/tui/format.ts
- apps/cli/src/render/tui/RunApp.tsx
- apps/cli/src/render/tui/final-summary.ts
- apps/cli/src/render/tui/ink-renderer.ts
- apps/cli/src/render/tui/run-view-model.test.ts
trackSeq already DETECTED a backward/duplicate sequenceNumber and warned, but reduceRunEvent still APPLIED
the event — so a replayed agent:token would double-append and a stale terminal event could overwrite a
fresher summary. trackSeq now returns an `apply` flag (false when seq <= last); reduceRunEvent records the
warning + keeps the high-water mark but returns without applying the stale event. Forward gaps still apply
(the event is genuine). On the in-process no-drop monotonic stream this never triggers — it's defense in
depth, now coherent with the detect-and-warn posture.
+ tests: a duplicate agent:token doesn't double-append ('hi' not 'hihi'); the backward node:completed is
not applied (node stays 'running'). 200 cli tests; full gate green; format clean; Leakwatch 0.
Refs: phase-2-cli.md 2.E
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t LLM stream
Root cause of the real-TTY exit-130-on-Ctrl-C (found via the 2.E manual acceptance; a PRE-EXISTING 2.D host
wiring bug, NOT an ink/2.E regression — ink never touches Ctrl-C since RunApp uses no useInput, so the
kernel SIGINT reaches us fine):
createCliHost injected the engine's IN-HOUSE `createAbortController` — which is for TESTS ONLY. Its `signal`
is a plain object, NOT `instanceof AbortSignal`. The LLM adapters gate on `isAbortSignal` (anthropic.ts
buildRequestOptions → `instanceof AbortSignal`), so they DROPPED the signal and never passed it to the SDK's
`fetch`. A run cancel therefore could not abort an in-flight streaming request: the agent node stayed
`running`, the engine never reached `running===0` to emit `run:cancelled`, run.ts's loop never saw a
terminal event, and the first Ctrl-C looked inert. Because run.ts uses `process.once('SIGINT')`, the
listener had already detached, so a reflexive SECOND Ctrl-C hit a listener-less process → Node default
terminate at 128+SIGINT = 130.
Fix: inject a NATIVE `AbortController` (`() => new AbortController()`) — exactly what host.ts's own docstring
("the global AbortController") and execution-host.ts ("a real surface injects `() => new AbortController()`")
already prescribe. Its real `AbortSignal` passes `isAbortSignal` and is threaded into `fetch`, so the first
Ctrl-C aborts the live stream → cancelled chunk → node settles → `run:cancelled` → renderer.finalize()
prints "run cancelled" → exit 1 (no second press, no 130). The native controller structurally satisfies the
engine's `AbortControllerLike` (typecheck confirms), so the engine is unchanged.
+ host.test.ts: asserts `createCliHost().newAbortController().signal instanceof AbortSignal` (would have
failed before this fix) + abort flips `aborted`. 202 cli tests; full gate green; format clean; Leakwatch 0;
packages/core + packages/llm untouched; engine-deps within allowlists.
Recommended follow-ups (NOT done here — the host fix is necessary + sufficient for the symptom): an in-loop
`throwIfAborted` inside the engine's streamOneTurn (defence-in-depth if a future adapter ignores the signal),
and a re-arming SIGINT handler in run.ts so an impatient second Ctrl-C forces a clean exit-1 rather than 130.
Refs: ADR-0038 (host-injected seam), phase-2-cli.md 2.D/2.E
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…se → 130 The SECOND half of the Ctrl-C-exits-130 root cause (the first was the native AbortController, cc2f637). Manual real-TTY acceptance now PASSES: a single Ctrl-C on a live agent run cooperatively cancels, prints the "run cancelled" summary, restores the terminal, and exits 1 (verified: `echo $?` → 1, not 130). ink mounts via `signal-exit`, whose SIGINT listener RE-RAISES SIGINT (`process.kill(pid, sig)` → 128+2 = 130) — but ONLY when it is the sole remaining SIGINT listener (signal-exit/index.js: `if (process.listeners(sig).length === emitter.count)`). run.ts used `process.once('SIGINT')`, which removes itself the instant it fires; so on Ctrl-C our handler ran (cancel) and detached, signal-exit then saw only its own listener and re-raised → the process was killed at 130 before the cooperative cancel could drain. That is why a SINGLE Ctrl-C exited 130, and why the host AbortController fix alone didn't change it. Fix: register the handler with `process.on` (persistent, removed in the finally). Staying in the SIGINT listener set means signal-exit sees >1 listener and does NOT re-raise, so cancel → run:cancelled → renderer.finalize() ("run cancelled") → exit 1 wins. A second Ctrl-C while the cancel drains forces a clean, deterministic exit 1 (never a hang, never the bare-signal 130). Combined with the native AbortController (cc2f637), a single Ctrl-C now cooperatively cancels an agent run and exits 1. The SIGINT/STALL run test stays green (202 cli tests); full gate green; format clean; Leakwatch 0. Refs: phase-2-cli.md 2.D/2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/commands/run.ts (1)
163-175: 🩺 Stability & Availability | 🟡 MinorMove
process.removeListener('SIGINT', onSigint)to afterrenderer?.finalize?.()completes — the current order creates an exit-130 race during unmount.The code at line 140–144 documents that ink registers a signal-exit SIGINT listener that re-raises only when it becomes the sole remaining listener. Removing
onSigintat line 163 beforefinalize()at line 175 creates a window where a Ctrl-C landing during unmount would find ink's signal-exit listener as the only handler, triggering the re-raise and exit 130. Deferring the removal until after unmount completes preserves your deterministic-exit guarantee: any Ctrl-C during finalize calls the idempotenthandle.cancel(), and a second one forces exit 1 viaEXIT_CODES.workflowFailed.Suggested reorder
} finally { - process.removeListener('SIGINT', onSigint); // No terminal/paused outcome means we're unwinding abnormally (renderer construction threw, or the // event stream rejected) — cancel the still-live engine run so it doesn't keep executing unsupervised // in the background while the error propagates (cancel is idempotent + safe post-terminal). if (outcome === undefined) { handle.cancel(); } // Tear the renderer down even on a throw: the ink TUI must unmount to restore the terminal and write // its persistent final summary. The `?.` is a no-op for the line/NDJSON renderers and when `renderer` // is still undefined (construction threw). A teardown error must NOT mask the run's real // outcome/error — surface it to stderr and move on. try { await renderer?.finalize?.(); } catch (teardownErr) { deps.io.writeErr( `renderer teardown failed: ${teardownErr instanceof Error ? teardownErr.message : String(teardownErr)}\n`, ); } + // Detach only after ink has unmounted: while finalize() awaits the unmount, leaving our listener in + // place prevents signal-exit from becoming the sole SIGINT listener and re-raising (exit 130). + process.removeListener('SIGINT', onSigint); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/run.ts` around lines 163 - 175, Move the `process.removeListener('SIGINT', onSigint);` call to after the `await renderer?.finalize?.();` statement completes. Currently it is positioned before the finalize call, which creates a race condition where a Ctrl-C signal arriving during renderer unmount would find ink's signal-exit listener as the only handler, triggering exit 130. By deferring the removal until after finalize completes, any Ctrl-C during unmount will correctly call the idempotent handle.cancel() and maintain deterministic exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/cli/src/commands/run.ts`:
- Around line 163-175: Move the `process.removeListener('SIGINT', onSigint);`
call to after the `await renderer?.finalize?.();` statement completes. Currently
it is positioned before the finalize call, which creates a race condition where
a Ctrl-C signal arriving during renderer unmount would find ink's signal-exit
listener as the only handler, triggering exit 130. By deferring the removal
until after finalize completes, any Ctrl-C during unmount will correctly call
the idempotent handle.cancel() and maintain deterministic exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88c6ec13-5dfa-4439-8b2f-1287a44bee1b
📒 Files selected for processing (5)
apps/cli/src/commands/run.tsapps/cli/src/engine/host.test.tsapps/cli/src/engine/host.tsapps/cli/src/render/tui/run-view-model.test.tsapps/cli/src/render/tui/run-view-model.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/render/tui/run-view-model.test.ts
…nsport The node-level follow-up to the Ctrl-C fix (found via the same real-TTY acceptance): a cancelled agent node showed `✗ slow — provider_unavailable` instead of `cancelled`. When a run cancel aborts an IN-FLIGHT stream, the abort can reach the adapter as a wrapped connection error (not the SDK's user-abort type) → classified `transport` → mapped by the engine to `provider_unavailable` — so a cancel read as a provider outage. Fix (provider-agnostic, at the chain — the right layer): FallbackChain now reclassifies ANY surfaced error as `cancelled` when `req.signal` is aborted (`#abortAware`), applied at every error-surfacing point on the stream + generate paths (the committed-stream error chunk, the pre-content failure, and the throw catch). The engine already maps the `cancelled` kind → the `cancelled` ERROR_CODE (agent-turn.ts), so the node now reads `cancelled` end-to-end. Also (parity / standalone correctness): the Anthropic adapter gains the `err.name === 'AbortError'` branch the OpenAI adapter already had — a native AbortError outside the SDK's request wrapper classifies as `cancelled`, not the catch-all `unknown` (the asymmetry was a latent bug for direct adapter use). + tests: a committed mid-stream abort yields a `cancelled` chunk (not `transport`); anthropicErrorToLlmError on a name-'AbortError' → `cancelled`. 442 llm tests; full gate green; format clean; seam holds; Leakwatch 0. `cancelled` is fatal (not retryable), so the chain surfaces it without failover/retry. Refs: ADR-0011 (seam), error-handling.md, phase-2-cli.md 2.D/2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nt-window race) Valid review finding. The finally removed our SIGINT listener BEFORE awaiting renderer.finalize() (ink's unmount + waitUntilExit, which takes a beat). In that window our listener was gone but ink's `signal-exit` listener was still registered — so a second, impatient Ctrl-C during unmount would find signal-exit as the SOLE SIGINT listener → it re-raises → exit 130 (the very failure mode a67cbd6 fixed for the first press). Moving `process.removeListener('SIGINT', onSigint)` to AFTER finalize keeps us in the SIGINT set across the whole unmount: a Ctrl-C during finalize hits our handler (cancelRequested already true → clean exit 1) and signal-exit never sees itself as the sole listener. After finalize, ink has unsubscribed its own listener, so removing ours then leaves the set clean. The abnormal-unwind `handle.cancel()` stays before finalize. 202 cli tests green (SIGINT/STALL + finalize-wiring unchanged); full gate green; format clean; Leakwatch 0. (The exact race — a second Ctrl-C within the ~tens-of-ms unmount window — is hard to hit manually; the fix is the same listener-set invariant a67cbd6 established, now held through teardown.) Refs: phase-2-cli.md 2.D/2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/src/render/tui/format.test.ts (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the sub-minute carry edge (
59_950ms).Current coverage checks carry in the minute branch, but not the
59.95s -> 60.0spath from the sub-minute branch.🧪 Suggested test addition
describe('formatDuration', () => { it('formats sub-second, second, and minute scales', () => { expect(formatDuration(0)).toBe('0ms'); expect(formatDuration(420)).toBe('420ms'); expect(formatDuration(3200)).toBe('3.2s'); expect(formatDuration(64_000)).toBe('1m04s'); }); it('carries a rounded-up 60s into the next minute (never "1m60s")', () => { expect(formatDuration(119_600)).toBe('2m00s'); // 119.6s → round(59.6)=60 → carry to 2m00s + expect(formatDuration(59_950)).toBe('1m00s'); // 59.95s should not render as "60.0s" });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/tui/format.test.ts` around lines 30 - 33, Add a regression test case after the existing test for the 119_600 milliseconds case. Create a new test that verifies the sub-minute carry edge case where formatDuration(59_950) should round up to 60 seconds and carry into the next minute, expecting the result to be '1m00s'. This test ensures the carry behavior works correctly in the sub-minute branch, not just when already in the minute range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/render/tui/format.ts`:
- Around line 76-79: The boundary check for seconds formatting occurs before
rounding, so values like 59.95 seconds get rounded to 60.0s with toFixed(1),
which is inconsistent since 60 seconds should be formatted as minutes. In the
section starting with the totalSeconds calculation and the if condition checking
totalSeconds < 60, you need to round the value first before checking the
boundary, or adjust the logic so that after rounding with toFixed(1), if the
result is 60.0 or greater, it falls through to the minute formatting branch
instead of being returned as seconds.
---
Nitpick comments:
In `@apps/cli/src/render/tui/format.test.ts`:
- Around line 30-33: Add a regression test case after the existing test for the
119_600 milliseconds case. Create a new test that verifies the sub-minute carry
edge case where formatDuration(59_950) should round up to 60 seconds and carry
into the next minute, expecting the result to be '1m00s'. This test ensures the
carry behavior works correctly in the sub-minute branch, not just when already
in the minute range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cd2aef5-a8e5-4e0f-9397-218c5f2ecb53
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
CLAUDE.mdREADME.mdapps/cli/package.jsonapps/cli/src/commands/run.test.tsapps/cli/src/commands/run.tsapps/cli/src/engine/host.test.tsapps/cli/src/engine/host.tsapps/cli/src/render/renderer.tsapps/cli/src/render/select.test.tsapps/cli/src/render/select.tsapps/cli/src/render/tui/RunApp.tsxapps/cli/src/render/tui/final-summary.test.tsapps/cli/src/render/tui/final-summary.tsapps/cli/src/render/tui/format.test.tsapps/cli/src/render/tui/format.tsapps/cli/src/render/tui/ink-renderer.test.tsapps/cli/src/render/tui/ink-renderer.tsapps/cli/src/render/tui/projection.test.tsapps/cli/src/render/tui/projection.tsapps/cli/src/render/tui/run-store.test.tsapps/cli/src/render/tui/run-store.tsapps/cli/src/render/tui/run-view-model.test.tsapps/cli/src/render/tui/run-view-model.tsapps/cli/tsconfig.jsondocs/reference/cli/commands.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mdpackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tspnpm-workspace.yaml
…verage gaps A comprehensive 6-dimension adversarial re-review of the whole PR (0 blockers, 0 high). Acted on the valid findings (the cancel chain + TUI were confirmed sound): Code: - formatDuration: the sub-minute→minute boundary now carries correctly — 59.95–59.999s no longer renders the invalid "60.0s" (the `< 60` check ran before rounding). Branch at `< 59.95`, then round whole seconds and carry (this also subsumes the old `seconds === 60` special-case). + boundary tests. - run-store: a terminal/parked summary stops the spinner animation — an abandoned `running` node (a parallel sibling dropped on failure/cancel) no longer keeps the frame loop repainting after the run ended. - renderer.ts: future-tense comments updated to present tense (2.E has landed). - commands.md: the "Output modes" table now notes `--no-color` keeps the TUI (suppresses ANSI), not a swap. Tests (the bulk — closing verified coverage gaps): - run.ts: the SECOND-Ctrl-C forced-exit path (process.exit(1), stubbed to throw so it can't fall through to a double-cancel) and the abnormal-unwind path (selectRenderer throws → engine cancelled + SIGINT listener removed + error propagates). - fallback-chain: #abortAware on the GENERATE catch (abort mid-call → cancelled, no failover) and the PRE-content stream path (abort before content → cancelled, no failover) — previously only the committed path was tested. - run-store: the idle-tick negative property (no dirty + no running node → no flush) + the post-summary stop. - run-view-model: cost snapshot when run:completed total disagrees with the last cost:updated (closing total wins). - ink-renderer: the mount-throw clearInterval/re-throw path. Skipped (verified not-applicable / by-design): an ink-mount + rendered-frame ANSI test (would need ink-testing-library — a new dep; the mount is covered by the real-TTY manual acceptance); a node-pty SIGINT regression test (heavy infra; same manual acceptance covers it); #abortAware reclassifying an in-flight sibling-failure as cancelled (acknowledged acceptable — the run is already doomed once the shared signal aborts); the ~80ms coalesce delay before an out-of-order *warning* paints (a defect-signal, immaterial). 209 cli + 444 llm tests; full gate green (incl. build); format clean; engine-deps within allowlists; seam holds; packages/core untouched; Leakwatch 0. Refs: ADR-0047, ADR-0011, phase-2-cli.md 2.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 24, 2026
…xt pickup 2.G PR #46 merged. The ink streaming TUI is shipped — the third RunRenderer over the one event bus (live per-node status + spinners, the active node's streaming tokens, a running cost footer, a persistent final summary), with cooperative Ctrl-C cancel (the native-AbortController + persistent-SIGINT + abort→cancelled chain). Behind ADR-0047 (ink ^6.8.0 + React 19, confined to apps/cli); no new ADR. - phase-2-cli.md: 2.E heading + status header + Remaining-build-order status line all marked ✅ Done (PR #46); the build-order table drops the 2.E row, renumbers (2.G now #1), and flips 2.E → ✓ in the 2.G + chat blocker cells; the gate-closing backbone shrinks to `2.G → 2.I → 2.L` (2.E joins 2.K + 2.H + 2.C as done). - current.md: 2.E added to the Landed list (behind ADR-0047); next pickup → 2.G; date bump (2026-06-24). - CLAUDE.md + README.md: status paragraphs updated (2.E landed; next pickup 2.G). Refs: phase-2-cli.md 2.E, ADR-0047 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 24, 2026
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 & why
Phase-2 workstream 2.E — the
inkstreaming TUI forrelavium run. On an interactive TTY the run isrendered live (per-node status + spinners, the active node's streaming tokens, a running cost/duration
footer, a persistent final summary). It is the third renderer over one event bus, alongside the
--jsonNDJSON renderer (2.F) and the plain line renderer — all behind one
RunRendererseam, so the surfaces neverfork ("renderer, not a fork"). No engine change —
packages/coreis untouched; the TUI consumes only the@relavium/sharedRunEventcontract.Also bundles a tiny 2.C Done bookkeeping commit (
b80772c, PR #45 follow-up) that preceded this work onthe branch.
Architecture (framework-free cores per ADR-0047)
The logic is pure and unit-tested without a TTY/React; ink is a thin lifecycle wrapper:
render/tui/run-view-model.tsRunEvent[] → RunViewState(status, bounded token buffer, cost,sequenceNumbergap/out-of-order detection, terminal summary)render/tui/format.ts,final-summary.ts,projection.tsrender/tui/run-store.tsagent:*/cost:updatedcoalesce to the next frame — no flood, no drop) + theuseSyncExternalStorecontractrender/tui/RunApp.tsxrender/tui/ink-renderer.tsfinalize()(unmount → restore terminal → persistent summary);mountis injectable so finalize is tested without a real ink mountrender/renderer.ts(finalize?()),render/select.tsselectRenderer()routes bydetectOutputMode;run.tsawaitsfinalize?()in the loop'sfinallyDecisions (no new ADR — ADR-0047 already records ink + react at 2.E)
ink ^6.8.0+react ^19.2.7(+@types/reactdevDep), catalog-pinned, confined toapps/cli.ink 6 is the major that honors BOTH the tech-stack React 19 pin AND the Node ≥20.11 supported
floor (ink 7 needs Node 22; ink 5 wants React 18).
apps/cli/tsconfiggainsjsx: react-jsx+.tsx.finalize?()added to theRunRendererseam (shared by later 2.G / 2.M); json/plain omit it.sequenceNumbergap = detect + warn (the in-process stream is no-drop, so a gap signals a defect); adurable-state resync is deferred to the 2.I read side (reconciled in
phase-2-cli.md §2.E).--no-colorkeeps the TUI and suppresses ANSI color/dim (it does NOT swap renderers — that is theno-TTY /
CI=truefallback;--json→ NDJSON).Review trail
A 5-dimension multi-agent adversarial review (each finding independently verified) → fixes in
4a79f2a;then three more independent reviews (all merge-ready, 0 blockers) → fixes in
bf970b1(the--no-colordim-leak, SIGINT-ordering, narrow-terminal
wrap=truncate, Unicode-safeclip, DRYnodeSuffix, therun:timeout→run:failed override test, and the §2.E as-built reconcile).
Tests
197 cli tests (24+ in the TUI layer), all exercising the reducer / store / formatters / projection / select
routing + the ink-renderer finalize lifecycle (via an injected mount) without a TTY.
Verification
pnpm turbo run lint typecheck test buildgreen (the build compiles.tsx+ bundles ink); format clean;engine-deps within allowlists; the
@relavium/llmseam untouched; ink/react confined to apps/cli (noengine package imports them);
packages/coreuntouched; Leakwatch 0. Secrets: the TUI rendersengine-masked
RunEvents and never unwraps them (tool input is already sanitized by the engine — onlytoolId+ summaries are shown).One manual acceptance (can't be unit-tested)
Per §2.E and the reviews: in a real terminal, confirm Ctrl-C cancels a live
relavium run(routes tohandle.cancel()→run:cancelled→ clean exit), not just re-renders. RunApp uses nouseInput, so inkstays in cooked mode and the kernel delivers SIGINT to
run.ts— but this is the one path no test can cover.🤖 Generated with Claude Code
Summary by CodeRabbit
--json, and CI; provider/key commands support OS keychain storage.