Skip to content

feat(review): upload visual-capture screenshots to a public R2 bucket - #4209

Closed
JSONbored wants to merge 1 commit into
mainfrom
feat/visual-capture-public-r2-bucket
Closed

feat(review): upload visual-capture screenshots to a public R2 bucket#4209
JSONbored wants to merge 1 commit into
mainfrom
feat/visual-capture-public-r2-bucket

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Advances fix(review): visual-capture screenshots render as broken images on self-host (private origin) #4184. PR fix(review): warn at boot when PUBLIC_API_ORIGIN/PUBLIC_SITE_ORIGIN look private #4185 added a boot-time advisory for a PUBLIC_API_ORIGIN that looks private, but for an instance that's deliberately, permanently private (e.g. Tailscale-only, no public HTTP surface by design), no value of PUBLIC_API_ORIGIN can ever fix the broken-screenshot symptom — the whole approach of linking through this instance's own origin is the wrong shape for that deployment.
  • Adds an optional, config-driven alternative: R2_PUBLIC_ACCOUNT_ID / R2_PUBLIC_BUCKET / R2_PUBLIC_ACCESS_KEY_ID / R2_PUBLIC_SECRET_ACCESS_KEY / R2_PUBLIC_BASE_URL (all five required together, otherwise not configured — byte-identical to today). When set, every captured screenshot (before/after PNGs, pixel-diff overlays, scroll-through GIFs, and the actions-fallback workflow's own captures) is uploaded a second time to a dedicated, deliberately public Cloudflare R2 bucket, and its direct URL used in the PR comment instead of the private instance's own origin — so the private instance itself never has to answer a single public request.
  • New src/selfhost/r2-public-upload.ts: a minimal, self-host-only (Node node:crypto) AWS SigV4 request signer for R2's S3-compatible API. No new dependency — hand-rolled rather than pulling in @aws-sdk/client-s3 for the one operation this needs (a single-object PUT).
  • A cache-hit is trusted to already be mirrored to the public bucket (since both writes always happen together going forward), so there's no double-upload cost on repeat reviews of the same head SHA — documented as a one-time transition caveat for any local cache entry written before this feature was enabled.

Scope

Validation

  • git diff --check
  • npm run typecheck
  • npm run docs:drift-check
  • npm run selfhost:env-reference — regenerated, 5 new vars appear.
  • npm run db:migrations:check, npm run cf-typegen:check — clean (no schema/binding changes; these are plain self-host runtime env vars).
  • npm audit --audit-level=moderate — 0 vulnerabilities.
  • SigV4 signer independently cross-checked against a Python (hmac/hashlib) reference implementation for a fixed test vector — exact match — before trusting it in any test.
  • End-to-end verified against a real R2 bucket before writing any test: created a dedicated bucket, enabled its public dev URL, signed and uploaded a real object, fetched it back publicly, confirmed byte-identical content, then deleted the test object.
  • npx vitest run test/unit/r2-public-upload.test.ts test/unit/visual-capture.test.ts test/unit/queue.test.ts — all green; 100% line/branch/function coverage on src/selfhost/r2-public-upload.ts; every changed line/branch in src/review/visual/capture.ts and src/queue/processors.ts covered (confirmed via git diff cross-referenced against the coverage report — pre-existing gaps in code this PR doesn't touch were left alone).
  • npm run test:changed (against origin/main) — 2181/2181 passing.
  • npm run test:workers, build:mcp, test:mcp-pack, ui:openapi:check, ui:lint, ui:typecheck, ui:build — not re-run locally (no worker-entry/MCP/OpenAPI/UI-component code touched); CI runs them authoritatively.

Safety

  • No secrets committed — the R2 credentials used for end-to-end verification were passed only as inline environment variables for one-off local commands, never written to a file in this repo or worktree.
  • No behavior change for any repo/instance that doesn't opt in.
  • Non-blocking by design: an upload failure falls back to the existing private-origin URL, never breaks the review.

Notes

Remaining before this actually fixes the live instance: cut a new self-host release image including this code, deploy it, then set the five R2_PUBLIC_* values in the live .env — none of that is part of this PR.

…#4184)

For a self-host instance that stays deliberately private (Tailscale-only,
no public HTTP surface), PUBLIC_API_ORIGIN can never be fetched by GitHub,
so every visual-capture screenshot embedded in a PR comment renders as a
broken image. Adds an optional, config-driven upload path: when
R2_PUBLIC_ACCOUNT_ID/BUCKET/ACCESS_KEY_ID/SECRET_ACCESS_KEY/BASE_URL are
all set, every captured PNG/GIF (before/after shots, diff overlays, scroll
GIFs, and the actions-fallback workflow's own captures) is also uploaded
to a dedicated public R2 bucket via a hand-rolled SigV4 signer (no new
dependency), and its direct URL used instead of the private origin's.
Absent config is byte-identical to today.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 8, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui dddff88 Commit Preview URL

Branch Preview URL
Jul 08 2026, 12:12 PM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.77%. Comparing base (8048163) to head (dddff88).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4209      +/-   ##
==========================================
+ Coverage   93.75%   93.77%   +0.01%     
==========================================
  Files         389      390       +1     
  Lines       36495    36549      +54     
  Branches    13373    13384      +11     
==========================================
+ Hits        34217    34273      +56     
+ Misses       1621     1620       -1     
+ Partials      657      656       -1     
Files with missing lines Coverage Δ
src/queue/processors.ts 95.32% <100.00%> (+<0.01%) ⬆️
src/review/visual/capture.ts 93.29% <100.00%> (+1.90%) ⬆️
src/selfhost/r2-public-upload.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-08 19:54:47 UTC

9 files · 1 AI reviewer · 4 blockers · readiness 93/100 · CI failing · dirty

🛑 Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
  • AI reviewers agree on a likely critical defect: src/review/visual/capture.ts capturePage (~line 274) and captureScrollGif (~line 373), and the storeVisualCaptureFallbackShots (src/queue/processors.ts:4540) / resolveFallbackAfterShot (src/review/visual/capture.ts ~line 300) pair, all write to env.REVIEW_AUDIT before attempting the public R2 upload
  • if that upload transiently fails (network blip, R2 rate limit — a case uploadToPublicR2Bucket explicitly catches and returns undefined for), the current request correctly falls back to localUrl, but the key is now cached, so every subsequent cache-hit on that fingerprint trusts it's mirrored and returns publicUrlForKey(...) for an object that was never actually PUT to the bucket — a permanently broken public URL with no retry path, reproducing the exact bug fix(review): visual-capture screenshots render as broken images on self-host (private origin) #4184 is meant to fix. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.
  • Possible leaked secret in the diff (aws_access_key, generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Review summary
This adds an optional, config-gated mirror of visual-capture screenshots to a public R2 bucket so PR comments render even when the self-host instance's PUBLIC_API_ORIGIN is deliberately private. The SigV4 signer is hand-rolled but well-verified (cross-checked against an independent Python-computed test vector), and the default (unconfigured) path is byte-identical to today. However, all four call sites (capturePage, captureScrollGif, and the fallback-shot store/resolve pair) write the object into the local REVIEW_AUDIT cache unconditionally, then attempt the public-bucket mirror as a separate, non-atomic step whose failure is silently swallowed — a later cache hit on that same key blindly trusts it was already mirrored and returns a public URL for an object that was never actually uploaded, permanently reproducing the exact broken-image symptom this PR sets out to fix.

Blockers

  • src/review/visual/capture.ts capturePage (~line 274) and captureScrollGif (~line 373), and the storeVisualCaptureFallbackShots (src/queue/processors.ts:4540) / resolveFallbackAfterShot (src/review/visual/capture.ts ~line 300) pair, all write to env.REVIEW_AUDIT before attempting the public R2 upload; if that upload transiently fails (network blip, R2 rate limit — a case uploadToPublicR2Bucket explicitly catches and returns undefined for), the current request correctly falls back to localUrl, but the key is now cached, so every subsequent cache-hit on that fingerprint trusts it's mirrored and returns publicUrlForKey(...) for an object that was never actually PUT to the bucket — a permanently broken public URL with no retry path, reproducing the exact bug fix(review): visual-capture screenshots render as broken images on self-host (private origin) #4184 is meant to fix.
Nits — 5 non-blocking
  • uploadToPublicR2Bucket (src/selfhost/r2-public-upload.ts) swallows every failure with a bare catch and no logging, so an operator has no signal that public mirroring is silently degrading beyond noticing broken images in PR comments — worth at least a debug/warn log on the failure branch.
  • resolveR2PublicUploadConfig(env) is re-parsed from scratch on every capturePage/captureScrollGif/uploadDiffImage/resolveFallbackAfterShot invocation instead of being resolved once per request/build and threaded through — cheap, but repeated string checks on a hot path.
  • test/unit/r2-public-upload.test.ts and queue.test.ts use the well-known AWS documentation example key AKIAIOSFODNN7EXAMPLE and a matching example secret — these are the standard public test-vector credentials, not a real leak, but worth a one-line comment noting that for anyone scanning secrets later.
  • Only write to env.REVIEW_AUDIT after (or store alongside a marker of) a confirmed-or-attempted public upload, e.g. gate the local put on the public upload outcome when r2Public is configured, or store the mirrored-URL result (success/localUrl) as part of the cached value instead of re-deriving it from bucket presence alone.
  • Add a regression test that simulates: first request's public upload fails (falls back to localUrl correctly) → second request for the same fingerprint hits the REVIEW_AUDIT cache → assert it does NOT blindly return a public URL for an unmirrored object.

Why this is blocked

  • src/review/visual/capture.ts capturePage (~line 274) and captureScrollGif (~line 373), and the storeVisualCaptureFallbackShots (src/queue/processors.ts:4540) / resolveFallbackAfterShot (src/review/visual/capture.ts ~line 300) pair, all write to env.REVIEW_AUDIT before attempting the public R2 upload; if that upload transiently fails (network blip, R2 rate limit — a case uploadToPublicR2Bucket explicitly catches and returns undefined for), the current request correctly falls back to localUrl, but the key is now cached, so every subsequent cache-hit on that fingerprint trusts it's mirrored and returns publicUrlForKey(...) for an object that was never actually PUT to the bucket — a permanently broken public URL with no retry path, reproducing the exact bug fix(review): visual-capture screenshots render as broken images on self-host (private origin) #4184 is meant to fix.
  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
  • Possible leaked secret in the diff (aws_access_key, generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

CI checks failing

  • validate
  • validate-code
Signal Result Evidence
Code review ❌ 4 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 43 merged, 497 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 52 PR(s), 497 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 52 PR(s), 497 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop after /
/ mobile after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@JSONbored

Copy link
Copy Markdown
Owner Author

Closing as superseded by #4203, which landed a more complete version of this same idea (advances #4184) — a generic S3-compatible backend (aws4fetch, region-aware, bounded retry budget, delete() parity) replacing REVIEW_AUDIT's own backend selection, rather than this PR's parallel-upload-to-a-second-bucket approach. #4203 was opened before this PR and is already merged.

The bucket + credentials created for this PR (gittensory-visual-capture-public) can still be reused as the actual S3-compatible bucket #4203's REVIEW_AUDIT_S3_* config points at — no need to provision a second one.

@JSONbored JSONbored closed this Jul 8, 2026
@JSONbored
JSONbored deleted the feat/visual-capture-public-r2-bucket branch July 19, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant