UI upgraded to Ink with fixed positions and more UX tweaks#2
Conversation
WalkthroughThis PR introduces a new Ink-powered Text User Interface (TUI) for interactive agent-driven coding alongside a Classic mode, refactors agent communication through a new TypeScript AgentClient module, adds local shell command execution capabilities with event-driven logging, updates configuration management to consume YAML defaults, and extends session/context tracking features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI as TUI App
participant AgentClient
participant Agent
participant ContinuationHandler
User->>TUI: Input message / slash command
TUI->>TUI: Parse command or prepare user message
alt Local command (!)
TUI->>TUI: Execute local shell command
TUI->>User: Display result as system message
else Slash command (/)
TUI->>TUI: Handle session/context/goal/diff actions
TUI->>User: Update UI or display output
else User message
TUI->>AgentClient: sendMessage(message, mode, sessionId)
AgentClient->>Agent: POST with message, agentMode
Agent-->>AgentClient: Stream primary response + tool calls
AgentClient->>TUI: onStatus('stream'), onToken(chunk)
TUI->>TUI: Append chunks to assistant message, update UI
alt Tool calls present
AgentClient->>ContinuationHandler: handleContinuation(toolCalls)
ContinuationHandler->>ContinuationHandler: emitToolStart(toolName, params)
ContinuationHandler->>ContinuationHandler: Execute tool locally
ContinuationHandler->>ContinuationHandler: emitToolComplete(result)
ContinuationHandler->>Agent: POST continuation with tool results
Agent-->>ContinuationHandler: Stream continuation response
ContinuationHandler->>AgentClient: Aggregate final response
end
AgentClient->>TUI: onStatus('complete'), return AgentMessageResult
TUI->>TUI: Save to session history, persist context
TUI->>User: Display final response, ready for next input
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/undo-manager.ts (1)
130-144: Persisting raw commands and outputs may leak secrets
recordCommandwrites the full command string and the first 500 characters of output to the on‑disk change log. For shell commands this can easily include:
- tokens, passwords, or API keys passed as flags/env,
- file paths or content that should not be stored long‑term.
Because this log lives under the user’s home directory, it’s effectively audit storage for everything executed via the tool.
Consider tightening this by one or more of:
- Redacting known‑sensitive patterns (e.g.,
token,password,secret,key=) before persisting.- Allowing a configuration flag to disable command/output logging or cap it much more aggressively.
- Storing only a short human label (e.g., “Executed git status”) or an ID, instead of the entire command line/output.
That keeps the UX benefit of having command history without silently persisting sensitive material.
cli.js (1)
25-28:--projectcurrently doesn’t affect the working directory used by setup/session/local commands.You capture:
const ACTUAL_TERMINAL_CWD = process.env.ORIGINAL_TERMINAL_CWD || process.env.PWD || process.cwd();and then consistently use
ACTUAL_TERMINAL_CWDfor:
runLocalCommand(trimmed, ACTUAL_TERMINAL_CWD)inexecuteLocalCommand,new ContinuationHandler(ACTUAL_TERMINAL_CWD),new SetupManager(ACTUAL_TERMINAL_CWD)/new SessionManager(ACTUAL_TERMINAL_CWD)inclassicInteractiveMode,- project detection in
detectProject.However,
--projectonly callsprocess.chdir(options.project)and logs the newprocess.cwd(). It does not change the path passed into any of the above, so most of the tool’s behavior (config, sessions, local commands, detection) still targets the original terminal directory.If the intent of
--projectis to run against a chosen project directory, you’ll want to thread aworkingDirectoryvariable (derived from eitherACTUAL_TERMINAL_CWDoroptions.project) through to:
ContinuationHandler,SetupManager/SessionManager,runLocalCommand,detectProjectand TUIrunTui.Right now,
--projectis mostly cosmetic.Also applies to: 161-195, 524-556, 752-783, 820-823
🧹 Nitpick comments (18)
example.js (2)
35-44: Consider more robust ID generation.Using
Date.now()for task IDs can produce collisions if multiple tasks are added within the same millisecond.For production code, consider:
- UUID library (e.g.,
crypto.randomUUID())- Incremental counter
- Combination of timestamp and random value
addTask(task) { const newTask = { - id: Date.now(), + id: crypto.randomUUID(), text: task, completed: false, createdAt: new Date().toISOString(), }; this.tasks.push(newTask); return newTask; }
46-53: Make task-not-found case more explicit.When a task isn't found, the function returns
undefinedimplicitly. Consider returningnullexplicitly or throwing an error to distinguish between "not found" and "no return value."completeTask(id) { const task = this.tasks.find((t) => t.id === id); if (task) { task.completed = true; task.completedAt = new Date().toISOString(); + return task; } - return task; + return null; }src/lib/undo-manager.ts (2)
242-304: Undo branches and progress reporting are robust; consider minor consistency tweaksPer-type undo behaviour matches how each change is recorded, and failures are surfaced via
progressManager.failwhile leavingreversibleunchanged, which is the right call.Once you move delete undo to use
backupPath(see earlier comment), the branches will be fully symmetric:
file_editandfile_deleteboth restore from backups.file_createandfile_moveperform destructive operations (unlink/rename) and then mark the record irreversible.No blocking changes needed here beyond the delete/backup adjustment.
309-320: Backup naming could be made collision-resistant (optional)
backupName = \${timestamp}_${fileName}`` is usually fine, but in extreme cases multiple operations on the same file within the same millisecond could overwrite the backup.Not a practical blocker here, but you could cheaply harden it by including the generated change ID or a random suffix:
- const timestamp = Date.now(); - const fileName = path.basename(filePath); - const backupName = `${timestamp}_${fileName}`; + const timestamp = Date.now(); + const fileName = path.basename(filePath); + const unique = Math.random().toString(36).substring(2, 8); + const backupName = `${timestamp}_${unique}_${fileName}`;Purely a nice‑to‑have.
src/lib/progress-manager.ts (1)
148-162: Consider surfacingfilenamein the file progress bar format
updateFileProgresspasses a truncatedfilenamein the payload, but theSingleBarformat string doesn’t reference{filename}, so it never appears in the output. If the intent is to show the current file, consider including it:- format: `${operation} |${chalk.cyan('{bar}')}| {percentage}% | {value}/{total} Files | ETA: {eta}s`, + format: `${operation} |${chalk.cyan('{bar}')}| {percentage}% | {value}/{total} Files | ETA: {eta}s | {filename}`,This keeps the rest of the behavior unchanged while making use of the existing payload.
Also applies to: 170-180, 191-207
src/tools/project-context.ts (1)
25-36: Spinner usage for analyze/get‑commands looks good; consider error cleanupThe clearer spinner messages for the
analyzeandget-commandsactions look good and keep UX aligned with other progress usages. To harden this further, you could wrap the body of each case in atry/finally(callingprogressManager.failorstopon error) so the spinner can’t get stuck ifanalyze()or config access throws.Also applies to: 45-54
cli/tui/file-index.ts (1)
1-47: File index traversal looks solid; minor redundancy in ignore listThe depth-first traversal, limit checks, and hidden-entry filtering all look correct and robust to read errors. The entries in
IGNORED_DIRECTORIESthat start with.(like.git,.agentuity,.next) are effectively redundant because of theentry.name.startsWith('.')check, so you could drop them from the set to make intent a bit clearer, but it’s purely cosmetic.cli/config-utils.ts (1)
28-65: YAML-driven defaults look good; consider validating the port typeCentralizing defaults via
readYamlDefaults()and reusing the YAML-derived port across all paths ingetAgentConfigs()is a nice cleanup and makes behavior predictable.One small hardening you might consider is type-checking the YAML
development.portfield before assigning it:if (typeof config?.development?.port === 'number') { port = config.development.port; }This avoids accidentally propagating a non-numeric value from a misconfigured
agentuity.yamlinto URLs.Also applies to: 72-130
src/agents/CloudCoder/index.ts (1)
334-385: Continuation flow works, buttoolResultContentis unused and comments are misleadingThe continuation branch correctly turns
toolResultsinto a human-readablecontinuationMessageand appends it as a user message, so Claude can respond based on actual tool outcomes.However:
toolResultContentis computed but never used.- The surrounding comments talk about “converting tool results into tool result messages for Claude” and “finding the last assistant message with tool calls”, but the implementation only adds a textual summary as a new user message.
To avoid confusion (and unused-local warnings), either:
- Remove
toolResultContentand update the comments to describe the current “text summary as user message” approach, or- Actually wire
toolResultContentinto the model call (e.g., by feeding it as structured context alongsidemessages, if/when the SDK pattern supports it).For a minimal fix, you could do:
- const toolResultContent = toolResults.map((result) => { - if (result.success) { - return { - type: 'tool-result' as const, - toolCallId: result.id, - toolName: result.id.split('_')[0] || 'unknown', // Extract tool name from ID - result: result.result || 'Success', - }; - } else { - return { - type: 'tool-result' as const, - toolCallId: result.id, - toolName: result.id.split('_')[0] || 'unknown', - result: `Error: ${result.error}`, - isError: true, - }; - } - }); - - ctx.logger.info( - `Converted ${toolResultContent.length} tool results for Claude` - ); + ctx.logger.info( + `Processing continuation with ${toolResults.length} tool results` + );This keeps behavior unchanged while making intent clearer.
cli/local-command.ts (1)
25-81: runLocalCommand error handling and executor design are solidThe function cleanly:
- Short‑circuits empty commands with a structured failure result.
- Builds sane
ExecOptions(cwd/env/maxBuffer, optionalSHELL).- Normalizes both success and typical failure cases into
LocalCommandResultwhile preserving partial stdout/stderr.The injectable
executormakes this easy to unit‑test and swap in other backends if needed. If you ever see unexpected error shapes bubbling up, a small follow‑up would be to broaden the caught error typing and always return aLocalCommandResult(with a generic message/code) instead of rethrowing, but the current behavior is reasonable for a CLI.cli/continuation-handler.ts (3)
10-55: Event interface and defaults give a good abstraction boundaryThe
ToolStartEvent/ToolCompleteEventshapes andContinuationEventsinterface provide a clear contract for both classic and TUI frontends. ThedefaultEventsimplementation (with chalk styling) is a sensible fallback, and keeping it internal keeps the public API focused on the handler itself.If you later find multiple callers wanting to reuse or tweak these defaults, exposing
defaultEvents(or a factory) could make composition even easier, but it’s not necessary right now.
59-95: Centralizing logging througheventsand ToolProxy is well-structuredPassing a logger into
ToolProxythat delegates toemitInfo/emitWarn/emitErrorensures all tool‑level output flows through the same event pipeline. Theemit*helpers andRequired<ContinuationEvents>typing remove the need for null checks and make it easy for UIs to override just the pieces they care about.If you ever want stronger type‑safety around
events, you could avoid theas Required<ContinuationEvents>cast by constructing the merged object in a helper that enforces all keys, but the current approach is pragmatic.
292-352: HTTP continuation diagnostics are thorough; consider masking session IDsThe added
emitInfo/emitError/emitWarncalls aroundsendContinuationprovide:
- Clear status updates (sending, HTTP errors, rate limiting, timeouts).
- Detailed debugging info on failures (response body, headers, body length, URL).
Truncating the API key in logged headers is good practice. Since session IDs appear in logs as well, if these outputs are ever sent to shared log sinks, you might consider partially masking
'x-session-id'too; for a purely local CLI this is more of a future‑proofing tweak than a requirement.src/lib/context-limit.ts (1)
1-1: Centralized context message limit is a good moveExporting
CONTEXT_MESSAGE_LIMITmakes the cap easy to adjust and reuse across components. Adding a short comment about why 20 was chosen (e.g., UX vs. file size trade‑off) could help future tuning, but the current change is fine as-is.cli/tui/index.tsx (1)
16-42: runTui dynamic imports and fallbacks look solid; consider making failures more visible in dev.The dynamic import pattern with
SetupManager/SessionManagerand null fallbacks is a good way to keep the TUI resilient when optional features are unavailable. If you care about catching misconfigurations early during development (e.g., bad import paths), consider either:
- tightening the
catchto only swallow known/expected errors, or- rethrowing in non‑production environments while keeping the current behavior in production.
This would prevent silent degradation if the imports ever break unintentionally.
agentuity.yaml (1)
9-15: Config changes look fine; consider aligning hard‑coded fallback agent ID.The updated
project_id,name, and agentidlook consistent for this project. Note thatcli.jsstill usesfallbackId = 'agent_3918f7879297cf4159ea3d23b54f835b'for last‑ditch URL construction; if you intend that fallback to correspond to the agent defined here, you may want to update (or remove) that hard‑coded ID to avoid confusion when the fallback path is hit.Also applies to: 71-75
cli/agent-client.ts (1)
28-33:timeoutMsoption is unused; either wire it through or drop it.
SendMessageParamsexposestimeoutMs?: number, butsendMessagealways uses the fixedPRIMARY_TIMEOUTandCONTINUATION_TIMEOUTconstants and never readsparams.timeoutMs. This is misleading for callers.Either:
- plumb
timeoutMsthrough tofetchWithTimeout/streamResponse(e.g., overridePRIMARY_TIMEOUTwhen provided), or- remove
timeoutMsfromSendMessageParamsuntil there’s a concrete need.Also applies to: 40-42
cli.js (1)
372-500: Streaming UX viaAgentClientis well-structured; just note header/indicator edge cases.The buffered streaming with
lineBuffers,processStreamingLine, and status callbacks fromAgentClient.sendMessageis a nice improvement over raw logging. A few minor UX edges to be aware of:
- If
showSpinneris false,onStatusis effectively ignored; that’s intentional but means “start/stream/complete” messages won’t adjust any indicator.- On error, you call
stopIndicators()in the catch, but any partially buffered line inlineBuffersis not flushed; that’s acceptable, but it can drop the last incomplete line.None of this is functionally wrong; just keep in mind if you later want exact “last token” fidelity even on failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.claude/settings.local.json(1 hunks)README.md(2 hunks)agentuity.yaml(2 hunks)cli.js(22 hunks)cli/agent-client.ts(1 hunks)cli/config-utils.ts(3 hunks)cli/continuation-handler.ts(7 hunks)cli/local-command.test.ts(1 hunks)cli/local-command.ts(1 hunks)cli/slash-commands.ts(1 hunks)cli/tui/app.tsx(1 hunks)cli/tui/file-index.ts(1 hunks)cli/tui/index.tsx(1 hunks)example.js(1 hunks)package.json(1 hunks)src/agents/CloudCoder/index.ts(1 hunks)src/lib/context-limit.ts(1 hunks)src/lib/progress-manager.ts(8 hunks)src/lib/project-analyzer.d.ts(1 hunks)src/lib/session-manager.d.ts(1 hunks)src/lib/session-manager.ts(4 hunks)src/lib/setup-manager.d.ts(1 hunks)src/lib/setup-manager.ts(3 hunks)src/lib/undo-manager.ts(11 hunks)src/tools/project-context.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
cli/config-utils.ts (1)
src/lib/project-analyzer.ts (1)
readFile(135-141)
src/agents/CloudCoder/index.ts (4)
cli.js (2)
result(170-170)result(429-474)example.js (2)
result(103-103)result(152-157)test-riza-error.js (1)
result(27-27)test-cloud.js (1)
result(52-57)
src/lib/session-manager.ts (1)
src/lib/context-limit.ts (1)
CONTEXT_MESSAGE_LIMIT(1-1)
cli/local-command.ts (1)
cli.js (2)
result(170-170)result(429-474)
cli/agent-client.ts (1)
cli/continuation-handler.ts (1)
ContinuationHandler(57-403)
cli/local-command.test.ts (1)
cli/local-command.ts (4)
CommandExecutor(10-13)runLocalCommand(25-81)LocalCommandResult(18-23)formatCommandResult(83-97)
cli/continuation-handler.ts (2)
cli/tool-proxy.ts (1)
ToolProxy(746-812)tools/interface.ts (2)
ToolCall(4-9)ToolResult(11-16)
cli/tui/index.tsx (4)
cli/agent-client.ts (1)
AgentMode(3-3)cli.js (3)
setupManager(525-525)sessionManager(526-526)agentMode(803-803)src/lib/setup-manager.ts (1)
SetupManager(28-221)src/lib/session-manager.ts (1)
SessionManager(22-295)
src/lib/project-analyzer.d.ts (1)
src/lib/project-analyzer.ts (2)
ProjectAnalyzer(33-297)ProjectInfo(4-31)
src/lib/progress-manager.ts (1)
cli.js (1)
progressManager(527-527)
src/lib/undo-manager.ts (2)
cli.js (3)
sessionId(284-284)progressManager(527-527)undoManager(528-528)src/lib/progress-manager.ts (1)
progressManager(234-234)
src/tools/project-context.ts (1)
src/lib/project-analyzer.ts (1)
ProjectAnalyzer(33-297)
🔇 Additional comments (27)
example.js (4)
100-115: LGTM!The retry logic with exponential backoff is well-implemented. Error handling and backoff strategy are appropriate.
118-130: LGTM!The counter factory pattern using closures is correctly implemented with proper encapsulation of the count variable.
133-162: LGTM!The examples demonstrate appropriate usage patterns for the exported utilities.
1-170: Verify file relevance to PR objectives.The PR description mentions "TUI/Ink love and an ascii banner," but this file contains generic JavaScript examples unrelated to the stated objectives. Confirm whether this file was included intentionally or if it's part of test/example code.
src/lib/undo-manager.ts (8)
32-43: Constructor and initialization wiring look soundSession scoping via
sessionIdand per-sessionchanges/backupsdirectories is straightforward and consistent;initialize()does the minimum needed (mkdir + load log) without side effects on import. Nothing to change here.
48-84: File create/edit recording and backup strategy are coherent
recordFileCreate/recordFileEditcorrectly capture type, human-readable description, and sufficient detail; edit backups go to disk and only a 200‑char preview is stored in JSON, which keeps logs compact while usingbackupPathas the real source of truth for undo. This is a solid pattern.
111-125: File move recording and undo logic are alignedThe move record captures both
oldPathandnewPath, andundoChange('file_move')correctly renames back, with a clear progress message. This is a good, minimal reversible representation for renames.
149-174: Recent-changes listing UX is clear and boundedUsing
slice(-limit).reverse()to show the newest entries first, with a reversible icon and optional path/command lines, is straightforward and keeps output concise. The defaultlimit = 10is a sensible cap.
179-237: Interactive undo flow is solidFiltering to reversible changes, capping to the last 10, adding a “Cancel” option, and requiring an explicit confirm prompt before undoing is a nice, safe UX. Given this runs in a single CLI process, the race of
changedisappearing between prompt and selection is not a concern.
325-348: Change log trimming and backup cleanup look safeAppending to
changesLog, trimming from the front when exceedingmaxChanges, and then best‑effort unlinking ofbackupPathfor removed entries (with errors ignored) is a good balance between retention and disk usage. Saving the log once at the end keeps disk writes bounded.
353-392: Log persistence and time-ago helper are straightforward
loadChangesLog/saveChangesLogare minimal and resilient to missing/corrupt logs (start with an empty array), andgetTimeAgogives clear human-readable durations. No issues here.
395-396: Original review comment verified as unnecessary; code is correct as-isThe concern about conflicting session ID usages has been verified. cli.js imports only the
UndoManagerclass constructor (line 536) and creates its own instance with explicitsessionIdmanagement (line 558), rather than using the exported singleton. No other code imports or uses the singleton export. There are no conflicting usages or incompatible session ID expectations—the design is sound and intentional.src/lib/session-manager.d.ts (1)
2-13: Session state and manager declarations remain stableThe updated declarations are consistent with the described implementation and appear to be formatting-only; no API or typing changes to flag.
Also applies to: 16-34
src/lib/setup-manager.ts (1)
41-46: Config path construction and hash tweak are behavior‑preservingThe multi-line
path.joinand the reordered hash expression are semantically identical to the prior logic; no behavioral or reliability concerns from these edits.Also applies to: 50-57, 61-67
src/lib/setup-manager.d.ts (1)
3-19: SetupManager declarations stay in sync with implementationThe d.ts reflects the current implementation accurately; changes are indentation/formatting only, with no public surface changes.
Also applies to: 22-41
src/lib/project-analyzer.d.ts (1)
2-45: Type declaration reformat onlyThis is a pure formatting change to the
ProjectInfoandProjectAnalyzerdeclarations; the public shape and unions remain the same, so there’s no behavioral impact.README.md (1)
114-125: TUI / classic mode and command docs align with CLI behaviorThe updated examples for
coder --interactivevscoder --classic --interactive, and the added bullets for Ink TUI, classic mode,!<cmd>, and//@usage give a clear mental model of the new UX surface and match the implemented flags and features.Also applies to: 148-167
package.json (1)
2-67: Package manifest and scripts are consistent with new featuresThe Bun-based
testscript and the dependency set (Ink, React, YAML, etc.) line up with the new TUI and config utilities. Nothing here looks out of place given the rest of the PR.cli/local-command.test.ts (1)
1-136: Thorough coverage of local command execution and formattingThese tests exercise the key branches of
runLocalCommand(empty commands, env propagation, structured exec errors, unknown errors) andformatCommandResult, and the SHELL environment is cleanly saved/restored around each test. This should give high confidence in the local-command behavior.cli/slash-commands.ts (1)
1-19: Slash command registry looks clean and extensibleInterface and command list are straightforward and consistent; this is a good central place to evolve the slash surface over time.
cli/local-command.ts (1)
83-97: Command result formatting is user-friendlyIncluding the echoed command, trimmed stdout/stderr, and a final success/exit‑code line gives a nice, compact summary for the TUI/CLI. Trailing newline trimming avoids double spacing while preserving internal formatting.
cli/continuation-handler.ts (3)
141-143: Graceful handling of malformed tool-call JSONOn parse failure you emit a clear error message and fall back to treating the response as a normal assistant reply, which is a reasonable trade‑off for robustness when the agent emits bad markup.
148-198: Tool execution events and previews look correctThe
executeToolCallsloop:
- Emits a
toolStartevent for each call with index/total and concise key parameters.- Executes tools sequentially through
ToolProxy.- On success, computes a tool‑specific preview only when
shouldShowPreviewreturns true and emitstoolCompletewith that preview.- On failure, emits
toolCompletewith an error message.This keeps user feedback focused (no full JSON dumps) while still surfacing enough context to understand what’s happening.
399-401: Tool call flow surfaces errors without swallowing themEmitting an error summary via
emitErrorand then rethrowing keeps both the UI and the caller informed, which is appropriate for this orchestrator layer.src/lib/session-manager.ts (2)
5-5: Importing CONTEXT_MESSAGE_LIMIT improves configurabilitySwitching from a hard-coded limit to an imported constant makes the session manager respect the shared setting and avoids magic numbers scattered through the code.
51-51: Hashing logic remains unchanged functionallyThe bit‑shift/add pattern and subsequent 32‑bit coercion are standard for a simple path hash; the formatting tweak here doesn’t alter behavior.
cli.js (1)
502-517: Import paths are supported in Bun.The verification confirms that Bun's runtime and bundler transparently support importing
.tsand.tsxmodules directly from JavaScript entrypoints with#!/usr/bin/env bun, including the dynamic imports used in the code. The import paths (./cli/tui/index.tsx,./cli/agent-client.ts) will work as expected across Bun environments.
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "permissions": { | |||
| "allow": ["Bash(bun run format:*)", "Bash(node -e:*)", "Bash(node -p:*)"], | |||
There was a problem hiding this comment.
🧩 Analysis chain
Restrict node -e and node -p patterns or move to ask array.
The node -e:* and node -p:* patterns are overly permissive. The -e and -p flags allow arbitrary JavaScript code execution, and the * wildcard places no constraints on what code can run. This creates a significant attack surface if Claude or an AI assistant executes these commands during development.
Recommended actions:
- If these commands are essential for your workflow, move them to the
askarray to require user confirmation before execution. - If possible, replace the wildcards with more specific patterns, e.g.,
node -e:console.log(*)or other restricted code patterns. - Document why these broad permissions are necessary for the project.
Clarify: Is this configuration file intended only for local development, and will it be committed to the repository? If yes, consider the security implications for other developers who clone the repo.
🏁 Script executed:
#!/bin/bash
# Examine the .claude/settings.local.json file
echo "=== File content ==="
cat .claude/settings.local.json 2>/dev/null || echo "File not found"
echo ""
echo "=== Check if file is in .gitignore ==="
if [ -f .gitignore ]; then
grep -n "settings.local.json" .gitignore || echo "Not found in .gitignore"
grep -n "\.claude/" .gitignore || echo ".claude/ not excluded"
else
echo ".gitignore not found"
fi
echo ""
echo "=== Check .claude directory structure ==="
find .claude -type f 2>/dev/null | head -20
echo ""
echo "=== Check for README or documentation ==="
find . -maxdepth 3 -name "*README*" -o -name "*readme*" 2>/dev/null | head -10Length of output: 368
🏁 Script executed:
#!/bin/bash
# Check README for documentation on .claude settings
echo "=== Searching README.md for claude/settings references ==="
grep -i -n "claude\|settings\|permissions\|node -e\|node -p" ./README.md || echo "No matches found"
echo ""
echo "=== Full README.md content (first 100 lines) ==="
head -100 ./README.mdLength of output: 4361
Remove or restrict node -e:* and node -p:* patterns in version-controlled configuration.
The .claude/settings.local.json file is committed to version control and shared with all repository contributors. The patterns Bash(node -e:*) and Bash(node -p:*) allow arbitrary JavaScript code execution without restrictions, contradicting the project's stated security model ("Command whitelisting - Only safe shell commands allowed").
Required actions:
- Move these patterns to the
askarray to require user confirmation, or remove them entirely if not essential. - If these patterns are necessary, add documented justification explaining the security implications.
- Consider more restrictive patterns or specific use cases instead of wildcards.
The file being shared across all developers amplifies the risk surface.
🤖 Prompt for AI Agents
In .claude/settings.local.json around line 3, the allow list currently includes
overly broad patterns "Bash(node -e:*)" and "Bash(node -p:*)" which permit
arbitrary JS execution in a version-controlled file; remove these patterns or
move them from "allow" into the "ask" array so they require user confirmation,
or replace them with narrow, specific command patterns; if you must keep them,
add a short documented justification adjacent in the file explaining the
security tradeoffs and restrict the wildcard to minimal necessary use cases.
| let cachedGlobalConfig = null; | ||
| let attemptedGlobalConfig = false; | ||
|
|
||
| async function loadGlobalConfig() { | ||
| if (attemptedGlobalConfig) { | ||
| return cachedGlobalConfig; | ||
| } | ||
| attemptedGlobalConfig = true; | ||
| try { | ||
| const globalConfigPath = join( | ||
| homedir(), | ||
| '.config', | ||
| 'agentuity-coder', | ||
| 'agentuity-coder.config.json' | ||
| ); | ||
| const configContent = await readFile(globalConfigPath, 'utf-8'); | ||
| cachedGlobalConfig = JSON.parse(configContent); | ||
| } catch { | ||
| cachedGlobalConfig = null; | ||
| } | ||
| return cachedGlobalConfig; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Global config API key is never used when env vars are unset.
loadGlobalConfig can set process.env.GLOBAL_API_KEY, but:
const API_KEY = process.env.API_KEY || process.env.AGENTUITY_PROJECT_KEY || process.env.GLOBAL_API_KEY;- and the
if (!API_KEY) { … process.exit(1); }check - plus
new AgentClient({ apiKey: API_KEY, … })
all run before any call to getAgentUrl, so loadGlobalConfig is never invoked prior to constructing API_KEY. As a result, a valid apiKey in agentuity-coder.config.json won’t prevent the “API_KEY environment variable is not set” exit.
You’ll need to either:
- compute
API_KEYafter an awaited call toloadGlobalConfig()(e.g., with top‑levelawait), or - move API key resolution into an async initializer that runs before you construct
AgentClient/ do the missing‑API‑key check.
As written, the new global config support for the key is effectively dead code.
Also applies to: 220-271, 283-292
🏁 Script executed:
#!/bin/bash
# Show that GLOBAL_API_KEY is not considered when API_KEY is computed.
rg -n "API_KEY =" cli.js && rg -n "loadGlobalConfig" cli.jsLength of output: 170
I need to verify the control flow context around these lines to determine if the original review comment is accurate or misleading.The web search results are general guidance and not specific to the cli.js file. Let me now examine the actual file structure with precise context around the critical lines:
This will show me the actual sequence and nesting of operations around loadGlobalConfig, GLOBAL_API_KEY, and API_KEY assignment.
🏁 Script executed:
#!/bin/bash
# Get detailed context around lines 200, 226, 229, 268
# Show full control flow structure
sed -n '195,280p' cli.js | cat -nLength of output: 49
Let me access the file with the repository cloned:
🏁 Script executed:
#!/bin/bash
# Get detailed context around the critical lines
sed -n '195,280p' cli.js | cat -nLength of output: 2798
The control flow issue is now clear. Let me verify when getAgentUrl() is actually called to confirm the timing problem:
🏁 Script executed:
#!/bin/bash
# Find all calls to getAgentUrl to understand execution timing
rg -n "getAgentUrl\(" cli.js | head -20Length of output: 47
Global config API key is never used—review comment verified as accurate.
The original review comment correctly identifies a critical bug. Analysis confirms:
getAgentUrl()is defined at line 221 but never called at module initialization timeconst API_KEY = ...at line 268 executes synchronously during module loadprocess.env.GLOBAL_API_KEYis only set insidegetAgentUrl()at line 229- The API_KEY check (line 279) exits before
getAgentUrl()can ever run
Result: the global config apiKey mechanism is dead code and cannot work as intended. The feature requires either:
- Deferring the API_KEY resolution and check into an async initializer that runs before any AgentClient construction, or
- Using top-level
awaitto callloadGlobalConfig()before evaluatingAPI_KEY
| const toolCallResult = await this.continuationHandler.handleToolCallFlow( | ||
| primaryText, | ||
| targetUrl, | ||
| this.apiKey, | ||
| sessionId, | ||
| message | ||
| ); | ||
|
|
||
| if (toolCallResult.cleanedResponse) { | ||
| cleanedResponse = toolCallResult.cleanedResponse; | ||
| combinedResponse = cleanedResponse; | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid treating an empty cleanedResponse as “no value”.
The current check:
if (toolCallResult.cleanedResponse) {
cleanedResponse = toolCallResult.cleanedResponse;
combinedResponse = cleanedResponse;
}skips updates when cleanedResponse is a valid empty string (e.g., all content was tool‑call markup). In that case you’d fall back to the original primaryText, potentially leaking hidden/tool metadata.
Safer would be to distinguish “missing” from “empty”, e.g.:
if ('cleanedResponse' in toolCallResult) {
cleanedResponse = toolCallResult.cleanedResponse ?? '';
combinedResponse = cleanedResponse;
}This preserves the intent even when the cleaned text is empty.
Also applies to: 124-129
🤖 Prompt for AI Agents
In cli/agent-client.ts around lines 87-99 (and similarly at 124-129), the code
treats an empty string cleanedResponse as absent by using a truthy check; change
the check to detect presence of the property (e.g., if ('cleanedResponse' in
toolCallResult) or hasOwnProperty) and then assign cleanedResponse =
toolCallResult.cleanedResponse ?? '' and combinedResponse = cleanedResponse so
an intentionally empty cleanedResponse is preserved instead of falling back to
primaryText.
| private async streamResponse( | ||
| response: Response, | ||
| source: TokenEvent['source'], | ||
| onToken?: SendMessageCallbacks['onToken'], | ||
| timeoutMs?: number | ||
| ): Promise<string> { | ||
| if (!response.body) { | ||
| return ''; | ||
| } | ||
|
|
||
| const reader = response.body.getReader(); | ||
| const decoder = new TextDecoder(); | ||
| let fullText = ''; | ||
|
|
||
| const timeoutHandle = | ||
| timeoutMs !== undefined | ||
| ? setTimeout(() => { | ||
| reader.cancel().catch(() => { | ||
| // Ignore cancellation errors | ||
| }); | ||
| }, timeoutMs) | ||
| : null; | ||
|
|
||
| try { | ||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
|
|
||
| const chunk = decoder.decode(value, { stream: true }); | ||
| fullText += chunk; | ||
| onToken?.({ chunk, source }); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof Error && error.name === 'AbortError') { | ||
| throw new Error('Streaming timed out'); | ||
| } | ||
| throw error; | ||
| } finally { | ||
| if (timeoutHandle) { | ||
| clearTimeout(timeoutHandle); | ||
| } | ||
| reader.releaseLock(); | ||
| } | ||
|
|
||
| return fullText; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Flush TextDecoder at end of stream and consider timeouts consistently.
streamResponse uses TextDecoder.decode(value, { stream: true }) per chunk but never calls a final decoder.decode() without arguments after the loop. This can drop trailing bytes for multi‑byte characters.
A safer pattern is:
let fullText = '';
try {
while (true) {
const { done, value } = await reader.read();
if (done) break;
fullText += decoder.decode(value, { stream: true });
onToken?.({ chunk: fullTextChunk, source });
}
// Flush any remaining bytes
fullText += decoder.decode();
} …You may also want to optionally pass a timeout for primary streaming (not just continuation) via an argument, so a stalled stream doesn’t hang indefinitely.
🌐 Web query:
How should TextDecoder be used with streaming reads to avoid truncating multi-byte characters at the end of the stream?
💡 Result:
Use TextDecoder.decode(..., {stream: true}) for every chunk except the final one so the decoder buffers any incomplete multi‑byte sequence at a chunk boundary; then call decode() once with no input (or with stream: false) to flush and emit the buffered bytes. Example:
-
For manual chunking:
const dec = new TextDecoder('utf-8');
let out = '';
for (const chunk of chunks) {
out += dec.decode(chunk, { stream: true }); // keep incomplete bytes
}
out += dec.decode(); // flush remaining bytes at end -
Or simply pipe the response body through a TextDecoderStream to get a string stream that handles boundaries for you.
References: MDN TextDecoder (use of stream) [1], Encoding Standard (decode with stream + final flush) [2], TextDecoderStream overview [3].
Add final TextDecoder.decode() flush after streaming completes.
The code currently calls decoder.decode(value, { stream: true }) for each chunk but never flushes the decoder at the end. This drops any incomplete multi-byte sequences buffered by the decoder. After the read loop completes, call decoder.decode() with no arguments to flush remaining bytes.
Add after the while loop:
// Flush any remaining bytes
fullText += decoder.decode();🤖 Prompt for AI Agents
In cli/agent-client.ts around lines 172 to 217, the TextDecoder is used with
stream:true for each chunk but never flushed at the end, which can drop buffered
partial multi-byte sequences; after the streaming loop completes and before
returning, call decoder.decode() (with no arguments) and append its result to
fullText, ensuring any remaining bytes are emitted and also invoke onToken with
the flushed text if desired (or just append to fullText) before returning.
| useInput((input, key) => { | ||
| if (key.ctrl && input === 'c') { | ||
| exit(); | ||
| } | ||
|
|
||
| if (key.tab) { | ||
| const completed = attemptTabCompletion(); | ||
| if (completed) { | ||
| setInputValue(completed); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (key.upArrow && history.length > 0) { | ||
| const nextIndex = | ||
| historyIndex + 1 >= history.length | ||
| ? history.length - 1 | ||
| : historyIndex + 1; | ||
| const nextValue = history[nextIndex]; | ||
| if (nextValue !== undefined) { | ||
| setHistoryIndex(nextIndex); | ||
| setInputValue(nextValue); | ||
| } | ||
| } else if (key.downArrow && history.length > 0) { | ||
| const nextIndex = historyIndex - 1; | ||
| if (nextIndex >= 0) { | ||
| setHistoryIndex(nextIndex); | ||
| setInputValue(history[nextIndex] || ''); | ||
| } else { | ||
| setHistoryIndex(-1); | ||
| setInputValue(''); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Status/footer hints mention “esc” and “?” shortcuts that aren’t implemented.
The status line and footer say:
(... • esc to interrupt)while streaming, and{contextPercentLeft}% context left · ? for shortcuts
But useInput only handles:
- Ctrl+C (
exit()), - Tab (completion),
- Up/Down arrows (history).
There’s no handling for key.escape or input === '?', so those hints are currently misleading.
Either wire up the corresponding handlers (e.g., escape to cancel the current request, ? to show a help message) or adjust the text to only advertise shortcuts that exist.
Also applies to: 683-705, 800-887
🤖 Prompt for AI Agents
In cli/tui/app.tsx around lines 284-317 (and similarly at 683-705 and 800-887),
the UI hints advertise "esc to interrupt" and "? for shortcuts" but the useInput
handler only supports Ctrl+C, Tab, and Up/Down history; add handlers or remove
the hints. Fix by wiring key.escape to cancel the current streaming/request flow
(call the existing cancel/abort routine or set a state that stops streaming) and
wire input === '?' (or key.shift and input === '?') to open or toggle the
help/shortcuts overlay; ensure both actions are debounced/guarded when input
focus or modal state would conflict. If you prefer not to add functionality,
update the status/footer text to remove the misleading "esc" and "?" references
in the same file ranges.
| .reduce((sum, num) => sum + num, 0); // Sum them up | ||
| }; | ||
|
|
||
| console.log('Processed numbers:', processNumbers(numbers)); // 60 |
There was a problem hiding this comment.
Top-level console.log causes side effect on import.
This console statement executes immediately when the module is imported, which may not be intended for a library module.
Consider moving this into the runExamples() function or removing it:
-console.log('Processed numbers:', processNumbers(numbers)); // 60
+// Moved to runExamples() or removed for library usageCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In example.js around line 27, the top-level console.log('Processed numbers:',
processNumbers(numbers)); causes a side effect during module import; move this
statement into the runExamples() function (or remove it) so importing the module
doesn't execute it automatically. Modify runExamples() to call
processNumbers(numbers) and log the result, and ensure example.js exports
runExamples (or documents that consumers should call it) so no work runs at
import time.
| email, | ||
| preferences: { theme = 'light' }, | ||
| } = user; | ||
| console.log(`${name} (${age}) prefers ${theme} theme`); |
There was a problem hiding this comment.
Top-level console.log causes side effect on import.
This console statement executes immediately when the module is imported.
Move this into runExamples() or remove for library usage.
🤖 Prompt for AI Agents
In example.js around line 85, the top-level console.log causes a side effect on
module import; move the console.log(`${name} (${age}) prefers ${theme} theme`)
into the runExamples() function (or another explicitly invoked function) so it
only runs when the module's example routine is executed, or remove it entirely
for library usage; ensure any variables used (name, age, theme) are in scope
inside the moved function and export or call runExamples() explicitly where
needed.
| if (import.meta.url === `file://${process.argv[1]}`) { | ||
| runExamples(); | ||
| } |
There was a problem hiding this comment.
Fix the execution guard logic.
The condition import.meta.url === \file://${process.argv[1]}`` won't work correctly because:
import.meta.urlis already a properly formatted URL (e.g.,file:///absolute/path/file.js)process.argv[1]is a filesystem path that may not match URL encoding or path separators (especially on Windows)
Apply this diff to fix the execution guard:
+import { pathToFileURL } from 'url';
+
// ... rest of code ...
// Run examples if this file is executed directly
-if (import.meta.url === `file://${process.argv[1]}`) {
+if (import.meta.url === pathToFileURL(process.argv[1]).href) {
runExamples();
}📝 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.
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| runExamples(); | |
| } | |
| import { pathToFileURL } from 'url'; | |
| if (import.meta.url === pathToFileURL(process.argv[1]).href) { | |
| runExamples(); | |
| } |
🤖 Prompt for AI Agents
In example.js around lines 168–170, the execution guard incorrectly compares
import.meta.url to a raw filesystem path; convert the module URL to a filesystem
path before comparing (or convert the argv path to a file:// URL) to handle URL
encoding and platform differences. Update the guard to use the url/file
utilities (e.g., fileURLToPath(import.meta.url)) and compare that to a
normalized absolute path for process.argv[1] (e.g.,
path.resolve(process.argv[1])); add the necessary import(s) for url and/or path
depending on ESM/CJS. Ensure the comparison uses the same canonical form on both
sides so the script runs correctly across platforms.
| // Keep only last 50 messages to prevent file from getting too large | ||
| if (this.currentSession!.contextMessages.length > 50) { | ||
| if (this.currentSession!.contextMessages.length > CONTEXT_MESSAGE_LIMIT) { | ||
| this.currentSession!.contextMessages = | ||
| this.currentSession!.contextMessages.slice(-50); | ||
| this.currentSession!.contextMessages.slice(-CONTEXT_MESSAGE_LIMIT); | ||
| } |
There was a problem hiding this comment.
Update the comment to match the new context limit
The code now uses CONTEXT_MESSAGE_LIMIT (20) but the comment still says “Keep only last 50 messages,” which is misleading.
Recommend updating the comment to reference the constant instead of a specific number, e.g.:
// Keep only the last CONTEXT_MESSAGE_LIMIT messages to prevent the file from getting too large
if (this.currentSession!.contextMessages.length > CONTEXT_MESSAGE_LIMIT) {
this.currentSession!.contextMessages =
this.currentSession!.contextMessages.slice(-CONTEXT_MESSAGE_LIMIT);
}🤖 Prompt for AI Agents
In src/lib/session-manager.ts around lines 120 to 124, the inline comment
incorrectly references "last 50 messages" while the code uses
CONTEXT_MESSAGE_LIMIT; update the comment to reference the constant (e.g., "Keep
only the last CONTEXT_MESSAGE_LIMIT messages to prevent the file from getting
too large") or otherwise remove the hard-coded number so the comment matches the
current logic.
| async recordFileDelete(filePath: string, content: string): Promise<void> { | ||
| const backupPath = await this.createBackup(filePath, content); | ||
|
|
||
| const change: ChangeRecord = { | ||
| id: this.generateId(), | ||
| timestamp: new Date().toISOString(), | ||
| type: 'file_delete', | ||
| description: `Deleted file: ${path.basename(filePath)}`, | ||
| details: { | ||
| details: { | ||
| path: filePath, | ||
| backupPath, | ||
| originalContent: content | ||
| originalContent: content, | ||
| }, | ||
| reversible: true, | ||
| }; | ||
|
|
||
| await this.addChange(change); | ||
| } |
There was a problem hiding this comment.
Avoid storing full deleted file contents in the JSON log
For deletes you both:
- write a full backup file (
createBackup), and - store the entire
contentindetails.originalContent, then use that inundoChange('file_delete').
This has a few downsides:
- Large deleted files bloat
changes.json. - Deleted content (which might be sensitive) is kept inline in the log instead of only in a scoped backup file.
- Behaviour is inconsistent with edits, which restore from
backupPathinstead oforiginalContent.
Consider switching delete undo to use the backup file, and only keep a short preview (or nothing) in JSON, mirroring the edit flow. For example:
- async recordFileDelete(filePath: string, content: string): Promise<void> {
+ async recordFileDelete(filePath: string, content: string): Promise<void> {
const backupPath = await this.createBackup(filePath, content);
const change: ChangeRecord = {
id: this.generateId(),
timestamp: new Date().toISOString(),
type: 'file_delete',
description: `Deleted file: ${path.basename(filePath)}`,
- details: {
- path: filePath,
- backupPath,
- originalContent: content,
- },
+ details: {
+ path: filePath,
+ backupPath,
+ // optional: keep at most a small preview for display, or drop entirely
+ // originalContent: content.substring(0, 200),
+ },
reversible: true,
};
}
@@
- case 'file_delete':
- if (change.details.path && change.details.originalContent) {
- await fs.writeFile(
- change.details.path,
- change.details.originalContent
- );
- progressManager.succeed(
- `Restored deleted file: ${change.details.path}`
- );
- }
- break;
+ case 'file_delete':
+ if (change.details.path && change.details.backupPath) {
+ const backupContent = await fs.readFile(
+ change.details.backupPath,
+ 'utf8'
+ );
+ await fs.writeFile(change.details.path, backupContent);
+ progressManager.succeed(
+ `Restored deleted file: ${change.details.path}`
+ );
+ }
+ break;This keeps logs smaller and treats backups consistently across edit/delete.
Also applies to: 270-280
🤖 Prompt for AI Agents
In src/lib/undo-manager.ts around lines 89-106 (and similarly 270-280), the code
currently stores the full deleted file contents in details.originalContent and
also writes a backup file; change the delete-record behavior to avoid embedding
full file content in the JSON log: record the backupPath, a short preview (e.g.,
first N bytes or null) or omit originalContent entirely, and set reversible:
true; update the corresponding undo handler for 'file_delete' to restore from
the backupPath (not from originalContent) and to remove or ignore
originalContent when reading changes.json so large or sensitive content is kept
only in the backup file and logs remain small and consistent with edit behavior.
1 participant
Added a bit of TUI / Ink love and an ascii banner
Summary by CodeRabbit
New Features
--interactivemode for traditional prompt-based CLI experience!prefix for running shell commands/help,/session,/continue,/context,/goal,/diff,/quit)Documentation
✏️ Tip: You can customize this high-level summary in your review settings.