feat: custom tools — inject user-defined shell-command tools#648
Closed
y0usaf wants to merge 13 commits into
Closed
feat: custom tools — inject user-defined shell-command tools#648y0usaf wants to merge 13 commits into
y0usaf wants to merge 13 commits into
Conversation
On NixOS with Bun, node-lief fails to load because libstdc++.so.6 is not on the system library path. Previously this produced a generic "Could not extract JS from native binary" error with no hint about the cause or how to fix it. Now nativeInstallationLoader.ts tracks the load error, and installationDetection.ts surfaces it with a NixOS-specific suggestion to use Node.js instead (nix shell nixpkgs#nodejs -c npx tweakcc).
LIEF crashes (SIGSEGV) when parsing large ELF Bun binaries (~237MB). On Linux/NixOS, Bun stores its data as a plain file overlay — no ELF section parsing needed. This adds raw file-based extraction and repacking for ELF, skipping LIEF entirely for that format. LIEF is still used for Mach-O and PE where it reads actual binary sections that can't be found by tail-reading alone.
- isELFFile: use try/finally to prevent fd leak; validate bytesRead - extractBunDataFromELFRaw: correct docblock (uses BunOffsets.byteCount, not footer) - repackELFRaw: derive elfSize from BunOffsets.byteCount instead of the unreliable trailing u64 footer - nativeInstallationLoader: clear loadError on successful module load - installationDetection: tighten NixOS hint to only trigger on shared-object errors (drop the over-broad 'No such file' match) - config.test: add two tests covering enriched error message paths
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class customTools types/defaults and a patch that generates and injects runtime custom tool objects into Claude Code bundles (two injection strategies) with comprehensive tests; validates config customTools entries; and implements ELF raw Bun extraction/repack plus native module load-error reporting. Changes
Sequence DiagramsequenceDiagram
participant User
participant Config
participant Patcher
participant Bundle
participant Executor
User->>Config: add settings.customTools
Config->>Patcher: provide validated customTools
Patcher->>Patcher: detect buildTool & cwd helper names
Patcher->>Patcher: generate minified tool objects (schemas, validators, renderers, call)
Patcher->>Bundle: inject generated tool objects (Strategy A or B)
User->>Bundle: trigger tool usage
Bundle->>Executor: tool.call(params)
Executor->>Executor: validate input, template command, set cwd/env/timeout
Executor->>Executor: run spawnSync and collect result
Executor-->>Bundle: return execution result
Bundle-->>User: render tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
y0usaf
force-pushed
the
feat/custom-tools
branch
from
b07b3e0 to
944035a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/tests/config.test.ts (1)
1528-1610: Add a non-shared-object load-error case here too.Both new tests use the same
cannot open shared objectmessage, so they only cover the branch that appends the NixOS hint. Please add one generic load failure case to prove we still include the load reason without the Nix-specific note.As per coding guidelines,
**/*.test.{ts,tsx}: Test edge cases and error conditions in test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/config.test.ts` around lines 1528 - 1610, Add a third test case similar to the two existing ones that stubs nativeInstallation.getNativeModuleLoadError to return a non-shared-object message (e.g. "unexpected header in node-lief") and otherwise sets up the same mocks (whichMock, fs.stat/realpath/open, lstatSpy, mockMagicInstance.detect, nativeInstallation.extractClaudeJsFromNativeInstallation) and then assert findClaudeCodeInstallation(mockConfig, { interactive: true }) rejects with an error that includes that exact load message (verifying findClaudeCodeInstallation and nativeInstallation.getNativeModuleLoadError handling without triggering the NixOS hint).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.ts`:
- Around line 223-229: The current mapping of config.settings.customTools uses
deepMergeWithDefaults(tool, DEFAULT_CUSTOM_TOOL), which backfills required
execution fields (e.g., command) and turns malformed entries into executable
placeholders; update the logic in the config.settings.customTools handling to
first validate that each tool contains required properties (such as command and
any other required metadata) and either reject/drop the entry or throw a config
error, and only apply defaults for optional fields (merge only optional keys)
for valid tools; reference the mapping that calls deepMergeWithDefaults and the
DEFAULT_CUSTOM_TOOL constant and replace it with a two-step process: validate
required fields on each tool, then selectively merge optional defaults for the
validated tool.
In `@src/nativeInstallation.ts`:
- Around line 791-799: The current branch unconditionally uses
extractBunDataFromELFRaw for any ELF (isELFFile) which breaks section-based ELF
builds; update the logic in the nativeInstallationPath handling so you first
detect the legacy overlay footer (or attempt raw parsing and check its success)
before assigning ({ bunOffsets, bunData, moduleStructSize } =
extractBunDataFromELFRaw(nativeInstallationPath)); if the raw helper fails or
the footer is not present, fall back to the LIEF path that calls
LIEF.parse(nativeInstallationPath) and getBunData(binary) (and ensure
repackELFSection remains used for section-based ELFs) so only true
legacy-overlay ELFs use the raw helpers.
In `@src/patches/customTools.ts`:
- Around line 71-72: The code currently JSON-serializes tool.shell (shellJson)
but call() always uses the POSIX '-c' flag, which breaks non-POSIX shells (e.g.,
cmd.exe). Fix by either validating tool.shell in the parsing/creation path
(reject or normalize non-POSIX shells) or, preferably, add an explicit
shellExecFlag option (e.g., shellExecFlag or execFlag) to the tool config and
use that in call() instead of hardcoding '-c'; implement a default mapping for
common shells (bash/sh -> '-c', cmd.exe -> '/c', powershell -> '-Command') and
fall back to a validation/error if unknown, and update all call() usages
(including the locations referenced around lines 145-150) to read the exec flag
instead of assuming '-c'.
- Around line 92-97: Create a single, reusable escaped substitution template and
use it everywhere instead of duplicating logic: replace the current per-site
generation (the substitutions const, the inline replacement in
renderToolUseMessage, and the uses at the later sites) with one variable like
escapedSubstitution that (1) uses a safe regex construction (escape the
parameter name before embedding it into a RegExp or build the RegExp with new
RegExp(...)) to avoid invalid regex for names like "foo/bar", and (2) uses a
consistent fallback (pick one fallback, e.g. "?" or "" and apply it everywhere).
Add or reuse a small helper escapeRegExp(name: string): string if needed, then
reference escapeRegExp(k) when building the regex and use the same fallback
expression (String(args[KEY] ?? FALLBACK)) in substitutions,
renderToolUseMessage, and the two reuse sites so the displayed and executed
commands match.
---
Nitpick comments:
In `@src/tests/config.test.ts`:
- Around line 1528-1610: Add a third test case similar to the two existing ones
that stubs nativeInstallation.getNativeModuleLoadError to return a
non-shared-object message (e.g. "unexpected header in node-lief") and otherwise
sets up the same mocks (whichMock, fs.stat/realpath/open, lstatSpy,
mockMagicInstance.detect,
nativeInstallation.extractClaudeJsFromNativeInstallation) and then assert
findClaudeCodeInstallation(mockConfig, { interactive: true }) rejects with an
error that includes that exact load message (verifying
findClaudeCodeInstallation and nativeInstallation.getNativeModuleLoadError
handling without triggering the NixOS hint).
🪄 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: 266a94af-94be-48b9-9740-12268989e223
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/config.tssrc/defaultSettings.tssrc/installationDetection.tssrc/nativeInstallation.tssrc/nativeInstallationLoader.tssrc/patches/customTools.tssrc/patches/index.tssrc/tests/config.test.tssrc/types.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/patches/customTools.ts (2)
145-150:⚠️ Potential issue | 🟠 MajorThe
-cflag is POSIX-specific; non-POSIX shells will fail.The
shellfield is configurable (e.g., users could specifycmd.exeorpowershell), but the execution always passes["-c", cmd]. This breaks on Windows shells:cmd.exerequires/c, PowerShell requires-Command.Either validate
shellto POSIX-compatible shells only, or make the exec flag configurable (e.g., add ashellExecFlagfield).🛠️ Option 1: Validate shell to POSIX-only
Add validation in
src/config.tswhen merging custom tools:const ALLOWED_SHELLS = ['sh', 'bash', 'zsh', 'fish', 'dash', 'ash']; if (tool.shell && !ALLOWED_SHELLS.includes(tool.shell)) { console.warn(`Custom tool "${tool.name}": shell "${tool.shell}" is not supported. Using "sh".`); tool.shell = 'sh'; }🛠️ Option 2: Add shellExecFlag to CustomTool interface
Extend the interface in
src/types.ts:export interface CustomTool { // ... existing fields ... /** * Flag passed to shell before the command. Defaults to "-c" for POSIX shells. * Use "/c" for cmd.exe, "-Command" for PowerShell. */ shellExecFlag?: string; }Then use it in the generated code:
-const result=spawnSync(${shellJson},["-c",cmd],{ +const result=spawnSync(${shellJson},[${JSON.stringify(tool.shellExecFlag ?? '-c')},cmd],{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 145 - 150, Add a configurable exec-flag instead of hardcoding ["-c", cmd]: extend the CustomTool type to include an optional shellExecFlag (e.g., in src/types.ts/CustomTool) and update the code that builds args for spawnSync (see spawnSync call using shellJson and cmd in src/patches/customTools.ts) to use tool.shellExecFlag when present, otherwise default to "-c" for POSIX shells; also add a small mapping/validation for common shells (cmd.exe -> "/c", powershell -> "-Command") and fall back to "-c" or warn if unknown so non-POSIX shells no longer break.
92-105:⚠️ Potential issue | 🟠 MajorParameter names must be escaped before embedding in regex patterns.
Parameter names are interpolated directly into regex literals without escaping. A parameter named
foo/barorparam.namewill produce invalid regex syntax (e.g.,/\{\{foo/bar\}\}/g) and throw at runtime.Additionally, the fallback values differ: line 95 uses
""for the executed command while line 104 uses"?"for the UI preview. This causes command drift where the user approves a different command than what executes.🛠️ Proposed fix: escape parameter names and unify fallbacks
+// Escape special regex characters in parameter names +const escapeRegex = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + // Build parameter substitution code: one replace per param const substitutions = Object.keys(tool.parameters) .map( k => - `cmd=cmd.replace(/\\{\\{${k}\\}\\}/g,String(args[${JSON.stringify(k)}]??""));` + `cmd=cmd.replace(/\\{\\{${escapeRegex(k)}\\}\\}/g,String(args[${JSON.stringify(k)}]??""));` ) .join(''); // renderToolUseMessage: show the tool name and interpolated command so the // user can see what's about to run before approving. const renderToolUseMessage = `(input)=>{ let cmd=${cmdJson}; ${Object.keys(tool.parameters) - .map(k => `try{cmd=cmd.replace(/\\{\\{${k}\\}\\}/g,String(input[${JSON.stringify(k)}]??"?"));}catch(_){}`) + .map(k => `try{cmd=cmd.replace(/\\{\\{${escapeRegex(k)}\\}\\}/g,String(input[${JSON.stringify(k)}]??""));}catch(_){}`) .join('')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 92 - 105, The regex interpolation for parameter keys in the substitutions string and in renderToolUseMessage is unsafe: escape each parameter name before embedding into the /.../g regex (e.g., use an escapeRegex helper to backslash-escape regex metacharacters for keys from tool.parameters) and make the fallback value consistent between the executed command and the preview (use the same fallback—prefer ""—for both the substitutions (args) and renderToolUseMessage (input)); update references to substitutions generation and the renderToolUseMessage construction (they use cmdJson, args, input, and tool.parameters) so both use escaped keys and identical fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/patches/customTools.ts`:
- Around line 145-150: Add a configurable exec-flag instead of hardcoding ["-c",
cmd]: extend the CustomTool type to include an optional shellExecFlag (e.g., in
src/types.ts/CustomTool) and update the code that builds args for spawnSync (see
spawnSync call using shellJson and cmd in src/patches/customTools.ts) to use
tool.shellExecFlag when present, otherwise default to "-c" for POSIX shells;
also add a small mapping/validation for common shells (cmd.exe -> "/c",
powershell -> "-Command") and fall back to "-c" or warn if unknown so non-POSIX
shells no longer break.
- Around line 92-105: The regex interpolation for parameter keys in the
substitutions string and in renderToolUseMessage is unsafe: escape each
parameter name before embedding into the /.../g regex (e.g., use an escapeRegex
helper to backslash-escape regex metacharacters for keys from tool.parameters)
and make the fallback value consistent between the executed command and the
preview (use the same fallback—prefer ""—for both the substitutions (args) and
renderToolUseMessage (input)); update references to substitutions generation and
the renderToolUseMessage construction (they use cmdJson, args, input, and
tool.parameters) so both use escaped keys and identical fallbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 422acb21-ad54-4746-bd4b-8a2177b98917
📒 Files selected for processing (5)
src/config.tssrc/defaultSettings.tssrc/patches/customTools.tssrc/patches/index.tssrc/types.ts
✅ Files skipped from review due to trivial changes (2)
- src/config.ts
- src/patches/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/defaultSettings.ts
- src/types.ts
Uses CC's own buildTool() factory (detected via regex in the minified bundle) so custom tools are constructed the same way every native CC tool is. Rendering uses React.createElement with the detected Text/Box component variables rather than plain strings. Permission checking delegates to BashTool so custom tool commands match the user's existing Bash allow/deny rules. New helpers in helpers.ts: - getCwdFuncName: detects the minified getCwd() for correct session cwd - findBuildToolFunc: detects the minified buildTool() factory function Patch strategies (same as before): - Strategy A: post-toolsets injection (appends after toolset filter) - Strategy B: direct spread into the tool aggregation array
y0usaf
force-pushed
the
feat/custom-tools
branch
from
944035a to
05167f3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/patches/customTools.ts (2)
194-199:⚠️ Potential issue | 🟠 MajorMake the exec flag follow the configured shell.
tool.shellis configurable, butspawnSync()always receives['-c', cmd]. That only works for POSIX shells;cmd.exeneeds/c, PowerShell needs-Command, etc. Either validatetool.shelldown to POSIX shells or carry an exec-flag mapping/config alongside it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 194 - 199, The current spawnSync invocation in customTools.ts always uses ['-c', cmd], which breaks non-POSIX shells; update the code that builds the spawnSync args (around spawnSync, cmd, and tool.shell) to choose the correct exec flag based on tool.shell (e.g., map 'sh'/'bash' -> '-c', 'cmd.exe' -> '/c', 'pwsh'/'powershell' -> '-Command') or validate/restrict tool.shell to POSIX only; replace the hard-coded ['-c', cmd] with a computed args array (using the mapping or validation) so spawnSync(spawn, args, ...) passes the proper flag for the configured shell and preserve existing env, cwd, timeout, and encoding behavior.
118-130:⚠️ Potential issue | 🟠 MajorEscape placeholder keys and reuse one substitution path.
Execution/classifier/permissions substitute missing params with
"", but the approval UI uses"?", so the user can approve a different command from the one that actually runs. Also,kis embedded directly into a regex literal, so keys likea.bover-match andfoo/barwill generate invalid JS in the patched bundle. Build one escapednew RegExp(...)template and reuse it everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 118 - 130, The substitution helpers (makeSubst, argsSubst, inputSubst, safeInputSubst) embed raw parameter keys into regex literals and use different missing-value defaults ("" vs "?"), causing over-matching/invalid JS and mismatched preview vs runtime; fix by building a single escaped RegExp-based substitution template (escape special regex chars in each key and construct new RegExp(escapedKey,'g')) and reuse that same substitution code path for args and input and for safeInputSubst, and change the safe/preview missing-param replacement to the same empty-string behavior used by execution/classifier/permissions so the rendered approval command matches the actual runtime command.
🧹 Nitpick comments (2)
src/patches/customTools.test.ts (1)
117-374: Add regressions for prompt overrides and non-identifier parameter names.The suite covers the happy path well, but two brittle codegen branches still have no direct coverage:
tool.promptbypassing auto-prompt generation, and parameter keys that need regex escaping (for examplea.borfoo/bar). One focused test for each would catch future breakage quickly.As per coding guidelines "Test edge cases and error conditions in test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.test.ts` around lines 117 - 374, Add two tests to the writeCustomTools suite to cover the missing branches: (1) a test that verifies when a CustomTool has an explicit prompt (tool.prompt) the generator uses it rather than auto-generating one—call writeCustomTools with a tool containing a prompt string and assert the generated result contains that prompt text (reference writeCustomTools and tool.prompt); (2) a test that verifies parameter keys with non-identifier characters (e.g. "a.b" and "foo/bar") are handled/escaped correctly—construct a CustomTool whose parameters include those keys, run writeCustomTools, and assert the output contains those exact keys or the correctly-escaped/quoted form (reference writeCustomTools and the parameter handling/regex-escaping logic). Ensure both tests stub console.warn/error like the existing tests and restore spies in finally blocks.src/patches/customTools.ts (1)
309-331: Tighten both injection regexes to the emitted minified shape.These patterns are much looser than the rest of the patch set. For bundle patches,
\s*-tolerant matching makes it easier to hit the wrongelse/aggregation site or to pass tests against prettified fixtures while missing the real minified target.Based on learnings "In patches under src/patches/, assume code works with minified, original Claude Code installations. Patch regexes must be strict and match the minified format exactly (ignore formatting differences like whitespace/newlines)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 309 - 331, The current loose regexes toolsetsPattern and originalPattern allow whitespace and can match non-minified/prettified code; tighten them to the exact minified emission shape (no \s* gaps) so we only match sequences like "}else{VAR=CALL; }let" and "letVAR=CALL," in the minified bundle. Replace toolsetsPattern with a strict form that disallows whitespace between tokens (e.g. match "}" "else" "{" then capture the aggregation var and the assemble-call ending with ";" before "let") and replace originalPattern with a strict "let([$\w]+)=([$\w]+\([$\w]+,[$\w]+\.tools,[$\w]+\))," style regex; keep using toolAggVar, toolsArrayCode and the same insertAt/injection logic after updating the patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/customTools.ts`:
- Around line 255-262: The loop currently only checks custom tool names against
BUILTIN_TOOL_NAMES; additionally detect duplicate names among the customTools
array itself (e.g., using a Set to track seen tool.name values) and treat
duplicates as an error: when a duplicate is found log a similar error message
(referencing customTools and the conflicting tool.name) and return null to abort
injection; update the check around the for (const tool of customTools) loop that
references BUILTIN_TOOL_NAMES and tool.name to include this duplicate-name guard
so no two user-defined tools with the same name are injected.
---
Duplicate comments:
In `@src/patches/customTools.ts`:
- Around line 194-199: The current spawnSync invocation in customTools.ts always
uses ['-c', cmd], which breaks non-POSIX shells; update the code that builds the
spawnSync args (around spawnSync, cmd, and tool.shell) to choose the correct
exec flag based on tool.shell (e.g., map 'sh'/'bash' -> '-c', 'cmd.exe' -> '/c',
'pwsh'/'powershell' -> '-Command') or validate/restrict tool.shell to POSIX
only; replace the hard-coded ['-c', cmd] with a computed args array (using the
mapping or validation) so spawnSync(spawn, args, ...) passes the proper flag for
the configured shell and preserve existing env, cwd, timeout, and encoding
behavior.
- Around line 118-130: The substitution helpers (makeSubst, argsSubst,
inputSubst, safeInputSubst) embed raw parameter keys into regex literals and use
different missing-value defaults ("" vs "?"), causing over-matching/invalid JS
and mismatched preview vs runtime; fix by building a single escaped RegExp-based
substitution template (escape special regex chars in each key and construct new
RegExp(escapedKey,'g')) and reuse that same substitution code path for args and
input and for safeInputSubst, and change the safe/preview missing-param
replacement to the same empty-string behavior used by
execution/classifier/permissions so the rendered approval command matches the
actual runtime command.
---
Nitpick comments:
In `@src/patches/customTools.test.ts`:
- Around line 117-374: Add two tests to the writeCustomTools suite to cover the
missing branches: (1) a test that verifies when a CustomTool has an explicit
prompt (tool.prompt) the generator uses it rather than auto-generating one—call
writeCustomTools with a tool containing a prompt string and assert the generated
result contains that prompt text (reference writeCustomTools and tool.prompt);
(2) a test that verifies parameter keys with non-identifier characters (e.g.
"a.b" and "foo/bar") are handled/escaped correctly—construct a CustomTool whose
parameters include those keys, run writeCustomTools, and assert the output
contains those exact keys or the correctly-escaped/quoted form (reference
writeCustomTools and the parameter handling/regex-escaping logic). Ensure both
tests stub console.warn/error like the existing tests and restore spies in
finally blocks.
In `@src/patches/customTools.ts`:
- Around line 309-331: The current loose regexes toolsetsPattern and
originalPattern allow whitespace and can match non-minified/prettified code;
tighten them to the exact minified emission shape (no \s* gaps) so we only match
sequences like "}else{VAR=CALL; }let" and "letVAR=CALL," in the minified bundle.
Replace toolsetsPattern with a strict form that disallows whitespace between
tokens (e.g. match "}" "else" "{" then capture the aggregation var and the
assemble-call ending with ";" before "let") and replace originalPattern with a
strict "let([$\w]+)=([$\w]+\([$\w]+,[$\w]+\.tools,[$\w]+\))," style regex; keep
using toolAggVar, toolsArrayCode and the same insertAt/injection logic after
updating the patterns.
🪄 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: e6bcf5c7-6cc9-43c9-afb6-ed1ee13c8b55
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
src/defaultSettings.tssrc/installationDetection.tssrc/nativeInstallation.tssrc/nativeInstallationLoader.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/tests/config.test.tssrc/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/patches/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/defaultSettings.ts
- src/types.ts
- customTools.ts: escape param names in regex literals to handle special chars - customTools.ts: use consistent empty-string fallback in all substitution sites - customTools.ts: detect duplicate custom tool names (not just built-in collisions) - config.ts: validate required CustomTool fields when loading config; drop invalid entries - nativeInstallation.ts: gate raw ELF path on legacy footer detection; fall back to LIEF - config.test.ts: fix broken tests (use vi.spyOn for getNativeModuleLoadError) - config.test.ts: add test for generic (non-shared-object) load error without NixOS hint
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/patches/customTools.ts (2)
133-134:⚠️ Potential issue | 🟠 Major
tool.shellis broader than the hardcoded-cexecution path.
spawnSync(shell, ['-c', cmd])only works for POSIX-style shells.
Values likecmd.exe,powershell, orpwshneed different flags, so
those configs are accepted but won't execute correctly. Either validate
tool.shellto POSIX shells only, or pair it with an exec-flag mapping.♻️ Proposed fix
- const shellJson = JSON.stringify(tool.shell ?? 'sh'); + const shell = tool.shell ?? 'sh'; + const execFlag = + /^(cmd(?:\.exe)?)$/i.test(shell) + ? '/c' + : /^(powershell(?:\.exe)?|pwsh)$/i.test(shell) + ? '-Command' + : '-c'; + const shellJson = JSON.stringify(shell); ... - const result=spawnSync(${shellJson},["-c",cmd],{ + const result=spawnSync(${shellJson},[${JSON.stringify(execFlag)},cmd],{Also applies to: 244-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 133 - 134, The code currently stringifies tool.shell into shellJson and then runs spawnSync(shell, ['-c', cmd]) which only works for POSIX shells; update the execution to derive the correct execution flag from the shell value (e.g., '-c' for sh/bash, '/c' for cmd.exe, '-Command' or '-c' for PowerShell variants) by adding a small helper like getShellExecFlag(shell) and use spawnSync(shell, [execFlag, cmd]); validate the returned execFlag (throw or log and skip if unknown) so configs like cmd.exe, powershell, and pwsh execute correctly; apply the same change where shellJson/timeoutVal and spawnSync are used elsewhere (the other block referenced around the later range).
156-179:⚠️ Potential issue | 🟠 MajorStop emitting regex literals for placeholder replacement.
escapeForRegex()still leaves/untouched, so a parameter like
foo/barproduces invalid generated code
(/\{\{foo/bar\}\}/g) and can break the patched bundle. Build the
matcher withnew RegExp(...)instead of a regex literal, or escape the
literal delimiter too.♻️ Proposed fix
const escapeForRegex = (s: string): string => s.replace(/[.*+?^${}()|[\]\\]/g, '\\\\$&'); + const makeRegex = (name: string): string => + `new RegExp(${JSON.stringify(`\\{\\{${escapeForRegex(name)}\\}\\}`)},'g')`; const makeSubst = (varName: string): string => Object.keys(tool.parameters) .map( k => - `cmd=cmd.replace(/\\{\\{${escapeForRegex(k)}\\}\\}/g,String(${varName}[${JSON.stringify(k)}]??""));` + `cmd=cmd.replace(${makeRegex(k)},String(${varName}[${JSON.stringify(k)}]??""));` ) .join(''); const safeInputSubst = Object.keys(tool.parameters) .map( k => - `try{cmd=cmd.replace(/\\{\\{${escapeForRegex(k)}\\}\\}/g,String(input[${JSON.stringify(k)}]??""));}catch(_){}` + `try{cmd=cmd.replace(${makeRegex(k)},String(input[${JSON.stringify(k)}]??""));}catch(_){}` ) .join('');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` around lines 156 - 179, The generated replacement currently emits regex literals (see makeSubst, argsSubst, inputSubst, safeInputSubst and escapeForRegex) which fails for parameter names containing '/'; stop emitting regex literals and build matchers with the RegExp constructor instead. Update the template generation in makeSubst and the safeInputSubst mapping to produce code that calls new RegExp(...) with a properly escaped pattern string (escapeForRegex should either also escape '/' or you should escape it when interpolating into the string passed to RegExp) and the 'g' flag, and keep the same String(...??"") fallback logic; ensure the same change is applied where safeInputSubst currently emits try/catch per param.
🧹 Nitpick comments (1)
src/nativeInstallation.ts (1)
1520-1541: Consider extracting atomic file write logic to reduce duplication.The temp-file-then-rename pattern with busy-file error handling is duplicated from
atomicWriteBinary. A shared helper could simplify maintenance.♻️ Example extraction
function atomicWriteBuffer( buffer: Buffer, outputPath: string, originalPath: string ): void { const tempPath = outputPath + '.tmp'; fs.writeFileSync(tempPath, buffer); const origStat = fs.statSync(originalPath); fs.chmodSync(tempPath, origStat.mode); try { fs.renameSync(tempPath, outputPath); } catch (error) { try { fs.unlinkSync(tempPath); } catch { // ignore cleanup errors } if ( error instanceof Error && 'code' in error && (error.code === 'ETXTBSY' || error.code === 'EBUSY' || error.code === 'EPERM') ) { throw new Error( 'Cannot update the Claude executable while it is running.\n' + 'Please close all Claude instances and try again.' ); } throw error; } }Then
repackELFRawbecomes:- const tempPath = outputPath + '.tmp'; - fs.writeFileSync(tempPath, newBinary); - const origStat = fs.statSync(binPath); - fs.chmodSync(tempPath, origStat.mode); - try { - fs.renameSync(tempPath, outputPath); - } catch (error) { - try { - fs.unlinkSync(tempPath); - } catch { - // ignore cleanup errors - } - if ( - error instanceof Error && - 'code' in error && - (error.code === 'ETXTBSY' || - error.code === 'EBUSY' || - error.code === 'EPERM') - ) { - throw new Error( - 'Cannot update the Claude executable while it is running.\n' + - 'Please close all Claude instances and try again.' - ); - } - throw error; - } + atomicWriteBuffer(newBinary, outputPath, binPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nativeInstallation.ts` around lines 1520 - 1541, The temp-file-then-rename pattern and busy-file error handling in repackELFRaw is duplicated from atomicWriteBinary; extract this into a shared helper (e.g., atomicWriteBuffer or atomicReplaceFile) that takes the Buffer (or source path), outputPath and originalPath, performs write to tempPath, copies mode from original via fs.statSync+fs.chmodSync, attempts fs.renameSync and on failure unlinks tempPath and maps ETXTBSY/EBUSY/EPERM to the existing "Cannot update the Claude executable while it is running" Error; then replace the duplicated logic in repackELFRaw (and any other caller) to call the new helper and remove the duplicated try/catch block around renameSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.ts`:
- Around line 230-245: The validation assumes config.settings.customTools is a
well-formed array of objects and uses JSON.stringify(tool) which can leak
secrets; instead first guard that config.settings.customTools is an Array (if
not, set to []), then iterate with index (e.g., replace the .filter call) and
for each entry validate types explicitly: ensure typeof tool === "object" &&
tool !== null, typeof tool.name === "string" && tool.name.trim() !== "", typeof
tool.command === "string", typeof tool.description === "string", and that
tool.parameters is the expected shape (use Array.isArray(tool.parameters) or
typeof tool.parameters === "object" depending on what writeCustomTools()
expects). For entries that fail, log only the index, the tool.name (if present),
and a list of the invalid/missing keys (do not JSON.stringify the whole tool),
and skip/drop those entries so writeCustomTools() receives only well-typed
tools.
In `@src/tests/config.test.ts`:
- Around line 1562-1568: The test currently only checks the generic error
prefix; update the assertion so it verifies the concrete load reason returned by
the mock (e.g. assert the error message contains the unique substring
"libstdc++.so.6") by changing the expectation on findClaudeCodeInstallation
(which is mocked via vi.spyOn(nativeInstallation, 'getNativeModuleLoadError')
and uses mockConfig) to check for that specific substring (use rejects.toThrow
with the full/partial string or rejects.toMatch if preferred) so the test fails
if the detailed reason is dropped.
---
Duplicate comments:
In `@src/patches/customTools.ts`:
- Around line 133-134: The code currently stringifies tool.shell into shellJson
and then runs spawnSync(shell, ['-c', cmd]) which only works for POSIX shells;
update the execution to derive the correct execution flag from the shell value
(e.g., '-c' for sh/bash, '/c' for cmd.exe, '-Command' or '-c' for PowerShell
variants) by adding a small helper like getShellExecFlag(shell) and use
spawnSync(shell, [execFlag, cmd]); validate the returned execFlag (throw or log
and skip if unknown) so configs like cmd.exe, powershell, and pwsh execute
correctly; apply the same change where shellJson/timeoutVal and spawnSync are
used elsewhere (the other block referenced around the later range).
- Around line 156-179: The generated replacement currently emits regex literals
(see makeSubst, argsSubst, inputSubst, safeInputSubst and escapeForRegex) which
fails for parameter names containing '/'; stop emitting regex literals and build
matchers with the RegExp constructor instead. Update the template generation in
makeSubst and the safeInputSubst mapping to produce code that calls new
RegExp(...) with a properly escaped pattern string (escapeForRegex should either
also escape '/' or you should escape it when interpolating into the string
passed to RegExp) and the 'g' flag, and keep the same String(...??"") fallback
logic; ensure the same change is applied where safeInputSubst currently emits
try/catch per param.
---
Nitpick comments:
In `@src/nativeInstallation.ts`:
- Around line 1520-1541: The temp-file-then-rename pattern and busy-file error
handling in repackELFRaw is duplicated from atomicWriteBinary; extract this into
a shared helper (e.g., atomicWriteBuffer or atomicReplaceFile) that takes the
Buffer (or source path), outputPath and originalPath, performs write to
tempPath, copies mode from original via fs.statSync+fs.chmodSync, attempts
fs.renameSync and on failure unlinks tempPath and maps ETXTBSY/EBUSY/EPERM to
the existing "Cannot update the Claude executable while it is running" Error;
then replace the duplicated logic in repackELFRaw (and any other caller) to call
the new helper and remove the duplicated try/catch block around renameSync.
🪄 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: ecfb591c-710c-4ab1-9359-8f6dbf77dc0d
📒 Files selected for processing (4)
src/config.tssrc/nativeInstallation.tssrc/patches/customTools.tssrc/tests/config.test.ts
Contributor
Author
|
Addressed all review comments:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/customTools.ts`:
- Around line 67-70: The generatePromptString function currently treats a falsy
prompt as missing, so a deliberate empty string prompt gets ignored; change the
check to explicitly test for undefined (e.g., if (tool.prompt !== undefined)) so
that an explicit '' is honored and returned; update the logic in
generatePromptString that references CustomTool.prompt to return tool.prompt
when it is defined rather than when it is truthy.
- Around line 218-236: The generated handlers (inputSchema.safeParse/parse,
toAutoClassifierInput, checkPermissions, and validateInput) assume input/args
are objects/arrays and can throw on null/undefined; ensure you normalize at the
top of those functions by coercing input to a safe default (e.g., input = input
?? {} and const args = Array.isArray(input.args) ? input.args : []) before any
indexing or substitutions, and update toAutoClassifierInput and checkPermissions
to use the normalized variables (refer to the functions/methods named
inputSchema:{safeParse,parse}, toAutoClassifierInput, checkPermissions, and
validateInput) so validateInput can return structured errors instead of runtime
TypeErrors.
- Around line 175-193: makeSubst and safeInputSubst currently pass a string to
String.prototype.replace which allows $-sequences in parameter values to be
interpreted; change the generated replacement to use a function callback so
values are inserted literally. Update makeSubst's generation from
`cmd=cmd.replace(/\\{\\{${escapeForRegex(k)}\\}\\}/g,String(${varName}[${JSON.stringify(k)}]??""));`
to use a replacer function like
`cmd=cmd.replace(/\\{\\{${escapeForRegex(k)}\\}\\}/g,()=>String(${varName}[${JSON.stringify(k)}]??""));`
and change each entry in safeInputSubst similarly from
`cmd=cmd.replace(...,String(input[...]))` to
`cmd=cmd.replace(...,()=>String(input[...]??""));` so argsSubst, inputSubst and
safeInputSubst all use function callbacks and avoid `$`-sequence interpretation.
🪄 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: f51ed78f-4543-4bf3-94d2-79ccfe7c83a3
📒 Files selected for processing (3)
src/config.tssrc/patches/customTools.tssrc/tests/config.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/config.ts
- src/tests/config.test.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/patches/customTools.test.ts (2)
419-436: Test title doesn't match what's being asserted forcall.The title says "returns structured errors for non-object input and args", but the
call(undefined)assertion is verifying graceful handling (empty stdout/stderr, exit 0 from the mocked spawn), not a structured error. Only thevalidateInput(null)assertion actually checks the structured-error shape. Consider splitting into two tests, or renaming to reflect the combined behavior (e.g. "rejects non-object input invalidateInputand normalizes non-object args incall").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.test.ts` around lines 419 - 436, Rename or split the test so its title matches the assertions: either split into two tests—one that verifies validateInput(null) returns the structured error (keep title like "returns structured errors for non-object input") and a second that asserts toAutoClassifierInput(undefined) and call(undefined) normalize non-object args and produce the mocked spawnSync result (title like "normalizes non-object args for call and spawns command"); locate the assertions around validateInput, toAutoClassifierInput, call, buildGeneratedTool and MINIMAL_TOOL and move or rename them accordingly so each test's name matches its verified behavior, and ensure spawnSync expectation remains in the test that verifies call.
207-220: Add coverage for the duplicate custom-tool-name guard.The collision-guard suite only exercises the built-in name path. The
seenToolNamescheck added incustomTools.ts(lines 332–338) has no corresponding assertion, so a regression in that branch would go undetected.♻️ Suggested addition
describe('collision guard', () => { it('returns null and logs error for a built-in tool name', () => { ... }); + + it('returns null and logs error for duplicate custom tool names', () => { + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const result = writeCustomTools(MOCK_STRATEGY_B, [ + MINIMAL_TOOL, + { ...MINIMAL_TOOL }, + ]); + expect(result).toBeNull(); + expect(err).toHaveBeenCalledWith( + expect.stringContaining('duplicate') + ); + } finally { + err.mockRestore(); + } + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.test.ts` around lines 207 - 220, Add a new test in the "collision guard" suite that exercises the duplicate custom-tool-name branch introduced by the seenToolNames check: call writeCustomTools with a strategy (e.g., MOCK_STRATEGY_B) and an array containing two custom tools that share the same name (e.g., two copies of MINIMAL_TOOL with identical name), spy on console.error, assert that writeCustomTools returns null and that console.error was called with a message containing the duplicate tool name, and finally restore the spy; reference writeCustomTools and the seenToolNames behavior to locate where this guard should be covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/customTools.ts`:
- Around line 168-184: The generated regex-escaping currently double-escapes
backslashes and omits '/', breaking parameter names with characters like '.' '$'
or '/'; update escapeForRegex so it escapes forward slash and returns
single-backslash escapes (i.e. change the character class to include '/' and
replace with a single backslash escape, not a double one) so that makeSubst
produces valid regex literals for substitutions; update any uses in makeSubst
(which builds cmd.replace(/{{...}}/g,...)) to rely on the fixed escapeForRegex,
and add a unit test exercising param names with '.', '$' and '/' to prevent
regressions.
---
Nitpick comments:
In `@src/patches/customTools.test.ts`:
- Around line 419-436: Rename or split the test so its title matches the
assertions: either split into two tests—one that verifies validateInput(null)
returns the structured error (keep title like "returns structured errors for
non-object input") and a second that asserts toAutoClassifierInput(undefined)
and call(undefined) normalize non-object args and produce the mocked spawnSync
result (title like "normalizes non-object args for call and spawns command");
locate the assertions around validateInput, toAutoClassifierInput, call,
buildGeneratedTool and MINIMAL_TOOL and move or rename them accordingly so each
test's name matches its verified behavior, and ensure spawnSync expectation
remains in the test that verifies call.
- Around line 207-220: Add a new test in the "collision guard" suite that
exercises the duplicate custom-tool-name branch introduced by the seenToolNames
check: call writeCustomTools with a strategy (e.g., MOCK_STRATEGY_B) and an
array containing two custom tools that share the same name (e.g., two copies of
MINIMAL_TOOL with identical name), spy on console.error, assert that
writeCustomTools returns null and that console.error was called with a message
containing the duplicate tool name, and finally restore the spy; reference
writeCustomTools and the seenToolNames behavior to locate where this guard
should be covered.
🪄 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: bd5ed2a0-91bc-4cb3-9a7d-71d790920c41
📒 Files selected for processing (2)
src/patches/customTools.test.tssrc/patches/customTools.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/patches/customTools.ts (1)
207-207: ExtractmaxResultSizeCharsas a named constant.The
100000magic number is inlined into the generated tool object. Lift it into a top-levelMAX_RESULT_SIZE_CHARSconstant (or expose it as an optionalCustomToolfield) so intent is clear and it's adjustable without edits to the codegen template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.ts` at line 207, Extract the inline magic number 100000 used for maxResultSizeChars into a named top-level constant (e.g. MAX_RESULT_SIZE_CHARS) and replace the hard-coded value in the generated tool object with that constant; alternatively add an optional maxResultSizeChars field to the CustomTool type and prefer that value falling back to MAX_RESULT_SIZE_CHARS when constructing the tool. Update the declaration near the top of src/patches/customTools.ts to export or define const MAX_RESULT_SIZE_CHARS = 100000, change the generated tool object property maxResultSizeChars to reference that constant or the CustomTool field (e.g. tool.maxResultSizeChars ?? MAX_RESULT_SIZE_CHARS), and adjust any related type/interface (CustomTool) to include the optional property so callers can override it.src/patches/customTools.test.ts (1)
427-485: Optional: cover theresult.error/ timeout branch incall().The generated runtime suite only exercises the happy path from
spawnSync(status 0, noerror). Theif(result.error)return{data:{stdout:"",stderr:result.error.message,exitCode:-1}}branch incustomTools.ts(Line 263) and theresult.status??-1fallback (Line 264) aren't covered. A single test that mocksspawnSyncto return{ error: new Error('ETIMEDOUT'), stdout:null, stderr:null, status:null }would lock down the stderr/exit-code contract promised in the PR test plan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.test.ts` around lines 427 - 485, Add a unit test that mocks spawnSync to return { error: new Error('ETIMEDOUT'), stdout: null, stderr: null, status: null } and then calls the generated tool's call() with any input (e.g., undefined) to exercise the error branch in call(); assert the promise resolves to { data: { stdout: '', stderr: 'ETIMEDOUT', exitCode: -1 } } and verify spawnSync was invoked with the expected args; place this alongside the other "generated tool runtime" tests and reuse buildGeneratedTool and the tool variable to locate call() and spawnSync used in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/customTools.ts`:
- Line 146: The timeout value is interpolated into generated JS without
protection; replace the simple assignment "const timeoutVal = tool.timeout ??
30000" with a validated numeric coercion so only a safe numeric literal is
emitted — e.g. coerce via Number and fallback on invalid values: compute let t =
Number(tool.timeout); if (!Number.isFinite(t)) t = 30000; then use const
timeoutVal = t; (alternatively, JSON.stringify(timeoutVal) at the interpolation
sites also works). Update the variable "timeoutVal" in customTools.ts (used in
the timeout:${timeoutVal} interpolations) so the generated bundle always
contains a safe number.
---
Nitpick comments:
In `@src/patches/customTools.test.ts`:
- Around line 427-485: Add a unit test that mocks spawnSync to return { error:
new Error('ETIMEDOUT'), stdout: null, stderr: null, status: null } and then
calls the generated tool's call() with any input (e.g., undefined) to exercise
the error branch in call(); assert the promise resolves to { data: { stdout: '',
stderr: 'ETIMEDOUT', exitCode: -1 } } and verify spawnSync was invoked with the
expected args; place this alongside the other "generated tool runtime" tests and
reuse buildGeneratedTool and the tool variable to locate call() and spawnSync
used in the diff.
In `@src/patches/customTools.ts`:
- Line 207: Extract the inline magic number 100000 used for maxResultSizeChars
into a named top-level constant (e.g. MAX_RESULT_SIZE_CHARS) and replace the
hard-coded value in the generated tool object with that constant; alternatively
add an optional maxResultSizeChars field to the CustomTool type and prefer that
value falling back to MAX_RESULT_SIZE_CHARS when constructing the tool. Update
the declaration near the top of src/patches/customTools.ts to export or define
const MAX_RESULT_SIZE_CHARS = 100000, change the generated tool object property
maxResultSizeChars to reference that constant or the CustomTool field (e.g.
tool.maxResultSizeChars ?? MAX_RESULT_SIZE_CHARS), and adjust any related
type/interface (CustomTool) to include the optional property so callers can
override it.
🪄 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: 29864643-d0bb-4389-b4d5-fc2a6818fead
📒 Files selected for processing (2)
src/patches/customTools.test.tssrc/patches/customTools.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/patches/customTools.test.ts (1)
114-128: AssertrequireFuncis defined before passing it tonew Function.
getRequireFuncNamereturnsstring | undefined, but onlybuildToolFuncandcwdFuncare asserted defined. If a future fixture tweak breaks the loader-detection pattern,requireFuncwill beundefinedandnew Function(buildToolFunc!, 'R', 'Tx', 'Bx', undefined, cwdFunc!, ...)will throw a crypticTypeErrorabout an invalid parameter name instead of a clean assertion failure pointing at the broken fixture.🛡️ Proposed fix
expect(buildToolFunc).toBeDefined(); + expect(requireFunc).toBeDefined(); expect(cwdFunc).toBeDefined(); @@ const tools = new Function( buildToolFunc!, 'R', 'Tx', 'Bx', - requireFunc, + requireFunc!, cwdFunc!,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/customTools.test.ts` around lines 114 - 128, The test calls getRequireFuncName and then passes requireFunc into new Function without asserting it's present; add an explicit assertion or guard for requireFunc (e.g., expect(requireFunc).toBeDefined() or throw a clear error) before the new Function call so that if getRequireFuncName returns undefined the test fails with a readable message; update the test around the variables buildToolFunc, requireFunc, cwdFunc (references to getRequireFuncName, requireFunc, new Function) to assert requireFunc is defined just like buildToolFunc and cwdFunc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/patches/customTools.test.ts`:
- Around line 114-128: The test calls getRequireFuncName and then passes
requireFunc into new Function without asserting it's present; add an explicit
assertion or guard for requireFunc (e.g., expect(requireFunc).toBeDefined() or
throw a clear error) before the new Function call so that if getRequireFuncName
returns undefined the test fails with a readable message; update the test around
the variables buildToolFunc, requireFunc, cwdFunc (references to
getRequireFuncName, requireFunc, new Function) to assert requireFunc is defined
just like buildToolFunc and cwdFunc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9b626d6-3ff2-4e7b-97fa-c52d0cd2fe65
📒 Files selected for processing (2)
src/patches/customTools.test.tssrc/patches/customTools.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/patches/customTools.ts
Contributor
Author
|
Closing in favor of clean replacement #711, rebased on current main and stripped of unrelated native/Nix changes. |
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
New helpers
Config shape
{ "settings": { "customTools": [ { "name": "DateTool", "description": "Get the current date and time", "parameters": {}, "command": "date" }, { "name": "WeatherTool", "description": "Get weather for a city", "parameters": { "city": { "type": "string", "description": "City name" } }, "command": "curl -s wttr.in/{{city}}?format=3", "shell": "bash", "timeout": 10000 } ] } }Optional fields:
shell(default"sh"),timeout(default30000ms),workingDir,env(extra env vars),prompt(override the auto-generated LLM description).Test plan
customTools— patch skips cleanly, no change to cli.js{{param}}placeholders substituted in rendered command and executionpromptoverride — custom prompt string is sent to the modelSummary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests