Repository navigation
Fix/3748 omit directory download url - #3848
Open
RealThanhNguyxn wants to merge 9 commits into
Open
RealThanhNguyxn wants to merge 9 commits into
RealThanhNguyxn wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The README still contradicts the new default behavior, leaving the documentation check failing, and filtered directory telemetry overstates field-selection byte savings.
2 open issues
What changed in this PR
Addresses #3748 by omitting credential-bearing download URLs from default get_file_contents directory listings, reducing exposure in model context and client logs.
Changes:
- Omits
download_urlfrom copied directory entries while preserving explicitfieldsopt-in and the large-file fallback. - Adds regression coverage for omission, metadata preservation, immutability, and protocol compatibility.
- Updates the tool input description and schema snapshot.
| File | Description |
|---|---|
pkg/github/typed_compatibility_test.go |
Tests default omission and explicit opt-in across legacy and modern protocols. |
pkg/github/repositories.go |
Removes download URLs from default directory responses without mutating API entries. |
pkg/github/repositories_test.go |
Tests token absence, retained metadata, and clone behavior, including nil entries. |
pkg/github/__toolsnaps__/get_file_contents.snap |
Records the updated field-selection description. |
Review effort: Max (dynamic)
Comment on lines
+1203
to
+1204
| } else { | ||
| payload = cloneDirectoryEntriesWithoutDownloadURLs(dirContent) |
| } | ||
| schema.Properties["fields"] = fieldsSchemaProperty( | ||
| "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", | ||
| "Subset of fields to return for each entry when the path is a directory. 'download_url' is omitted by default; request it explicitly in 'fields' when needed. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", |
./script/lint reported two G101 findings on the synthetic tokenized download URLs added for the default-omission coverage. Annotate both fixtures with the repository's nolint convention. - ./script/lint: 0 issues - ./script/test (go test -race ./...): all packages pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3748.
Omit credential-bearing
download_urlvalues fromget_file_contentsdirectory listings by default. Callers can still explicitly request this field throughfields: ["download_url"].Why
GitHub's Contents API can return short-lived tokenized raw-content URLs for files in private repositories.
Directory listings generally need metadata such as names, paths, sizes, and SHAs—not bearer URLs that independently grant access to private files and may be exposed in model context, transcripts, or client logs.
The large-file single-file fallback remains unchanged and continues to provide its download URL.
What changed
pkg/github/repositories.go: Shallow-copy directory entries and clearDownloadURLbefore marshaling the default response. Explicitfieldsprojections continue to use the original entries, preserving opt-in behavior without mutating the API response.pkg/github/repositories_test.go: Add regression coverage for default omission, token absence, retained metadata, and clone immutability, including nil entries.pkg/github/typed_compatibility_test.go: Cover default omission and explicit opt-in across legacy (2025-11-25) and modern (2026-07-28) protocol responses, including modern structured output.pkg/github/__toolsnaps__/get_file_contents.snap: Update the input description to document the default behavior.MCP impact
download_urlby default; callers can explicitly request it throughfields. The output schema remains unchanged.Prompts tested (tool changes only)
fields: ["download_url"].Manual prompt testing has not been performed.
Security / limits
Tool renaming
Lint & tests
./script/lint./script/testBoth scripts ran locally on this branch:
./script/lintreports 0 issues and./script/test(go test -race ./...) passes across all packages.Docs