Repository navigation
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| **Key Details:** | ||
| - **Language:** Go 1.27.3+ (~38k lines of code) | ||
| - **Type:** MCP server application with CLI interface | ||
| - **Primary Package:** github-mcp-server (stdio MCP server - **this is the main focus**) |
There was a problem hiding this comment.
🧹 Primary focus stays explicit: The same local-server work and testing priority remains in the primary-focus reminder; removing this copy does not broaden the task audience.
| - **Primary Package:** github-mcp-server (stdio MCP server - **this is the main focus**) | ||
| - **Secondary Package:** mcpcurl (testing utility - don't break it, but not the priority) | ||
| - **Framework:** Uses modelcontextprotocol/go-sdk for MCP protocol, google/go-github for GitHub API | ||
| - **Size:** ~60MB repository, 70 Go files |
There was a problem hiding this comment.
🧹 Avoid a misleading size shortcut: The fixed file count understates the tracked Go inventory. It is removed rather than replaced with another snapshot; actual tool registration remains the guide to that work.
|
|
||
| **Code Quality Standards:** | ||
| - **Popular Open Source Repository** - High bar for code quality and clarity | ||
| - **Comprehension First** - Code must be clear to a wide audience |
There was a problem hiding this comment.
🧹 Clarity rule stays covered: The more complete clarity rule still requires code a wide audience can understand; this removal drops only a repeated mandate.
| │ ├── github/ # GitHub API MCP tools implementation | ||
| │ │ └── __toolsnaps__/ # Tool schema snapshots (*.snap files) | ||
| │ ├── toolsets/ # Toolset configuration & management | ||
| │ ├── inventory/ # Toolset configuration & management |
There was a problem hiding this comment.
🛠️ Find toolset configuration: The old directory name points to a missing package. The inventory builder owns toolset configuration, so the tree now points there.
| - **server.json:** MCP server metadata for registry | ||
| - **.goreleaser.yaml:** Release automation config | ||
| - **.gitignore:** Excludes bin/, dist/, vendor/, *.DS_Store, github-mcp-server binary | ||
| - **.gitignore:** Excludes bin/, dist/, vendor/, .DS_Store, github-mcp-server binary |
There was a problem hiding this comment.
🛠️ Match staging expectations: The old wildcard suggests a broader pattern than the actual ignore entry. Other listed exclusions stay unchanged.
| * 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 cannot be placed in the server-tool registry. The current constructor returns inventory.ServerTool; compatible generic-map handlers remain possible.
| ``` | ||
|
|
||
| The corresponding input schema in modelcontextprotocol/go-sdk would look like this: | ||
| The corresponding schema-bearing `mcp.Tool` component would look like this: |
There was a problem hiding this comment.
🛠️ Separate the schema from the constructor: The current constructor wraps an mcp.Tool component rather than returning that component alone. The illustration now identifies only that component, consistent with the repaired constructor type.
| ```go | ||
| ... | ||
| return mcp.Tool{ | ||
| mcp.Tool{ |
There was a problem hiding this comment.
🛠️ Avoid returning the wrong value: The registry consumes server tools, not bare mcp.Tool values. Removing return keeps this a component illustration instead of 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 JSON default: The same state default in the actual tool is JSON-encoded bytes, not a Go string. The value, enums and required fields 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 every migration breaks snapshots: The snapshot comparison reports whether a mismatch actually exists. The update command and intentional-change safeguard remain, without predicting a failure.
🧠 How this helps Copilot
The old guidance could send Copilot to a missing package or retired test helper, require a personal token when other sign-in methods work, and suggest tool migrations that do not fit the current server. These changes point Copilot to the supported setup, testing and migration contracts instead.
Automated details
Source:
curate-memoriesDream, analyzingmainatf2a07cd.