Raw API OpenRouter, persistent stdin with Escape cancel, slash commands, agent tools#89
Open
yashdev9274 wants to merge 1 commit into
Open
Raw API OpenRouter, persistent stdin with Escape cancel, slash commands, agent tools#89yashdev9274 wants to merge 1 commit into
yashdev9274 wants to merge 1 commit into
Conversation
…gent mode - Replace AI SDK with raw fetch-based OpenRouter API to support server tools (openrouter:web_search, openrouter:web_fetch) alongside function tools - Add write_file, run_command tool definitions with permission gating for agent mode - Implement persistent single stdin setup with module-level streamAbort controller — Escape cancels streaming across all modes/providers - Add AbortSignal plumbing to all providers (Google, OpenRouter, NVIDIA, server-proxy) with silent AbortError re-throw - Add /model slash command with codebase-native model lists - Change default provider to OpenRouter; auto-switch on tool/agent mode - Rewrite system prompt for autonomous agent workflows - Pause MiniMax with 402 balance error handling - Bump maxSteps from 5 to 25 across all providers
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
WalkthroughThis PR adds AbortSignal-based request cancellation to all AI provider services, rewrites OpenRouter streaming to use direct fetch, introduces slash-command model switching in the CLI chat loop, implements two new tools (write-file and run-command) with a permission-manager framework, and updates system prompts and tool configurations accordingly. ChangesStreaming Cancellation, Chat Features, and Tool Execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/supercode-cli/server/src/cli/ai/minimax-service.ts (1)
21-36:⚠️ Potential issue | 🟠 MajorWire
AbortSignalintoMinimaxService.sendMessageto preserve cancellation
AIProvider.sendMessageis wired to receivesignal?: AbortSignal, and other services (e.g.,google-service.ts) forward it to streaming asabortSignal: signal, butMinimaxService.sendMessageomits thesignalparameter and itsstreamOptionsdoesn’t setabortSignal, so aborts won’t reliably stop in-flight MiniMax streaming.Suggested fix
async sendMessage( messages: ModelMessage[], onChunk?: (chunk: string) => void, tools?: any, onToolCall?: any, + signal?: AbortSignal, ) { try { const systemMessages = messages.filter(m => m.role === "system") const nonSystemMessages = messages.filter(m => m.role !== "system") const system = systemMessages.map(m => m.content).join("\n") const streamOptions: any = { model: this.model, messages: nonSystemMessages, maxTokens: Number(process.env.MINIMAX_MAX_TOKENS) || 4096, + abortSignal: signal, } @@ - } catch (error) { + } catch (error: any) { + if (error?.name === "AbortError") throw error const message = error instanceof Error ? error.message : String(error) if (message.includes("insufficient balance") || message.includes("402") || message.includes("1008")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/supercode-cli/server/src/cli/ai/minimax-service.ts` around lines 21 - 36, The sendMessage method is missing an AbortSignal parameter and doesn't forward it to MiniMax streaming; update MinimaxService.sendMessage to accept signal?: AbortSignal (matching AIProvider.sendMessage) and add abortSignal: signal to the streamOptions object (in the sendMessage function) so in-flight MiniMax streams can be cancelled; ensure the parameter is threaded through any callers that invoke MinimaxService.sendMessage if needed.
🧹 Nitpick comments (3)
apps/supercode-cli/server/src/cli/commands/ai/init.ts (1)
70-70: ⚡ Quick winAdd an explanatory comment for the commented-out minimax option.
The minimax provider option is commented out but remains fully supported in the codebase (still in
ModelProvidertype,providerMeta, slash commands, and nested model lists). Based on the PR context mentioning "MiniMax paused with 402 balance handling," this appears to be a temporary removal.Add a comment explaining why this option is disabled to improve code clarity and help future maintainers understand the intent.
📝 Suggested comment
{ value: "google", label: "Gemini 2.5 Flash", hint: "free · fast" }, + // TODO: Re-enable minimax after resolving balance/billing issues (see 402 error handling) // { value: "minimax", label: "MiniMax M2", hint: "reasoning · powerful" }, { value: "openrouter", label: "OpenRouter", hint: "multi-provider · bring your own key" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/supercode-cli/server/src/cli/commands/ai/init.ts` at line 70, The commented-out minimax option lacks context; add a brief explanatory comment next to the commented line (referencing the "minimax" option) stating it's temporarily disabled due to the "MiniMax paused with 402 balance handling" issue and pointing to where it's still supported (ModelProvider type, providerMeta, related slash commands and nested model lists) so future maintainers know it's intentional and temporary.apps/supercode-cli/server/src/index.ts (1)
1-11: 🏗️ Heavy liftConsider migrating to Bun.serve() per coding guidelines.
This server implementation uses Express.js, but the coding guidelines specify using
Bun.serve()with WebSocket and routes support for files matchingapps/supercode-cli/server/**/{index,server}.{ts,tsx}. This would also enable hot module reloading withbun --hot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/supercode-cli/server/src/index.ts` around lines 1 - 11, The file currently initializes an Express app (imports: express, cors; symbols: app, port, serverUrl, clientUrl) but the repo guideline requires using Bun.serve(); replace the Express setup with a Bun.serve() HTTP handler that registers routes and WebSocket support and delegates auth via toNodeHandler(auth) (retain the existing toNodeHandler and auth usage), wire CORS handling inline in the fetch handler instead of cors middleware, and export/start the server via Bun.serve({ port, fetch }) so the file matches apps/supercode-cli/server/**/{index,server}.{ts,tsx} pattern and supports bun --hot; ensure prisma usage remains available inside the handler and that any previously exported Express middleware is adapted into fetch-compatible request/response handling.Source: Coding guidelines
apps/supercode-cli/server/src/cli/commands/slashCommands/model.ts (1)
4-4: ⚡ Quick winRemove unused import.
createProvideris imported but never used in this file.-import { createProvider, type ModelProvider } from "src/cli/ai/provider.ts" +import type { ModelProvider } from "src/cli/ai/provider.ts"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/supercode-cli/server/src/cli/commands/slashCommands/model.ts` at line 4, The import statement currently brings in createProvider and ModelProvider but createProvider is never used; remove createProvider from the import so only ModelProvider is imported from "src/cli/ai/provider.ts" (update the import that references createProvider to import only the needed ModelProvider symbol).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/supercode-cli/server/src/cli/ai/openrouter-service.ts`:
- Around line 144-160: The accumulation logic for streaming tool call deltas
uses tc.id to find existing entries but OpenRouter uses tc.index for subsequent
chunks; update the loop in openrouter-service.ts to prefer tc.index when
locating/updating entries: if tc.index is defined, use toolCalls[tc.index] (or
assign into that slot) to append function.arguments; otherwise fall back to
finding by tc.id. When creating a new entry, place it at toolCalls[tc.index] if
index is provided (and set id from tc.id if present), or push normally if no
index; ensure concatenation uses tc.function.arguments when appending.
In `@apps/supercode-cli/server/src/index.ts`:
- Line 244: The server defines a hardcoded maxIter = 10 which conflicts with
openrouter-service.ts's maxToolIterations = 25 and can cause divergent behavior;
unify them by extracting a single source of truth (e.g., a shared constant or
environment/config value) and use it in both places (replace index.ts's maxIter
and openrouter-service.ts's maxToolIterations to read the same exported CONFIG
or process.env value), then update any references that rely on
maxIter/maxToolIterations to the shared symbol so both CLI and proxied requests
use the identical iteration limit.
- Around line 292-305: The loop that accumulates streaming tool calls currently
matches existing entries by tc.id (using toolCalls.find(t => t.id === tc.id))
but streaming deltas reference tool calls by tc.index; change the logic in the
delta.tool_calls processing so you locate or place entries by tc.index: if
tc.index is defined, use toolCalls[tc.index] (or assign directly to
toolCalls[tc.index] when creating) instead of find-by-id; if tc.index is missing
fallback to the existing id-based find; also defensively concatenate arguments
(e.g., existing.function.arguments = (existing.function.arguments || "") +
(tc.function?.arguments || "")) when merging to avoid undefined.
In `@apps/supercode-cli/server/src/tools/definitions/run-command.ts`:
- Line 2: Replace usage of child_process.exec (imported as exec) with the Bun.$
template literal API: remove the import from "node:child_process", update any
call sites that use exec(...) (and any helper functions in run-command.ts) to
execute shell commands using Bun.$`...` instead, returning a Promise that
resolves with the command output; if timeout behavior is required by the
existing API, wrap the Bun.$ call in a Promise.race with a setTimeout-based
reject to emulate the timeout. Ensure references to the exported functions or
symbols in run-command.ts that previously relied on exec (e.g., the runCommand /
exec wrapper functions) preserve their signature and error handling while
switching implementation to Bun.$.
- Around line 50-64: The exitCode initialization is inconsistent: currently
result.exitCode is set to error?.code ?? 0 and then later corrected to -1 when
an error exists without a code; change the initialization logic for
result.exitCode to be conditional on whether error exists (e.g., if error then
use error.code ?? -1 else 0) so you don't need the separate correction block,
and remove the redundant branch that sets exitCode = -1; keep the existing
handling for error.killed (signal and timeout message) and the stderr/message
concatenation in the same places (referencing the result object, its exitCode
property, and the error.killed check).
- Around line 34-36: The check that throws when resolvedCwd is outside the
workspaceRoot is vulnerable to path traversal and OS differences; fix by
normalizing and resolving both workspaceRoot and the target path (use
path.resolve/path.normalize) and compare using a safe prefix check: append a
path separator to the normalized workspaceRoot (e.g. ensure
workspaceRootNormalized endsWith(path.sep) or use
path.join(workspaceRootNormalized, path.sep)), compute the relative path via
path.relative(workspaceRootNormalized, resolvedCwdNormalized) or compare the
normalized lowercase values on Windows (to handle case-insensitivity), and only
throw if the relative path starts with '..' or the resolvedCwdNormalized does
not start with the workspaceRootNormalized+sep; update the logic around the
resolvedCwd and workspaceRoot variables (and any code in the run-command
handling) accordingly.
In `@apps/supercode-cli/server/src/tools/definitions/write-file.ts`:
- Line 3: Replace Node's fs/promises usage with Bun.file-based operations:
remove the import of mkdir and writeFile from "node:fs/promises" and update any
code that calls mkdir or writeFile in this module (referenced symbols: mkdir,
writeFile) to use Bun.file(...).write and Bun.mkdir/Bun.file APIs instead (also
update the sections around lines 38-41 where writeFile is used). Ensure
directory creation uses Bun.mkdir or Bun.file path creation semantics and that
file writes use Bun.file(path).write(data, options) so the module follows the
project's Bun file I/O conventions.
- Around line 43-44: The variables are misleading: change the misleading
existingSize to something like newSize or contentSize and derive it from
content.length; determine whether the target file already exists (use
fs.existsSync or fs.promises.stat) and set action dynamically to "created" or
"overwritten" instead of hardcoding "created"; update any logs/usages that
reference existingSize/action to use the new variable names and the computed
action so the message accurately reflects whether the file was created or
overwritten (refer to the symbols existingSize, action, and content.length in
write-file.ts).
- Around line 26-28: The current check using fullPath.startsWith(workspaceRoot)
is unsafe; replace it by first normalizing and resolving both workspaceRoot and
fullPath (use path.resolve/path.normalize), on Windows compare
case-insensitively (lowercase both), and then verify containment using a
relative-path check instead of startsWith: compute rel =
path.relative(workspaceRoot, fullPath) and ensure rel is not empty, not starting
with '..' and not equal to '..' (this handles trailing-separator edge cases).
Update the check around the symbols fullPath, workspaceRoot and filePath to
throw the error when the containment test fails, ensuring the
normalized/possibly lowercased values are compared.
In `@apps/supercode-cli/server/src/tools/registry.ts`:
- Around line 10-26: The permission check is using tool.description instead of
the tool's registry key so rules never match; change withPermission to accept
the tool's registry key (e.g., add a second param like toolKey: string) and call
permissionManager.check(toolKey, args) instead of (tool.name ||
tool.description); update all call sites that wrap tools (where registry
iterates keys and calls withPermission) to pass the key (e.g., "write_file",
"run_command") when constructing the wrapped tool; keep the wrapped execute
behavior and return values unchanged.
---
Outside diff comments:
In `@apps/supercode-cli/server/src/cli/ai/minimax-service.ts`:
- Around line 21-36: The sendMessage method is missing an AbortSignal parameter
and doesn't forward it to MiniMax streaming; update MinimaxService.sendMessage
to accept signal?: AbortSignal (matching AIProvider.sendMessage) and add
abortSignal: signal to the streamOptions object (in the sendMessage function) so
in-flight MiniMax streams can be cancelled; ensure the parameter is threaded
through any callers that invoke MinimaxService.sendMessage if needed.
---
Nitpick comments:
In `@apps/supercode-cli/server/src/cli/commands/ai/init.ts`:
- Line 70: The commented-out minimax option lacks context; add a brief
explanatory comment next to the commented line (referencing the "minimax"
option) stating it's temporarily disabled due to the "MiniMax paused with 402
balance handling" issue and pointing to where it's still supported
(ModelProvider type, providerMeta, related slash commands and nested model
lists) so future maintainers know it's intentional and temporary.
In `@apps/supercode-cli/server/src/cli/commands/slashCommands/model.ts`:
- Line 4: The import statement currently brings in createProvider and
ModelProvider but createProvider is never used; remove createProvider from the
import so only ModelProvider is imported from "src/cli/ai/provider.ts" (update
the import that references createProvider to import only the needed
ModelProvider symbol).
In `@apps/supercode-cli/server/src/index.ts`:
- Around line 1-11: The file currently initializes an Express app (imports:
express, cors; symbols: app, port, serverUrl, clientUrl) but the repo guideline
requires using Bun.serve(); replace the Express setup with a Bun.serve() HTTP
handler that registers routes and WebSocket support and delegates auth via
toNodeHandler(auth) (retain the existing toNodeHandler and auth usage), wire
CORS handling inline in the fetch handler instead of cors middleware, and
export/start the server via Bun.serve({ port, fetch }) so the file matches
apps/supercode-cli/server/**/{index,server}.{ts,tsx} pattern and supports bun
--hot; ensure prisma usage remains available inside the handler and that any
previously exported Express middleware is adapted into fetch-compatible
request/response handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae0ff973-5205-4d46-a669-ca9d48a999d8
📒 Files selected for processing (18)
apps/supercode-cli/server/src/cli/ai/chat/chat.tsapps/supercode-cli/server/src/cli/ai/google-service.tsapps/supercode-cli/server/src/cli/ai/minimax-service.tsapps/supercode-cli/server/src/cli/ai/nvidia-service.tsapps/supercode-cli/server/src/cli/ai/openrouter-service.tsapps/supercode-cli/server/src/cli/ai/provider.tsapps/supercode-cli/server/src/cli/ai/server-proxy-service.tsapps/supercode-cli/server/src/cli/commands/ai/init.tsapps/supercode-cli/server/src/cli/commands/slashCommands/index.tsapps/supercode-cli/server/src/cli/commands/slashCommands/model.tsapps/supercode-cli/server/src/cli/utils/tui.tsapps/supercode-cli/server/src/cli/workspace/context.tsapps/supercode-cli/server/src/config/tools.config.tsapps/supercode-cli/server/src/index.tsapps/supercode-cli/server/src/tools/definitions/run-command.tsapps/supercode-cli/server/src/tools/definitions/write-file.tsapps/supercode-cli/server/src/tools/permission-manager.tsapps/supercode-cli/server/src/tools/registry.ts
Comment on lines
+144
to
+160
| if (delta?.tool_calls) { | ||
| for (const tc of delta.tool_calls) { | ||
| const existing = toolCalls.find(t => t.id === tc.id) | ||
| if (existing) { | ||
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | ||
| } else { | ||
| toolCalls.push({ | ||
| id: tc.id, | ||
| type: tc.type || "function", | ||
| function: { | ||
| name: tc.function?.name || "", | ||
| arguments: tc.function?.arguments || "", | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Tool call argument accumulation uses wrong identifier.
OpenRouter/OpenAI streaming sends tool calls with an index field for delta accumulation, not repeated id values. After the first chunk, subsequent deltas have index but may omit id, causing this logic to create duplicate entries instead of appending arguments.
🐛 Proposed fix using index-based accumulation
if (delta?.tool_calls) {
for (const tc of delta.tool_calls) {
- const existing = toolCalls.find(t => t.id === tc.id)
+ const idx = tc.index ?? toolCalls.length
+ const existing = toolCalls[idx]
if (existing) {
if (tc.function?.arguments) existing.function.arguments += tc.function.arguments
+ if (tc.function?.name) existing.function.name = tc.function.name
+ if (tc.id) existing.id = tc.id
} else {
- toolCalls.push({
- id: tc.id,
+ toolCalls[idx] = {
+ id: tc.id || "",
type: tc.type || "function",
function: {
name: tc.function?.name || "",
arguments: tc.function?.arguments || "",
},
- })
+ }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (delta?.tool_calls) { | |
| for (const tc of delta.tool_calls) { | |
| const existing = toolCalls.find(t => t.id === tc.id) | |
| if (existing) { | |
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | |
| } else { | |
| toolCalls.push({ | |
| id: tc.id, | |
| type: tc.type || "function", | |
| function: { | |
| name: tc.function?.name || "", | |
| arguments: tc.function?.arguments || "", | |
| }, | |
| }) | |
| } | |
| } | |
| } | |
| if (delta?.tool_calls) { | |
| for (const tc of delta.tool_calls) { | |
| const idx = tc.index ?? toolCalls.length | |
| const existing = toolCalls[idx] | |
| if (existing) { | |
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | |
| if (tc.function?.name) existing.function.name = tc.function.name | |
| if (tc.id) existing.id = tc.id | |
| } else { | |
| toolCalls[idx] = { | |
| id: tc.id || "", | |
| type: tc.type || "function", | |
| function: { | |
| name: tc.function?.name || "", | |
| arguments: tc.function?.arguments || "", | |
| }, | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/cli/ai/openrouter-service.ts` around lines 144
- 160, The accumulation logic for streaming tool call deltas uses tc.id to find
existing entries but OpenRouter uses tc.index for subsequent chunks; update the
loop in openrouter-service.ts to prefer tc.index when locating/updating entries:
if tc.index is defined, use toolCalls[tc.index] (or assign into that slot) to
append function.arguments; otherwise fall back to finding by tc.id. When
creating a new entry, place it at toolCalls[tc.index] if index is provided (and
set id from tc.id if present), or push normally if no index; ensure
concatenation uses tc.function.arguments when appending.
| } | ||
|
|
||
| const allMessages = [...apiMessages] | ||
| const maxIter = 10 |
There was a problem hiding this comment.
Inconsistent tool iteration limit with client.
Server uses maxIter = 10 while openrouter-service.ts uses maxToolIterations = 25. This inconsistency could cause different behavior between CLI and server-proxied requests.
- const maxIter = 10
+ const maxIter = 25📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const maxIter = 10 | |
| const maxIter = 25 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/index.ts` at line 244, The server defines a
hardcoded maxIter = 10 which conflicts with openrouter-service.ts's
maxToolIterations = 25 and can cause divergent behavior; unify them by
extracting a single source of truth (e.g., a shared constant or
environment/config value) and use it in both places (replace index.ts's maxIter
and openrouter-service.ts's maxToolIterations to read the same exported CONFIG
or process.env value), then update any references that rely on
maxIter/maxToolIterations to the shared symbol so both CLI and proxied requests
use the identical iteration limit.
Comment on lines
+292
to
+305
| if (delta?.tool_calls) { | ||
| for (const tc of delta.tool_calls) { | ||
| const existing = toolCalls.find(t => t.id === tc.id) | ||
| if (existing) { | ||
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | ||
| } else { | ||
| toolCalls.push({ | ||
| id: tc.id, | ||
| type: tc.type || "function", | ||
| function: { name: tc.function?.name || "", arguments: tc.function?.arguments || "" }, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same tool call accumulation bug as client-side implementation.
This code has the same issue as openrouter-service.ts: using tc.id to find existing tool calls instead of tc.index. Streaming deltas reference tool calls by index, not ID.
🐛 Proposed fix using index-based accumulation
if (delta?.tool_calls) {
for (const tc of delta.tool_calls) {
- const existing = toolCalls.find(t => t.id === tc.id)
+ const idx = tc.index ?? toolCalls.length
+ const existing = toolCalls[idx]
if (existing) {
if (tc.function?.arguments) existing.function.arguments += tc.function.arguments
+ if (tc.function?.name) existing.function.name = tc.function.name
+ if (tc.id) existing.id = tc.id
} else {
- toolCalls.push({
- id: tc.id,
+ toolCalls[idx] = {
+ id: tc.id || "",
type: tc.type || "function",
function: { name: tc.function?.name || "", arguments: tc.function?.arguments || "" },
- })
+ }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (delta?.tool_calls) { | |
| for (const tc of delta.tool_calls) { | |
| const existing = toolCalls.find(t => t.id === tc.id) | |
| if (existing) { | |
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | |
| } else { | |
| toolCalls.push({ | |
| id: tc.id, | |
| type: tc.type || "function", | |
| function: { name: tc.function?.name || "", arguments: tc.function?.arguments || "" }, | |
| }) | |
| } | |
| } | |
| } | |
| if (delta?.tool_calls) { | |
| for (const tc of delta.tool_calls) { | |
| const idx = tc.index ?? toolCalls.length | |
| const existing = toolCalls[idx] | |
| if (existing) { | |
| if (tc.function?.arguments) existing.function.arguments += tc.function.arguments | |
| if (tc.function?.name) existing.function.name = tc.function.name | |
| if (tc.id) existing.id = tc.id | |
| } else { | |
| toolCalls[idx] = { | |
| id: tc.id || "", | |
| type: tc.type || "function", | |
| function: { name: tc.function?.name || "", arguments: tc.function?.arguments || "" }, | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/index.ts` around lines 292 - 305, The loop that
accumulates streaming tool calls currently matches existing entries by tc.id
(using toolCalls.find(t => t.id === tc.id)) but streaming deltas reference tool
calls by tc.index; change the logic in the delta.tool_calls processing so you
locate or place entries by tc.index: if tc.index is defined, use
toolCalls[tc.index] (or assign directly to toolCalls[tc.index] when creating)
instead of find-by-id; if tc.index is missing fallback to the existing id-based
find; also defensively concatenate arguments (e.g., existing.function.arguments
= (existing.function.arguments || "") + (tc.function?.arguments || "")) when
merging to avoid undefined.
| @@ -0,0 +1,71 @@ | |||
| import { z } from "zod" | |||
| import { exec } from "node:child_process" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Bun.$ instead of child_process.exec for shell execution.
As per coding guidelines, shell commands in apps/supercode-cli/server/**/*.{ts,tsx,js} should use Bun.$ template literal syntax instead of execa or child_process.exec.
♻️ Refactor to use Bun.$ API
-import { exec } from "node:child_process"
import path from "node:path"-return new Promise((resolve) => {
- const child = exec(
- command,
- {
- cwd: resolvedCwd,
- encoding: "utf-8",
- timeout,
- maxBuffer: 10 * 1024 * 1024,
- env: { ...process.env, PATH: process.env.PATH || "/usr/local/bin:/usr/bin:/bin" },
- },
- (error, stdout, stderr) => {
- const result: Record<string, unknown> = {
- exitCode: error?.code ?? 0,
- stdout: stdout || "",
- stderr: stderr || "",
- }
-
- if (error && error.killed) {
- result.signal = error.signal || "SIGTERM"
- result.stderr = (result.stderr as string) + `\nCommand timed out after ${timeout}ms`
- }
-
- if (error && error.code === undefined && !error.killed) {
- result.exitCode = -1
- result.stderr = (result.stderr as string) + `\n${error.message}`
- }
-
- resolve(JSON.stringify(result))
- },
- )
-})
+try {
+ const proc = Bun.$`${command}`.cwd(resolvedCwd).env({
+ ...process.env,
+ PATH: process.env.PATH || "/usr/local/bin:/usr/bin:/bin",
+ }).quiet()
+
+ const result = await proc
+ return JSON.stringify({
+ exitCode: result.exitCode,
+ stdout: await result.text(),
+ stderr: await result.stderr.text(),
+ })
+} catch (error: any) {
+ return JSON.stringify({
+ exitCode: error.exitCode ?? -1,
+ stdout: error.stdout?.toString() ?? "",
+ stderr: error.stderr?.toString() ?? error.message,
+ })
+}Note: Bun.$ doesn't have a built-in timeout, so you may need to wrap it with Promise.race() and setTimeout() if timeout support is required.
Also applies to: 40-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/run-command.ts` at line 2,
Replace usage of child_process.exec (imported as exec) with the Bun.$ template
literal API: remove the import from "node:child_process", update any call sites
that use exec(...) (and any helper functions in run-command.ts) to execute shell
commands using Bun.$`...` instead, returning a Promise that resolves with the
command output; if timeout behavior is required by the existing API, wrap the
Bun.$ call in a Promise.race with a setTimeout-based reject to emulate the
timeout. Ensure references to the exported functions or symbols in
run-command.ts that previously relied on exec (e.g., the runCommand / exec
wrapper functions) preserve their signature and error handling while switching
implementation to Bun.$.
Source: Coding guidelines
Comment on lines
+34
to
+36
| if (!resolvedCwd.startsWith(workspaceRoot)) { | ||
| throw new Error(`Working directory "${subdir}" is outside workspace root`) | ||
| } |
There was a problem hiding this comment.
Insufficient path traversal protection.
The workspace boundary check on Line 34 has the same vulnerability as write-file.ts: missing normalization, no trailing separator, and case-sensitivity issues on Windows.
🔒 Recommended fix with proper normalization
let resolvedCwd = workspaceRoot
if (subdir) {
- resolvedCwd = path.resolve(workspaceRoot, subdir)
- if (!resolvedCwd.startsWith(workspaceRoot)) {
+ const normalizedRoot = path.normalize(workspaceRoot) + path.sep
+ resolvedCwd = path.normalize(path.resolve(workspaceRoot, subdir)) + path.sep
+ if (!resolvedCwd.startsWith(normalizedRoot)) {
throw new Error(`Working directory "${subdir}" is outside workspace root`)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/run-command.ts` around lines
34 - 36, The check that throws when resolvedCwd is outside the workspaceRoot is
vulnerable to path traversal and OS differences; fix by normalizing and
resolving both workspaceRoot and the target path (use
path.resolve/path.normalize) and compare using a safe prefix check: append a
path separator to the normalized workspaceRoot (e.g. ensure
workspaceRootNormalized endsWith(path.sep) or use
path.join(workspaceRootNormalized, path.sep)), compute the relative path via
path.relative(workspaceRootNormalized, resolvedCwdNormalized) or compare the
normalized lowercase values on Windows (to handle case-insensitivity), and only
throw if the relative path starts with '..' or the resolvedCwdNormalized does
not start with the workspaceRootNormalized+sep; update the logic around the
resolvedCwd and workspaceRoot variables (and any code in the run-command
handling) accordingly.
Comment on lines
+50
to
+64
| const result: Record<string, unknown> = { | ||
| exitCode: error?.code ?? 0, | ||
| stdout: stdout || "", | ||
| stderr: stderr || "", | ||
| } | ||
|
|
||
| if (error && error.killed) { | ||
| result.signal = error.signal || "SIGTERM" | ||
| result.stderr = (result.stderr as string) + `\nCommand timed out after ${timeout}ms` | ||
| } | ||
|
|
||
| if (error && error.code === undefined && !error.killed) { | ||
| result.exitCode = -1 | ||
| result.stderr = (result.stderr as string) + `\n${error.message}` | ||
| } |
There was a problem hiding this comment.
Inconsistent exit code handling logic.
Line 51 sets exitCode: error?.code ?? 0, which defaults to 0 (success) when an error occurs without a code. Then Lines 61-64 attempt to correct this by checking error.code === undefined && !error.killed and setting exitCode = -1, but this creates redundant logic.
🛠️ Simplified fix
const result: Record<string, unknown> = {
- exitCode: error?.code ?? 0,
+ exitCode: error?.code ?? (error ? -1 : 0),
stdout: stdout || "",
stderr: stderr || "",
}
if (error && error.killed) {
result.signal = error.signal || "SIGTERM"
result.stderr = (result.stderr as string) + `\nCommand timed out after ${timeout}ms`
}
-if (error && error.code === undefined && !error.killed) {
- result.exitCode = -1
+if (error && !error.killed) {
result.stderr = (result.stderr as string) + `\n${error.message}`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result: Record<string, unknown> = { | |
| exitCode: error?.code ?? 0, | |
| stdout: stdout || "", | |
| stderr: stderr || "", | |
| } | |
| if (error && error.killed) { | |
| result.signal = error.signal || "SIGTERM" | |
| result.stderr = (result.stderr as string) + `\nCommand timed out after ${timeout}ms` | |
| } | |
| if (error && error.code === undefined && !error.killed) { | |
| result.exitCode = -1 | |
| result.stderr = (result.stderr as string) + `\n${error.message}` | |
| } | |
| const result: Record<string, unknown> = { | |
| exitCode: error?.code ?? (error ? -1 : 0), | |
| stdout: stdout || "", | |
| stderr: stderr || "", | |
| } | |
| if (error && error.killed) { | |
| result.signal = error.signal || "SIGTERM" | |
| result.stderr = (result.stderr as string) + `\nCommand timed out after ${timeout}ms` | |
| } | |
| if (error && !error.killed) { | |
| result.stderr = (result.stderr as string) + `\n${error.message}` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/run-command.ts` around lines
50 - 64, The exitCode initialization is inconsistent: currently result.exitCode
is set to error?.code ?? 0 and then later corrected to -1 when an error exists
without a code; change the initialization logic for result.exitCode to be
conditional on whether error exists (e.g., if error then use error.code ?? -1
else 0) so you don't need the separate correction block, and remove the
redundant branch that sets exitCode = -1; keep the existing handling for
error.killed (signal and timeout message) and the stderr/message concatenation
in the same places (referencing the result object, its exitCode property, and
the error.killed check).
| @@ -0,0 +1,52 @@ | |||
| import { z } from "zod" | |||
| import path from "node:path" | |||
| import { mkdir, writeFile } from "node:fs/promises" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Bun.file instead of Node.js fs for file operations.
As per coding guidelines, files in apps/supercode-cli/server/**/*.{ts,tsx,js} should prefer Bun.file over Node.js node:fs readFile/writeFile for file operations.
♻️ Refactor to use Bun.file API
-import { mkdir, writeFile } from "node:fs/promises"
+import { mkdir } from "node:fs/promises" const fileDir = path.dirname(fullPath)
await mkdir(fileDir, { recursive: true })
-await writeFile(fullPath, content, "utf-8")
+await Bun.write(fullPath, content)Also applies to: 38-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/write-file.ts` at line 3,
Replace Node's fs/promises usage with Bun.file-based operations: remove the
import of mkdir and writeFile from "node:fs/promises" and update any code that
calls mkdir or writeFile in this module (referenced symbols: mkdir, writeFile)
to use Bun.file(...).write and Bun.mkdir/Bun.file APIs instead (also update the
sections around lines 38-41 where writeFile is used). Ensure directory creation
uses Bun.mkdir or Bun.file path creation semantics and that file writes use
Bun.file(path).write(data, options) so the module follows the project's Bun file
I/O conventions.
Source: Coding guidelines
Comment on lines
+26
to
+28
| if (!fullPath.startsWith(workspaceRoot)) { | ||
| throw new Error(`Path "${filePath}" is outside workspace root`) | ||
| } |
There was a problem hiding this comment.
Insufficient path traversal protection.
The workspace boundary check on Line 26 is vulnerable to path traversal and edge cases:
- Missing normalization:
path.resolve()doesn't normalize.and..on all platforms before comparing. An attacker could craft paths likeworkspace/../etc/passwd. - Trailing separator: If
fullPathis/workspacefileandworkspaceRootis/workspace, the check passes incorrectly. - Case sensitivity: On Windows, filesystem paths are case-insensitive, but
startsWith()is case-sensitive.
🔒 Recommended fix with proper normalization
const workspaceRoot = process.env.SUPERCODE_WORKSPACE_ROOT || process.cwd()
-const fullPath = path.resolve(workspaceRoot, filePath)
+const normalizedRoot = path.normalize(workspaceRoot) + path.sep
+const fullPath = path.normalize(path.resolve(workspaceRoot, filePath)) + path.sep
-if (!fullPath.startsWith(workspaceRoot)) {
+if (!fullPath.startsWith(normalizedRoot)) {
throw new Error(`Path "${filePath}" is outside workspace root`)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/write-file.ts` around lines
26 - 28, The current check using fullPath.startsWith(workspaceRoot) is unsafe;
replace it by first normalizing and resolving both workspaceRoot and fullPath
(use path.resolve/path.normalize), on Windows compare case-insensitively
(lowercase both), and then verify containment using a relative-path check
instead of startsWith: compute rel = path.relative(workspaceRoot, fullPath) and
ensure rel is not empty, not starting with '..' and not equal to '..' (this
handles trailing-separator edge cases). Update the check around the symbols
fullPath, workspaceRoot and filePath to throw the error when the containment
test fails, ensuring the normalized/possibly lowercased values are compared.
Comment on lines
+43
to
+44
| const existingSize = content.length | ||
| const action = "created" |
There was a problem hiding this comment.
Misleading variable name and hardcoded action.
Line 43 uses existingSize for the new content length (not the existing file's size), and Line 44 hardcodes action as "created" even though the tool description states it can "overwrite an existing file."
🛠️ Suggested fix
-const existingSize = content.length
-const action = "created"
+const size = content.length
+const existsAlready = await Bun.file(fullPath).exists()
+const action = existsAlready ? "updated" : "created"
return JSON.stringify({
path: filePath,
- size: existingSize,
+ size,
action,
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/write-file.ts` around lines
43 - 44, The variables are misleading: change the misleading existingSize to
something like newSize or contentSize and derive it from content.length;
determine whether the target file already exists (use fs.existsSync or
fs.promises.stat) and set action dynamically to "created" or "overwritten"
instead of hardcoding "created"; update any logs/usages that reference
existingSize/action to use the new variable names and the computed action so the
message accurately reflects whether the file was created or overwritten (refer
to the symbols existingSize, action, and content.length in write-file.ts).
Comment on lines
+10
to
+26
| function withPermission(tool: Record<string, unknown>): Record<string, unknown> { | ||
| const originalExecute = tool.execute as ((args: any) => Promise<string>) | undefined | ||
| if (!originalExecute) return tool | ||
| return { | ||
| ...tool, | ||
| execute: async (args: any) => { | ||
| const allowed = await permissionManager.check( | ||
| (tool.name || tool.description) as string, | ||
| args, | ||
| ) | ||
| if (!allowed) { | ||
| return JSON.stringify({ cancelled: true, reason: "Permission denied by user" }) | ||
| } | ||
| return originalExecute(args) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Tool name lookup broken in permission check.
Line 17 attempts tool.name || tool.description, but the tool objects don't have a name property—they only have description, parameters, and execute. This causes permissionManager.check() to receive the full description string (e.g., "Create a new file or overwrite an existing file in the workspace...") instead of the tool key ("write_file").
The PermissionManager.check() method (Line 71 in permission-manager.ts) looks up rules using this.rules.get(toolName), where the keys are "write_file", "run_command", etc. With the current implementation, permission rules will never match, breaking the entire permission system.
🐛 Fix by refactoring to pass tool key explicitly
-function withPermission(tool: Record<string, unknown>): Record<string, unknown> {
+function withPermission(toolKey: string, tool: Record<string, unknown>): Record<string, unknown> {
const originalExecute = tool.execute as ((args: any) => Promise<string>) | undefined
if (!originalExecute) return tool
return {
...tool,
execute: async (args: any) => {
const allowed = await permissionManager.check(
- (tool.name || tool.description) as string,
+ toolKey,
args,
)
if (!allowed) {
return JSON.stringify({ cancelled: true, reason: "Permission denied by user" })
}
return originalExecute(args)
},
}
}Then update the call sites:
export const tools = {
read_file: readFileTool,
search_files: searchFilesTool,
- write_file: withPermission(writeFileTool as unknown as Record<string, unknown>),
- run_command: withPermission(runCommandTool as unknown as Record<string, unknown>),
+ write_file: withPermission("write_file", writeFileTool as unknown as Record<string, unknown>),
+ run_command: withPermission("run_command", runCommandTool as unknown as Record<string, unknown>),
url_fetch: urlFetchTool,
web_search: webSearchTool,
- code_exec: withPermission(codeExecTool as unknown as Record<string, unknown>),
+ code_exec: withPermission("code_exec", codeExecTool as unknown as Record<string, unknown>),
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/registry.ts` around lines 10 - 26, The
permission check is using tool.description instead of the tool's registry key so
rules never match; change withPermission to accept the tool's registry key
(e.g., add a second param like toolKey: string) and call
permissionManager.check(toolKey, args) instead of (tool.name ||
tool.description); update all call sites that wrap tools (where registry
iterates keys and calls withPermission) to pass the key (e.g., "write_file",
"run_command") when constructing the wrapped tool; keep the wrapped execute
behavior and return values unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major rework of the AI chat infrastructure: replaces the AI SDK-based OpenRouter
integration with raw fetch API (enabling server tools), fixes Escape-to-cancel
with a persistent stdin architecture, adds agent tool definitions and permission
gating, and introduces the
/modelslash command.Key Changes
🚀 OpenRouter Raw API
@openrouter/ai-sdk-providerwith direct fetch tohttps://openrouter.ai/api/v1/chat/completions(both client and server)openrouter:web_search,openrouter:web_fetch) alongside regular function toolshandled (no tool_calls returned)
⌨️ Persistent Stdin + Escape Cancel
chatLoopstart (setupStdin()) — never torn downstreamAbort: AbortControllershared across all modes/providersstreamAbort.abort()→ caught as AbortErrorprocess.exit)Escshown as "clear input / cancel response"🔌 AbortSignal Plumbing
signal?: AbortSignaladded toAIProvider.sendMessage()interfaceabortSignalto AI SDKstreamTextsignalto fetch + explicitsignal?.abortedcheck intool loop
signalto fetchsignalto fetch🛠️ Agent Tools
write_file— Creates/overwrites files with size/safety checksrun_command— Executes shell commands with timeout and cwd supportcode_exec— Now permission-gatedPermissionManager— Asks user for dangerous operations (rm -rf, DROP TABLE,force-push, etc.); session-level allow/deny
tools.config.ts🎮
/modelSlash CommandslashCommands/directory with typed registry/modelopens@clack/promptsinteractive pickerSlashCommandResult— provider swapped mid-session⚙️ Other
google→openroutertoolsoragentmodelNameconstructor param for per-instance modelmaxStepsbumped 5 → 25 across Google, MiniMax, and OpenRouterSummary by CodeRabbit
Release Notes
New Features
/model) now allow you to switch AI providers and models during chatImprovements
Chores