Feat/ai-extension-builder#382
Conversation
Add "Build Extension with AI" inside the create-extension feature: an in-app orchestrator drives a bundled Claude Agent SDK sidecar that gates feasibility, then scaffolds, codes, builds, and smoke-tests a working extension — with a live progress view, questions surfaced as notifications, and auto-activation on success. Sandboxed Bash allowlist + path-traversal guards; example knowledge fetched live from canonical URLs.
![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 an AI-assisted extension builder (asyar-ext-builder) using the Claude Agent SDK and integrates it into the asyar-launcher Tauri application. The implementation includes feasibility gating, a secure command gate for Bash commands, secret scanning, and frontend views for build progress and extension management. The review feedback highlights opportunities to improve robustness and performance, such as adding safety checks for toolInput to prevent TypeErrors, optimizing sorting operations in both TypeScript and Rust, strengthening path resolution checks, and documenting command injection risks in terminal command generation.
| // tool is permitted (Write/Edit are also covered by acceptEdits). | ||
| canUseTool: async (toolName, toolInput) => { | ||
| if (toolName === 'Bash') { | ||
| const command = typeof toolInput.command === 'string' ? toolInput.command : ''; |
There was a problem hiding this comment.
If toolInput is null or undefined, accessing toolInput.command will throw a TypeError. Add a safety check to ensure toolInput is a non-null object before accessing its properties.
| const command = typeof toolInput.command === 'string' ? toolInput.command : ''; | |
| const command = (toolInput && typeof toolInput === 'object' && typeof toolInput.command === 'string') ? toolInput.command : ''; |
| if (dirs.length > 1) { | ||
| // Most recently modified wins. | ||
| dirs.sort((a, b) => statSync(join(baseDir, b)).mtimeMs - statSync(join(baseDir, a)).mtimeMs); | ||
| return assertSafeExtensionId(dirs[0], baseDir); | ||
| } |
There was a problem hiding this comment.
Calling statSync repeatedly inside dirs.sort is inefficient and unsafe, as it can throw an unhandled exception if a directory is concurrently modified or deleted. Cache the mtimeMs values first, then sort the cached results.
if (dirs.length > 1) {\n const dirsWithTime = dirs.map((name) => {\n try {\n return { name, mtimeMs: statSync(join(baseDir, name)).mtimeMs };\n } catch {\n return { name, mtimeMs: 0 };\n }\n });\n dirsWithTime.sort((a, b) => b.mtimeMs - a.mtimeMs);\n return assertSafeExtensionId(dirsWithTime[0].name, baseDir);\n }| }); | ||
| } | ||
| } | ||
| out.sort_by(|a, b| a.name.to_lowercase().cmp(&b.name.to_lowercase())); |
There was a problem hiding this comment.
Sorting with to_lowercase() on every comparison allocates new strings repeatedly. Use sort_by_cached_key to cache the lowercase keys and avoid redundant allocations.
| out.sort_by(|a, b| a.name.to_lowercase().cmp(&b.name.to_lowercase())); | |
| out.sort_by_cached_key(|a| a.name.to_lowercase()); |
| if cfg!(windows) { ".exe" } else { "" } | ||
| )); | ||
| let candidates = binary_candidates(exe_dir.as_deref(), resource_dir.as_deref(), dev, name); | ||
| resolve_first(&candidates, |p| p.exists()) |
There was a problem hiding this comment.
Use p.is_file() instead of p.exists() to ensure that the resolved path is a file and not accidentally a directory.
| resolve_first(&candidates, |p| p.exists()) | |
| resolve_first(&candidates, |p| p.is_file()) |
| if cfg!(windows) { ".exe" } else { "" } | ||
| )); | ||
| let candidates = binary_candidates(exe_dir.as_deref(), resource_dir.as_deref(), dev, name); | ||
| resolve_first(&candidates, |p| p.exists()) |
There was a problem hiding this comment.
Use p.is_file() instead of p.exists() to ensure that the resolved path is a file and not accidentally a directory.
| resolve_first(&candidates, |p| p.exists()) | |
| resolve_first(&candidates, |p| p.is_file()) |
| * @param dir - Absolute path to cd into before running `command`. | ||
| * @param command - Shell command to run in the new terminal window. | ||
| */ | ||
| export function buildTerminalCommand(plat: string, dir: string, command: string): TerminalCommand { |
There was a problem hiding this comment.
The command parameter is inserted directly into shell strings without escaping. Document this behavior with a JSDoc warning to ensure future callers only pass trusted, hardcoded constants.
/**\n * Builds a terminal launch command for the given platform.\n * \n * @security This function does not escape the `command` parameter. The caller\n * must ensure that `command` is a trusted, hardcoded string and does not contain\n * unsanitized user input to prevent command injection.\n */\nexport function buildTerminalCommand(plat: string, dir: string, command: string): TerminalCommand {
1 participant
No description provided.