Skip to content

adding trailing slash to uploads url#1947

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.

Sign up for GitHub

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

Merged
tonytrg merged 1 commit into from
Feb 3, 2026
Merged

adding trailing slash to uploads url #1947

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/ghmcp/server.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -497,7 +497,7 @@ func newGHECHost(hostname string) (apiHost, error) {
return apiHost{}, fmt.Errorf("failed to parse GHEC GraphQL URL: %w", err)
}

uploadURL, err := url.Parse(fmt.Sprintf("https://uploads.%s", u.Hostname()))
uploadURL, err := url.Parse(fmt.Sprintf("https://uploads.%s/", u.Hostname()))

CopilotAIFeb 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change lacks test coverage. The newGHECHost function doesn't have unit tests to verify the URL construction, including the trailing slash behavior. Given that similar functions (newDotcomHost, newGHESHost) also lack direct tests, and this is an internal configuration function, consider adding tests for parseAPIHost that indirectly verify these URL constructions are correct, including the presence of trailing slashes where expected.

Copilot uses AI. Check for mistakes.

CopilotAIFeb 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description is incomplete - it's missing the Summary, Why, What changed, and all checklist sections. While the code change itself appears reasonable (adding trailing slash for consistency), the lack of context makes it difficult to understand the motivation. Was this causing an issue? Is this for consistency? Please complete the PR description following the template.

Copilot uses AI. Check for mistakes.

CopilotAIFeb 3, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistency detected: The dotcom host configuration (line 461) uses "https://uploads.github.com" without a trailing slash, while the GHEC and GHES host configurations all use trailing slashes. For consistency across the codebase, the dotcom upload URL should also have a trailing slash, or there should be a documented reason for the difference. Based on the pattern in GHES (lines 541, 544) and the REST URL patterns, all upload URLs should consistently use trailing slashes.

Copilot uses AI. Check for mistakes.

@tonytrgtonytrgFeb 3, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not know the full ghec ghes url pattern so adding the trailing slash.

if err != nil {
return apiHost{}, fmt.Errorf("failed to parse GHEC Upload URL: %w", err)
}
Expand Down
Loading