feat(shell): introduce shell tool for persistent session management #175
Conversation
- Added ShellToolHandler to manage shell sessions with actions like exec, send, wait, and kill. - Enhanced ComputerSidebar to display accurate action text based on shell actions. - Updated MessagePartHandler to handle new shell tool messages. - Implemented session management script for tmux integration, allowing for persistent interactive shell sessions. - Refactored terminal-related components to support new shell tool functionality and improve output handling. - Updated Dockerfile to include tmux for shell session management. - Enhanced sidebar content extraction to accommodate new shell tool data structure.
- Moved shell action labels from inline mapping to a dedicated constant for improved readability and maintainability. - Updated the ComputerSidebar component to utilize the new constant for displaying terminal action text.
- Introduced SharedMessagePartHandler to streamline rendering of message parts, improving code organization and readability. - Updated SharedMessages component to utilize SharedMessagePartHandler for consistent rendering of various message types. - Refactored individual tool handlers (FileToolsHandler, TerminalToolHandler, ShellToolHandler, PythonToolHandler) to support external sidebar opening, making them context-agnostic. - Added TodoBlockProvider to SharedChatView for improved state management. - Updated tests to reflect changes in message rendering and state handling.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a tmux-backed persistent shell tool and session-manager, replaces the legacy run_terminal_cmd with a new shell tool, extends tool/context types with chatId and shell metadata, wires a ShellToolHandler and shared UI handlers, and standardizes sidebar/label behavior across multiple tool handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant ShellHandler as ShellToolHandler
participant Tool as Shell Tool
participant Sandbox
participant SessionMgr as Session Manager
participant Sidebar as ComputerSidebar
User->>UI: trigger shell action (exec/view/wait/send/kill)
UI->>ShellHandler: handle tool call / render
ShellHandler->>Tool: invoke shell action (includes chatId, session)
Tool->>Sandbox: acquire sandbox & ensure readiness
Tool->>Sandbox: write/install session-manager script
Tool->>SessionMgr: run session-manager command in sandbox
SessionMgr-->>Tool: stream "STREAM:..." deltas
Tool-->>UI: emit streaming deltas (partial messages)
UI->>Sidebar: update sidebar content (sessionName, showContentOnly)
SessionMgr-->>Tool: final JSON result
Tool-->>UI: write final output/result
UI->>Sidebar: render final terminal state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 7
🤖 Fix all issues with AI agents
In `@app/components/tools/HttpRequestToolHandler.tsx`:
- Around line 118-121: getActionText currently returns "Requesting" for any
non-"input-streaming" state, which is misleading for error states; update
getActionText to return a specific error string (e.g., "Request failed") when
state === "output-error" and keep "Preparing request" for "input-streaming" and
"Requesting" for the normal success/doing state—apply the same change to the
second similar mapping used around the output rendering logic so the UI shows a
clear "Request failed" message for error states (reference getActionText and the
output rendering branch that handles "output-error").
In `@app/components/tools/ShellToolHandler.tsx`:
- Around line 6-9: You redefined the ShellAction type locally which duplicates
the exported type in "@/types/chat"; update the import list to import
ShellAction from "@/types/chat" (alongside isSidebarTerminal, ChatStatus,
SidebarTerminal), remove the local type declaration for ShellAction, and ensure
any references in this file use the imported ShellAction symbol so the type is
canonical and maintained from types/chat.
In `@lib/ai/tools/shell.ts`:
- Around line 398-405: The replacement using new RegExp(tmuxSessionName, "g")
can misinterpret regex metacharacters and risk ReDoS; change the logic in the
block that constructs result (where parsedResult, tmuxSessionName and session
are used) to perform a literal global replacement instead of a regex—either
escape tmuxSessionName with a safe escapeRegExp helper before creating the
RegExp, or replace using a non-regex approach such as splitting
parsedResult.content on tmuxSessionName and joining with session—ensure the
replacement is global and preserves the existing behavior of returning the
modified parsedResult.content.
In `@lib/ai/tools/utils/session-manager-script.ts`:
- Around line 326-336: The is_command_running() check has a TOCTOU race before
send_keys() — fix by making the start of a new command atomic: acquire an
exclusive session-level lock or write a unique start-marker to the pane and
re-verify ownership before sending input; specifically, modify the flow around
is_command_running(session_id) and send_keys() so you either (a) obtain a lock
tied to session_id, re-check is_command_running(session_id) while holding the
lock, then call send_keys(), or (b) emit a unique marker (e.g., a sentinel
string) into the pane, wait/read until that marker appears via
capture_pane()/clean_output() to confirm the pane is yours, and only then call
send_keys(); ensure failure to obtain the lock or detect another command returns
the existing make_result(...) error path.
- Around line 645-648: The current send branch builds input_text via "
".join(sys.argv[3:]) which can’t recover original quoting/spacing; change the
send handling so that if there is exactly one argv after the action you use it
as-is, but if the caller supplied zero or multiple tokens you read the full raw
payload from stdin (or require a single quoted argument/--text flag) and pass
that raw string into action_send(session_id, input_text); update the send branch
that references sys.argv and the call to action_send to implement this stdin
fallback so original formatting is preserved.
In `@lib/ai/tools/utils/session-manager-utils.ts`:
- Around line 91-103: The current greedy regex stdout.match(/\{[\s\S]*\}/) can
capture too much when stdout contains multiple JSON objects; update the logic in
session-manager-utils.ts around jsonMatch/JSON.parse to extract the last JSON
object instead (e.g., locate the last '{' and the following matching '}' and
parse that substring) before mapping parsed.content/status/exit_code/working_dir
to camelCase; alternatively, look for a unique delimiter emitted by the Python
script and parse the JSON after that delimiter to ensure you always parse the
final result.
In `@lib/utils/sidebar-utils.ts`:
- Around line 60-68: The sidebar code wrongly treats part.input.action as a
displayable command; update the condition and command fallback in the function
that builds the command variable for parts with type "tool-run_terminal_cmd" or
"tool-shell" (look for uses of part.type === "tool-run_terminal_cmd" ||
part.type === "tool-shell" and the surrounding logic that sets const command =
...). Remove part.input.action from the predicate and from the fallback chain so
the presence check and command assignment only consider part.input.command and
part.input.input (falling back to "shell" if neither exists).
🧹 Nitpick comments (13)
app/components/tools/PythonToolHandler.tsx (1)
140-151: Consider using past tense for completed states.Using "Executing Python" for
output-availablestate is semantically misleading since execution has already completed. Consider using "Executed Python" to accurately reflect the tool's state. The same applies to theoutput-errorcase on lines 152-163.While I understand this aligns with the PR's standardization effort, accurate state representation improves user experience by clearly indicating whether an operation is in progress or finished.
Suggested differentiation
case "output-available": return ( <ToolBlock key={toolCallId} icon={<Code2 />} - action="Executing Python" + action="Executed Python" target={codePreview} isClickable={true} onClick={handleOpenInSidebar} onKeyDown={handleKeyDown} /> ); case "output-error": return ( <ToolBlock key={toolCallId} icon={<Code2 />} - action="Executing Python" + action="Executed Python" target={codePreview} isClickable={true} onClick={handleOpenInSidebar} onKeyDown={handleKeyDown} /> );lib/ai/tools/run-terminal-cmd.ts (1)
1-480: Consider removing this commented-out code instead of keeping it.The entire file is now commented out since the
shelltool replacesrun_terminal_cmd. Keeping 480 lines of commented code creates maintenance burden and confusion. If this implementation is no longer needed, delete the file entirely. If preservation is needed for reference during transition, consider keeping it in a separate branch or documenting the key differences in the new implementation instead.lib/ai/tools/utils/session-manager-script.ts (3)
156-172: Command detection may have false positives/negatives.The shell detection compares
pane_current_commandagainst a hardcoded set of shell names. Programs that spawn subshells or have shell-like names may be misdetected. Additionally, if a user runs a command likebash -c "long_running_cmd", the pane command showsbashand the check incorrectly reports no command running.Consider also checking if the shell is in an interactive prompt state (e.g., by verifying the PS1 marker appears at the end of the captured pane content).
472-486: Escape sequence decoding could interpret unexpected unicode.The
decode_escape_sequencesfunction uses Python'sraw_unicode_escapecodec, which decodes\uXXXXsequences. If user input contains arbitrary escape sequences, this could produce unexpected characters. The fallback with manual replacements is safer but incomplete.Consider documenting which escape sequences are intentionally supported and rejecting or escaping others.
570-576: Duplicate signal attempts in kill methods array.The
methodslist containsC-candC-dtwice each. While this may be intentional for retry purposes, it adds unnecessary delay (total ~2.2s just for signals). Consider removing duplicates or documenting why multiple attempts of the same signal are needed.Suggested simplification
methods = [ ("C-c", 0.5), - ("C-d", 0.5), - ("C-c", 0.2), - ("C-d", 0.3), + ("C-c", 0.3), # retry + ("C-d", 0.5), ("C-\\\\", 0.5), ]app/components/tools/TerminalToolHandler.tsx (1)
126-149: Same action text for completed and error states may be confusing.Using "Executing" for both
output-availableandoutput-errorstates doesn't communicate the outcome to users. Consider using "Executed" for completed states and "Execution failed" for error states to provide clearer feedback.If the intent is to keep labels simple and let users discover the outcome by clicking through to the sidebar, this is acceptable but worth documenting as a deliberate UX decision.
app/share/[shareId]/SharedMessages.tsx (1)
100-104: Consider replacinganywith a more specific union type.Using
anyforresultbypasses TypeScript's type checking. A union type would improve safety:Suggested improvement
const terminalOutput = part.output as { - result?: any; + result?: string | { + output?: string; + content?: string; + stdout?: string; + stderr?: string; + }; output?: string; content?: string; };app/components/ComputerSidebar.tsx (1)
222-224: Clarify the comment to match the actual behavior.The comment says "session ID" but the code displays
sidebarContent.command. If for shell tools thecommandfield actually contains the session identifier, consider renaming or clarifying this in the type definition. Otherwise, update the comment to reflect what's actually being displayed.lib/ai/tools/shell.ts (4)
98-101: Consider validating thesessionparameter format.The
sessionparameter is used directly in the tmux session name (hackerai-{session}-{chatId}). If it contains special characters (spaces, quotes, dashes, etc.), it could cause issues with shell escaping or tmux session naming. Consider adding a regex validation to restrict to alphanumeric characters and underscores.Suggested validation
session: z .string() .default("default") + .regex(/^[a-zA-Z0-9_-]+$/, "Session name must contain only alphanumeric characters, underscores, and hyphens") .describe("The unique identifier of the target shell session"),
195-203: The sandbox reset pattern could be cleaner.The
setSandbox(null as never)type assertion is a workaround. While functional, this suggests a potential type mismatch in the sandbox manager interface. Consider updating thesetSandboxsignature to acceptnullexplicitly if this is a valid state.
336-348: Consider buffering partial lines in the streaming handler.The
customStdoutHandlersplits data by newlines and processes each line. If stdout data arrives mid-line (partial JSON), the parse will fail silently. For robustness with streaming data, consider buffering incomplete lines until a newline is received.Buffered line processing example
let lineBuffer = ""; const customStdoutHandler = (data: string) => { lineBuffer += data; const lines = lineBuffer.split("\n"); // Keep the last potentially incomplete line in the buffer lineBuffer = lines.pop() || ""; for (const line of lines) { const streamData = parseStreamOutput(line); if (streamData) { hasStreamedOutput = true; createTerminalWriter(streamData.output); } } handler.stdout(data); };
362-377: Consider adding logging for retry attempts.The
loggercallback is set to a no-op, which silences retry attempt logging. For production observability, consider logging retries to help diagnose intermittent sandbox issues.lib/ai/tools/index.ts (1)
48-48: Align thechatIdparameter with its required usage increateTools.The
chatIdparameter is declared as optional (chatId?: string) with a fallback to"unknown", but analysis shows thatchatIdis always required in the request payload and always passed tocreateToolsat the only call site inlib/api/chat-handler.ts. The fallback to"unknown"is unreachable in practice.For code clarity, make the parameter required: change
chatId?: stringtochatId: stringin the function signature at line 48 and remove the fallback|| "unknown"at line 82. This better reflects actual usage and removes the misleading defensive pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/components/ComputerSidebar.tsxapp/components/MessagePartHandler.tsxapp/components/TerminalCodeBlock.tsxapp/components/tools/FileToolsHandler.tsxapp/components/tools/GetTerminalFilesHandler.tsxapp/components/tools/HttpRequestToolHandler.tsxapp/components/tools/MatchToolHandler.tsxapp/components/tools/MemoryToolHandler.tsxapp/components/tools/PythonToolHandler.tsxapp/components/tools/ShellToolHandler.tsxapp/components/tools/TerminalToolHandler.tsxapp/components/tools/WebToolHandler.tsxapp/share/[shareId]/SharedMessages.tsxdocker/Dockerfilelib/ai/tools/index.tslib/ai/tools/run-terminal-cmd.tslib/ai/tools/shell.tslib/ai/tools/utils/session-manager-script.tslib/ai/tools/utils/session-manager-utils.tslib/api/chat-handler.tslib/utils/message-processor.tslib/utils/sidebar-utils.tstypes/agent.tstypes/chat.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T01:16:26.536Z
Learnt from: fkesheh
Repo: hackerai-tech/hackerai PR: 114
File: lib/ai/tools/utils/convex-sandbox.ts:95-145
Timestamp: 2025-11-28T01:16:26.536Z
Learning: In the local sandbox architecture (lib/ai/tools/utils/convex-sandbox.ts), background command execution semantics are handled by the local sandbox client (packages/local/src/index.ts), not by the ConvexSandbox class. When background: true, the local client immediately submits a result with empty output and PID after spawning the process, causing waitForResult() to resolve immediately and maintaining E2B-compatible non-blocking behavior.
Applied to files:
lib/utils/sidebar-utils.tslib/ai/tools/run-terminal-cmd.tslib/ai/tools/utils/session-manager-utils.tslib/ai/tools/shell.ts
📚 Learning: 2025-08-18T20:33:42.057Z
Learnt from: RostyslavManko
Repo: hackerai-tech/hackerai PR: 10
File: app/components/ComputerCodeBlock.tsx:154-158
Timestamp: 2025-08-18T20:33:42.057Z
Learning: In ComputerCodeBlock component, the wrap state should be controlled by the parent component (ComputerSidebar) which passes the wrap prop. The internal isWrapped state conflicts with parent state management when used in ComputerSidebar context.
Applied to files:
app/components/TerminalCodeBlock.tsx
🧬 Code graph analysis (1)
lib/ai/tools/utils/session-manager-utils.ts (1)
lib/ai/tools/utils/session-manager-script.ts (2)
SESSION_MANAGER_PATH(670-670)SESSION_MANAGER_SCRIPT(5-665)
🪛 ast-grep (0.40.5)
lib/ai/tools/shell.ts
[warning] 401-401: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(tmuxSessionName, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (39)
docker/Dockerfile (2)
133-133: LGTM!The tmux installation is correctly added to support the new persistent shell session management feature.
290-290: LGTM!Validation check for tmux is properly added to ensure the tool is available in the container.
app/components/tools/MemoryToolHandler.tsx (1)
97-117: LGTM!The consolidation to use
getActionTextfor all states simplifies the code while maintaining appropriate action labels. The present-progressive tense ("Creating", "Updating", "Deleting") works well for both streaming and completed states in this context.app/components/tools/WebToolHandler.tsx (1)
45-47: LGTM!The simplified
getActionfunction removes the unusedisCompletedparameter, making the code cleaner while maintaining consistent action labels across all states.types/agent.ts (1)
35-35: LGTM!The
chatIdfield is appropriately added toToolContextfor shell session identification. The call site inlib/api/chat-handler.tsprovides this field, and thecreateToolsfunction includes a fallback value ("unknown") for cases where it's not supplied.app/components/tools/MatchToolHandler.tsx (1)
40-42: LGTM - simplified label aligns with standardization effort.The result label now shows the action performed ("Finding files" / "Searching") rather than outcome-based text. Users can still click to view actual results in the sidebar. This aligns with the broader UI standardization across tool handlers in this PR.
app/components/tools/FileToolsHandler.tsx (5)
185-185: LGTM - consistent action label for completed state.
275-275: LGTM - consistent action label for completed state.
340-340: LGTM - action label standardized to match streaming state.The delete tool now shows "Deleting" consistently rather than conditional success/failure text. This aligns with the UI standardization across tool handlers.
411-413: LGTM - action logic reused from input-available state.
470-470: LGTM - consistent pattern with other file tools.lib/ai/tools/utils/session-manager-script.ts (2)
667-670: LGTM - path constant for session manager script.The
/tmplocation is appropriate for a sandbox environment. The hidden file prefix keeps it out of casual listings.
86-96: The shell injection concern in this review is incorrect. Thesend_keysfunction uses POSIX-compliant single-quote escaping ('\\\\''), which safely prevents all shell expansions (backticks,$(), variables) by wrapping the keys parameter in single quotes. Backticks and command substitution inside single-quoted strings are treated as literal characters, not executed.The suggestion to use tmux's
-l(literal) flag is inappropriate here. The-lflag causes tmux to interpret arguments as literal UTF-8 characters instead of named keys, which would break the current functionality—the string"Enter"would be sent as the individual letters E-n-t-e-r rather than an actual Enter key press.Likely an incorrect or invalid review comment.
lib/ai/tools/utils/session-manager-utils.ts (3)
6-30: LGTM - well-defined types for session manager integration.The types correctly abstract the sandbox interface and define the expected result structure matching the Python script's output format.
40-71: LGTM - robust script installation with caching.The WeakMap-based caching prevents redundant installations per sandbox instance. Content comparison ensures updates when the script changes. Error handling returns false allowing callers to handle failures.
73-80: LGTM - correct POSIX shell argument escaping.The single-quote wrapping with
'\''escape sequence is the standard approach for safely passing arbitrary strings as shell arguments.lib/utils/sidebar-utils.ts (1)
78-93: LGTM — Output extraction handles multiple formats gracefully.The cascading fallback logic (
result.output→result.content→ legacystdout/stderr→ string result) provides good backward compatibility while supporting the new shell tool output format.app/components/tools/GetTerminalFilesHandler.tsx (1)
50-82: LGTM — Unified "Sharing" label is appropriate for file operations.The consistent "Sharing" action text across all states simplifies the UI without losing meaningful context, as file transfer operations don't require the same state differentiation as command execution.
lib/api/chat-handler.ts (2)
226-240: ThecreateToolsfunction signature inlib/ai/tools/index.ts(line 48) correctly includeschatId?: stringas a parameter. The function has been properly updated to accept this parameter, and the call in chat-handler.ts line 239 is consistent with the function definition.
447-453: Telemetry tracking forshelltool is correctly aligned.The tool factory registers the tool as
shell: createShell(context). Theailibrary uses these property names as tool identifiers inchunk.chunk.toolName, so telemetry will correctly capture "shell" tool calls. No transition issue exists—run_terminal_cmdis already removed from the factory (only commented), and the telemetry array properly includes "shell". The use of"tool-run_terminal_cmd"in message storage and UI is for backward compatibility with existing persisted messages, not for real-time telemetry tracking.types/chat.ts (1)
26-39: LGTM! Clean type extensions for shell tool support.The
ShellActiontype andSidebarTerminalinterface extensions are well-structured. The optional fields preserve backward compatibility, and the JSDoc comments provide helpful context for future maintainers.app/components/MessagePartHandler.tsx (1)
91-93: LGTM! Consistent integration with existing tool handler patterns.The new
tool-shellcase follows the established pattern used by other tool handlers, passing the same props (message,part,status).app/share/[shareId]/SharedMessages.tsx (1)
112-126: Output resolution logic handles multiple formats correctly.The cascading fallback logic properly handles the various output shapes from
tool-run_terminal_cmdandtool-shell. The approach is defensive and ensures graceful degradation.app/components/TerminalCodeBlock.tsx (1)
214-220: LGTM! Clean conditional content assembly.The
showContentOnlyprop correctly suppresses the command prefix for non-exec shell actions (likeview,wait,send), displaying only the terminal output when appropriate.app/components/tools/ShellToolHandler.tsx (3)
56-71: LGTM! Memoization correctly optimizes streaming output computation.The filtering of
data-terminalparts bytoolCallIdensures the component only processes relevant streaming data. Dependencies are properly specified.
155-164: Real-time sidebar updates implemented correctly.The effect properly gates updates behind
isSidebarActiveto avoid unnecessary state mutations. Theeslint-disableforupdateSidebarContentis a reasonable tradeoff to prevent infinite render loops when the function has an unstable identity.
173-200: LGTM! State-based rendering handles all tool call states appropriately.The switch statement covers the full lifecycle of a tool call with appropriate UI feedback (shimmer during streaming, clickable blocks for completed states). Keyboard accessibility is properly implemented.
lib/utils/message-processor.ts (3)
81-101: LGTM! Type extensions are well-structured.The expanded
TerminalToolPartinterface properly accommodates both the legacytool-run_terminal_cmdand the newtool-shelltype with optional fields that allow flexibility in how each tool populates its data.
266-274: LGTM!The conditional correctly routes the new
tool-shelltype through the same terminal transformation path astool-run_terminal_cmd.
289-305: LGTM!The
tool-shellbranch correctly constructs an output structure matching the shell tool's result format, withcontentinstead ofstdoutand appropriate error messaging for interrupted operations.lib/ai/tools/index.ts (1)
93-96: LGTM!Clean replacement of
run_terminal_cmdwithshell. The commented code provides a clear rollback path if needed.app/components/ComputerSidebar.tsx (3)
178-190: LGTM! Shell action text mapping is well-structured.The
shellActionMapprovides clear, user-friendly labels for all five shell actions, with a sensible fallback for legacy terminal entries.
300-306: LGTM!Session name display in the header is cleanly implemented with appropriate truncation for long names.
392-405: LGTM!The
showContentOnlyprop is correctly passed through toTerminalCodeBlock, enabling the new content display mode for shell tool outputs.lib/ai/tools/shell.ts (5)
25-26: LGTM!Timeout constants are reasonable defaults (30s default, 5 minutes max) for shell operations.
128-149: LGTM!Parameter validation correctly enforces required fields for each action type. Using
undefinedcheck forinputallows sending empty strings (like just a newline character), which is valid for terminal interactions.
151-183: LGTM!Security guardrails and scope exclusions are properly applied before command execution, with clear error messages for blocked commands.
280-308: LGTM!Abort signal handling is well-implemented with proper cleanup and guard against double resolution.
28-78: Well-documented tool description.The tool description provides comprehensive guidance for LLM usage, including supported actions, best practices, and recommended usage patterns. This should help the AI agent use the shell tool effectively.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/tools/TerminalToolHandler.tsx (1)
133-156: Action label "Executing" is misleading for completed states.Using "Executing" for
output-availableandoutput-errorstates is semantically incorrect since execution has already completed. This could confuse users about the current state of the operation.Consider using state-specific labels or a generic label like "Ran" or "Terminal" for completed states.
app/components/tools/PythonToolHandler.tsx (1)
147-170: Same label issue: "Executing Python" for completed states.Similar to TerminalToolHandler, using "Executing Python" for
output-availableandoutput-errorstates is misleading since execution has finished.
🤖 Fix all issues with AI agents
In `@app/components/ComputerSidebar.tsx`:
- Around line 186-191: The current lookup of SHELL_ACTION_LABELS using
sidebarContent.shellAction can return undefined for unknown keys; update the
logic in the function that renders the label (referencing
sidebarContent.shellAction and SHELL_ACTION_LABELS) to check that
SHELL_ACTION_LABELS[sidebarContent.shellAction] is a valid string and, if not,
fall back to the existing generic "Executing command" string (or another safe
default). Ensure you only return the mapped label when it exists and is a string
to avoid rendering undefined.
- Around line 301-306: The terminal header can be empty because
sidebarContent.sessionName is assigned directly from shellInput?.session without
the fallback used in ShellToolHandler.getSidebarTitle; update the logic so
sessionName uses the same fallback (e.g., shellInput.session || "default" or a
human label like "Shell"/"Terminal") when creating SidebarTerminal items, or
alternatively change the render in ComputerSidebar (the isTerminal branch that
checks sidebarContent.sessionName) to display a default label or icon when
sessionName is falsy; also audit other creators of SidebarTerminal
(TerminalToolHandler, HttpRequestToolHandler) to ensure they apply the same
fallback when setting sessionName.
In `@app/share/`[shareId]/SharedMessages.tsx:
- Around line 116-135: The render uses otherParts (a filtered array) but passes
its local idx as partIndex to SharedMessagePartHandler, causing the handler to
index into the full message.parts incorrectly (see parts[partIndex - 1] and
collectReasoningText(parts, partIndex)). Fix by computing the corresponding
index in the original message.parts before rendering: derive originalIndex for
each part (e.g., find its position in message.parts or carry an originalIndex
when building otherParts) and pass that originalIndex instead of idx to
SharedMessagePartHandler (or alternatively refactor SharedMessagePartHandler to
accept the part object and not index into message.parts). Ensure all references
to partIndex (used by parts[partIndex - 1] and collectReasoningText) use the
corrected original index.
🧹 Nitpick comments (3)
app/components/tools/TerminalToolHandler.tsx (1)
83-101: Redundant check in useEffect condition.Line 94 checks
externalOpenSidebaragain, butisSidebarActive(line 85) already includes!externalOpenSidebarin its computation, making the second check redundant.Suggested simplification
useEffect(() => { - if (!isSidebarActive || externalOpenSidebar) return; + if (!isSidebarActive) return; updateSidebarContent({ output: finalOutput, isExecuting, }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isSidebarActive, finalOutput, isExecuting, externalOpenSidebar]); + }, [isSidebarActive, finalOutput, isExecuting]);app/components/tools/PythonToolHandler.tsx (1)
99-118: Redundant check in useEffect condition (same as TerminalToolHandler).The check at line 110 for
externalOpenSidebaris redundant sinceisSidebarActive(line 101) already includes!externalOpenSidebar.Suggested simplification
useEffect(() => { - if (!isSidebarActive || externalOpenSidebar) return; + if (!isSidebarActive) return; updateSidebarContent({ code: codePreview, output: finalOutput, isExecuting, }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isSidebarActive, codePreview, finalOutput, isExecuting, externalOpenSidebar]); + }, [isSidebarActive, codePreview, finalOutput, isExecuting]);app/share/[shareId]/SharedMessagePartHandler.tsx (1)
18-23: Consider adding proper types instead ofany.Using
anyformessageandpartreduces type safety and IDE support. Sincetypes/chat.tsalready defines message and part types (including the newShellActionand enhancedSidebarTerminal), consider importing and using those types here.Suggested improvement
+import { Message, MessagePart } from "@/types/chat"; + interface SharedMessagePartHandlerProps { - message: any; - part: any; + message: Message; + part: MessagePart; partIndex: number; isUser: boolean; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/components/ComputerSidebar.tsxapp/components/tools/FileToolsHandler.tsxapp/components/tools/PythonToolHandler.tsxapp/components/tools/ShellToolHandler.tsxapp/components/tools/TerminalToolHandler.tsxapp/share/[shareId]/SharedChatView.tsxapp/share/[shareId]/SharedMessagePartHandler.tsxapp/share/[shareId]/SharedMessages.tsxapp/share/[shareId]/__tests__/SharedMessages.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/tools/FileToolsHandler.tsx
- app/components/tools/ShellToolHandler.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T20:33:42.057Z
Learnt from: RostyslavManko
Repo: hackerai-tech/hackerai PR: 10
File: app/components/ComputerCodeBlock.tsx:154-158
Timestamp: 2025-08-18T20:33:42.057Z
Learning: In ComputerCodeBlock component, the wrap state should be controlled by the parent component (ComputerSidebar) which passes the wrap prop. The internal isWrapped state conflicts with parent state management when used in ComputerSidebar context.
Applied to files:
app/components/tools/PythonToolHandler.tsxapp/share/[shareId]/SharedChatView.tsx
🧬 Code graph analysis (5)
app/share/[shareId]/SharedMessages.tsx (1)
app/share/[shareId]/SharedMessagePartHandler.tsx (1)
SharedMessagePartHandler(39-169)
app/components/tools/PythonToolHandler.tsx (2)
types/chat.ts (3)
ChatStatus(88-88)SidebarContent(49-49)isSidebarPython(63-67)app/contexts/GlobalState.tsx (1)
useGlobalState(752-758)
app/share/[shareId]/__tests__/SharedMessages.test.tsx (3)
app/contexts/GlobalState.tsx (1)
GlobalStateProvider(166-750)app/contexts/TodoBlockContext.tsx (1)
TodoBlockProvider(20-40)app/share/[shareId]/SharedChatContext.tsx (1)
SharedChatProvider(17-39)
app/share/[shareId]/SharedChatView.tsx (2)
app/contexts/TodoBlockContext.tsx (1)
TodoBlockProvider(20-40)app/share/[shareId]/SharedChatContext.tsx (1)
SharedChatProvider(17-39)
app/components/tools/TerminalToolHandler.tsx (2)
types/chat.ts (3)
ChatStatus(88-88)SidebarContent(49-49)isSidebarTerminal(57-61)app/contexts/GlobalState.tsx (1)
useGlobalState(752-758)
🔇 Additional comments (12)
app/components/ComputerSidebar.tsx (2)
31-37: LGTM!The
SHELL_ACTION_LABELSmapping provides clear, user-friendly labels for shell actions. Good extraction of these labels into a constant for maintainability.
393-406: LGTM!The
showContentOnlyprop is properly propagated toTerminalCodeBlock, following the existing pattern for other terminal-specific props.app/components/tools/TerminalToolHandler.tsx (1)
14-27: LGTM!The external sidebar pattern is well-implemented with a clean fallback to GlobalState when
externalOpenSidebaris not provided. This enables context-agnostic usage of the handler.app/components/tools/PythonToolHandler.tsx (1)
13-26: LGTM!Consistent implementation of the external sidebar pattern with proper memoization of handlers via
useCallback. The fallback mechanism is clean and well-documented with comments.app/share/[shareId]/SharedChatView.tsx (2)
233-236: LGTM!The closing tags are properly adjusted to match the new provider nesting structure.
148-149: No action required.TodoBlockProviderusesuseTodoBlockManager(), which is a self-contained hook that depends only on React's built-inuseStateanduseCallbackhooks—it has no dependencies onGlobalStateProvideror any other external contexts. The provider can function correctly at its current nesting level inSharedChatView.tsx.Likely an incorrect or invalid review comment.
app/share/[shareId]/SharedMessages.tsx (1)
3-4: LGTM!Clean simplification of imports by delegating rendering logic to
SharedMessagePartHandler. This improves maintainability and ensures consistent rendering across different part types.app/share/[shareId]/__tests__/SharedMessages.test.tsx (3)
182-198: LGTM - Test data updated to reflect new input shapes.The test correctly uses
target_fileinstead offile_pathand includesoutputfor the read file operation. The assertion change from "Read" to "Reading" aligns with the UI label changes.
299-326: LGTM - Todo test updated with realistic output structure.The test now includes a proper output structure with
result,counts, andcurrentTodos, and the assertion correctly checks for the rendered todo item text rather than a generic status label.
11-19: No action needed—the test setup is correct.The provider nesting in the test accurately reflects production.
GlobalStateProvideris provided at the app root inapp/layout.tsx(line 96), and the test correctly wraps the component with all necessary providers:GlobalStateProvider > TodoBlockProvider > SharedChatProvider.SharedChatView.tsxitself doesn't wrap with these providers; it relies on the root layout to supply them, which is the expected behavior.app/share/[shareId]/SharedMessagePartHandler.tsx (2)
49-70: LGTM!The reasoning deduplication logic is well-implemented—the first reasoning part in a consecutive sequence renders all combined text, while subsequent reasoning parts correctly return null to avoid duplicates. The REDACTED filtering regex is also correct.
72-168: LGTM!The tool handler routing is well-organized with consistent patterns. The
status="ready"is appropriate for historical/shared views, and the legacy terminal handler maintains backward compatibility. Good distinction between handlers that need sidebar access and those that don't.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Import ShellAction from types/chat.ts instead of redefining (ShellToolHandler) - Use string split/join instead of regex for session name replacement (shell.ts) - Handle all shell actions in sidebar extraction including view/wait/kill (sidebar-utils) - Add fallback for unknown shellAction values (ComputerSidebar) - Add "Shell" fallback when sessionName is missing (ComputerSidebar) - Fix partIndex mismatch by computing original index in message.parts (SharedMessages) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/sidebar-utils.ts (1)
106-112: Guardis_backgroundaccess when input is missing.
part.inputcan be undefined for some terminal parts; direct access can throw. Use optional chaining with a nullish fallback.✅ Safer null handling
- isBackground: part.input.is_background || false, + isBackground: part.input?.is_background ?? false,
🤖 Fix all issues with AI agents
In `@lib/ai/tools/shell.ts`:
- Around line 355-378: The session manager command is being retried via
retryWithBackoff which can re-run non‑idempotent shell actions; change the call
around sandboxInstance.commands.run(sessionManagerCmd, ...) to avoid automatic
retries for non‑idempotent operations: either call
sandboxInstance.commands.run(...) directly (remove retryWithBackoff) or add an
explicit idempotent flag/option to the call and only wrap with retryWithBackoff
when that flag is set; adjust the isPermanentError or caller logic so
retryWithBackoff is only used for idempotent commands and ensure
sessionManagerCmd is treated as non‑idempotent if it sends input/state changes.
🧹 Nitpick comments (1)
app/share/[shareId]/SharedMessages.tsx (1)
116-142: Optional: avoid repeatedindexOfscans when mapping parts.
message.parts.indexOf(part)is O(n) per element and can mis-handle duplicate references. Consider carryingoriginalIndexwhile filtering.♻️ Precompute original indices
- const otherParts = message.parts.filter( - (part) => - !( - (part.type === "file" || part.type === "image") && - part.placeholder - ), - ); + const otherParts = message.parts + .map((part, originalIndex) => ({ part, originalIndex })) + .filter( + ({ part }) => + !( + (part.type === "file" || part.type === "image") && + part.placeholder + ), + ); ... - {otherParts.map((part, idx) => { - const originalIndex = message.parts.indexOf(part); + {otherParts.map(({ part, originalIndex }, idx) => { return ( <SharedMessagePartHandler key={`${message.id}-${idx}`} message={message} part={part} - partIndex={originalIndex !== -1 ? originalIndex : idx} + partIndex={originalIndex} isUser={isUser} /> ); })} ... - otherParts.map((part, idx) => { - const originalIndex = message.parts.indexOf(part); + otherParts.map(({ part, originalIndex }, idx) => { return ( <SharedMessagePartHandler key={`${message.id}-${idx}`} message={message} part={part} - partIndex={originalIndex !== -1 ? originalIndex : idx} + partIndex={originalIndex} isUser={isUser} /> ); })
…on management improvements - Added formatShellInput function to convert special key combinations into a readable format for shell input display. - Updated ShellToolHandler to utilize formatShellInput for better representation of user input. - Improved session management script to ensure consistent behavior across platforms by explicitly using bash. - Enhanced session creation logic with fallback mechanisms and robust prompt handling for better user experience. - Refactored session result parsing to include token-based content truncation for improved output handling.
|
Will need to run "cd packages/local && npm run build && npm publish --access public" before merge. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/components/tools/ShellToolHandler.tsx`:
- Around line 115-118: The current useMemo for finalOutput uses logical OR which
treats an explicit empty string in shellOutput.result.content as falsy and falls
back to streamingOutput or errorText; change the fallback to a null/undefined
check so an intentionally empty content is preserved (modify the expression in
useMemo that computes finalOutput to prefer shellOutput?.result?.content when it
is not null/undefined, falling back to streamingOutput then errorText), keeping
the same dependencies and the hook (useMemo) and referencing finalOutput,
shellOutput.result.content, streamingOutput, and errorText.
- Around line 94-98: The component currently always calls useGlobalState (which
throws outside GlobalStateProvider) even when externalOpenSidebar is provided;
change the code so it does not call useGlobalState when externalOpenSidebar is
supplied: branch on externalOpenSidebar first and only call useGlobalState when
needed, or implement a non-throwing safe hook (e.g., safeUseGlobalState) that
returns undefined/defaults for sidebarOpen, sidebarContent and
updateSidebarContent; then update usages of openSidebar, sidebarOpen,
sidebarContent and updateSidebarContent in ShellToolHandler to handle the
undefined/default case accordingly.
♻️ Duplicate comments (1)
lib/ai/tools/utils/session-manager-utils.ts (1)
87-148: The line-by-line approach from the end addresses the previous greedy regex concern.The improved implementation at lines 94-115 correctly iterates backwards to find the last valid JSON line with expected fields (
statusandworking_dir). This handles the common case robustly.The greedy regex fallback at lines 117-132 still has the potential matching issue noted in previous review, but since it's now a fallback for backwards compatibility and the primary approach handles the expected format, the risk is significantly reduced.
🧹 Nitpick comments (2)
lib/ai/tools/utils/bash-commands.ts (1)
52-70: Unquotedscopeparameter is inconsistent withbuildGlobCommandand potentially unsafe.The
scopevariable on line 69 is passed unquoted to the shell, unlikebuildGlobCommandwhich properly quotes all user input. This creates two issues:
- Shell metacharacters: Paths containing
$, backticks, or other metacharacters could lead to unexpected behavior- Paths with spaces: Would be incorrectly split into multiple arguments
While
match.tsvalidates that scope starts with/, this doesn't prevent metacharacter issues.♻️ Suggested fix: Quote the scope parameter
// grep: -r recursive, -I skip binary, -H filename, -n line numbers, -E extended regex - return `grep -r -I -H -n -E ${contextFlags} '${escapedRegex}' ${scope} 2>/dev/null | head -n ${MAX_GREP_LINES}`; + const escapedScope = escapeForBashSingleQuote(scope); + return `grep -r -I -H -n -E ${contextFlags} '${escapedRegex}' '${escapedScope}' 2>/dev/null | head -n ${MAX_GREP_LINES}`;Note: If glob expansion is intentional, consider using
findwith-exec grepinstead to properly handle the glob pattern while maintaining safety.lib/ai/tools/utils/session-manager-utils.ts (1)
74-81: Minor duplication withescapeForBashSingleQuotein bash-commands.ts.This function is identical to
escapeForBashSingleQuotein./bash-commands.ts. Consider importing from there to avoid duplication.♻️ Optional: Import instead of duplicating
import { SESSION_MANAGER_SCRIPT, SESSION_MANAGER_PATH, } from "./session-manager-script"; +import { escapeForBashSingleQuote } from "./bash-commands"; import { truncateContent, TOOL_DEFAULT_MAX_TOKENS } from "@/lib/token-utils"; // ... later in the file ... -/** - * Escape a string for use as a shell argument. - * Uses single quotes and escapes any embedded single quotes. - */ -export const escapeShellArg = (arg: string): string => { - // Replace single quotes with '\'' (end quote, escaped quote, start quote) - return `'${arg.replace(/'/g, "'\\''")}'`; -}; +// Re-export for consumers that expect this name +export const escapeShellArg = (arg: string): string => { + return `'${escapeForBashSingleQuote(arg)}'`; +};Note: The slight difference is that
escapeShellArgwraps the result in quotes whileescapeForBashSingleQuotedoes not. This can be preserved by composing.
- Only retry idempotent shell actions (view, wait, kill) to prevent duplicate command execution or input being sent twice on transient errors - Add useOptionalGlobalState hook for context-agnostic component usage - Use nullish coalescing in finalOutput to preserve empty string content Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…uidance - Add note that commands are never killed on timeout - Add background process guidance using & for daemons/servers - Extend special keys list with official tmux key names (Home, End, F1-F12, PageUp, PageDown, Space) - Add modifier combinations guidance (M-key, S-key, C-S-key) - Add tmux naming convention note (BSpace not Backspace, etc.) - Add get_terminal_files reminder for sharing created files - Simplify input param description Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.