feat(console): friendly chat views for shell::* function calls#253
Merged
Conversation
Add a shell renderer module to the console chat, covering all 16 shell worker functions (exec, exec_bg, status, kill, list, config-status and the 10 fs::* operations) with terminal-style visualizations matching the existing sandbox/coder/web modules, instead of the raw REQUEST/RESPONSE JSON fallback. - zod schemas mirror the shell worker wire types exactly (serde-faithful optionality: target optional-not-nullable, kill reason skip-serialized, transparent fs::stat, chmod updated alias, stream-only fs::read) - parseShellErrorDisplay recovers S-codes both from flat error bodies and from harness gate-denial envelopes whose reason embeds the code as text (the path real shell errors take today) - approval previews for shell::exec, exec_bg and kill - reuse sandbox primitives; extract FsEntriesTable, GrepMatchList, SedResultsTable and export collectErrorCandidates from the sandbox module (verbatim moves, additive only) - 86 tests pinning wire-shape decisions and error paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 15 skipped (no docs/).
Four for four. Nicely done. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds comprehensive shell-function support: Zod schemas, parsing/error normalization, formatting helpers, many shell UI views (exec, exec_bg, status, kill, list, config-status, and 10 fs ops), tests, sandbox view extractions, and wiring into FunctionCallMessage for previews and terminals. ChangesShell Tool Support
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@console/web/src/components/chat/shell/FsRmView.tsx`:
- Around line 43-52: The TargetChip is being conditionally hidden when both
recursive and sandboxed are false; update the FsRmView render so TargetChip
(rendering req.data.target) is always displayed and only the "recursive" Chip
remains conditional. Locate the JSX in FsRmView that currently checks
"{recursive || sandboxed ? (...) : null}" and change it to always render
<TargetChip target={req.data.target} /> while keeping the <Chip
label="recursive" ...> block wrapped by the conditional on "recursive".
In `@console/web/src/components/chat/shell/KillView.tsx`:
- Around line 35-37: The headline should use the parsed response instead of only
`running`: change the span's text logic in KillView to: if `running` show
"killing job…", else if `resp` exists show `resp.killed ? 'killed job' : 'did
not kill job'` (or another concise failure message derived from `resp.reason`),
otherwise fall back to the previous default; update the conditional that
currently reads `{running ? 'killing job…' : 'killed job'}` to inspect
`resp?.killed` and leave the job id display using `truncateMiddle(resp?.job_id
?? req.data.job_id, 24)` unchanged.
In `@console/web/src/components/chat/shell/ListView.tsx`:
- Around line 13-17: The helper function formatDurationMs is duplicated in
ListView and StatusView; extract it into a new shared module (e.g., format.ts)
that imports/uses formatAgeSecs, export formatDurationMs from that module, then
replace the local definitions by importing formatDurationMs into ListView and
StatusView (remove the duplicate functions). Make sure the new format.ts
preserves the current behavior (ms < 10_000 → `${ms}ms`, otherwise call
formatAgeSecs(Math.floor(ms/1000))) and update any imports so ListView and
StatusView reference the shared function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e644c572-a069-499f-bf27-ec317136229b
📒 Files selected for processing (27)
console/web/src/components/chat/FunctionCallMessage.tsxconsole/web/src/components/chat/sandbox/FsGrepView.tsxconsole/web/src/components/chat/sandbox/FsLsView.tsxconsole/web/src/components/chat/sandbox/FsSedView.tsxconsole/web/src/components/chat/sandbox/parsers.tsconsole/web/src/components/chat/shell/ConfigStatusView.tsxconsole/web/src/components/chat/shell/ExecBgView.tsxconsole/web/src/components/chat/shell/ExecView.tsxconsole/web/src/components/chat/shell/FsChmodView.tsxconsole/web/src/components/chat/shell/FsGrepView.tsxconsole/web/src/components/chat/shell/FsLsView.tsxconsole/web/src/components/chat/shell/FsMkdirView.tsxconsole/web/src/components/chat/shell/FsMvView.tsxconsole/web/src/components/chat/shell/FsReadView.tsxconsole/web/src/components/chat/shell/FsRmView.tsxconsole/web/src/components/chat/shell/FsSedView.tsxconsole/web/src/components/chat/shell/FsStatView.tsxconsole/web/src/components/chat/shell/FsWriteView.tsxconsole/web/src/components/chat/shell/KillView.tsxconsole/web/src/components/chat/shell/ListView.tsxconsole/web/src/components/chat/shell/StatusView.tsxconsole/web/src/components/chat/shell/__tests__/format.test.tsconsole/web/src/components/chat/shell/__tests__/parsers.test.tsconsole/web/src/components/chat/shell/format.tsconsole/web/src/components/chat/shell/index.tsxconsole/web/src/components/chat/shell/parsers.tsconsole/web/src/components/chat/shell/shared.tsx
Comment on lines
+43
to
+52
| {recursive || sandboxed ? ( | ||
| <div className="flex flex-wrap items-center gap-1.5"> | ||
| {recursive ? ( | ||
| <Chip label="recursive" className="border-warn text-warn"> | ||
| true | ||
| </Chip> | ||
| ) : null} | ||
| <TargetChip target={req.data.target} /> | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
Always show the target for rm results.
This is the only shell fs view here that hides TargetChip for the default host/non-recursive case. That makes host vs sandbox removals harder to disambiguate, especially since the surrounding copy already branches on target-specific semantics (was_present is host-only). Render the target unconditionally and keep only the recursive chip conditional.
Proposed fix
- {recursive || sandboxed ? (
- <div className="flex flex-wrap items-center gap-1.5">
- {recursive ? (
- <Chip label="recursive" className="border-warn text-warn">
- true
- </Chip>
- ) : null}
- <TargetChip target={req.data.target} />
- </div>
- ) : null}
+ <div className="flex flex-wrap items-center gap-1.5">
+ {recursive ? (
+ <Chip label="recursive" className="border-warn text-warn">
+ true
+ </Chip>
+ ) : null}
+ <TargetChip target={req.data.target} />
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {recursive || sandboxed ? ( | |
| <div className="flex flex-wrap items-center gap-1.5"> | |
| {recursive ? ( | |
| <Chip label="recursive" className="border-warn text-warn"> | |
| true | |
| </Chip> | |
| ) : null} | |
| <TargetChip target={req.data.target} /> | |
| </div> | |
| ) : null} | |
| <div className="flex flex-wrap items-center gap-1.5"> | |
| {recursive ? ( | |
| <Chip label="recursive" className="border-warn text-warn"> | |
| true | |
| </Chip> | |
| ) : null} | |
| <TargetChip target={req.data.target} /> | |
| </div> |
🤖 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 `@console/web/src/components/chat/shell/FsRmView.tsx` around lines 43 - 52, The
TargetChip is being conditionally hidden when both recursive and sandboxed are
false; update the FsRmView render so TargetChip (rendering req.data.target) is
always displayed and only the "recursive" Chip remains conditional. Locate the
JSX in FsRmView that currently checks "{recursive || sandboxed ? (...) : null}"
and change it to always render <TargetChip target={req.data.target} /> while
keeping the <Chip label="recursive" ...> block wrapped by the conditional on
"recursive".
Comment on lines
+35
to
+37
| <span>{running ? 'killing job…' : 'killed job'}</span> | ||
| <code className="bg-paper-2 border border-rule-2 px-1.5 py-0.5 text-[12px] text-ink"> | ||
| {truncateMiddle(resp?.job_id ?? req.data.job_id, 24)} |
There was a problem hiding this comment.
Use the response outcome in the headline, not just running.
Once a parsed response exists, this line still renders killed job for the documented killed: false / reason: "not running" case, so the primary message contradicts the footer. Drive the verb from resp.killed when resp is present and reserve running only for the in-flight state.
Proposed fix
- <span>{running ? 'killing job…' : 'killed job'}</span>
+ <span>
+ {running
+ ? 'killing job…'
+ : resp
+ ? resp.killed
+ ? 'killed job'
+ : 'job not running'
+ : 'kill requested'}
+ </span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span>{running ? 'killing job…' : 'killed job'}</span> | |
| <code className="bg-paper-2 border border-rule-2 px-1.5 py-0.5 text-[12px] text-ink"> | |
| {truncateMiddle(resp?.job_id ?? req.data.job_id, 24)} | |
| <span> | |
| {running | |
| ? 'killing job…' | |
| : resp | |
| ? resp.killed | |
| ? 'killed job' | |
| : 'job not running' | |
| : 'kill requested'} | |
| </span> | |
| <code className="bg-paper-2 border border-rule-2 px-1.5 py-0.5 text-[12px] text-ink"> | |
| {truncateMiddle(resp?.job_id ?? req.data.job_id, 24)} |
🤖 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 `@console/web/src/components/chat/shell/KillView.tsx` around lines 35 - 37, The
headline should use the parsed response instead of only `running`: change the
span's text logic in KillView to: if `running` show "killing job…", else if
`resp` exists show `resp.killed ? 'killed job' : 'did not kill job'` (or another
concise failure message derived from `resp.reason`), otherwise fall back to the
previous default; update the conditional that currently reads `{running ?
'killing job…' : 'killed job'}` to inspect `resp?.killed` and leave the job id
display using `truncateMiddle(resp?.job_id ?? req.data.job_id, 24)` unchanged.
Comment on lines
+13
to
+17
| /** `1234` → `"1234ms"`; ≥ 10 s humanize via `formatAgeSecs` so | ||
| long-lived jobs read as `2m`/`3h` instead of raw millis. */ | ||
| function formatDurationMs(ms: number): string { | ||
| return ms < 10_000 ? `${ms}ms` : formatAgeSecs(Math.floor(ms / 1000)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Duplicate helper: extract formatDurationMs to shared format module.
formatDurationMs is defined identically in both StatusView.tsx (lines 33-35) and here. Extract it to format.ts to eliminate duplication.
🤖 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 `@console/web/src/components/chat/shell/ListView.tsx` around lines 13 - 17, The
helper function formatDurationMs is duplicated in ListView and StatusView;
extract it into a new shared module (e.g., format.ts) that imports/uses
formatAgeSecs, export formatDurationMs from that module, then replace the local
definitions by importing formatDurationMs into ListView and StatusView (remove
the duplicate functions). Make sure the new format.ts preserves the current
behavior (ms < 10_000 → `${ms}ms`, otherwise call
formatAgeSecs(Math.floor(ms/1000))) and update any imports so ListView and
StatusView reference the shared function.
Add shell-fixtures.ts with 28 wire-accurate fixtures covering all 16 shell worker functions and a ShellFamily gallery story alongside the existing sandbox/coder/web families. - mirrors the serde wire shapes pinned by the shell parsers: transparent fs::stat, channel-ref-only fs::read, batch + streamed fs::write, kill reason skip-serialized, sandbox target variant - state variants: running, approval previews for exec/exec_bg/kill - both error paths: flat S215 ErrorBody and the harness gate-denial envelope with S211 embedded in the reason text Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
is this expected? |
sergiofilhowz
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
shell::*function calls in the console chat currently fall back to raw REQUEST/RESPONSE JSON panes. This adds ashell/renderer module covering all 16 shell worker functions with the same terminal-style visualization quality as the existingsandbox/coder/webmodules.What's included
console/web/src/components/chat/shell/— new module (parsers.ts,format.ts,shared.tsx,index.tsx, 16 view components, 2 test suites)ExecView(+ approval preview),ExecBgView(↻ started <job_id>with copyable full job id, host-bgtimeout_ms ignoredhint),StatusView(full job record terminal),KillView(+ preview),ListView(jobs table),ConfigStatusViewtargetoptional-not-nullable (serde default), killreasonskip-serialized, transparentfs::stat, chmodupdated→entries_changedalias,argsnull-vs-[]tokenization semantics preserved, explicitstdout_truncated/stderr_truncatedflagsparseShellErrorDisplayrecovers S-codes from flat{code, message}bodies and from harness gate-denial envelopes whosereasonembeds the code as text (trigger_failed: IIIInvocationError: S211: …) — the path real shell errors take today; falls back to the shared sandbox error normalizer for denialscollectErrorCandidates, extractFsEntriesTable,GrepMatchList,SedResultsTableas reusable components; existing views become thin wrappers, suites stay greenTest plan
pnpm test— 38 files, 717/717 (86 new tests pinning schema decisions + error paths)pnpm typecheck— cleanpnpm lint— feature files cleanshell::exec/shell::exec_bgand confirm terminal cards + S-code error cards (raw json tab still shows original payloads)Out of scope (flagged for follow-up)
shell::fs::sed/mv/chmod/mkdirare currently auto-acceptable inlib/backend/auto-accept-policy.tsdespite mutating the host fs — intentionally left untouched per discussion.🤖 Generated with Claude Code
Summary by CodeRabbit