Repository navigation
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| * The initial tool file and its corresponding test file will have the `//go:build ignore` build tag, as the tests will fail if the code is not ignored. The `ignore` build tag should be removed before work begins. | ||
| * The import for `github.com/mark3labs/mcp-go/mcp` should be changed to `github.com/modelcontextprotocol/go-sdk/mcp` | ||
| * The return type for the tool constructor function should be updated from `mcp.Tool, server.ToolHandlerFunc` to `(mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])`. | ||
| * The return type for the tool constructor function should be updated from `mcp.Tool, server.ToolHandlerFunc` to `inventory.ServerTool`. |
There was a problem hiding this comment.
🛠️ Return a registrable tool: The old tuple does not match the current constructor contract. The migration still supports the existing generic handler shape, but its constructor must return inventory.ServerTool.
| ``` | ||
|
|
||
| The corresponding input schema in modelcontextprotocol/go-sdk would look like this: | ||
| The corresponding `mcp.Tool` component inside the `inventory.ServerTool` constructor would look like this: |
There was a problem hiding this comment.
🛠️ Distinguish schema from constructor: The old illustration could be read as a complete constructor return. The current constructor embeds mcp.Tool inside the server-tool wrapper, so the example now identifies only that component.
| ```go | ||
| ... | ||
| return mcp.Tool{ | ||
| mcp.Tool{ |
There was a problem hiding this comment.
🛠️ Avoid the wrong return value: Returning this bare value conflicts with the server-tool constructor. Removing return keeps this a component illustration rather than a contradictory constructor body.
| Description: "Filter dependabot alerts by state. Defaults to open", | ||
| Enum: []any{"open", "fixed", "dismissed", "auto_dismissed"}, | ||
| Default: "open", | ||
| Default: json.RawMessage(`"open"`), |
There was a problem hiding this comment.
🛠️ Use a copyable default: The string default does not match the JSON-encoded default used by the tool. The allowed values and default meaning remain unchanged.
| After migrating the tool code and test file, ensure that all tests pass successfully. If any tests fail, review the error messages and adjust the migrated code as necessary to resolve any issues. If you encounter any challenges or need further assistance during the migration process, please let me know. | ||
|
|
||
| At the end of your changes, you will continue to have an issue with the `toolsnaps` tests, these validate that the schema has not changed unexpectedly. You can update the snapshots by setting `UPDATE_TOOLSNAPS=true` before running the tests, e.g.: | ||
| You can update the snapshots by setting `UPDATE_TOOLSNAPS=true` before running the tests, e.g.: |
There was a problem hiding this comment.
🛠️ Do not assume snapshot failure: The snapshot comparison succeeds when schemas are equivalent. Intentional changes may still require an update; the update command and intentional-change safeguard remain.
| 3. **ALWAYS** trust these instructions first - only search if information is incomplete or incorrect | ||
| 4. **NEVER** use `script/tag-release` or push tags | ||
| 5. **NEVER** skip `script/lint` before committing Go code changes | ||
| 6. **ALWAYS** update toolsnaps when changing MCP tool schemas |
There was a problem hiding this comment.
🧹 Schema snapshots stay required: The tool-change recipe retains generation and commit duties for intentional schema changes.
| 4. **NEVER** use `script/tag-release` or push tags | ||
| 5. **NEVER** skip `script/lint` before committing Go code changes | ||
| 6. **ALWAYS** update toolsnaps when changing MCP tool schemas | ||
| 7. **ALWAYS** run `script/generate-docs` after modifying tools |
There was a problem hiding this comment.
🧹 Generated documentation stays required: The required-command rule already covers both tool and toolset changes.
| 6. **ALWAYS** update toolsnaps when changing MCP tool schemas | ||
| 7. **ALWAYS** run `script/generate-docs` after modifying tools | ||
| 8. For specific test files, use `go test ./path -run TestName` not full suite | ||
| 9. E2E tests require PAT token - you likely cannot run them |
There was a problem hiding this comment.
🧹 Live-test prerequisites stay explicit: The e2e prerequisite and invocation retain the token warning and dedicated variable name.
| 9. E2E tests require PAT token - you likely cannot run them | ||
| 10. Toolsnaps are API documentation - treat changes seriously | ||
| 11. Build/test/lint are very fast (~1s each) - run frequently | ||
| 12. CI failures for docs-check or license-check have simple fixes (run the script) |
There was a problem hiding this comment.
🧹 Concrete recovery steps stay available: The documentation recovery and license recovery retain both failure triggers and their exact scripts.
| 10. Toolsnaps are API documentation - treat changes seriously | ||
| 11. Build/test/lint are very fast (~1s each) - run frequently | ||
| 12. CI failures for docs-check or license-check have simple fixes (run the script) | ||
| 13. mcpcurl is secondary - don't break it, but it's not the priority No newline at end of file |
There was a problem hiding this comment.
🧹 The testing utility stays protected: The secondary-package rule still says not to break mcpcurl and retains its lower priority.
There was a problem hiding this comment.
🟡 Changes recommended
The documentation change omits the supported optional GITHUB_PERSONAL_ACCESS_TOKEN authentication path, leaving an active issue that requires correction.
1 open issue
What changed in this PR
Updates Copilot guidance to reflect the repository’s current testing, authentication, workflow, startup, tool-registration, and MCP SDK migration conventions.
Changes:
- Corrects test commands and removes obsolete package, helper, and workflow references.
- Clarifies authentication options and CLI startup behavior.
- Updates MCP tool migration guidance for current inventory APIs, typed handlers, schemas, and result helpers.
| File | Summary |
|---|---|
.github/copilot-instructions.md |
Refreshes development, testing, workflow, authentication, and startup guidance. |
.github/agents/go-sdk-tool-migrator.md |
Updates MCP migration instructions for the current SDK, inventory APIs, handlers, schemas, and result helpers. |
Review effort: Lite (auto)
| ## Environment Variables | ||
|
|
||
| - **GITHUB_PERSONAL_ACCESS_TOKEN** - Required for server operation and e2e tests | ||
| - **GITHUB_HOST** - For GitHub Enterprise Server (prefix with `https://`) |
🧠 How this helps Copilot
The old guidance could lead Copilot to run a test command that selects no tests, look for a removed package or test helper, and produce tool migrations that do not fit the server. It could also prompt unnecessary personal-token setup or suggest that starting the program without a subcommand starts the server. These changes point Copilot to the supported testing, registration, and startup contracts.
Automated details
Source:
curate-memoriesDream, analyzingmainat99d6d65.Discarded guidance: The general table-driven-testing rule stays unchanged because the tool-specific testing instructions do not cover every test. The script catalogue keeps its release warning at the point where readers find the script.