Skip to content

--attach stack 4/8: Upload an asset to GitHub - #14180

Merged
BagToad merged 1 commit into
bagtoad/attach-markdown-rewritingfrom
bagtoad/attach-upload
Aug 25, 2026
Merged

BagToad merged 1 commit into
bagtoad/attach-markdown-rewritingfrom
bagtoad/attach-upload

Conversation

@BagToad

@BagToad BagToad commented Aug 18, 2026

Copy link
Copy Markdown
Member

Part of the pull request stack tracked in #14186.

Description

This is the fourth layer of the stack. It adds the uploader that posts a file to GitHub's user attachments endpoint and returns the asset URL, plus a small helper that builds the upload host for a given GitHub host. Nothing here is reachable from a command yet.

Three refusals happen before any request is made, in this order:

  1. GitHub Enterprise Server, because the endpoint does not exist there.
  2. A kind of credential the endpoint will not accept.
  3. A role on the repository below write.

The host is checked first on purpose. On an enterprise server no token and no permission would ever make an upload work, so any other order would name a fault the user cannot fix.

How did you test this change?

The whole stack was built and files were uploaded to a private test repository from two accounts. One had write access and every upload succeeded. One had read only access, and every command refused before any request left the machine, which was confirmed by counting the requests rather than by trusting the message.

The upload host was also checked against a GitHub Enterprise Cloud tenant host, where it derives from the target host correctly.

Key points

The uploader never stores the token. It is told what kind of credential is active and nothing more, and the credential actually sent is chosen by the transport from the request host, like all gh requests.

The upload host is derived from the target host, so a tenant with data residency uploads to its own host rather than to github.com.

Refusing early matters more here than in most places, because an upload cannot be undone.

Notes for reviewers

Read the three refusals first, and the reason the host is checked before the rest.

A note in the code explains why the request is built by hand rather than through the usual REST client. The endpoint requires both a content type and a content length on every request, and the shared client sets neither.

Authorship and follow-up

Who wrote this:

  • A human wrote it.
  • An agent wrote it under close human direction.
  • An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

  • @BagToad will read and reply directly. Name the account.
  • An agent will draft replies and @BagToad will read them before they are posted.
  • Nobody has explicitly committed to replying.

@BagToad BagToad changed the title Upload an asset to GitHub --attach stack 4/8: Upload an asset to GitHub Aug 18, 2026
@BagToad
BagToad marked this pull request as ready for review August 18, 2026 05:56
@BagToad
BagToad requested a review from a team as a code owner August 18, 2026 05:56
@BagToad
BagToad requested review from sergiou87 and a balanced review from Copilot August 18, 2026 05:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds infrastructure for uploading validated attachments to GitHub’s user-attachment endpoint.

Changes:

  • Adds upload-host derivation for GitHub.com and GHE tenants.
  • Adds credential, host, repository, and permission validation.
  • Implements uploads, response handling, and tests.
Show a summary per file
File Description
internal/ghinstance/host.go Builds attachment upload URLs.
internal/ghinstance/host_test.go Tests upload-host derivation.
internal/attachments/client.go Implements uploader validation and requests.
internal/attachments/client_test.go Tests uploads, validation, and failures.

Review details

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread internal/attachments/client.go
Comment thread internal/attachments/client_test.go
@BagToad BagToad mentioned this pull request Aug 18, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@babakks babakks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this one, @BagToad! 🙏

Solid, well-tested addition. The validation ordering in NewUploader is thoughtfully justified in the doc comments, the allowlist checks are clean, and the status-aware uploadError messages do a nice job translating the endpoint's quirks (like the 404-for-no-write-access case) into something actionable. The test coverage is thorough too.

A few themes in my comments, none blocking:

  • Future integration with #14104: the new DoRequest method there means we can likely drop the hand-built request and the go-gh TODO down the line. Worth a couple of pointers in the code so the follow-up is easy to find.
  • Test ergonomics: an os.Open indirection (mirroring lookupEnvFunc elsewhere) would let testAsset stub the reader and skip real temp files.
  • Small polish: an optional 429 branch for rate limits, a clearer stand-in for the intentional WRTIE typo, a couple of comments that reference far-away table rows, and some error-message nitpicks.

Nothing here needs to hold up the stack. Nice work! 🚀

Comment thread internal/attachments/client.go
Comment thread internal/attachments/client.go
Comment thread internal/attachments/client.go
Comment thread internal/attachments/client.go
Comment thread internal/attachments/client.go
Comment thread internal/attachments/client_test.go
Comment thread internal/attachments/client_test.go
Comment thread internal/attachments/client_test.go
Comment thread internal/attachments/client.go
Comment thread internal/attachments/client_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants