Skip to content

fix(server): load private GitHub release images - #25027

Open
maria-rcks wants to merge 2 commits into
mainfrom
maria/takeover-pr-21015
Open

maria-rcks wants to merge 2 commits into
mainfrom
maria/takeover-pr-21015

Conversation

@maria-rcks

@maria-rcks maria-rcks commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Private GitHub release images in pull request descriptions load without authentication and return 404.

Change

Recognize release-download URLs and use the existing authenticated REST service to select the exact asset by tag and name, then stream its binary API response. Keep credentials off signed object-host redirects and retain anonymous fetching for public releases when lookup is unavailable. Existing media caching is unchanged; web, desktop, and mobile share the classifier.

Verification

  • Confirmed the bug on baseline main (e3592efe7f): three regression failures before the fix, then all 14 focused tests passed.
  • vp run check:changed passed through Blacksmith on merged head ad56e4719f: format, lint, knip, GraphQL generation check, affected package typechecks, and focused tests.
  • Both independent GPT-6.1-Sol reviewers approved merged head ad56e4719f with no findings. CodeRabbit passed with no actionable findings.
  • Chrome browser check: real release images in feat(review): unify pull request and Git review across clients #19703 load through the server proxy (HTTP 200; 1320px decoded image). With direct browser release downloads denied, main shows Image unavailable and the original fix head 316b27e378 renders the images. The four task files are unchanged on the merged head. The screenshots use matching public GitHub data and a simulated anonymous 404, not a live private account.
  • GitHub CI is green on merged head ad56e4719f: all 30 reported checks completed without failures, including lint, build/typecheck, package tests, web tests, and all eight server shards.
  • Macroscope requires human approval for authentication-sensitive behavior; that approval is still outstanding.
  • Live private-account rendering is unverified: the author's private fixture returns 404 for this account. Native clients and remote/relay/tunnel execution remain unverified.

Evidence

Same PR description and crop. Direct browser release downloads return a simulated 404 in both captures; the server uses real GitHub requests.

before on main: release images are unavailable

after: release images load through the authenticated media proxy

Takeover

Replaces #21015, with thanks to @mwolson for the original implementation.

Built with GPT-6.1-Sol in the Codex harness on T3 Code.

Co-authored-by: Mike Olson <1510193+mwolson@users.noreply.github.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 25, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This change adds authenticated GitHub REST lookups and private release-asset streaming, including bearer-token and signed-redirect handling. Because it changes production authentication-sensitive media behavior, the change warrants human review.

Notes:

  • All code in this push has already been reviewed. Approvability was decided on eligibility alone.

