-
Notifications
You must be signed in to change notification settings - Fork 3.6k
refactor: avoid utils package name to fix revive issue #1895
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?
refactor: avoid utils package name to fix revive issue #1895
Conversation
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.
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. |
1e7807f to
5076b51
Compare
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.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
|
@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. |
23b4bc9 to
f2deffa
Compare
f2deffa to
4dfcc2c
Compare
|
@SamMorrowDrums I have updated the PR and fixed merge conflicts. While it was under review, two more files were added to the
|
Summary
Rename the
pkg/utilspackage topkg/mcpresultandpkg/githubapiwith clearer function names to resolve arevive.var-namingissue.Why
The
pkg/utilspackage name is "bad", triggering a revive linter warning. Renaming topkg/mcpresultandpkg/githubapimakes 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/githubapipackagemcpresultorgithubapidirectlyMCP 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