-
-
Notifications
You must be signed in to change notification settings - Fork 193
ref: Migrate next-steps to manifest-driven templates #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Move next-step definitions from inline tool logic into YAML manifests
using a new nextStepTemplates system. Templates support static params,
${arg} substitution, and runtime merging with dynamic response params.
Removes hardcoded next-step construction from individual tool handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| ...templateStep, | ||
| params: fallbackStep.params, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index misalignment between filtered template steps and original templates
High Severity
mergeTemplateAndResponseNextSteps accesses tool.nextStepTemplates by positional index from the filtered templateSteps array, but buildTemplateNextSteps can skip entries when a target tool isn't in the runtime catalog (e.g., filtered by predicates). This breaks the 1:1 index correspondence, causing consumeDynamicParams to look up the wrong toolId and apply incorrect params. For example, list_devices references build_device (predicated), test_device (predicated), and get_device_app_path. In XcodeAgentMode, the first two are excluded, so templateSteps[0] is the get_device_app_path step but templates[0] points to the build_device template — applying build_device's dynamic params to the wrong step.
Additional Locations (1)
| `Important: Before working on the project make sure to read the README.md file in the workspace root directory.`, | ||
| `Build for simulator: build_sim({ workspacePath: "${projectPath}/${params.customizeNames ? params.projectName : 'MyProject'}.xcworkspace", scheme: "${params.customizeNames ? params.projectName : 'MyProject'}", simulatorName: "iPhone 16" })`, | ||
| `Build and run on simulator: build_run_sim({ workspacePath: "${projectPath}/${params.customizeNames ? params.projectName : 'MyProject'}.xcworkspace", scheme: "${params.customizeNames ? params.projectName : 'MyProject'}", simulatorName: "iPhone 16" })`, | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scaffold tools lost dynamic workspace path and scheme params
Medium Severity
Both scaffold handlers removed the dynamic next-step information (computed workspace path, scheme name, simulator name) from the response body without returning equivalent nextStepParams. The manifest templates for build_sim/build_run_sim/build_macos/build_run_macos have no params or ${arg} substitutions, so the resulting next steps will render without any params — e.g., build_sim() instead of build_sim({ workspacePath: "...", scheme: "...", simulatorName: "iPhone 16" }).
Additional Locations (2)
| } | ||
|
|
||
| return step; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported test helper function is never imported anywhere
Low Severity
expectNextStepByToolId is exported from the new next-step-assertions.ts utility file but is never imported or used anywhere in the codebase. The module also eagerly calls loadManifest() at module scope via createMcpToolToIdResolver(), meaning any test that imports this file pays the cost of parsing the full YAML manifest even if it doesn't use the helper.
commit: |


Move next-step definitions out of individual tool handlers and into YAML
manifests, introducing a declarative
nextStepsarray on each toolmanifest entry.
Previously every tool handler constructed its own
nextStepsarrayinline, duplicating structure and coupling presentation logic to tool
business logic. This made it hard to audit which tools suggest which
follow-up actions and required code changes to adjust next-step wording
or ordering.
The new system works in three layers:
Manifest templates (
manifests/tools/*.yaml) declare staticnext-step entries with a
label, optionaltoolId, optionalparams(supporting${arg}substitution from the invocationargs), and optional
priority.Template builder (
buildTemplateNextSteps) resolves templatesat invocation time, looking up target tools in the catalog and
substituting params.
Merge layer (
mergeTemplateAndResponseNextSteps) combinestemplate-driven steps with any dynamic
nextStepParamsthe toolhandler returns at runtime, so tools can still provide
context-dependent param values without owning the full next-step
structure.
Tool handlers now return only
nextStepParams(a map of toolId toparam values) when they need dynamic data, rather than constructing
entire
NextStepobjects. Most tools no longer touch next-steps at all.Includes a
NEXT_STEPS_MIGRATION_TODO.mdtracking remaining tools tomigrate and shared test helpers in
src/test-utils/next-step-assertions.ts.