feat: add custom shell-command tools#711
Open
y0usaf wants to merge 1 commit intoPiebald-AI:mainfrom
Open
Conversation
7 tasks
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 a "Custom tools" feature: new Settings types and default, config normalization, UI for listing/editing/reordering tools, bundle patch writer plus helper matchers to inject runtime tool definitions, and tests covering detection, injection, and runtime execution. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "CustomToolsView / Edit"
participant Config as "Settings Normalizer"
participant Patch as "writeCustomTools"
participant Bundle as "Bundled Runtime"
participant Shell as "Shell (spawn)"
User->>UI: create/edit custom tool
UI->>Config: update settings.customTools
User->>Patch: run tweakcc --apply
Patch->>Config: normalize & validate customTools
Patch->>Bundle: detect buildTool & cwd helpers in bundle
Patch->>Bundle: inject generated buildTool(...) code
User->>Bundle: invoke custom tool at runtime
Bundle->>Bundle: validate params & interpolate {{param}}
Bundle->>Shell: spawn(command, { shell, timeout, env, cwd })
Shell-->>Bundle: stdout / stderr / exitCode
Bundle-->>User: display tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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: 2
🤖 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/ui/components/CustomToolEditView.tsx`:
- Around line 239-242: The UI validation currently only checks for duplicate
names using duplicateName (iterating settings.customTools with toolIndex and
name) but doesn't block reserved built-in Claude tool names that
writeCustomTools rejects; update the validation in CustomToolEditView (where
duplicateName is computed and where the save/validation path around name exists)
to also check against the same reserved-name set used by writeCustomTools (e.g.,
the built-in names like "Bash", "Read", "Write") and mark the name invalid if it
matches any reserved value (case/whitespace-normalized), surfacing the error in
the same validation UI before allowing save or injection.
- Around line 276-342: The effect is causing an autosave loop because
updateSettings creates a new settings object (deep copy) so
settings.customTools[toolIndex] gets a new reference and the effect retriggers
via the tool dependency; remove tool from the dependency list and stop
persisting on every keystroke by validating inputs and only calling
updateSettings when the form is valid (e.g. validate
name/command/parsedParameters/parsedEnv and parsedTimeout) or on an explicit
save action (or after a debounce); locate the effect in CustomToolEditView.tsx
(the useEffect that reads tool, toolIndex and calls updateSettings) and change
it to trigger only on validated state changes or save events instead of every
input change.
🪄 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: 1bcaaa8d-8251-4610-8bfb-0af85a220788
📒 Files selected for processing (12)
README.mdsrc/config.tssrc/defaultSettings.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/types.tssrc/ui/App.tsxsrc/ui/components/CustomToolEditView.tsxsrc/ui/components/CustomToolsView.tsxsrc/ui/components/MainMenu.tsx
Member
|
Hi @y0usaf, I'm sorry for neglecting the PRs; we're working on a Piebald release and afterwards we'll spend some time clearing the backlog here. |
Contributor
Author
|
No worries, it's my bad for making messy PRs! Thanks for being patient with me. |
y0usaf
force-pushed
the
feat/custom-tools-clean
branch
from
f6b672b to
e36765d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 330-345: The checks in the customTools validation loop compare raw
tool.name values, allowing case/whitespace variants to slip past; normalize each
tool name (e.g., const normalized = tool.name.trim().toLowerCase()) before using
it for the BUILTIN_TOOL_NAMES.has check and for membership in seenToolNames,
store normalized names in seenToolNames, and keep using the original tool.name
only for user-facing messages; align this normalization with the same logic used
in CustomToolEditView.
In `@src/ui/components/CustomToolEditView.tsx`:
- Around line 541-561: The useInput callback references hasBlockingErrors before
it's initialized when tool is falsy; add an early guard at the top of the
callback (e.g. return early if !tool) so the handler doesn't access
hasBlockingErrors when the component returned early; update the useInput
callback (the function handling key events) to check tool (or otherwise ensure
hasBlockingErrors is defined) before evaluating !hasBlockingErrors and calling
persistTool/onBack so Escape does not cause a runtime error.
🪄 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: 744c021d-f13b-4bfb-a03a-d83f14019f2e
📒 Files selected for processing (12)
README.mdsrc/config.tssrc/defaultSettings.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/types.tssrc/ui/App.tsxsrc/ui/components/CustomToolEditView.tsxsrc/ui/components/CustomToolsView.tsxsrc/ui/components/MainMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/components/MainMenu.tsx
- src/ui/App.tsx
- src/patches/index.ts
- README.md
- src/defaultSettings.ts
y0usaf
force-pushed
the
feat/custom-tools-clean
branch
from
e36765d to
4b04e8b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/config.ts (1)
170-304: ⚡ Quick winExtract custom-tool validation into a shared helper.
This block now mirrors
parseTimeoutInput,parseParametersInput, andparseEnvInputinsrc/ui/components/CustomToolEditView.tsx. Keeping two copies in sync is brittle; the editor can drift from config normalization and start saving tools that the loader later drops. Pull the custom-tool parser/validator into one shared module and reuse it in both paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 170 - 304, The normalizeCustomTool function duplicates validation logic already implemented in parseTimeoutInput, parseParametersInput, and parseEnvInput inside CustomToolEditView.tsx; extract the shared validation/parsing for a CustomTool into a new module (e.g., src/utils/customToolParser) and have normalizeCustomTool call that shared parser, and update CustomToolEditView.tsx to import and reuse the same parser instead of its local helpers; ensure the shared API validates name, description, command, parameters schema (types/description/required), optional shell, timeout, workingDir, env, and prompt and returns either a typed CustomTool or a structured error/nullable result so both normalizeCustomTool and the editor consume identical validation behavior.
🤖 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 228-235: The checkPermissions implementation currently always
delegates to the Bash tool; update checkPermissions (in the same object that
implements call()) to select the permission checker based on the configured
shell used by call() — look up context.options.tools for a tool whose name
matches the runtime shell (e.g., "PowerShell" or "Pwsh" for PowerShell shells,
"Cmd" or "Command Prompt" for cmd, otherwise "Bash") and call that tool's
checkPermissions with the same args ({command: cmd, timeout: ...}); if no
matching built-in checker exists, return a conservative Promise.resolve response
(e.g., behavior: "deny" or "requireConfirmation"/passthrough per project
convention) rather than always approving via Bash to avoid mismatched parsing
rules.
- Around line 259-272: The async call implementation currently uses spawnSync
(reference: call and spawnSync) which blocks the event loop; change it to use a
non-blocking child process API (e.g., spawn or execFile) and return a Promise
that resolves when the child exits or errors. Replace the spawnSync invocation
and synchronous result handling with spawn/execFile, hook into 'exit'/'close'
and 'error' events and collect stdout/stderr (or use execFile with a callback)
so you can resolve with the same return shape {data:{stdout,stderr,exitCode}},
ensure timeout and cwd/env/stdio are applied appropriately, and preserve
trimming and exitCode fallback logic.
- Around line 163-174: The properties object should be created with a null
prototype to avoid prototype mutation for parameter names like "__proto__";
change the initialization of properties (used alongside required, iterating
tool.parameters) to use an object with no prototype and keep the rest of the
loop that assigns properties[paramName] and pushes to required unchanged so
JSON.stringify({ type: 'object', properties, required }) emits the "__proto__"
key correctly.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 170-304: The normalizeCustomTool function duplicates validation
logic already implemented in parseTimeoutInput, parseParametersInput, and
parseEnvInput inside CustomToolEditView.tsx; extract the shared
validation/parsing for a CustomTool into a new module (e.g.,
src/utils/customToolParser) and have normalizeCustomTool call that shared
parser, and update CustomToolEditView.tsx to import and reuse the same parser
instead of its local helpers; ensure the shared API validates name, description,
command, parameters schema (types/description/required), optional shell,
timeout, workingDir, env, and prompt and returns either a typed CustomTool or a
structured error/nullable result so both normalizeCustomTool and the editor
consume identical validation behavior.
🪄 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: a49a21b3-96dd-4339-a9e6-c67299b92fc4
📒 Files selected for processing (12)
README.mdsrc/config.tssrc/defaultSettings.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/types.tssrc/ui/App.tsxsrc/ui/components/CustomToolEditView.tsxsrc/ui/components/CustomToolsView.tsxsrc/ui/components/MainMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- src/defaultSettings.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/components/MainMenu.tsx
- src/ui/App.tsx
- src/ui/components/CustomToolsView.tsx
- README.md
- src/patches/index.ts
y0usaf
force-pushed
the
feat/custom-tools-clean
branch
from
4b04e8b to
8f3e025
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/components/CustomToolsView.tsx (1)
166-181: 💤 Low valueUsing array index as React key.
Using
indexas the key (line 170) is generally discouraged, but it's acceptable here since:
- The list is managed locally with explicit reordering handlers that update
selectedIndexin sync- Ink's CLI rendering doesn't involve animations where stale keys would cause visual glitches
Consider adding a unique
idfield toCustomToolin a future iteration if tools need stable identity across sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/CustomToolsView.tsx` around lines 166 - 181, The map in CustomToolsView uses array index as the React key (key={index} in the customTools.map loop), which can cause unstable identity when items are reordered; change to a stable unique key by adding an id property to the CustomTool objects and use tool.id as the key instead, updating any creation/serialization code that constructs CustomTool to generate a stable id (e.g., UUID) and ensuring reordering handlers that touch selectedIndex continue to reference tools by id; references to locate changes: customTools, selectedIndex, summarizeTool, truncate and the map render block.
🤖 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/ui/components/CustomToolsView.tsx`:
- Around line 166-181: The map in CustomToolsView uses array index as the React
key (key={index} in the customTools.map loop), which can cause unstable identity
when items are reordered; change to a stable unique key by adding an id property
to the CustomTool objects and use tool.id as the key instead, updating any
creation/serialization code that constructs CustomTool to generate a stable id
(e.g., UUID) and ensuring reordering handlers that touch selectedIndex continue
to reference tools by id; references to locate changes: customTools,
selectedIndex, summarizeTool, truncate and the map render block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0866d140-3de9-4a2a-b59b-c34e05c15a49
📒 Files selected for processing (12)
README.mdsrc/config.tssrc/defaultSettings.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/types.tssrc/ui/App.tsxsrc/ui/components/CustomToolEditView.tsxsrc/ui/components/CustomToolsView.tsxsrc/ui/components/MainMenu.tsx
✅ Files skipped from review due to trivial changes (3)
- src/ui/App.tsx
- README.md
- src/patches/customTools.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/MainMenu.tsx
- src/ui/components/CustomToolEditView.tsx
- src/config.ts
y0usaf
force-pushed
the
feat/custom-tools-clean
branch
from
8f3e025 to
0b30b0e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/components/CustomToolEditView.tsx (1)
62-107: ⚡ Quick winMove the reserved-name list into shared validation code.
This list now has to stay in sync with
BUILTIN_TOOL_NAMESinsrc/patches/customTools.ts. If they drift, the editor and patcher will disagree again about which names are valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/CustomToolEditView.tsx` around lines 62 - 107, RESERVED_TOOL_NAMES (used in CustomToolEditView.tsx) must be factored out and sourced from a single shared validation module so it stays in sync with BUILTIN_TOOL_NAMES in src/patches/customTools.ts; create or use a shared file (e.g., a new exported symbol in a validation/shared module) that exports the canonical reserved/builtin names (or a single exported BUILTIN_TOOL_NAMES array/set) and replace the local RESERVED_TOOL_NAMES definition in CustomToolEditView.tsx with an import of that shared symbol, and update any consumers to use the shared validator function/constant instead of their own hardcoded list so both the editor (CustomToolEditView) and the patcher (customTools.ts) use the same source of truth.
🤖 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 364-374: The current normalization in readConfigFile leaves
built-in-name collisions and duplicate custom tool names that later cause
writeCustomTools to fail; update the normalization pipeline that uses
normalizeCustomTool and the handling of config.settings.customTools to
additionally reject entries whose name is in the reserved/built-in set or that
duplicate an earlier accepted custom tool name: compute or import the
reservedNames (built-in tool names), iterate config.settings.customTools calling
normalizeCustomTool(tool, index) but then drop any nulls and also drop tools
whose name is in reservedNames or alreadySeenNames (track seen names as you
accept them) so only unique, non-reserved CustomTool objects remain before
returning from readConfigFile.
---
Nitpick comments:
In `@src/ui/components/CustomToolEditView.tsx`:
- Around line 62-107: RESERVED_TOOL_NAMES (used in CustomToolEditView.tsx) must
be factored out and sourced from a single shared validation module so it stays
in sync with BUILTIN_TOOL_NAMES in src/patches/customTools.ts; create or use a
shared file (e.g., a new exported symbol in a validation/shared module) that
exports the canonical reserved/builtin names (or a single exported
BUILTIN_TOOL_NAMES array/set) and replace the local RESERVED_TOOL_NAMES
definition in CustomToolEditView.tsx with an import of that shared symbol, and
update any consumers to use the shared validator function/constant instead of
their own hardcoded list so both the editor (CustomToolEditView) and the patcher
(customTools.ts) use the same source of truth.
🪄 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: 741eced8-9355-4c09-8bd8-28fe0e4e4d99
📒 Files selected for processing (12)
README.mdsrc/config.tssrc/defaultSettings.tssrc/patches/customTools.test.tssrc/patches/customTools.tssrc/patches/helpers.tssrc/patches/index.tssrc/types.tssrc/ui/App.tsxsrc/ui/components/CustomToolEditView.tsxsrc/ui/components/CustomToolsView.tsxsrc/ui/components/MainMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/MainMenu.tsx
- src/ui/App.tsx
- src/ui/components/CustomToolsView.tsx
Comment on lines
+364
to
+374
| // Validate each customTool entry — drop entries missing required fields. | ||
| if (!Array.isArray(config.settings.customTools)) { | ||
| console.warn( | ||
| 'config: customTools must be an array; ignoring invalid value' | ||
| ); | ||
| config.settings.customTools = []; | ||
| } else { | ||
| config.settings.customTools = config.settings.customTools | ||
| .map((tool, index) => normalizeCustomTool(tool, index)) | ||
| .filter((tool): tool is CustomTool => tool !== null); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject reserved and duplicate tool names during normalization.
Manual or remote configs can still load names that writeCustomTools() rejects later. One built-in-name collision or duplicate currently survives readConfigFile(), then causes the entire custom-tools patch to fail at apply time instead of being filtered here with the other invalid entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.ts` around lines 364 - 374, The current normalization in
readConfigFile leaves built-in-name collisions and duplicate custom tool names
that later cause writeCustomTools to fail; update the normalization pipeline
that uses normalizeCustomTool and the handling of config.settings.customTools to
additionally reject entries whose name is in the reserved/built-in set or that
duplicate an earlier accepted custom tool name: compute or import the
reservedNames (built-in tool names), iterate config.settings.customTools calling
normalizeCustomTool(tool, index) but then drop any nulls and also drop tools
whose name is in reservedNames or alreadySeenNames (track seen names as you
accept them) so only unique, non-reserved CustomTool objects remain before
returning from readConfigFile.
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
2 participants
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
settings.customToolssupport for user-defined shell-command toolsSupersedes #648 with a clean branch rebased on current
mainand without the unrelated native/Nix changes.Tests
bun run lintbunx vitest run src/patches/customTools.test.ts src/tests/config.test.tsNote:
pnpmis not available in this environment. Fullbun run teststill hits existing Bun/Vitest mock interop failures in unrelated suites (systemPrompts.test.ts,modelCustomizationsToggle.test.ts).Summary by CodeRabbit
New Features
Validation & Config
Documentation
Tests