feat(gadgets): add unified ToolDefinition type#729
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean type-only PR that establishes a well-designed ToolDefinition interface bridging gadget Zod schemas, oclif CLI flags, and JSON Schema manifests. The types accurately model existing patterns in the codebase (e.g., body-file / text-file alternatives, CASCADE_BASE_BRANCH env var resolution, allowNo boolean flags). No runtime code, all CI passes. Ready to merge.
Should Fix (non-blocking)
-
required/optionaldual booleans (line 96-98):BaseParameterDefinitionhas bothrequired?: booleanandoptional?: booleanwith a JSDoc comment saying "Exactly one should be set to true." This invariant isn't enforced at the type level —{ required: true, optional: true }compiles without error. Consider a discriminated union or a singlerequired: booleanfield instead. The factory functions in follow-up stories will need runtime validation for this either way, but catching it at compile time would be cleaner. -
Module cohesion (line 1-2 of index.ts):
src/gadgets/shared/index.tsis described as "Shared utilities for file editing gadgets," butToolDefinitionis a cross-cutting concern for all tools (PM, SCM, file ops). This is fine as a starting point, but as the follow-up stories build out factory functions, consider whether this type belongs in a more central location (e.g.,src/gadgets/types/or similar) since CLI commands will also consume it.
| */ | ||
| required?: boolean; | ||
| /** Whether this parameter is optional */ | ||
| optional?: boolean; |
There was a problem hiding this comment.
[SHOULD_FIX] The required + optional dual-boolean pattern creates an unenforceable invariant. A parameter definition like { type: "string", describe: "...", required: true, optional: true } compiles without error. Consider either:
- A single
required: booleanfield (defaulting totrueas stated in the JSDoc), or - A discriminated union:
{ mode: "required" } | { mode: "optional" }
This would let the type system prevent contradictory definitions rather than relying on runtime checks in factory functions.
Summary
ToolDefinitioninterface insrc/gadgets/shared/toolDefinition.ts— single source of truth for all tool metadata (name, description, timeout, parameters, examples, CLI metadata, hooks)ParameterDefinitionunion type coveringstring,number,boolean,enum,array, andobjectparameter types, each withrequired,optional,default,describe, and type-specific constraintsFileInputAlternativetype for mapping text parameters to file-input CLI flags (e.g.,text→text-file,body→body-file)CLIAutoResolvedtype for marking params auto-resolved from env vars or git remote (e.g.,owner/repo)gadgetOnlyflag on parameter definitions to exclude params likecomment(rationale) from CLI flags and manifestscliEnvVarfield for params that can be auto-populated from environment variables (e.g.,basefromCASCADE_BASE_BRANCH)GadgetPostExecuteHookandCLIPostExecuteHookfunction type placeholders for lifecycle hookssrc/gadgets/shared/index.tsCard: https://trello.com/c/6fenfE8R/270-as-a-developer-i-want-a-unified-tooldefinition-type-so-that-tool-metadata-has-a-single-source-of-truth
Test plan
npm run typecheckpasses with zero errorsnpm run lint— no new lint issues introduced (pre-existing error in web component unrelated to this change)