Add LiteLLM defaults and TUI tool UX #17
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds LiteLLM provider support and centralized LLM configuration; implements a permissioned coding-agent tool framework (parse/execute/permissions/prompt); surfaces provider and tools controls in CLI/UI and command handlers; wires tool execution into submit/stream flows; updates docs, defaults, deps, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Client App
participant Responses as Responses API
participant Parser as Tool Parser
participant Perms as Permission Resolver
participant Executor as Tool Executor
User->>App: Submit message
App->>Responses: Stream request (resolveLlmConfig(settings) + optional tool-system prompt)
Responses-->>App: Assistant response
App->>Parser: parseToolCalls(response)
Parser-->>App: [ToolCall...]
loop per ToolCall
App->>Perms: resolveToolPermission(call, settings, cwd, isDoomLoop)
alt deny
Perms-->>App: deny (reason)
App->>User: Notify denial
else ask
App->>User: Prompt for approval
User-->>App: Approve / Deny
alt approved
App->>Executor: executeToolCall(call, options)
else denied
App-->>App: Skip execution
end
else allow
App->>Executor: executeToolCall(call, options)
end
Executor-->>App: ToolResult
App->>App: Append formatted tool result to history
App->>Responses: Continue/reprompt with tool results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (55)
Comment |
Summary of ChangesHello @Zochory, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the CLI by integrating LiteLLM for flexible model provider support and introducing powerful coding agent capabilities. It allows the AI to interact with the local file system and execute commands in a controlled, permissioned environment, greatly expanding its utility for development tasks. Concurrently, the user experience for interacting with commands and mentions has been refined, and underlying dependencies have been brought up to date. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality, including support for LiteLLM providers and a comprehensive, permissioned tool execution loop for the coding agent. The changes are well-structured, with new logic correctly organized into llm and tools directories. The implementation of the tool loop, with its security considerations like permission levels and doom-loop detection, is particularly well-executed. The documentation has also been thoroughly updated to reflect these new features. My review includes a few suggestions to simplify some of the new logic and improve maintainability, particularly around environment variable handling and the structure of a large new command handler.
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | ||
| if [[ -n "${LITELLM_BASE_URL:-}" || -n "${LITELLM_API_KEY:-}" || -n "${LITELLM_MODEL:-}" ]]; then | ||
| export OPENAI_BASE_URL="${OPENAI_BASE_URL:-${LITELLM_BASE_URL:-}}" | ||
| export OPENAI_API_KEY="${OPENAI_API_KEY:-${LITELLM_API_KEY:-}}" | ||
| export OPENAI_MODEL="${OPENAI_MODEL:-${LITELLM_MODEL:-}}" | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | ||
| echo "[warn] OPENAI_BASE_URL / OPENAI_API_KEY / OPENAI_MODEL not set." | ||
| echo " The bridge uses OpenAIChatClient by default. Set these to a real backend." | ||
| echo " The bridge uses OpenAIChatClient by default. Set these or LITELLM_* vars." | ||
| fi |
There was a problem hiding this comment.
The logic for setting fallback environment variables and issuing a warning can be simplified to avoid redundant checks. The current implementation checks for missing OPENAI_* variables twice. You can combine these into a single, clearer block. The inner check for LITELLM_* variables is also redundant, as the parameter expansion ${VAR:-${FALLBACK:-}} safely handles cases where the fallback is empty.
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | |
| if [[ -n "${LITELLM_BASE_URL:-}" || -n "${LITELLM_API_KEY:-}" || -n "${LITELLM_MODEL:-}" ]]; then | |
| export OPENAI_BASE_URL="${OPENAI_BASE_URL:-${LITELLM_BASE_URL:-}}" | |
| export OPENAI_API_KEY="${OPENAI_API_KEY:-${LITELLM_API_KEY:-}}" | |
| export OPENAI_MODEL="${OPENAI_MODEL:-${LITELLM_MODEL:-}}" | |
| fi | |
| fi | |
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | |
| echo "[warn] OPENAI_BASE_URL / OPENAI_API_KEY / OPENAI_MODEL not set." | |
| echo " The bridge uses OpenAIChatClient by default. Set these to a real backend." | |
| echo " The bridge uses OpenAIChatClient by default. Set these or LITELLM_* vars." | |
| fi | |
| # If core OpenAI vars are not fully set, try to populate them from LiteLLM vars. | |
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | |
| export OPENAI_BASE_URL="${OPENAI_BASE_URL:-${LITELLM_BASE_URL:-}}" | |
| export OPENAI_API_KEY="${OPENAI_API_KEY:-${LITELLM_API_KEY:-}}" | |
| export OPENAI_MODEL="${OPENAI_MODEL:-${LITELLM_MODEL:-}}" | |
| # After attempting the fallback, if vars are still missing, issue a warning. | |
| if [[ -z "${OPENAI_BASE_URL:-}" || -z "${OPENAI_API_KEY:-}" || -z "${OPENAI_MODEL:-}" ]]; then | |
| echo "[warn] OPENAI_BASE_URL / OPENAI_API_KEY / OPENAI_MODEL not set." | |
| echo " The bridge uses OpenAIChatClient by default. Set these or LITELLM_* vars." | |
| fi | |
| fi |
| export function handleToolsCommand( | ||
| args: string | undefined, | ||
| context: CommandContext | ||
| ): CommandResult { | ||
| const { settings, setSettings } = context; | ||
| const trimmed = (args || "").trim().toLowerCase(); | ||
|
|
||
| if (!trimmed) { | ||
| const status = settings.tools?.enabled ? "Enabled" : "Disabled"; | ||
| const auto = settings.tools?.autoApprove ? "Enabled" : "Disabled"; | ||
| const perms = settings.tools?.permissions || {}; | ||
| const permSummary = [ | ||
| `read_file=${perms.read_file || "allow"}`, | ||
| `list_dir=${perms.list_dir || "allow"}`, | ||
| `write_file=${perms.write_file || "ask"}`, | ||
| `run_command=${perms.run_command || "ask"}`, | ||
| `external_directory=${perms.external_directory || "ask"}`, | ||
| `doom_loop=${perms.doom_loop || "ask"}`, | ||
| ].join(" "); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Tools: ${status}\nAuto-approve: ${auto}\nPermissions: ${permSummary}\nUsage: /tools on|off|auto [on|off]\n /tools perm <tool> <allow|ask|deny>`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const parts = trimmed.split(/\s+/); | ||
| const sub = parts[0] || ""; | ||
|
|
||
| if (sub === "perm" || sub === "permission") { | ||
| const rawTool = parts[1]; | ||
| const rawMode = parts[2]; | ||
| if (!rawTool || !rawMode) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Usage: /tools perm <tool> <allow|ask|deny>", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| const toolMap: Record<string, string> = { | ||
| read: "read_file", | ||
| read_file: "read_file", | ||
| list: "list_dir", | ||
| list_dir: "list_dir", | ||
| write: "write_file", | ||
| write_file: "write_file", | ||
| run: "run_command", | ||
| run_command: "run_command", | ||
| external: "external_directory", | ||
| external_directory: "external_directory", | ||
| doom: "doom_loop", | ||
| doom_loop: "doom_loop", | ||
| }; | ||
| const toolKey = toolMap[rawTool]; | ||
| if (!toolKey) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Unknown tool: ${rawTool}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| if (!["allow", "ask", "deny"].includes(rawMode)) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Mode must be allow, ask, or deny.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { | ||
| ...(prev.tools || {}), | ||
| permissions: { | ||
| ...(prev.tools?.permissions || {}), | ||
| [toolKey]: rawMode, | ||
| }, | ||
| }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Permission set: ${toolKey}=${rawMode}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "on" || sub === "enable" || sub === "enabled") { | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), enabled: true }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Tools enabled.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "off" || sub === "disable" || sub === "disabled") { | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), enabled: false }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Tools disabled.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "auto") { | ||
| const desired = parts[1] || "toggle"; | ||
| const nextValue = | ||
| desired === "on" | ||
| ? true | ||
| : desired === "off" | ||
| ? false | ||
| : !(settings.tools?.autoApprove ?? false); | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), autoApprove: nextValue }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Auto-approve ${nextValue ? "enabled" : "disabled"}.`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Usage: /tools on|off|auto [on|off]", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The handleToolsCommand function is quite long and handles multiple subcommands (on, off, auto, perm). To improve readability and maintainability, consider refactoring this into smaller, more focused functions for each subcommand (e.g., handleToolsPermissionCommand, handleToolsToggleCommand).
Additionally, the toolMap (lines 366-379) is defined inside the function. Since it's a constant mapping, it would be better to define it as a const at the module level. This avoids re-creating the object on every function call.
| } else { | ||
| baseUrl = settings?.endpoint || env.openaiBaseUrl || env.litellmBaseUrl; | ||
| apiKey = settings?.apiKey || env.openaiApiKey || env.litellmApiKey; | ||
| model = settings?.model || env.openaiModel || env.litellmModel; | ||
| } |
There was a problem hiding this comment.
The fallback logic for the custom provider could lead to unexpected behavior. When provider is 'custom', the code falls back to OPENAI_* and LITELLM_* environment variables if settings are not provided. A user selecting a 'custom' provider likely intends to provide all connection details manually via settings. Mixing a custom endpoint from settings with an API key or model from a different provider's environment variables could cause connection failures or unexpected costs.
Consider making the custom provider rely only on values from settings and not fall back to environment variables to make its behavior more predictable.
| function splitIntoSegments(content: string): Segment[] { | ||
| const segments: Segment[] = []; | ||
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; | ||
| let lastIndex = 0; | ||
| let match: RegExpExecArray | null; | ||
|
|
||
| while ((match = regex.exec(content)) !== null) { | ||
| if (match.index > lastIndex) { | ||
| const text = content.slice(lastIndex, match.index); | ||
| if (text.trim()) segments.push({ type: "text", content: text }); | ||
| } | ||
| const lang = match[1] ? match[1] : "plaintext"; | ||
| segments.push({ type: "code", content: match[2] || "", lang }); | ||
| lastIndex = match.index + match[0].length; | ||
| } | ||
|
|
||
| if (lastIndex < content.length) { | ||
| const text = content.slice(lastIndex); | ||
| if (text.trim()) segments.push({ type: "text", content: text }); | ||
| } | ||
|
|
||
| if (segments.length === 0) { | ||
| segments.push({ type: "text", content }); | ||
| } | ||
|
|
||
| return segments; | ||
| } |
There was a problem hiding this comment.
The splitIntoSegments function is a great addition for handling multiple code blocks. However, the regular expression /```(\\w+)?\\n([\\s\\S]*?)```/g might not correctly handle code blocks that don't have a newline immediately after the language identifier (e.g., ```python). While less common, it's a possible edge case. A more robust regex could be /```(\\w+)?(?:\\n|\\s)([\\s\\S]*?)```/g to allow for optional whitespace.
There was a problem hiding this comment.
Pull request overview
This PR modernizes qlaw-cli with LiteLLM provider support, introduces a permissioned coding agent tool system with file inlining, and refreshes dependencies. It adds significant new functionality including tool execution with configurable permissions, enhanced command menu UX with highlighted matching, and multi-provider configuration logic.
Key changes include:
- LiteLLM provider defaults and OpenAI-compatible config resolution with auto-detection
- Tool loop implementation with permission system (allow/ask/deny) for file operations and shell commands
- Enhanced @file mentions that inline actual file contents instead of just referencing them
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds LlmProvider and PermissionMode types, extends AppSettings with provider and tools configuration |
| src/tools/index.ts | New tool execution system with parsing, validation, and result formatting for read/list/write/run operations |
| src/tools/permissions.ts | Permission resolution logic with external directory and doom-loop protection |
| src/tools/prompt.ts | System prompt generation for tool-enabled conversations |
| src/llm/config.ts | Provider resolution and auth header logic supporting OpenAI, Azure, LiteLLM, and custom backends |
| src/llm/providerDefaults.ts | Provider-specific default handling and env variable fallback chains |
| src/llm/input.ts | Extracted prompt building logic for Responses API compatibility |
| src/services/streamingService.ts | Updated to use new config resolution and accept settings parameter |
| src/services/commandService.ts | Registers new /provider and /tools commands |
| src/index.tsx | Implements tool queue loop, async @file handling, and permission prompts in main app logic |
| src/hooks/useStreaming.ts | Adds onComplete callback for tool detection after streaming |
| src/hooks/useSettings.ts | Adds Coding Agent section with tools and auto-approve toggles, provider field |
| src/hooks/useInputMode.ts | Passes keywords field to UI suggestions |
| src/components/SuggestionList.tsx | Enhanced with query highlighting, action hints, and expanded detail view |
| src/components/StatusLine.tsx | Shows provider, model, and tools status in footer |
| src/components/MessageList.tsx | Implements multi-code-block rendering with segment parsing |
| src/mentionHandlers.ts | Adds async file reading and inlining for @file mentions with truncation |
| src/storage.ts | Adds LiteLLM env vars and tools configuration to defaults |
| src/sse.ts | Additional delta extraction patterns for broader compatibility |
| src/commandHandlers.ts | Implements /provider and /tools commands with permission management |
| src/commands.ts | Registers provider and tools in command metadata |
| src/cliHelp.ts | Updates status display to include provider and tools |
| src/api.ts | Simplified to re-export buildResponsesInput from llm/input |
| tests/streamingService.test.ts | Updated test to include settings parameter and new env vars |
| docs/ARCHITECTURE.md | Documents new llm/ and tools/ modules |
| docs/API-INTEGRATION.md | Adds LiteLLM proxy configuration section |
| docs/AGENTS.md | Documents coding agent tools and permissions |
| README.md | Adds LiteLLM and tools documentation |
| PLANS.md | New execution plan document |
| bridge/run.sh | Adds LITELLM_* to OPENAI_* fallback logic |
| bridge/requirements.txt | Updates Python dependency versions |
| bridge/requirements-dev.txt | Updates pytest version |
| bridge/README.md | Documents LiteLLM configuration |
| package.json | Bumps @OpenTui and React dependencies |
| bun.lock | Reflects updated dependency versions |
| .env.example | Adds LiteLLM environment variable examples |
Comments suppressed due to low confidence (1)
src/index.tsx:442
- The handleSubmit callback is declared as async but the command execution path (lines 434-443) calls handleExecuteCommand without awaiting it. This could lead to race conditions where the input is cleared before the command completes, or errors from command execution might not be properly caught. Either await the call or ensure proper error handling for the fire-and-forget pattern.
if (input.startsWith("/")) {
const parts = input.slice(1).split(" ");
const cmd = parts[0];
const args = parts.slice(1).join(" ");
if (cmd) {
handleExecuteCommand(cmd, args);
}
setInput("");
return;
src/components/MessageList.tsx
Outdated
|
|
||
| function splitIntoSegments(content: string): Segment[] { | ||
| const segments: Segment[] = []; | ||
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; |
There was a problem hiding this comment.
The regex pattern has incorrectly escaped backslashes. In JavaScript regex, \\w should be \w, and \\n should be \n, and \\s should be \s. The double backslashes will match literal backslashes in the string rather than the intended character classes and escape sequences. This will prevent code blocks from being properly detected and parsed.
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; | |
| const regex = /```(\w+)?\n([\s\S]*?)```/g; |
src/index.tsx
Outdated
| setPrompt({ | ||
| type: "confirm", | ||
| message: `Allow tool: ${call.tool}?`, |
There was a problem hiding this comment.
The tool permission prompt message is generic and doesn't show which file path or command is about to be executed. For security and UX reasons, users should see exactly what operation they're approving. The prompt should include details like the path being read/written or the command being executed, especially for sensitive operations.
| setPrompt({ | |
| type: "confirm", | |
| message: `Allow tool: ${call.tool}?`, | |
| const argsObject = (call as ToolCall).args || {}; | |
| const hasArgs = argsObject && Object.keys(argsObject).length > 0; | |
| let argsPreview = hasArgs ? JSON.stringify(argsObject, null, 2) : "no arguments"; | |
| const MAX_PREVIEW_LENGTH = 200; | |
| if (argsPreview.length > MAX_PREVIEW_LENGTH) { | |
| argsPreview = argsPreview.slice(0, MAX_PREVIEW_LENGTH) + "…"; | |
| } | |
| const permissionMessage = | |
| `Allow tool "${call.tool}" to run in:\n` + | |
| ` ${toolOptions.cwd}\n` + | |
| `with arguments:\n` + | |
| ` ${argsPreview}\n\n` + | |
| `Proceed?`; | |
| setPrompt({ | |
| type: "confirm", | |
| message: permissionMessage, |
src/index.tsx
Outdated
| const historyForApi = [ | ||
| ...(toolSystemMessage ? [toolSystemMessage] : []), | ||
| ...messagesRef.current.filter( | ||
| (m) => !(m.role === "system" && m.content.startsWith("Tool ")) |
There was a problem hiding this comment.
The filter condition on line 172 checks if the content "startsWith('Tool ')" but the actual system messages created for tool results use the format "Tool {tool_name} (ok)" or "Tool {tool_name} (error)" from formatToolResult. This check will correctly filter these messages. However, the "Skipped tool" messages don't match this pattern and won't be filtered, which could lead to accumulating error messages in the conversation history.
| (m) => !(m.role === "system" && m.content.startsWith("Tool ")) | |
| (m) => | |
| !( | |
| m.role === "system" && | |
| (m.content.startsWith("Tool ") || m.content.startsWith("Skipped tool")) | |
| ) |
README.md
Outdated
|
|
||
| ### Coding Agent Tools | ||
|
|
||
| When `/tools` is enabled, the assistant can request tool execution using fenced `tool` blocks. | ||
| Permissions follow `allow | ask | deny` and can be configured via `/tools perm`. | ||
| - `@code function example() { return true; }` will format as a code snippet |
There was a problem hiding this comment.
The documentation has a structural issue. Line 161 describing the @code mention is incorrectly placed after the "Coding Agent Tools" section header. This line should be moved up to be with the other mention examples (before line 156) to maintain proper section organization.
| ### Coding Agent Tools | |
| When `/tools` is enabled, the assistant can request tool execution using fenced `tool` blocks. | |
| Permissions follow `allow | ask | deny` and can be configured via `/tools perm`. | |
| - `@code function example() { return true; }` will format as a code snippet | |
| - `@code function example() { return true; }` will format as a code snippet | |
| ### Coding Agent Tools | |
| When `/tools` is enabled, the assistant can request tool execution using fenced `tool` blocks. | |
| Permissions follow `allow | ask | deny` and can be configured via `/tools perm`. |
| import type { AppSettings, LlmProvider } from "../types.ts"; | ||
|
|
||
| const DEFAULT_OPENAI_BASE_URL = "https://api.openai.com/v1"; | ||
|
|
||
| function normalizeProvider(provider?: string): LlmProvider | undefined { | ||
| if (!provider) return undefined; | ||
| const normalized = provider.toLowerCase(); | ||
| if (normalized === "openai" || normalized === "azure" || normalized === "litellm" || normalized === "custom") { | ||
| return normalized as LlmProvider; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| function isOpenAIEndpoint(endpoint: string | undefined, envOpenaiBase: string | undefined): boolean { | ||
| if (!endpoint) return true; | ||
| if (envOpenaiBase && endpoint === envOpenaiBase) return true; | ||
| if (endpoint === DEFAULT_OPENAI_BASE_URL) return true; | ||
| return endpoint.includes("api.openai.com"); | ||
| } | ||
|
|
||
| export function applyProviderDefaults(settings: AppSettings, provider?: string): AppSettings { | ||
| const normalized = normalizeProvider(provider); | ||
| if (!normalized) { | ||
| return { ...settings, provider }; | ||
| } | ||
|
|
||
| const env = { | ||
| openaiBaseUrl: process.env.OPENAI_BASE_URL, | ||
| openaiApiKey: process.env.OPENAI_API_KEY, | ||
| openaiModel: process.env.OPENAI_MODEL, | ||
| litellmBaseUrl: process.env.LITELLM_BASE_URL, | ||
| litellmApiKey: process.env.LITELLM_API_KEY, | ||
| litellmModel: process.env.LITELLM_MODEL, | ||
| }; | ||
|
|
||
| const next: AppSettings = { ...settings, provider: normalized }; | ||
|
|
||
| if (normalized === "litellm") { | ||
| if (isOpenAIEndpoint(next.endpoint, env.openaiBaseUrl)) { | ||
| next.endpoint = env.litellmBaseUrl || undefined; | ||
| } | ||
| if (!next.model || (env.openaiModel && next.model === env.openaiModel)) { | ||
| next.model = env.litellmModel || undefined; | ||
| } | ||
| if (!next.apiKey || (env.openaiApiKey && next.apiKey === env.openaiApiKey)) { | ||
| next.apiKey = env.litellmApiKey || undefined; | ||
| } | ||
| } | ||
|
|
||
| if (normalized === "openai") { | ||
| if (!next.endpoint || next.endpoint === env.litellmBaseUrl) { | ||
| next.endpoint = env.openaiBaseUrl || DEFAULT_OPENAI_BASE_URL; | ||
| } | ||
| if (!next.model || (env.litellmModel && next.model === env.litellmModel)) { | ||
| next.model = env.openaiModel || next.model; | ||
| } | ||
| if (!next.apiKey || (env.litellmApiKey && next.apiKey === env.litellmApiKey)) { | ||
| next.apiKey = env.openaiApiKey || next.apiKey; | ||
| } | ||
| } | ||
|
|
||
| return next; | ||
| } |
There was a problem hiding this comment.
The LLM configuration logic in src/llm/config.ts and src/llm/providerDefaults.ts lacks test coverage. These functions handle critical provider resolution and fallback logic, including inferProvider and applyProviderDefaults, which determine how different environment variables interact. Tests should verify the precedence rules and edge cases.
src/tools/permissions.ts
Outdated
| export function isExternalPath(path: string, cwd: string): boolean { | ||
| const resolved = resolve(cwd, path); | ||
| const root = resolve(cwd) + ""; // ensure normalized | ||
| if (resolved === root) return false; | ||
| return !resolved.startsWith(root + "/"); |
There was a problem hiding this comment.
The external path detection logic has a flaw. On Windows systems, path separators use backslashes (), but this code only checks for forward slash (/). This means paths within the project on Windows might incorrectly be flagged as external. Use path.sep instead of hardcoding "/" to ensure cross-platform compatibility.
| case "run_command": { | ||
| const command = String(args.command || "").trim(); | ||
| if (!command) throw new Error("Missing command"); | ||
| const { stdout, stderr } = await execAsync(command, { cwd, maxBuffer: maxOutputChars * 2 }); | ||
| const output = [stdout, stderr].filter(Boolean).join("\n"); | ||
| return { tool: call.tool, ok: true, output: truncate(output || "(no output)", maxOutputChars) }; |
There was a problem hiding this comment.
The run_command tool executes arbitrary shell commands without any validation or sanitization. This is a security risk as it could allow command injection if user input flows into the command argument. Consider adding validation, a command allowlist, or at minimum, clear warnings in documentation about the security implications of enabling this tool.
| try { | ||
| const fullPath = resolve(options.cwd, path); | ||
| const file = await readFile(fullPath); | ||
| const limited = file.length > options.maxFileBytes ? file.subarray(0, options.maxFileBytes) : file; | ||
| const text = limited.toString("utf-8"); | ||
| const suffix = file.length > options.maxFileBytes ? `\n…truncated (${file.length - options.maxFileBytes} bytes)` : ""; | ||
| const lang = guessLanguage(path); | ||
| return `[File: ${path}]\n\`\`\`${lang}\n${text}${suffix}\n\`\`\``; | ||
| } catch (e: any) { | ||
| return `[File Reference: ${path}]\n\nUnable to read file (${e?.message || e}). Please verify the path.`; | ||
| } |
There was a problem hiding this comment.
The @file mention handler reads files without validating that the path is within the project directory. This could allow reading arbitrary files on the system if a malicious or compromised model requests sensitive files. Consider applying the same external path validation used in the tools permission system (isExternalPath) to prevent reading files outside the working directory.
| import { readFile, writeFile, readdir, stat, mkdir } from "fs/promises"; | ||
| import { resolve } from "path"; | ||
| import { exec } from "child_process"; | ||
| import { promisify } from "util"; | ||
|
|
||
| const execAsync = promisify(exec); | ||
|
|
||
| export type ToolName = "read_file" | "list_dir" | "write_file" | "run_command"; | ||
|
|
||
| export interface ToolCall { | ||
| tool: ToolName; | ||
| args?: Record<string, any>; | ||
| } | ||
|
|
||
| export interface ToolResult { | ||
| tool: ToolName; | ||
| ok: boolean; | ||
| output: string; | ||
| error?: string; | ||
| } | ||
|
|
||
| export interface ToolExecutionOptions { | ||
| cwd: string; | ||
| maxFileBytes: number; | ||
| maxDirEntries: number; | ||
| maxOutputChars: number; | ||
| } | ||
|
|
||
| const TOOL_BLOCK_REGEX = /```tool\s*([\s\S]*?)```/gi; | ||
|
|
||
| export function parseToolCalls(text: string): ToolCall[] { | ||
| const calls: ToolCall[] = []; | ||
| if (!text) return calls; | ||
|
|
||
| let match: RegExpExecArray | null; | ||
| while ((match = TOOL_BLOCK_REGEX.exec(text)) !== null) { | ||
| const payload = match[1]?.trim(); | ||
| if (!payload) continue; | ||
| try { | ||
| const parsed = JSON.parse(payload); | ||
| if (Array.isArray(parsed)) { | ||
| parsed.forEach((item) => pushToolCall(item, calls)); | ||
| } else if (parsed && typeof parsed === "object") { | ||
| if (Array.isArray((parsed as any).tools)) { | ||
| (parsed as any).tools.forEach((item: any) => pushToolCall(item, calls)); | ||
| } else { | ||
| pushToolCall(parsed, calls); | ||
| } | ||
| } | ||
| } catch { | ||
| continue; | ||
| } | ||
| } | ||
| return calls; | ||
| } | ||
|
|
||
| function pushToolCall(raw: any, calls: ToolCall[]) { | ||
| if (!raw || typeof raw !== "object") return; | ||
| const tool = String(raw.tool || raw.name || "").trim(); | ||
| if (!tool) return; | ||
| if (tool !== "read_file" && tool !== "list_dir" && tool !== "write_file" && tool !== "run_command") return; | ||
| calls.push({ tool: tool as ToolName, args: raw.args || raw.parameters || {} }); | ||
| } | ||
|
|
||
| function truncate(value: string, maxChars: number): string { | ||
| if (value.length <= maxChars) return value; | ||
| return `${value.slice(0, maxChars)}\n…truncated (${value.length - maxChars} chars)`; | ||
| } | ||
|
|
||
| export async function executeToolCall(call: ToolCall, options: ToolExecutionOptions): Promise<ToolResult> { | ||
| const { cwd, maxFileBytes, maxDirEntries, maxOutputChars } = options; | ||
| const args = call.args || {}; | ||
|
|
||
| try { | ||
| switch (call.tool) { | ||
| case "read_file": { | ||
| const path = String(args.path || "").trim(); | ||
| if (!path) throw new Error("Missing path"); | ||
| const fullPath = resolve(cwd, path); | ||
| const file = await readFile(fullPath); | ||
| const limited = file.length > maxFileBytes ? file.subarray(0, maxFileBytes) : file; | ||
| const text = limited.toString("utf-8"); | ||
| const output = file.length > maxFileBytes ? `${text}\n…truncated (${file.length - maxFileBytes} bytes)` : text; | ||
| return { tool: call.tool, ok: true, output: truncate(output, maxOutputChars) }; | ||
| } | ||
| case "list_dir": { | ||
| const path = String(args.path || ".").trim(); | ||
| const fullPath = resolve(cwd, path); | ||
| const entries = await readdir(fullPath); | ||
| const sliced = entries.slice(0, maxDirEntries); | ||
| const labeled = await Promise.all( | ||
| sliced.map(async (entry) => { | ||
| const fullEntry = resolve(fullPath, entry); | ||
| try { | ||
| const stats = await stat(fullEntry); | ||
| return stats.isDirectory() ? `${entry}/` : entry; | ||
| } catch { | ||
| return entry; | ||
| } | ||
| }) | ||
| ); | ||
| const output = labeled.join("\n") + (entries.length > maxDirEntries ? `\n…truncated (${entries.length - maxDirEntries} more)` : ""); | ||
| return { tool: call.tool, ok: true, output: truncate(output, maxOutputChars) }; | ||
| } | ||
| case "write_file": { | ||
| const path = String(args.path || "").trim(); | ||
| const content = String(args.content ?? ""); | ||
| if (!path) throw new Error("Missing path"); | ||
| const fullPath = resolve(cwd, path); | ||
| await mkdir(resolve(fullPath, ".."), { recursive: true }); | ||
| await writeFile(fullPath, content, "utf-8"); | ||
| return { tool: call.tool, ok: true, output: `Wrote ${content.length} chars to ${path}` }; | ||
| } | ||
| case "run_command": { | ||
| const command = String(args.command || "").trim(); | ||
| if (!command) throw new Error("Missing command"); | ||
| const { stdout, stderr } = await execAsync(command, { cwd, maxBuffer: maxOutputChars * 2 }); | ||
| const output = [stdout, stderr].filter(Boolean).join("\n"); | ||
| return { tool: call.tool, ok: true, output: truncate(output || "(no output)", maxOutputChars) }; | ||
| } | ||
| default: | ||
| throw new Error(`Unsupported tool: ${call.tool}`); | ||
| } | ||
| } catch (e: any) { | ||
| return { | ||
| tool: call.tool, | ||
| ok: false, | ||
| output: "", | ||
| error: e?.message || String(e), | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export function formatToolResult(result: ToolResult): string { | ||
| const status = result.ok ? "ok" : "error"; | ||
| const header = `Tool ${result.tool} (${status})`; | ||
| const body = result.ok ? result.output : result.error || "Unknown error"; | ||
| return `${header}\n${body}`; | ||
| } |
There was a problem hiding this comment.
The new tool execution functionality in src/tools/index.ts, src/tools/permissions.ts, and src/tools/prompt.ts lacks test coverage. Given that the repository has comprehensive tests for other services (sse.test.ts, commandService.test.ts, etc.), these new security-sensitive features should also have corresponding test files to verify tool parsing, execution, permission resolution, and error handling.
src/components/MessageList.tsx
Outdated
|
|
||
| function splitIntoSegments(content: string): Segment[] { | ||
| const segments: Segment[] = []; | ||
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; |
There was a problem hiding this comment.
Character '\' is repeated in the same character class.
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; | |
| const regex = /```(\w+)?\n([\s\S]*?)```/g; |
| ...messagesRef.current.filter( | ||
| (m) => !(m.role === "system" && m.content.startsWith("Tool ")) | ||
| ), |
There was a problem hiding this comment.
The tool execution logic filters out system messages that start with "Tool " when building history for the API. However, this string-prefix check is fragile and could inadvertently filter out legitimate system messages that happen to start with "Tool ". Consider using a more robust approach like adding a metadata flag to tool-related messages or using a specific message type.
| function guessLanguage(path: string): string { | ||
| const ext = extname(path).toLowerCase().replace(".", ""); | ||
| if (!ext) return "text"; | ||
| if (ext === "ts") return "typescript"; | ||
| if (ext === "tsx") return "tsx"; | ||
| if (ext === "js") return "javascript"; | ||
| if (ext === "jsx") return "jsx"; | ||
| if (ext === "py") return "python"; | ||
| if (ext === "json") return "json"; | ||
| if (ext === "md") return "markdown"; | ||
| if (ext === "sh") return "bash"; | ||
| return ext; | ||
| } |
There was a problem hiding this comment.
The function guessLanguage returns language identifiers but some of the mappings may not align with common syntax highlighting libraries. For example, "jsx" and "tsx" are often aliased to "javascript" and "typescript" respectively in many highlighters. Consider documenting which syntax highlighting library is being targeted or ensuring these identifiers match the expected format.
src/llm/config.ts
Outdated
| ): LlmProvider { | ||
| const url = baseUrl || ""; | ||
| if (url.includes("azure.com") || url.includes("/openai/")) return "azure"; | ||
| if (url.includes("litellm") || url.includes("/llm/") || (model && model.includes("/"))) return "litellm"; |
There was a problem hiding this comment.
The provider inference logic uses model.includes("/") as a heuristic for LiteLLM-style models. However, this could produce false positives if a user sets a custom model name containing a slash for non-LiteLLM purposes. Consider adding a comment explaining this heuristic or making it more robust by checking for specific prefixes like "openai/", "anthropic/", etc.
| export function handleToolsCommand( | ||
| args: string | undefined, | ||
| context: CommandContext | ||
| ): CommandResult { | ||
| const { settings, setSettings } = context; | ||
| const trimmed = (args || "").trim().toLowerCase(); | ||
|
|
||
| if (!trimmed) { | ||
| const status = settings.tools?.enabled ? "Enabled" : "Disabled"; | ||
| const auto = settings.tools?.autoApprove ? "Enabled" : "Disabled"; | ||
| const perms = settings.tools?.permissions || {}; | ||
| const permSummary = [ | ||
| `read_file=${perms.read_file || "allow"}`, | ||
| `list_dir=${perms.list_dir || "allow"}`, | ||
| `write_file=${perms.write_file || "ask"}`, | ||
| `run_command=${perms.run_command || "ask"}`, | ||
| `external_directory=${perms.external_directory || "ask"}`, | ||
| `doom_loop=${perms.doom_loop || "ask"}`, | ||
| ].join(" "); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Tools: ${status}\nAuto-approve: ${auto}\nPermissions: ${permSummary}\nUsage: /tools on|off|auto [on|off]\n /tools perm <tool> <allow|ask|deny>`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const parts = trimmed.split(/\s+/); | ||
| const sub = parts[0] || ""; | ||
|
|
||
| if (sub === "perm" || sub === "permission") { | ||
| const rawTool = parts[1]; | ||
| const rawMode = parts[2]; | ||
| if (!rawTool || !rawMode) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Usage: /tools perm <tool> <allow|ask|deny>", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| const toolMap: Record<string, string> = { | ||
| read: "read_file", | ||
| read_file: "read_file", | ||
| list: "list_dir", | ||
| list_dir: "list_dir", | ||
| write: "write_file", | ||
| write_file: "write_file", | ||
| run: "run_command", | ||
| run_command: "run_command", | ||
| external: "external_directory", | ||
| external_directory: "external_directory", | ||
| doom: "doom_loop", | ||
| doom_loop: "doom_loop", | ||
| }; | ||
| const toolKey = toolMap[rawTool]; | ||
| if (!toolKey) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Unknown tool: ${rawTool}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| if (!["allow", "ask", "deny"].includes(rawMode)) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Mode must be allow, ask, or deny.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { | ||
| ...(prev.tools || {}), | ||
| permissions: { | ||
| ...(prev.tools?.permissions || {}), | ||
| [toolKey]: rawMode, | ||
| }, | ||
| }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Permission set: ${toolKey}=${rawMode}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "on" || sub === "enable" || sub === "enabled") { | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), enabled: true }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Tools enabled.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "off" || sub === "disable" || sub === "disabled") { | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), enabled: false }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Tools disabled.", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (sub === "auto") { | ||
| const desired = parts[1] || "toggle"; | ||
| const nextValue = | ||
| desired === "on" | ||
| ? true | ||
| : desired === "off" | ||
| ? false | ||
| : !(settings.tools?.autoApprove ?? false); | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| tools: { ...(prev.tools || {}), autoApprove: nextValue }, | ||
| })); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Auto-approve ${nextValue ? "enabled" : "disabled"}.`, | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: "Usage: /tools on|off|auto [on|off]", | ||
| timestamp: new Date(), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The handleToolsCommand function is very long (160+ lines) and handles multiple subcommands. Consider refactoring this into separate handler functions like handleToolsPermissionSubcommand, handleToolsToggleSubcommand, etc., to improve maintainability and testability.
| import type { AppSettings, LlmProvider } from "../types.ts"; | ||
|
|
||
| const DEFAULT_OPENAI_BASE_URL = "https://api.openai.com/v1"; | ||
|
|
||
| function normalizeProvider(provider?: string): LlmProvider | undefined { | ||
| if (!provider) return undefined; | ||
| const normalized = provider.toLowerCase(); | ||
| if (normalized === "openai" || normalized === "azure" || normalized === "litellm" || normalized === "custom") { | ||
| return normalized as LlmProvider; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| function isOpenAIEndpoint(endpoint: string | undefined, envOpenaiBase: string | undefined): boolean { | ||
| if (!endpoint) return true; | ||
| if (envOpenaiBase && endpoint === envOpenaiBase) return true; | ||
| if (endpoint === DEFAULT_OPENAI_BASE_URL) return true; | ||
| return endpoint.includes("api.openai.com"); | ||
| } | ||
|
|
||
| function looksLikeOpenAIModel(model: string | undefined, envOpenaiModel: string | undefined): boolean { | ||
| if (!model) return true; | ||
| if (envOpenaiModel && model === envOpenaiModel) return true; | ||
| if (model.includes("/")) return false; | ||
| const lower = model.toLowerCase(); | ||
| return ( | ||
| lower.startsWith("gpt-") || | ||
| lower.startsWith("o1") || | ||
| lower.startsWith("o3") || | ||
| lower.startsWith("gpt4") || | ||
| lower.startsWith("gpt3") | ||
| ); | ||
| } | ||
|
|
||
| export function applyProviderDefaults(settings: AppSettings, provider?: string): AppSettings { | ||
| const normalized = normalizeProvider(provider); | ||
| if (!normalized) { | ||
| return { ...settings, provider }; | ||
| } | ||
|
|
||
| const env = { | ||
| openaiBaseUrl: process.env.OPENAI_BASE_URL, | ||
| openaiApiKey: process.env.OPENAI_API_KEY, | ||
| openaiModel: process.env.OPENAI_MODEL, | ||
| litellmBaseUrl: process.env.LITELLM_BASE_URL, | ||
| litellmApiKey: process.env.LITELLM_API_KEY, | ||
| litellmModel: process.env.LITELLM_MODEL, | ||
| }; | ||
|
|
||
| const next: AppSettings = { ...settings, provider: normalized }; | ||
|
|
||
| if (normalized === "litellm") { | ||
| if (isOpenAIEndpoint(next.endpoint, env.openaiBaseUrl)) { | ||
| next.endpoint = env.litellmBaseUrl || undefined; | ||
| } else if (!env.litellmBaseUrl && next.endpoint) { | ||
| // Leave custom endpoint, but clear OpenAI defaults when LiteLLM envs are missing | ||
| next.endpoint = isOpenAIEndpoint(next.endpoint, env.openaiBaseUrl) ? undefined : next.endpoint; | ||
| } | ||
| if (env.litellmModel) { | ||
| if (looksLikeOpenAIModel(next.model, env.openaiModel)) { | ||
| next.model = env.litellmModel; | ||
| } | ||
| } else if (looksLikeOpenAIModel(next.model, env.openaiModel)) { | ||
| next.model = undefined; | ||
| } | ||
| if (env.litellmApiKey) { | ||
| if (!next.apiKey || (env.openaiApiKey && next.apiKey === env.openaiApiKey)) { | ||
| next.apiKey = env.litellmApiKey; | ||
| } | ||
| } else if (env.openaiApiKey && next.apiKey === env.openaiApiKey) { | ||
| next.apiKey = undefined; | ||
| } | ||
| } | ||
|
|
||
| if (normalized === "openai") { | ||
| if (!next.endpoint || next.endpoint === env.litellmBaseUrl) { | ||
| next.endpoint = env.openaiBaseUrl || DEFAULT_OPENAI_BASE_URL; | ||
| } | ||
| if (!next.model || (env.litellmModel && next.model === env.litellmModel)) { | ||
| next.model = env.openaiModel || next.model; | ||
| } | ||
| if (!next.apiKey || (env.litellmApiKey && next.apiKey === env.litellmApiKey)) { | ||
| next.apiKey = env.openaiApiKey || next.apiKey; | ||
| } | ||
| } | ||
|
|
||
| return next; | ||
| } |
There was a problem hiding this comment.
Test coverage is missing for the new LiteLLM provider configuration logic. The PR adds src/llm/config.ts and src/llm/providerDefaults.ts with complex provider resolution and default-handling logic, but no tests are included. Consider adding tests to verify provider inference, default resolution, and edge cases like missing environment variables.
| const content = String(args.content ?? ""); | ||
| if (!path) throw new Error("Missing path"); | ||
| const fullPath = resolve(cwd, path); | ||
| await mkdir(resolve(fullPath, ".."), { recursive: true }); |
There was a problem hiding this comment.
The write_file tool uses resolve(fullPath, "..") to get the parent directory, but this is fragile and could behave unexpectedly if fullPath ends with a slash or has other path quirks. Consider using path.dirname(fullPath) instead for more reliable parent directory resolution.
| if (key.name === "left" || key.name === "right") { | ||
| if (ranges.length > 0) { | ||
| const delta = key.name === "left" ? -1 : 1; | ||
| const current = activeSectionIndex >= 0 ? activeSectionIndex : 0; | ||
| const next = (current + delta + ranges.length) % ranges.length; | ||
| const target = ranges[next]; | ||
| if (target) { | ||
| context.setSettingsFocusIndex(target.start); | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The left/right arrow navigation logic for switching between settings sections uses complex index arithmetic that could be error-prone. The logic calculates ranges and wraps around, but if ranges is empty or if there's an off-by-one error, it could cause unexpected behavior. Consider adding a guard condition to handle edge cases or adding comments explaining the arithmetic.
| if (permission.mode === "allow") { | ||
| void runTool(); |
There was a problem hiding this comment.
The function uses void runTool() to explicitly ignore the Promise. While this is correct, adding a comment explaining why the promise is intentionally not awaited would help future maintainers understand that this is deliberate and not an oversight.
src/storage.ts
Outdated
| model: PREFER_LITELLM ? (LITELLM_MODEL || OPENAI_MODEL) : (OPENAI_MODEL || LITELLM_MODEL), | ||
| endpoint: PREFER_LITELLM ? (LITELLM_BASE_URL || OPENAI_BASE_URL) : (OPENAI_BASE_URL || LITELLM_BASE_URL), | ||
| apiKey: PREFER_LITELLM ? (LITELLM_API_KEY || OPENAI_API_KEY) : (OPENAI_API_KEY || LITELLM_API_KEY), |
There was a problem hiding this comment.
The complex ternary expressions for model, endpoint, and apiKey are difficult to read and maintain. Consider extracting this logic into a helper function like resolveLlmDefaults() that returns an object with these three properties. This would improve readability and testability.
src/components/MessageList.tsx
Outdated
|
|
||
| function splitIntoSegments(content: string): Segment[] { | ||
| const segments: Segment[] = []; | ||
| const regex = /```(\\w+)?\\n([\\s\\S]*?)```/g; |
There was a problem hiding this comment.
Character '\' is repeated in the same character class.
| function buildApiKeySelectOptions(): PromptSelectOption[] { | ||
| const options: PromptSelectOption[] = []; | ||
| if (process.env.LITELLM_API_KEY) { | ||
| options.push({ | ||
| name: "Use LITELLM_API_KEY", | ||
| description: "Apply the LiteLLM API key from env", | ||
| value: process.env.LITELLM_API_KEY, | ||
| }); | ||
| } | ||
| if (process.env.OPENAI_API_KEY) { | ||
| options.push({ | ||
| name: "Use OPENAI_API_KEY", | ||
| description: "Apply the OpenAI API key from env", | ||
| value: process.env.OPENAI_API_KEY, | ||
| }); | ||
| } | ||
| options.push({ name: "Custom…", description: "Type a custom API key", value: CUSTOM_SELECT_VALUE }); | ||
| return dedupeSelectOptions(options); |
There was a problem hiding this comment.
The function buildApiKeySelectOptions exposes actual API keys in plain text in the select options. While this might be intentional for user convenience, it creates a potential security concern where API keys could be visible in logs, screenshots, or UI traces. Consider either masking these keys (showing only the last 4 characters) or adding a warning in the description about the security implications of displaying API keys.
| case "run_command": { | ||
| const command = String(args.command || "").trim(); | ||
| if (!command) throw new Error("Missing command"); | ||
| const { stdout, stderr } = await execAsync(command, { cwd, maxBuffer: maxOutputChars * 2 }); | ||
| const output = [stdout, stderr].filter(Boolean).join("\n"); | ||
| return { tool: call.tool, ok: true, output: truncate(output || "(no output)", maxOutputChars) }; | ||
| } |
There was a problem hiding this comment.
The run_command tool executes arbitrary shell commands without any validation or sanitization. While there's a permission system in place, once approved, any command can run. Consider adding command validation (e.g., disallowing certain dangerous commands like rm -rf /, shell escaping validation) or implementing a command whitelist/blacklist pattern to prevent accidental or malicious execution of destructive commands.
| lower.startsWith("o1") || | ||
| lower.startsWith("o3") || |
There was a problem hiding this comment.
The model detection logic at line 28-29 will match models like "o10", "o100", "o3-preview-beta" unexpectedly. The pattern lower.startsWith("o1") or lower.startsWith("o3") is too broad. Consider using more specific patterns like checking for exact model names (e.g., "o1-preview", "o1-mini") or a pattern that ensures word boundaries to avoid false positives.
| const resolvedBaseUrl = baseUrl; | ||
|
|
||
| if (!resolvedBaseUrl || !apiKey || !model) return null; | ||
|
|
||
| return { | ||
| provider, | ||
| baseUrl: resolvedBaseUrl, |
There was a problem hiding this comment.
Line 74 assigns baseUrl to resolvedBaseUrl but this assignment is redundant. The variable resolvedBaseUrl can be removed, and line 80 should directly use baseUrl instead, simplifying the code.
| const masked = | ||
| options?.shouldMaskValue && typeof displayValue === "string" | ||
| ? displayValue | ||
| ? "***" + displayValue.slice(-4) | ||
| : "Not set" | ||
| : displayValue; |
There was a problem hiding this comment.
The initial value of masked is unused, since it is always overwritten.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…g sanitization Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
src/commandHandlers.ts
Outdated
|
|
||
| const msg = `${promptMessage}\nCurrent: ${masked}\nUsage: /${settingKey} <value>`; | ||
| return { systemMessage: { id: generateUniqueId(), role: "system", content: msg, timestamp: new Date() } }; |
There was a problem hiding this comment.
Lines 445-446 are unreachable code. The function always returns at line 439 or 443, so the code creating and returning a system message on lines 445-446 will never execute.
This appears to be dead code left over from refactoring. It should be removed to avoid confusion.
| const msg = `${promptMessage}\nCurrent: ${masked}\nUsage: /${settingKey} <value>`; | |
| return { systemMessage: { id: generateUniqueId(), role: "system", content: msg, timestamp: new Date() } }; |
| export function isExternalPath(path: string, cwd: string): boolean { | ||
| const resolved = resolve(cwd, path); | ||
| const root = resolve(cwd) + ""; // ensure normalized | ||
| if (resolved === root) return false; | ||
| return !resolved.startsWith(root + "/"); | ||
| } |
There was a problem hiding this comment.
The external path detection has a logical issue. On line 18, the check !resolved.startsWith(root + "/") will incorrectly flag paths as external when they should be internal.
For example, if cwd is /home/user/project and path is /home/user/project-docs/file.txt, the resolved path starts with /home/user/project but is actually outside the project directory. The current logic would incorrectly allow this as internal.
The issue is that line 16 adds an empty string to root (which does nothing), and line 18 checks if the resolved path starts with root + "/". However, if resolved === root, we return false early (line 17), but we should also handle the case where resolved is actually a parent or sibling of root.
Consider using a more robust check that normalizes both paths and ensures resolved is actually a child of root, not just a string prefix match.
src/llm/config.ts
Outdated
| if (url.includes("litellm") || url.includes("/llm/") || (model && model.includes("/"))) return "litellm"; | ||
| return "litellm"; |
There was a problem hiding this comment.
The inferProvider function has redundant logic. Lines 57-58 both return "litellm", meaning the function will always default to "litellm" regardless of the URL or model.
The conditional check on line 57 (if (url.includes("litellm") || url.includes("/llm/") || (model && model.includes("/")))) is unnecessary since line 58 will execute regardless of whether the condition is true or false.
Consider clarifying the intent: if the default should always be "litellm", the entire if statement can be removed. If there should be different logic for detecting non-LiteLLM providers (like detecting actual OpenAI endpoints), that logic needs to be added before the default return.
| try { | ||
| const fullPath = resolve(options.cwd, path); | ||
| const file = await readFile(fullPath); | ||
| const limited = file.length > options.maxFileBytes ? file.subarray(0, options.maxFileBytes) : file; | ||
| const text = limited.toString("utf-8"); | ||
| const suffix = file.length > options.maxFileBytes ? `\n…truncated (${file.length - options.maxFileBytes} bytes)` : ""; | ||
| const lang = guessLanguage(path); | ||
| return `[File: ${path}]\n\`\`\`${lang}\n${text}${suffix}\n\`\`\``; | ||
| } catch (e: any) { | ||
| return `[File Reference: ${path}]\n\nUnable to read file (${e?.message || e}). Please verify the path.`; | ||
| } |
There was a problem hiding this comment.
The @file mention handler reads arbitrary files from the filesystem without proper path validation or security checks. While it uses resolve(options.cwd, path), there's no validation to prevent path traversal attacks using sequences like ../../../etc/passwd.
This creates a security risk where:
- A malicious user could craft prompts with
@file ../../../sensitive-fileto read files outside the project directory - An LLM could be manipulated through prompt injection to request sensitive files
Consider adding:
- Path validation to ensure the resolved path stays within the project directory (similar to the tool permission system)
- A whitelist of allowed file extensions or patterns
- Checks against reading sensitive system files
- Integration with the external_directory permission check from the tools system
src/index.tsx
Outdated
| setPrompt({ | ||
| type: "confirm", | ||
| message: `Allow tool: ${call.tool}?`, |
There was a problem hiding this comment.
The confirmation prompt for tool execution doesn't display the tool arguments, making it difficult for users to make informed decisions about whether to approve a tool call. The prompt only shows "Allow tool: read_file?" without showing what file will be read or where it will write.
Consider enhancing the confirmation message to include relevant tool arguments, such as:
- For
read_file/write_file: show the path - For
run_command: show the command - For
list_dir: show the directory
This would help users make better security decisions, especially for potentially dangerous operations.
src/commandHandlers.ts
Outdated
| } | ||
| ): CommandResult { | ||
| const { settings, setSettings } = context; | ||
| const { settings, setSettings, setMessages, setPrompt } = context; |
There was a problem hiding this comment.
Unused variable setSettings.
| const { settings, setSettings, setMessages, setPrompt } = context; | |
| const { settings, setMessages, setPrompt } = context; |
| const masked = | ||
| options?.shouldMaskValue && typeof displayValue === "string" | ||
| ? displayValue | ||
| ? "***" + displayValue.slice(-4) | ||
| : "Not set" | ||
| : displayValue; |
There was a problem hiding this comment.
The initial value of masked is unused, since it is always overwritten.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 56 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (1)
src/commandHandlers.ts:261
- The
/helpcommand output references a "settings" section but doesn't include information about the new/toolscommand and its sub-commands (on/off/auto/perm). Consider adding a tools section to the help output to document the new functionality.
},
};
}
return {
systemMessage: {
id: generateUniqueId(),
role: "system",
content: "To clear, run: /clear confirm",
timestamp: new Date(),
},
};
}
export function handleHelpCommand(): CommandResult {
const systemMsg: Message = {
id: generateUniqueId(),
role: "system",
content: `Help
general
/help Show help
/clear [confirm] Clear messages
/status Show status
/theme Toggle theme
/terminal-setup Configure terminal keys
settings
/settings Open settings panel
/keybindings [...] View or edit keybindings
ai
/provider <name> Set provider (openai/azure/litellm/custom)
/model <name> Set model
/endpoint <url> Set API base URL
/api-key <key> Set API key
/tools [args] Toggle tool execution (on/off/auto, perm ...)
/af-bridge <url> Set Agent Framework bridge
/af-model <name> Set Agent Framework model
foundry
/login Login to Azure
/foundry-endpoint Set Foundry Project Endpoint
/agent List and select Foundry Agent
workflow
/mode <standard|workflow> Switch mode
/workflow Workflow controls
| const historyForApi = [ | ||
| ...(toolSystemMessage ? [toolSystemMessage] : []), | ||
| ...messagesRef.current.filter( | ||
| (m) => | ||
| !( | ||
| m.role === "system" && | ||
| (m.content.startsWith("Tool ") || m.content.startsWith("Skipped tool")) | ||
| ) | ||
| ), | ||
| toolResultsMessage, | ||
| continueMessage, | ||
| ]; |
There was a problem hiding this comment.
The continueWithToolResults function filters out system messages that start with "Tool " or "Skipped tool" on lines 175-177, but doesn't filter the toolSystemMessage or toolResultsMessage that it adds to the history. This could lead to duplicate system prompts accumulating across multiple tool loops.
| } | ||
|
|
||
| openInputPrompt(); | ||
| return { shouldReturn: true }; | ||
|
|
There was a problem hiding this comment.
The handleSettingCommand function has complex branching logic with multiple return paths. When selectOptions are provided, it opens a prompt and returns early (line 454). When no options are provided, it calls openInputPrompt() and returns (line 457), but then has unreachable code after. Consider restructuring to make the flow clearer and remove dead code.
| } | |
| openInputPrompt(); | |
| return { shouldReturn: true }; | |
| } else { | |
| openInputPrompt(); | |
| return { shouldReturn: true }; | |
| } |
| return; | ||
| } | ||
| ctx.setSettings((prev) => applyProviderDefaults(prev, value)); | ||
| }, | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
The AUTO_SELECT_VALUE constant is defined but only used in the /provider command. The "Auto (detect)" option sets the provider to undefined, which may not be the intended behavior. Consider whether this should clear the provider field or set it to an explicit "auto" string value.
| test("executeToolCall writes and reads files", async () => { | ||
| const dir = mkdtempSync(join(tmpdir(), "qlaw-tools-")); | ||
| const writeResult = await executeToolCall( | ||
| { tool: "write_file", args: { path: "note.txt", content: "hello" } }, | ||
| { cwd: dir, maxFileBytes: 10_000, maxDirEntries: 10, maxOutputChars: 10_000 } | ||
| ); | ||
| expect(writeResult.ok).toBe(true); | ||
|
|
||
| const readResult = await executeToolCall( | ||
| { tool: "read_file", args: { path: "note.txt" } }, | ||
| { cwd: dir, maxFileBytes: 10_000, maxDirEntries: 10, maxOutputChars: 10_000 } | ||
| ); | ||
| expect(readResult.ok).toBe(true); | ||
| expect(readResult.output).toContain("hello"); | ||
|
|
||
| const raw = readFileSync(join(dir, "note.txt"), "utf-8"); | ||
| expect(raw).toBe("hello"); | ||
| }); |
There was a problem hiding this comment.
The maxFileBytes parameter is set to 10,000 but the test writes "hello" (5 bytes). Consider adding a test case that verifies truncation behavior when a file exceeds the maxFileBytes limit.
|
|
||
| if (isDoomLoop) { | ||
| const doomMode = getPermission(settings, "doom_loop"); | ||
| if (doomMode === "deny") return { mode: "deny", reason: "doom-loop" }; | ||
| if (doomMode === "ask") mode = "ask"; | ||
| } | ||
|
|
||
| if (mode === "ask" && settings.tools?.autoApprove) { | ||
| if (tool === "read_file" || tool === "list_dir") { | ||
| mode = "allow"; | ||
| } | ||
| } | ||
|
|
||
| return { mode }; | ||
| } |
There was a problem hiding this comment.
The permission mode "doom_loop" is a great safety feature, but consider adding test coverage for the doom loop detection logic (when the same tool is called 3+ times with identical arguments).
| function inferProvider(baseUrl: string | undefined, model: string | undefined): LlmProvider { | ||
| if (isAzureHost(baseUrl)) return "azure"; | ||
| const host = getHost(baseUrl) || ""; | ||
| const path = getPathname(baseUrl); | ||
| if (host.includes("litellm") || path.includes("/llm/") || (model && model.includes("/"))) { | ||
| // Model slash heuristic covers LiteLLM-style prefixes (openai/, anthropic/, etc.). | ||
| return "litellm"; | ||
| } | ||
| return "litellm"; | ||
| } |
There was a problem hiding this comment.
The function inferProvider always returns "litellm" as the default on line 58, even when the URL doesn't contain litellm-specific patterns. This means any unknown provider will be treated as litellm. Consider returning "openai" as the fallback for standard OpenAI-compatible endpoints, or adding a "custom" provider type for truly unknown endpoints.
| { | ||
| title: "Providers", | ||
| items: [ | ||
| { | ||
| id: "useLitellmEnv", | ||
| label: "Apply LiteLLM env defaults", | ||
| value: "Use LITELLM_*", | ||
| description: "Sets endpoint/model/key from LITELLM_* and clears OpenAI defaults", | ||
| type: "info" as const, | ||
| onActivate: () => setSettings((prev) => applyProviderDefaults(prev, "litellm")), | ||
| }, | ||
| { | ||
| id: "useOpenaiEnv", | ||
| label: "Apply OpenAI env defaults", | ||
| value: "Use OPENAI_*", | ||
| description: "Sets endpoint/model/key from OPENAI_* and clears LiteLLM defaults", | ||
| type: "info" as const, | ||
| onActivate: () => setSettings((prev) => applyProviderDefaults(prev, "openai")), | ||
| }, | ||
| { | ||
| id: "clearOverrides", | ||
| label: "Clear API overrides", | ||
| value: "Reset", | ||
| description: "Clears endpoint/model/key so env defaults apply on next launch", | ||
| type: "info" as const, | ||
| onActivate: () => | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| endpoint: undefined, | ||
| model: undefined, | ||
| apiKey: undefined, | ||
| })), | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
The settingsSections sections now include a new "Providers" section with action items that mutate settings. However, these items have type: "info" which suggests they're informational, but they actually perform actions when activated. Consider using a different type name like "action" or "button" to better reflect their behavior.
| maxOutputChars: number; | ||
| } | ||
|
|
||
| const TOOL_BLOCK_REGEX = /```tool\s*([\s\S]*?)```/gi; |
There was a problem hiding this comment.
The regex pattern /```tool\s*([\s\S]*?)```/gi will match tool blocks anywhere in the text, including within code blocks or other contexts where they shouldn't be treated as tool calls. Consider making the regex more strict to only match tool blocks that appear on their own lines or adding validation to ensure they're not within other fenced code blocks.
| function pushToolCall(raw: any, calls: ToolCall[]) { | ||
| if (!raw || typeof raw !== "object") return; | ||
| const tool = String(raw.tool || raw.name || "").trim(); | ||
| if (!tool) return; | ||
| if (tool !== "read_file" && tool !== "list_dir" && tool !== "write_file" && tool !== "run_command") return; | ||
| calls.push({ tool: tool as ToolName, args: raw.args || raw.parameters || {} }); | ||
| } |
There was a problem hiding this comment.
The pushToolCall function silently ignores tool names that don't match the four supported tools. Consider logging or returning a validation error when an unsupported tool name is encountered, so users understand why their tool call was ignored.
| const masked = | ||
| options?.shouldMaskValue && typeof displayValue === "string" | ||
| ? displayValue | ||
| ? "***" + displayValue.slice(-4) | ||
| : "Not set" | ||
| : displayValue; |
There was a problem hiding this comment.
The initial value of masked is unused, since it is always overwritten.
| const masked = | |
| options?.shouldMaskValue && typeof displayValue === "string" | |
| ? displayValue | |
| ? "***" + displayValue.slice(-4) | |
| : "Not set" | |
| : displayValue; |
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
| const argsObject = call.args || {}; | ||
| const hasArgs = Object.keys(argsObject).length > 0; | ||
| let argsPreview = hasArgs ? JSON.stringify(argsObject, null, 2) : "no arguments"; | ||
| const MAX_PREVIEW_LENGTH = 200; |
There was a problem hiding this comment.
The MAX_PREVIEW_LENGTH constant is defined inline at line 256. For consistency with other configurable limits in the codebase (like maxFileBytes, maxDirEntries), consider moving this to a constant at the top of the file or making it part of the tool settings.
| const TOOL_PERMISSION_MAP: Record<string, string> = { | ||
| read: "read_file", | ||
| read_file: "read_file", | ||
| list: "list_dir", | ||
| list_dir: "list_dir", | ||
| write: "write_file", | ||
| write_file: "write_file", | ||
| run: "run_command", | ||
| run_command: "run_command", | ||
| external: "external_directory", | ||
| external_directory: "external_directory", | ||
| doom: "doom_loop", | ||
| doom_loop: "doom_loop", | ||
| }; |
There was a problem hiding this comment.
The TOOL_PERMISSION_MAP includes multiple keys mapping to the same values (e.g., both "read" and "read_file" map to "read_file"). While this provides flexibility for user input, it's not clear from the code which aliases are canonical. Consider documenting the supported aliases in the command help text or adding a comment explaining why both formats are supported.
| const signature = JSON.stringify({ tool: call.tool, args: call.args || {} }); | ||
| const count = (toolSignatureCountsRef.current[signature] || 0) + 1; | ||
| toolSignatureCountsRef.current[signature] = count; |
There was a problem hiding this comment.
The toolSignatureCountsRef is never reset except when a new user message is submitted. This means that if a tool is called 3+ times across different turns in the same session, it will be flagged as a doom loop even if the calls are legitimate. Consider adding a decay mechanism or resetting counts periodically to avoid false positives in long-running sessions.
| test("returns empty array for undefined or empty", () => { | ||
| expect(parseModelList(undefined)).toEqual([]); | ||
| expect(parseModelList("")).toEqual([]); | ||
| expect(parseModelList(" ")).toEqual([]); |
There was a problem hiding this comment.
The test "returns empty array for undefined or empty" includes a test case with spaces (" "), but the implementation trims the input before checking if it's empty. The test correctly expects an empty array, but adding a comment explaining this edge case would improve test clarity.
| if (!next.model || (env.litellmModel && next.model === env.litellmModel)) { | ||
| next.model = env.openaiModel || next.model; |
There was a problem hiding this comment.
The logic in lines 89-90 has a fallback chain env.openaiModel || next.model, but if both are undefined, it leaves the model as undefined. This could lead to the same "Missing model" error that was fixed in other parts of the codebase. Consider either ensuring a default model is always set or documenting that this is intentional.
| ...(prev.tools || {}), | ||
| permissions: { | ||
| ...(prev.tools?.permissions || {}), | ||
| [toolKey]: rawMode, |
There was a problem hiding this comment.
The /tools perm command doesn't validate that rawMode is properly typed as PermissionMode before assigning it to settings. While the string check ensures it's one of "allow", "ask", or "deny", TypeScript won't recognize this as a type guard. Consider casting the value explicitly: [toolKey]: rawMode as PermissionMode for better type safety.
| export function isExternalPath(path: string, cwd: string): boolean { | ||
| const resolved = resolve(cwd, path); | ||
| const root = resolve(cwd); | ||
| const rel = relative(root, resolved); | ||
| if (!rel) return false; | ||
| return rel.startsWith("..") || isAbsolute(rel); | ||
| } |
There was a problem hiding this comment.
The isExternalPath function has duplicated logic with a different implementation in src/mentionHandlers.ts (line 176). The implementations differ: the tools version checks isAbsolute(rel) while the mentionHandlers version does not. This inconsistency could lead to security issues if one implementation is more permissive than the other. Consider extracting this to a shared utility module to ensure consistent security checks across the codebase.
| if (!config) { | ||
| onError( | ||
| new Error("Missing OPENAI_BASE_URL, OPENAI_API_KEY, or OPENAI_MODEL") | ||
| new Error("Missing endpoint, API key, or model. Configure /provider, /endpoint, /api-key, and /model.") |
There was a problem hiding this comment.
The error message on line 32 suggests using multiple commands (/provider, /endpoint, /api-key, /model) but doesn't provide a clear order or priority for which to configure first. Consider improving the message to guide users through the configuration process more clearly, such as: "Missing LLM configuration. Set provider first with /provider, then configure /endpoint, /api-key, and /model."
| const hasArgs = Object.keys(argsObject).length > 0; | ||
| let argsPreview = hasArgs ? JSON.stringify(argsObject, null, 2) : "no arguments"; | ||
| const MAX_PREVIEW_LENGTH = 200; | ||
| if (argsPreview.length > MAX_PREVIEW_LENGTH) { | ||
| argsPreview = argsPreview.slice(0, MAX_PREVIEW_LENGTH) + "…"; | ||
| } |
There was a problem hiding this comment.
The permission message truncates args to 200 characters with an ellipsis, but this could cut off important information mid-value (e.g., in the middle of a file path or command). Consider truncating at word boundaries or adding a more structured preview format that shows complete key-value pairs up to the limit.
| const fullPath = resolve(cwd, path); | ||
| await mkdir(resolve(fullPath, ".."), { recursive: true }); |
There was a problem hiding this comment.
The write_file tool uses mkdir with { recursive: true } to create parent directories, but it resolves the parent path using resolve(fullPath, ".."). This could potentially create directories outside the intended workspace if fullPath is manipulated. While isExternalPath should catch this in permissions, consider adding an additional safety check before creating directories to ensure they're within the allowed workspace.
Description
Adds LiteLLM provider defaults and settings logic, introduces a permissioned tool loop with @file inlining, and improves command menu interactivity. Updates bridge runbook/docs and bumps JS/Python dependencies.
Type of Change
Related Issues
Fixes #
Related to #
Changes Made
Screenshots (if applicable)
Testing
Test Configuration
Test Cases
bun run typecheck)Checklist
Breaking Changes
Additional Notes
Reviewer Checklist