No code changes detected at ad56e47. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB −7 B (−0.1%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB −7 B (−0.6%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 44df336 · PR result: ad56e47 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 5f1f604 and 316b27e.

Files selected for processing: 4
  • packages/source-control-github/src/server/gitHubMediaFetch.test.ts
  • packages/source-control-github/src/server/gitHubMediaFetch.ts
  • packages/source-control-github/src/shared/definition.test.ts
  • packages/source-control-github/src/shared/gitHubMedia.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The change recognizes GitHub release-download URLs as media. When credentials are available, it looks up the release through GitHub REST and fetches the matching asset. Recognized release URLs fall back to an unauthenticated request.

Changes: GitHub release media

Layer / File(s) Summary
Release URL parsing and recognition
packages/source-control-github/src/shared/gitHubMedia.ts, packages/source-control-github/src/shared/definition.test.ts
The parser recognizes valid release-download URLs, decodes their components, and rejects URLs or components that fail validation. Tests cover encoded characters, canonical hosts, and rejected URL forms.
Authenticated release asset fetching
packages/source-control-github/src/server/gitHubMediaFetch.ts, packages/source-control-github/src/server/gitHubMediaFetch.test.ts
GitHubMediaFetch looks up releases by tag, matches assets by name, and requests matching asset IDs as octet-stream. Redirect-following requests add GitHub API headers. Tests cover credentialed downloads, missing credentials, lookup failure, and unauthenticated redirects.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant GitHubMediaFetch
  participant GitHubRest
  participant GitHubAPI
  participant ObjectStore
  GitHubMediaFetch->>GitHubRest: getReleaseByTag(owner, repo, tag)
  GitHubRest-->>GitHubMediaFetch: Release assets with IDs and names
  GitHubMediaFetch->>GitHubAPI: Request matching asset by ID as octet-stream
  GitHubAPI-->>GitHubMediaFetch: Asset response or redirect
  GitHubMediaFetch->>ObjectStore: Follow redirect without authorization
Loading

Possibly related issues

  • pingdotgg/t3code#21014: The issue describes unavailable private release-asset images in PR descriptions. This change adds release URL recognition and authenticated REST asset fetching.

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 316b2

No actionable issue currently blocks merging. Live private-account rendering remains unverified.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description clearly covers the problem, change, verification, evidence, and takeover context. It does not include the required “Scope and approval” section with a triaged issue or explicit maintai… Add a “## Scope and approval” section. Include the triaged issue or discussion link and the explicit maintainer approval comment for this behavior. If the fix qualifies as an obvious focused bug fix without prior approval, explain why. Reco…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: loading private GitHub release images.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly covers the problem, change, verification, evidence, and takeover context. It does not include the required “Scope and approval” section with a triaged issue or explicit maintainer approval, and it states that Macroscope approval remains outstanding for authentication-sensitive behavior.

Resolution

Add a “## Scope and approval” section. Include the triaged issue or discussion link and the explicit maintainer approval comment for this behavior. If the fix qualifies as an obvious focused bug fix without prior approval, explain why. Record the required Macroscope or equivalent human approval before merge.

Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Co-authored-by: Mike Olson <1510193+mwolson@users.noreply.github.com>
@juliusmarminge juliusmarminge added bugfix Fix PR opened by an issue lead; enters the PR loop pr:needs-julius Needs Julius: route gate, unverifiable surface, or blocked labels Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified exact head ad56e4719f9ae47c5458eff50b98fcb68fb26026 against github/main (c677d5fc45) on this linux boat vm. no code changes were needed.

  • installed each checkout with vp i and ran one background dev server at a time with isolated state. used repository playwright and /usr/bin/google-chrome. created a project and thread through each web app, selected codex gpt-6.1-sol, and completed real provider turns running pwd and git rev-parse --is-inside-work-tree.
  • reproduced main's missing release-media support through the authenticated websocket API: assets.createUrl rejected two existing private github release assets with AssetGitHubMediaUrlValidationError.
  • the same requests on this head minted signed asset URLs and returned real image/gif responses: 2,554,314 bytes and 17,563 bytes. both decoded at 480×480. both sha256 hashes exactly matched github's live release asset metadata. the second release tag contains / and #. no requests were intercepted or mocked.
  • nearby behavior: an existing private raw image returned 200 with identical bytes on both heads. the public release image and adjacent user-attachment image in https://git.cafe/pingdotgg/t3code/pulls/19703 rendered in the actual pr panel. main used direct release URLs; this head used /api/assets. opened the image gallery, advanced to the user-attachment image, and closed it with escape.
  • 14 existing focused tests in gitHubMediaFetch.test.ts and definition.test.ts passed through blacksmith-check. current-head ci reports 31 successful checks, zero pending or failing.

short server transcript, with private contents and signed URLs omitted

matching public description and viewport below. these show preserved public rendering; the private regression is proven by the API transcript, not these screenshots.

main: public release image and neighboring attachment render

pr: the same public images render through the media proxy

pr: open release image, advance to attachment, and close gallery

Not verified

  • a release image embedded in a live private pr description. the author's private fixture returns 404 for this account. private release downloading and browser decoding were verified separately against accessible existing assets through the running server.
  • native electron, macos/windows, ios/android, and remote/relay/tunnel execution were unavailable or outside this linux web pass.
  • live anonymous fallback without github credentials was not exercised; the focused tests cover it.

screenshots were inspected at full size. the exported video and uploaded t3 browser player were checked for playback and seeking. nothing was merged, rebased, force-pushed, closed, or relabeled.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fix PR opened by an issue lead; enters the PR loop pr:needs-julius Needs Julius: route gate, unverifiable surface, or blocked size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants