feat: add get_readme tool and refactor reference parsing #5
Conversation
- Add new get_readme MCP tool to fetch README files from GitHub repositories - Support both README.md and readme.md filename variations - Add support for action.yaml extension (in addition to action.yml) - Refactor reference parsing into shared ref.go module - Unify ActionRef and RepoRef into single Ref struct with flexible parsing - Add comprehensive tests for reference parsing - Reuse FetchRawFile helper for both actions and README fetching Breaking changes: None - all existing functionality maintained Closes #N/A
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for fetching README files from GitHub repositories by introducing a new get_readme MCP tool and refactoring the existing action reference parsing logic. The changes extract common GitHub reference parsing into a reusable ParseRef function and add new functionality to fetch README files from repositories.
Key Changes:
- Refactored action reference parsing into a generic
ParseReffunction that supports both actions and repositories - Added
ParseRepoRef,FetchReadme,GetReadme, andFetchRawFilefunctions to support README retrieval - Introduced a new
get_readmeMCP tool for fetching repository README files
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/github/ref.go | Introduces the generic Ref type and ParseRef function for parsing GitHub references, plus FetchRawFile helper for fetching raw content from GitHub CDN |
| internal/github/ref_test.go | Comprehensive tests for the new ParseRef function covering various input scenarios and edge cases |
| internal/github/actions.go | Refactors ParseActionRef to use ParseRef, adds ParseRepoRef, FetchReadme, and GetReadme functions, and updates FetchActionYAML to support both action.yml and action.yaml |
| internal/github/actions_test.go | Adds TestParseRepoRef with comprehensive test cases for the new repository reference parsing function |
| internal/cli/mcp/tools.go | Implements handleGetReadme handler for the new get_readme MCP tool with input validation and error handling |
| internal/cli/mcp/server.go | Registers the new get_readme tool with appropriate description and Sentry tracing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *ActionsService) FetchReadme(owner, repo, ref string) (string, error) { | ||
| // Try common README filenames in order of preference | ||
| readmeNames := []string{"README.md", "readme.md", "Readme.md", "README", "readme"} | ||
| urlPath := fmt.Sprintf("refs/heads/%s", ref) |
There was a problem hiding this comment.
The FetchReadme function always constructs the URL path as refs/heads/{ref}, which assumes the ref is a branch name. However, according to the function's documentation and the ParseRepoRef examples, the ref can also be a tag (e.g., "v5") or a commit SHA (e.g., "abc123def456").
When the ref is a tag or commit SHA, the URL should use refs/tags/{tag} or just {sha} respectively. Using refs/heads/ for these cases will result in 404 errors.
Consider updating the logic to handle different ref types appropriately, similar to how FetchActionYAML uses refs/tags/{version} for tags, or allow the caller to specify the URL path type.
| } | ||
|
|
||
| return data, nil | ||
| return nil, fmt.Errorf("action.yml or action.yaml not found for %s/%s@%s", owner, repo, version) |
There was a problem hiding this comment.
The function can return nil, nil when lastErr is nil, which is unreachable in practice since the loop always assigns to lastErr on error. However, if the actionFilenames slice were empty, this would be possible.
Consider simplifying the error handling by removing line 54 and only keeping the check on line 51-52, or adding a defensive check for an empty actionFilenames slice.
| return nil, fmt.Errorf("action.yml or action.yaml not found for %s/%s@%s", owner, repo, version) |
| // If we get here, none of the README files were found | ||
| if lastErr != nil { | ||
| return "", fmt.Errorf("README not found in repository %s/%s@%s: %w", owner, repo, ref, lastErr) | ||
| } | ||
| return "", fmt.Errorf("README not found in repository %s/%s@%s", owner, repo, ref) |
There was a problem hiding this comment.
Similar to the issue in FetchActionYAML, this code can return nil, "" when lastErr is nil, which is unreachable in practice since the loop always assigns to lastErr on error. However, if the readmeNames slice were empty, this would be possible.
Consider simplifying the error handling by removing line 142 and only keeping the check on line 139-140, or adding a defensive check for an empty readmeNames slice.
| func (s *ActionsService) FetchRawFile(owner, repo, urlPath, filename string) ([]byte, error) { | ||
| // Construct URL to raw file on GitHub | ||
| url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s", | ||
| owner, repo, urlPath, filename) | ||
|
|
||
| // Make HTTP GET request | ||
| resp, err := s.httpClient.Get(url) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch %s: %w", filename, err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Check for HTTP errors | ||
| if resp.StatusCode != 200 { | ||
| if resp.StatusCode == 404 { | ||
| return nil, fmt.Errorf("%s not found at %s (status: 404)", filename, url) | ||
| } | ||
| return nil, fmt.Errorf("failed to fetch %s from %s (status: %d)", filename, url, resp.StatusCode) | ||
| } | ||
|
|
||
| // Read response body | ||
| data, err := readAllBody(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read %s response: %w", filename, err) | ||
| } | ||
|
|
||
| return data, nil | ||
| } | ||
|
|
||
| // readAllBody is a helper to read all data from an io.Reader. | ||
| func readAllBody(r io.Reader) ([]byte, error) { | ||
| return io.ReadAll(r) | ||
| } |
There was a problem hiding this comment.
[nitpick] The FetchRawFile method is defined in ref.go but is a method on ActionsService, which is defined in actions.go. This creates a split definition where the type and its methods are defined across multiple files.
For better code organization and maintainability, consider either:
- Moving
FetchRawFiletoactions.gowhereActionsServiceis defined, or - Moving
ActionsServiceto a more general file if it's meant to handle both actions and general repository operations
This would make it easier to understand the complete API surface of ActionsService and follow Go best practices of keeping related code together.
| func (s *ActionsService) FetchRawFile(owner, repo, urlPath, filename string) ([]byte, error) { | |
| // Construct URL to raw file on GitHub | |
| url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s", | |
| owner, repo, urlPath, filename) | |
| // Make HTTP GET request | |
| resp, err := s.httpClient.Get(url) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch %s: %w", filename, err) | |
| } | |
| defer resp.Body.Close() | |
| // Check for HTTP errors | |
| if resp.StatusCode != 200 { | |
| if resp.StatusCode == 404 { | |
| return nil, fmt.Errorf("%s not found at %s (status: 404)", filename, url) | |
| } | |
| return nil, fmt.Errorf("failed to fetch %s from %s (status: %d)", filename, url, resp.StatusCode) | |
| } | |
| // Read response body | |
| data, err := readAllBody(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read %s response: %w", filename, err) | |
| } | |
| return data, nil | |
| } | |
| // readAllBody is a helper to read all data from an io.Reader. | |
| func readAllBody(r io.Reader) ([]byte, error) { | |
| return io.ReadAll(r) | |
| } | |
| // Moved to actions.go with ActionsService definition. |
| // readAllBody is a helper to read all data from an io.Reader. | ||
| func readAllBody(r io.Reader) ([]byte, error) { | ||
| return io.ReadAll(r) | ||
| } |
There was a problem hiding this comment.
[nitpick] The readAllBody helper function is a thin wrapper around io.ReadAll that provides no additional functionality. Since io.ReadAll is already clear and concise, this wrapper adds an unnecessary layer of indirection without adding value.
Consider using io.ReadAll directly at line 97 instead of maintaining this single-use helper function.
Summary
This PR adds a new
get_readmeMCP tool and refactors the codebase to reduce duplication and improve maintainability.New Features
1. get_readme Tool
owner/repo@branch,owner/repo@tag, orowner/repo(defaults tomain)2. Enhanced action.yml Support
action.ymlandaction.yamlfilename conventionsCode Improvements
Refactoring
internal/github/ref.gowith shared reference parsing logicActionRefandRepoRefinto a singleRefstructParseActionRefandParseRepoRefnow use the sharedParseReffunctionFetchRawFilemethod for fetching files from GitHubTesting
ParseReffunctionBreaking Changes
None - all existing functionality is maintained and backward compatible.
Files Changed
internal/github/ref.go- New shared reference parsing moduleinternal/github/ref_test.go- Tests for reference parsinginternal/github/actions.go- Refactored to use shared codeinternal/github/actions_test.go- Updated testsinternal/cli/mcp/tools.go- Added get_readme handlerinternal/cli/mcp/server.go- Registered new toolTesting
Verified both tools work correctly:
get_action_parameters- Fetches action.yml/action.yamlget_readme- Fetches README from repositories