test: tools — isToolOnPath discovery and BatchTool validation#412
test: tools — isToolOnPath discovery and BatchTool validation#412anandgupta42 wants to merge 2 commits intomainfrom
Conversation
Cover untested tool infrastructure: isToolOnPath PATH/env resolution and BatchTool's formatValidationError output formatting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01BJDADsQt5Q15LUSQgMgkas
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis pull request adds two new test suites to the opencode package. The first validates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opencode/test/cli/is-tool-on-path.test.ts (1)
9-22: Consider usingtmpdir({ git: true })if it provides equivalent functionality.The coding guidelines recommend using the
git: trueoption for git-initialized temp directories. If the built-in option doesn't configurefsmonitor,gpgsign, and user identity, then this custom helper is justified. Otherwise, consolidating on the standard pattern improves consistency.Based on learnings: "Use the
git: trueoption when initializing a temporary test directory that needs a git repository with a root commit."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/cli/is-tool-on-path.test.ts` around lines 9 - 22, The tmpdirGit helper duplicates git setup that may be provided by tmpdir({ git: true }); update the tmpdirGit function to call tmpdir({ git: true, init: async (dir) => { ... } }) or replace usages to use tmpdir({ git: true }) and only apply the extra git config steps (core.fsmonitor, commit.gpgsign, user.email, user.name and initial commit) when the built-in git option does not already perform them; keep the existing optional init callback behavior intact so callers relying on tmpdirGit(init) still run their init after the repo is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/cli/is-tool-on-path.test.ts`:
- Around line 101-102: The test hardcodes ":" when setting process.env.PATH
which breaks Windows; change the assignment to use Node's platform-aware
delimiter (e.g., path.delimiter) or the same conditional used in
skill-helpers.ts so that savedEnv.PATH and the new process.env.PATH assignment
use path.delimiter (or process.platform === "win32" ? ";" : ":") when building
path.join(tmp.path, "path-bin") + delimiter + (process.env.PATH ?? "").
In `@packages/opencode/test/tool/batch.test.ts`:
- Around line 56-80: The test currently only checks metadata but must also
assert the DISALLOWED guard; update the test that references BatchTool to call
the execute function and verify it rejects when given a call with call.tool ===
"batch" and that the thrown/error message includes the phrase "not allowed in
batch"; if execute requires Session.updatePart, supply a minimal stub/mock
session object or otherwise stub that dependency so execute can run and throw
the DISALLOWED error—target symbols: BatchTool, BatchTool.init, and the execute
function exported from "../../src/tool/batch".
- Around line 17-23: The test currently conditionally skips formatter assertions
when toolInfo.formatValidationError is missing; instead, assert that the
formatter exists before using it so the test fails fast. Update the test around
the result handling (the block using result, result.success, and
toolInfo.formatValidationError) to assert toolInfo.formatValidationError is
defined (or throw) when result.success is false, then call
toolInfo.formatValidationError(result.error) and perform the existing
expect(...).toContain checks for "Invalid parameters for tool 'batch'", "Provide
at least one tool call", and "Expected payload format".
---
Nitpick comments:
In `@packages/opencode/test/cli/is-tool-on-path.test.ts`:
- Around line 9-22: The tmpdirGit helper duplicates git setup that may be
provided by tmpdir({ git: true }); update the tmpdirGit function to call
tmpdir({ git: true, init: async (dir) => { ... } }) or replace usages to use
tmpdir({ git: true }) and only apply the extra git config steps (core.fsmonitor,
commit.gpgsign, user.email, user.name and initial commit) when the built-in git
option does not already perform them; keep the existing optional init callback
behavior intact so callers relying on tmpdirGit(init) still run their init after
the repo is created.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f986a5c1-deae-4846-adc9-2171fb2378c2
📒 Files selected for processing (2)
packages/opencode/test/cli/is-tool-on-path.test.tspackages/opencode/test/tool/batch.test.ts
| savedEnv.PATH = process.env.PATH | ||
| process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "") |
There was a problem hiding this comment.
Hardcoded : separator breaks Windows compatibility.
The implementation in skill-helpers.ts uses process.platform === "win32" ? ";" : ":" for the PATH separator. The test should mirror this to remain cross-platform.
🔧 Proposed fix
+ const sep = process.platform === "win32" ? ";" : ":"
savedEnv.PATH = process.env.PATH
- process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "")
+ process.env.PATH = path.join(tmp.path, "path-bin") + sep + (process.env.PATH ?? "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| savedEnv.PATH = process.env.PATH | |
| process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "") | |
| const sep = process.platform === "win32" ? ";" : ":" | |
| savedEnv.PATH = process.env.PATH | |
| process.env.PATH = path.join(tmp.path, "path-bin") + sep + (process.env.PATH ?? "") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/cli/is-tool-on-path.test.ts` around lines 101 - 102,
The test hardcodes ":" when setting process.env.PATH which breaks Windows;
change the assignment to use Node's platform-aware delimiter (e.g.,
path.delimiter) or the same conditional used in skill-helpers.ts so that
savedEnv.PATH and the new process.env.PATH assignment use path.delimiter (or
process.platform === "win32" ? ";" : ":") when building path.join(tmp.path,
"path-bin") + delimiter + (process.env.PATH ?? "").
| if (!result.success && toolInfo.formatValidationError) { | ||
| const msg = toolInfo.formatValidationError(result.error) | ||
| expect(msg).toContain("Invalid parameters for tool 'batch'") | ||
| expect(msg).toContain("Provide at least one tool call") | ||
| // Should include the expected payload format hint | ||
| expect(msg).toContain("Expected payload format") | ||
| } |
There was a problem hiding this comment.
Fail fast instead of conditionally skipping formatter assertions.
These branches silently skip message assertions when formatValidationError is missing, so tests can pass without validating the formatter contract.
🔧 Suggested tightening
- if (!result.success && toolInfo.formatValidationError) {
- const msg = toolInfo.formatValidationError(result.error)
+ const formatValidationError = toolInfo.formatValidationError
+ expect(typeof formatValidationError).toBe("function")
+ if (!formatValidationError) {
+ throw new Error("BatchTool.formatValidationError must be defined")
+ }
+ if (!result.success) {
+ const msg = formatValidationError(result.error)
expect(msg).toContain("Invalid parameters for tool 'batch'")
expect(msg).toContain("Provide at least one tool call")
// Should include the expected payload format hint
expect(msg).toContain("Expected payload format")
}Also applies to: 35-40, 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/tool/batch.test.ts` around lines 17 - 23, The test
currently conditionally skips formatter assertions when
toolInfo.formatValidationError is missing; instead, assert that the formatter
exists before using it so the test fails fast. Update the test around the result
handling (the block using result, result.success, and
toolInfo.formatValidationError) to assert toolInfo.formatValidationError is
defined (or throw) when result.success is false, then call
toolInfo.formatValidationError(result.error) and perform the existing
expect(...).toContain checks for "Invalid parameters for tool 'batch'", "Provide
at least one tool call", and "Expected payload format".
| describe("BatchTool: DISALLOWED guard", () => { | ||
| test("batch tool cannot be called recursively — error message is clear", async () => { | ||
| // The DISALLOWED set and the error message are defined in the execute path. | ||
| // We can verify by importing and inspecting the tool's behavior: | ||
| // The execute function checks `if (DISALLOWED.has(call.tool))` and throws | ||
| // with a message mentioning "not allowed in batch". | ||
| // | ||
| // Since execute requires Session.updatePart (heavy dependency), we verify | ||
| // the public contract by importing the source and checking the constants directly | ||
| // plus verifying the error string template. | ||
|
|
||
| // Import the DISALLOWED set via the module | ||
| const batchModule = await import("../../src/tool/batch") | ||
|
|
||
| // The module exports BatchTool but DISALLOWED is module-scoped. | ||
| // We can at minimum verify the tool registers with id "batch" | ||
| expect(batchModule.BatchTool.id).toBe("batch") | ||
|
|
||
| // Verify that the init function returns a valid tool shape | ||
| const toolInfo = await batchModule.BatchTool.init() | ||
| expect(toolInfo.description).toBeTruthy() | ||
| expect(toolInfo.parameters).toBeDefined() | ||
| expect(typeof toolInfo.execute).toBe("function") | ||
| expect(typeof toolInfo.formatValidationError).toBe("function") | ||
| }) |
There was a problem hiding this comment.
DISALLOWED guard test never exercises the guard path.
The test currently validates tool metadata only (id, shape) and would still pass if the recursive-call restriction were removed. Please either rename this test to match what it checks, or add an assertion that execute rejects a recursive batch call with the expected message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/tool/batch.test.ts` around lines 56 - 80, The test
currently only checks metadata but must also assert the DISALLOWED guard; update
the test that references BatchTool to call the execute function and verify it
rejects when given a call with call.tool === "batch" and that the thrown/error
message includes the phrase "not allowed in batch"; if execute requires
Session.updatePart, supply a minimal stub/mock session object or otherwise stub
that dependency so execute can run and throw the DISALLOWED error—target
symbols: BatchTool, BatchTool.init, and the execute function exported from
"../../src/tool/batch".
|
Superseded by #439 which consolidates all 12 test PRs into one, deduplicates overlapping tests, and fixes bugs found during review. |
2 participants
What does this PR do?
1.
isToolOnPath—src/cli/cmd/skill-helpers.ts(4 new tests)This function checks whether a CLI tool referenced by a skill is available on the user's system — searching
.opencode/tools/,ALTIMATE_BIN_DIR, and systemPATH. Zero tests existed previously. If this function regresses, skills would falsely report tools as "missing" even when they're installed, confusing users into unnecessary reinstalls.New coverage includes:
.opencode/tools/under cwd (primary lookup path)falsewhen tool genuinely doesn't exist anywhere (prevents false positives)ALTIMATE_BIN_DIRenvironment variable (bespoke feature, most regression-prone)PATHdirectory (system PATH scan)2.
BatchToolvalidation —src/tool/batch.ts(4 new tests)The batch tool allows parallel tool execution and has validation logic including
formatValidationError(user-facing error messages when the LLM sends malformed input) and anti-recursion guards. Zero tests existed previously. IfformatValidationErrorregresses, models receive cryptic errors instead of actionable guidance and may loop.New coverage includes:
formatValidationErrorproduces user-friendly output for emptytool_callsarray (most common bad payload)formatValidationErrorincludes nested field paths (e.g.tool_calls.0.tool) for precise error localizationtool_callsfield entirelyType of change
Issue for this PR
N/A — proactive test coverage from /test-discovery
How did you verify your code works?
Marker check:
bun run script/upstream/analyze.ts --markers --base main --strict— passed (no upstream files modified).Checklist
https://claude.ai/code/session_01BJDADsQt5Q15LUSQgMgkas
Summary by CodeRabbit