GitHub returns an HTML sign-in page for some PR uploads in organizations enforcing SAML SSO. The media proxy rejects that response, leaving images and videos unavailable.
Resolve GitHub upload media in an asset-domain Effect service. On the HTML response, render the upload through GitHub's Markdown API in its repository context and fetch the returned signed URL without a bearer token. Use the upload's repository for legacy URLs, otherwise the PR context carried by the lower layer, with checkout fallback for older clients. Cache successful signed URLs within the service, bounded by time and size and keyed by account, repository, and upload; refresh a cached 403. Preserve media bytes, ranges, response policy, and login behavior. Supply the service through request middleware once per application router.
Rebuilt from @cbangera2's signed-URL fallback in #19191. Its endpoint, signed-host handling, HTML decoding, legacy URL support, and bounded cache intent are retained. The global URL-only cache and checkout-only enterprise-context assumption were replaced to match current architecture and the human EMU finding.
Depends on #21631.
Independent Opus and Sol reviews completed two full-stack rounds; the final round found no code defects.
Verification: 44 passing / 4 expected SSO failures became 48 passing server tests; the 3 mobile preview tests also pass. Related service suites and scoped server types pass, as do targeted lint/format with one existing test warning. Scoped Knip has unrelated existing export findings and none in changed files. Independent reviews found a request-provider regression and legacy context precedence issue; both are addressed without adding redundant test cases.
Before/after protocol and runtime proof: the full production dev server, including actual server.ts composition and the asset GET route, returns exact binary bytes with 200 and a cached range request with 206, one Markdown render, and no authorization on signed-host requests. The RPC schema and capability minting/resolution are real; only external GitHub responses are mocked. The initial wrong request-provider composition reproduced HTTP 500 and was corrected.
A real SAML/EMU account and GitHub's signed response remain unverified. Also unverified: full app media UI, macOS, Windows, Electron desktop, iOS, Android, native playback, and remote/relay/tunnel execution. No layout change. Human approval of the authentication-sensitive external calls and live enterprise-account verification remain merge gates. No merge is planned.
CI/review status: All executed current-head CI jobs passed, including lint, build/types, package tests, all eight server shards and the latest native fingerprint job. Web and release smoke were skipped for this layer; an earlier redundant fingerprint job was cancelled, then superseded by success. CI run. Macroscope requires human approval of the authenticated Markdown call and signed-URL cache; this remains a merge gate. No unresolved review threads at the current snapshot.
Implemented with GPT-6 Astra and GPT-6.1 Sol on Codex through T3 Code; independently reviewed by Claude Opus 5.5 and GPT-6 Sol.
Review completed
Skipped - Not affected
Added 3 commits
Verdict: Needs Human Approval
Macroscope's review found this PR requires human approval — This changes the production GitHub-media proxy to use repository-context Markdown rendering, credentials, presigned URLs, and bounded caching when direct uploads return SSO HTML. Mocked tests cover the main flows, but the authentication-sensitive external calls and broad request wiring merit human review.
You can add or adjust custom eligibility rules. Learn more.
✅ Thread transfer remains within every enforced ceiling.
ℹ️ No successful
mainbaseline artifact is available yet. This run establishes the initial measurement.
| Provider | Metric | Main baseline | This PR | Impact | PR ceiling | |
|---|---|---|---|---|---|---|
| Codex | Total thread wire | — | 4.9 KiB | — | 6.8 KiB | ✅ |
| Codex | Thread snapshot wire | — | 3.8 KiB | — | 4.9 KiB | ✅ |
| Codex | Live turn WebSocket wire | — | 1.2 KiB | — | 2.0 KiB | ✅ |
| Codex | Live turn WebSocket decoded | — | 20.8 KiB | — | 29.3 KiB | ✅ |
| Codex | Live turn messages | — | 1 | — | 8 | ✅ |
| Claude | Total thread wire | — | 5.0 KiB | — | 6.8 KiB | ✅ |
| Claude | Thread snapshot wire | — | 3.8 KiB | — | 4.9 KiB | ✅ |
| Claude | Live turn WebSocket wire | — | 1.2 KiB | — | 2.0 KiB | ✅ |
| Claude | Live turn WebSocket decoded | — | 21.2 KiB | — | 29.3 KiB | ✅ |
| Claude | Live turn messages | — | 2 | — | 8 | ✅ |
Baseline: unavailable · PR result: 5ebb9aa · Source CI: success
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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.
No actionable comments were generated in the recent review. 🎉
7bb569c7-68a9-4794-8113-0593435e3cdeReviewing files that changed from the base of the PR and between 2503bd508df4fa5f827f6c23cef252fc65596134 and 5ebb9aa198f9d91824f6cc723c649421520ede6b.
apps/server/src/assets/AssetAccess.test.tsapps/server/src/assets/GitHubMediaFetch.tsapps/server/src/http.tsapps/server/src/server.tsLimit details: You’ve used all 10 included reviews currently available.
The change adds a scoped service for GitHub media fetching. It resolves repository context for eligible uploads, renders Markdown to obtain signed private image URLs, and caches qualifying media URLs. The HTTP route provides the service and logs the upstream hostname and error tag on fetch errors.
Changes: GitHub private media fetching
| Layer / File(s) | Summary |
|---|---|
Fetch contract and redirect handling apps/server/src/assets/GitHubMediaFetch.ts | The module defines the fetch service contract and tagged error. It validates initial URLs and redirects, limits redirect hops, and wraps HTTP client failures. |
Signed URL rendering apps/server/src/assets/GitHubMediaFetch.ts | The service renders eligible GitHub uploads as Markdown. It accepts extracted URLs only when they meet the HTTPS, host, and jwt query requirements. |
Cached fetch workflow and route integration apps/server/src/assets/GitHubMediaFetch.ts, apps/server/src/http.ts, apps/server/src/server.ts, apps/server/src/assets/AssetAccess.test.ts | The service reads credentials per request, resolves repository context, and caches qualifying signed media URLs. The server provides the service to the HTTP route. Tests cover repository selection, range requests, and cache refresh cases. Fetch errors still return the existing 502 response; route warnings now record the upstream hostname and error tag. |
Priority: ➖ Normal
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
| 1 | sequenceDiagram |
| 2 | participant HTTPRoute |
| 3 | participant GitHubMediaFetch |
| 4 | participant GitHubREST |
| 5 | participant HTTPClient |
| 6 | HTTPRoute->>GitHubMediaFetch: Fetch asset with request headers |
| 7 | GitHubMediaFetch->>GitHubREST: Render eligible upload as Markdown |
| 8 | GitHubREST-->>GitHubMediaFetch: Return rendered Markdown |
| 9 | GitHubMediaFetch->>HTTPClient: Fetch signed media URL |
| 10 | HTTPClient-->>GitHubMediaFetch: Return media response |
| 11 | GitHubMediaFetch-->>HTTPRoute: Return media response or null |
Suggested reviewers: maria-rcks
5ebb9This change makes PR uploads in SSO-enforced GitHub organizations load by fetching signed media URLs. No concrete merge-blocking defect was found. Live enterprise-account verification remains a pre-merge step.
| Check name | Status | Explanation |
|---|---|---|
| Title check | Passed | The title clearly and concisely describes the main change: enabling server-side loading of pull-request uploads from SSO-enforced organizations. |
| Description check | Passed | The description covers the problem, implementation, verification results, dependencies, limitations, and remaining approval gates. It does not use a separate Scope and approval section, but it provide… |
| 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. |
Explanation
The description covers the problem, implementation, verification results, dependencies, limitations, and remaining approval gates. It does not use a separate Scope and approval section, but it provides the required scope and approval context elsewhere.
Note
This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.
Comment @coderabbitai help to get the list of available commands.