refactor: rename utils to mcpresult to fix revive issue#1895
refactor: rename utils to mcpresult to fix revive issue#1895alexandear wants to merge 2 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the generic pkg/utils tool-result helpers into a more explicit pkg/mcpresult package, then updates all call sites while keeping a thin deprecated compatibility layer for backwards compatibility.
Changes:
- Introduces
pkg/mcpresultwithNewText,NewError,NewErrorFromErr, andNewResourcehelpers wrapping MCP call-tool results. - Converts all existing users in
pkg/githubandpkg/errorsfromutils.NewToolResult*tomcpresult.*. - Leaves
pkg/utils/result.goas a deprecated shim that delegates tomcpresult, preserving the old API surface.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/result.go | Turns utils result helpers into a deprecated shim that delegates to mcpresult, maintaining backward-compatible APIs. |
| pkg/mcpresult/mcpresult.go | Adds the new focused mcpresult package implementing the actual result-construction helpers used across the codebase. |
| pkg/github/server.go | Updates the JSON-marshalling helper to use mcpresult for success and error tool results. |
| pkg/github/security_advisories.go | Switches all advisory tools to construct error/success tool results via mcpresult. |
| pkg/github/secret_scanning.go | Routes secret scanning tools’ success and error responses through mcpresult. |
| pkg/github/search_utils.go | Uses mcpresult for search error handling and response marshalling. |
| pkg/github/search.go | Migrates repository/code/user/org search tools to the new mcpresult helpers. |
| pkg/github/repositories_test.go | Adjusts test expectations to use mcpresult.NewError rather than the deprecated utils helpers. |
| pkg/github/repositories_helper.go | Updates repository helper responses (match results and errors) to use mcpresult. |
| pkg/github/repositories.go | Replaces all repository tool result construction (commits, branches, files, tags, releases, stars) with mcpresult calls. |
| pkg/github/pullrequests.go | Refactors pull request tools (read, write, reviews, Copilot review) to use mcpresult for text and error responses. |
| pkg/github/projects.go | Updates projects tools (list/get/update/delete items, fields, discovery helpers) to rely on mcpresult for tool results. |
| pkg/github/notifications.go | Switches notification tools (list, dismiss, mark read, manage subscriptions) to mcpresult helpers. |
| pkg/github/labels.go | Adapts label read/write tools to construct responses via mcpresult. |
| pkg/github/issues.go | Uses mcpresult for all issue-related responses, including lockdown errors, sub-issues, comments, and issue create/update flows. |
| pkg/github/git.go | Changes repository tree tool result construction from utils to mcpresult. |
| pkg/github/gists.go | Migrates gist listing/reading/creating/updating tools to mcpresult. |
| pkg/github/feature_flags_test.go | Updates the test-only HelloWorldTool to use mcpresult in its test assertions. |
| pkg/github/dynamic_tools.go | Uses mcpresult for dynamic toolset enable/list helpers instead of utils. |
| pkg/github/discussions.go | Routes discussion tools (list, get, comments, categories) through mcpresult for text/error handling. |
| pkg/github/dependabot.go | Updates Dependabot alert tools to use mcpresult for results and error wrapping. |
| pkg/github/context_tools.go | Switches context tools (GetMe, teams, team members) to use mcpresult. |
| pkg/github/code_scanning.go | Refactors code scanning alert tools to use mcpresult helpers. |
| pkg/github/actions.go | Systematically replaces all Actions tools’ result construction (runs, jobs, logs, artifacts, reruns, cancellation) with mcpresult. |
| pkg/errors/error.go | Changes GitHub error helpers to construct MCP error results via mcpresult.NewErrorFromErr while preserving existing behavior. |
d5e5686 to
1e7807f
Compare
1e7807f to
5076b51
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@alexandear am I missing something? The linting setup in CI doesn't seem to flag this, and if we are making lint fix PRs I would expect that they include enabling rules in CI etc. otherwise there is nothing to prevent regression. |
| @@ -1,49 +1,27 @@ | |||
| package utils //nolint:revive //TODO: figure out a better name for this package | |||
| // Package utils is deprecated: use pkg/mcpresult instead | |||
| package utils //nolint:revive // vague package name | |||
There was a problem hiding this comment.
@SamMorrowDrums this //nolint:revive directive is reason the issue is not shown. If I remove it, we see the following:
$ ./script/lint
pkg/utils/result.go:2:9: var-naming: avoid meaningless package names (revive)
package utils
^
1 issues:
* revive: 1I intentionally kept this utils package to preserve backward compatibility. If you want, I can remove this package entirely, but that would be a breaking change for anyone who imports pkg/utils in their code.
There was a problem hiding this comment.
Right of course, sorry 😅. Was just passing through when I had a glance. That makes total sense and thank you for looking at it!
Summary
Rename the
pkg/utilspackage topkg/mcpresultwith clearer function names to resolve arevive.var-namingissue while maintaining backward compatibility through deprecated aliases.Why
The
pkg/utilspackage name is "bad", triggering a revive linter warning. Renaming topkg/mcpresultmakes the purpose explicit and improves code clarity. Function names are simplified fromNewToolResultXtoNewXto align with Go naming conventions and reduce redundancy.What changed
pkg/mcpresultpackage with clearer function names:NewText,NewError,NewErrorFromErr,NewResourcepkg/githubandpkg/errorsto import and usemcpresultdirectlypkg/utilsto a compatibility layer with deprecated wrapper functions that delegate tomcpresultMCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goThis refactoring renames internal utility functions and packages, not MCP tools.
Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs