fix: handle missing skill parameter in SkillTool

[gnanam1990]

Summary

• Address the missing Skill parameter runtime failure reported in #482.
• Allow omitted skill input to reach SkillTool.validateInput() so the tool can return an actionable error message instead of failing first in Zod schema parsing.
• Add focused regression coverage for the exact missing-parameter path while keeping the normal valid skill path unchanged.

Root Cause

• runToolUse() validates tool inputs with tool.inputSchema.safeParse(input) before calling tool.validateInput().
• SkillTool previously declared skill as a required z.string().
• Because of that, calls like Skill({}) or Skill({ skill: undefined }) failed during schema parsing and never reached the tool’s custom missing-skill guard.

What Changed

• Made SkillTool.inputSchema.skill optional so omitted skill input can reach tool-level validation.
• Kept the explicit actionable missing-skill message in validateInput().
• Updated the later skill handling in SkillTool to remain type-safe with the optional schema.
• Added a focused SkillTool regression test file covering:
  • missing skill
  • valid skill parsing

Test Plan

• npx bun test src/tools/SkillTool/SkillTool.test.ts
• npx bun test
• npx bun run build
• npx bun run smoke
• direct local repro:
  • SkillTool.inputSchema.safeParse({}) now succeeds
  • validateInput() now returns the actionable missing-skill error instead of failing first in schema parsing

Notes

• This is a narrow bugfix for the actual runtime execution-order issue discussed in #482.
• Prompt/schema wording improvements can still help model behavior, but this change specifically fixes the unreachable validation path in the current runtime flow.

[kevincodex1]

Looks great to me

[Vasanthdev2004]

Rechecked the latest head cdfaea5cedf88383ade635ec08cef335a35d3abf against current origin/main.

I still can't approve this change because it fixes a Codex-specific runtime path by weakening the public Skill tool schema for every provider.

src/tools/SkillTool/SkillTool.ts:293 now makes skill optional in inputSchema. That does let Skill({}) reach validateInput(), but toolToAPISchema() serializes this same Zod schema directly for Anthropic/Gemini (src/utils/api.ts:157-160, src/utils/zodToJsonSchema.ts:17-22). On this head, a direct repro of the exported schema produces:

{
  "type": "object",
  "properties": {
    "skill": { "type": "string" },
    "args": { "type": "string" }
  },
  "additionalProperties": false
}

There is no required entry anymore, so non-OpenAI providers now see Skill as accepting {}. Codex/OpenAI happen to recover because their schema normalization later forces every property into required[], but Anthropic/Gemini do not. That turns a narrow Codex bugfix into a cross-provider contract regression and can increase empty Skill({}) calls from models that previously had a required skill field.

I would keep the externally exposed schema required and move the actionable missing-skill handling somewhere before or alongside the schema-parse path instead of relaxing the public schema.

What I rechecked on this head:

• bun test src/tools/SkillTool/SkillTool.test.ts
• bun test src/utils/api.test.ts
• direct schema repro via toolToAPISchema(SkillTool, ...)
• bun run build
• bun run smoke

[gnanam1990]

Thanks for the detailed recheck. I pushed a narrow follow-up on the latest head to preserve the public SkillTool schema contract while still fixing the missing-parameter runtime path:

• be7ec1b fix: preserve SkillTool schema contract

What changed in this follow-up:

• SkillTool.inputSchema.skill is required again, so toolToAPISchema(SkillTool, ...) still exports required: ["skill"] for Anthropic/Gemini and other providers using the raw schema.
• The actionable missing-skill handling moved into toolExecution.ts, where the runtime can special-case the Skill({}) schema failure and return the clearer error message without weakening the public schema.
• Added focused coverage for both sides:
  1. runtime override for missing skill
  2. exported API schema still requiring skill

Local recheck on this head:

• npx bun test src/tools/SkillTool/SkillTool.test.ts src/services/tools/toolExecution.test.ts src/utils/api.test.ts
• npx bun test
• npx bun run build
• npx bun run smoke

When you have a moment, could you please re-review the latest commit?

[Vasanthdev2004]

Rechecked the latest head be7ec1b5fa44acd4731b312647b16a8da92b9639 against current origin/main.

The public schema regression from the previous revision is fixed: SkillTool once again exports required: ["skill"], so I no longer have the Anthropic/Gemini contract concern.

I still can't approve this head because the runtime fix is only partial.

src/services/tools/toolExecution.ts:694 still stores toolUseResult: \InputValidationError: ${parsedInput.error.message}`even whengetSchemaValidationErrorOverride()selected the actionable SkillTool message for the visibletool_resultblock. That meansSkill({})still surfaces the raw Zod schema failure in paths that consumetoolUseResult` directly rather than the rendered block.

One concrete example is src/utils/queryHelpers.ts:150-152, which forwards _.toolUseResult as tool_use_result to SDK consumers. Transcript/search paths also read msg.toolUseResult. So on this head the user-visible transcript block improves, but those structured consumers still get the generic schema error instead of the new Missing skill name... guidance.

I would expect the override to feed the structured result too, not just the rendered block.

What I rechecked on this head:

• direct code-path review of toolExecution.ts and queryHelpers.ts
• direct schema repro of toolToAPISchema(SkillTool, ...) confirming required: ["skill"] is restored
• bun run build
• bun run smoke

I also attempted the new focused test command locally, but hit Windows/Bun module-resolution failures in this worktree (zod/v4, lodash-es, then diff) before those tests executed, so I am not counting that as verification evidence here.

[gnanam1990]

Thanks again for the detailed recheck. I pushed one final narrow follow-up on the latest head:

• 1e72fc9 fix: align SkillTool schema error output

What changed in this follow-up:

• the public SkillTool schema remains required
• the visible tool_result block still uses the actionable missing-skill message
• toolUseResult now uses that same actionable override too, so structured consumers no longer receive the raw Zod error while the rendered block shows the nicer message

Local recheck on this head:

• npx bun test src/tools/SkillTool/SkillTool.test.ts src/services/tools/toolExecution.test.ts src/utils/api.test.ts
• npx bun test
• npx bun run build
• npx bun run smoke
• direct repro confirms:
  1. schema still requires skill
  2. visible error uses Missing skill name...
  3. structured toolUseResult also uses InputValidationError: Missing skill name...

When you have a moment, could you please re-review the latest commit?

[Vasanthdev2004]

Rechecked the latest head 1e72fc94cbd1c6d7a7db5d6b6a69ae16ef76ca2c against current origin/main.

The two blockers from my previous reviews are fixed on this head:

• SkillTool once again exports a required skill field, so the public schema contract for Anthropic/Gemini and other raw-schema consumers is preserved.
• toolUseResult now goes through the same missing-skill override path as the rendered tool_result block, so structured consumers no longer receive the raw Zod message while the visible transcript shows the actionable one.

What I rechecked on this head:

• direct code-path review of src/tools/SkillTool/SkillTool.ts, src/services/tools/toolExecution.ts, and the new focused tests
• bun run build -> success
• bun run smoke -> success

I also attempted the focused Bun tests in this Windows worktree, but they still failed before execution on unrelated module-resolution issues (zod/v4, lodash-es/uniqBy.js, then a react package resolution error). Since those failures are environmental and pre-date the actual test bodies here, I am not treating them as PR-specific blockers.

I did not find a new code-level blocker in the latest diff.

[kevincodex1]

found a bug here one testing. in main. please check.

Screen.Recording.2026-04-08.at.12.43.35.AM.mov

@gnanam1990 @Vasanthdev2004

[kevincodex1]

sorry not related to this PR. found the bug its in the previous PR before this. I created this to fix it.

#494