Skip to content

test(visual): cover the shot placeholder cards and R2 key traversal guard#1536

Open
RenzoMXD wants to merge 2 commits into
JSONbored:mainfrom
RenzoMXD:test/visual-shot-placeholder-r2-coverage
Open

test(visual): cover the shot placeholder cards and R2 key traversal guard#1536
RenzoMXD wants to merge 2 commits into
JSONbored:mainfrom
RenzoMXD:test/visual-shot-placeholder-r2-coverage

Conversation

@RenzoMXD

Copy link
Copy Markdown

Summary

handleShot (src/review/visual/shot.ts) has three modes, but the unit suite only exercised the on-demand ?url= SSRF-render path (Mode B). Two reachable modes had no tests:

  • Mode 0 — ?placeholder=loading|failed|auth (shot.ts:188-194): the static SVG cards served for an "after" cell that is still building, has failed, or is behind an auth wall. A three-way ternary selects the card; all three arms and the content-type / cache-control headers were untested.
  • Mode A — ?key=<r2key> (shot.ts:198-208): the R2 fast-serve path. Its key guard (!key.startsWith(r2Prefix) || key.includes("..") -> 400 bad key) is the security-adjacent bit -- a crafted ?key= must never read an object outside the namespace -- and it, the 404 miss, and the 200 PNG stream were all untested.

This adds nine cases covering both modes, including both rejection arms of the traversal guard and the custom-namespace option. No production behavior changes.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage -- test/unit/visual-shot.test.ts 18/18 pass; scoped coverage on src/review/visual/shot.ts shows lines 188-208 (placeholder + key modes) now covered.
  • New behavior has unit tests (the nine added cases: three placeholder cards + unknown fall-through, valid/missing/traversal/out-of-prefix/custom-namespace key cases).

Targeted run:

npx vitest run test/unit/visual-shot.test.ts --coverage --coverage.include='src/review/visual/shot.ts'
# 18/18 passed; shot.ts lines 188-208 covered

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • No auth, cookie, CORS, GitHub App, Cloudflare, or session changes -- test-only.
  • No UI changes.
  • No docs/changelog changes.

UI Evidence

Not applicable -- test-only change with no visible UI, frontend, docs, or extension surface.

…uard

handleShot has three modes; the suite only exercised the on-demand ?url=
SSRF-render path. The placeholder card mode (?placeholder=loading|failed|auth)
and the R2 key-serve mode (?key=) — including its path-traversal / out-of-prefix
400 guard — were reachable but untested.

Add nine cases: the three SVG placeholder cards (content-type + cache header +
body marker), the unknown-placeholder fall-through, a valid-key PNG stream, a
missing-key 404, a ..-traversal rejection, an out-of-prefix rejection, and a
custom-namespace prefix check. Introduces a minimal REVIEW_AUDIT R2 stub.

No source changes; coverage only.
@RenzoMXD RenzoMXD requested a review from JSONbored as a code owner June 26, 2026 20:26
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown

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

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.41%. Comparing base (b5d575e) to head (9014ae8).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1536      +/-   ##
==========================================
+ Coverage   95.37%   95.41%   +0.04%     
==========================================
  Files         199      199              
  Lines       21546    21546              
  Branches     7791     7791              
==========================================
+ Hits        20550    20559       +9     
+ Misses        416      409       -7     
+ Partials      580      578       -2     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb

gittensory-orb Bot commented Jun 27, 2026

Copy link
Copy Markdown

Caution

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

🛑 Gittensory review — blocked

1 file · 1 AI reviewers · no blockers · readiness 66/100 · CI failing · blocked

🛑 Blocked

Review summary
Test-only PR adding nine cases that cover the two previously untested modes of `handleShot`: the three SVG placeholder cards (Mode 0) and the R2 key fast-serve path including both arms of the traversal guard (Mode A). The `r2Env` stub correctly approximates the R2 object interface via `new Response(bytes).body`, and `shotRequest` cleanly generalises the existing `request` helper without redundancy. All nine test assertions are tightly scoped and the security-relevant guard paths (`..` traversal and out-of-prefix rejection) each get their own isolated case with explicit `'bad key'` body checks.

CI checks failing

  • validate
  • lint
Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
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.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:XS.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 21 open PR(s), 9 likely reviewable, 12 unlinked.
Contributor context ✅ Confirmed Gittensor contributor RenzoMXD; Gittensor profile; 28 PR(s), 7 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 4 non-blocking
  • The `placeholder=failed` and `placeholder=auth` tests omit a `cache-control` assertion while `placeholder=loading` explicitly checks `'public, max-age=60'`; if those two cards set a different or absent header the tests silently miss it (`visual-shot.test.ts` ~lines 175-186).
  • The custom-namespace success case verifies only status and content-type, unlike the default-namespace test which also validates response body bytes via `arrayBuffer`; a routing bug that garbles the stream would pass undetected.
  • Add `cache-control` assertions to the `failed` and `auth` placeholder tests to lock in the expected header contract for all three cards, matching the completeness of the `loading` test.
  • Extend the custom-namespace test with a byte-level `arrayBuffer` assertion (mirroring the default-namespace case) to confirm the full read path works, not just the routing decision.
Review context
  • Author: RenzoMXD
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 28 PR(s), 7 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Explain no-issue PR.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • 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.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Test-only PR adding nine cases that cover the two previously untested modes of `handleShot`: the three SVG placeholder cards (Mode 0) and the R2 key fast-serve path including both arms of the traversal guard (Mode A). The `r2Env` stub correctly approximates the R2 object interface via `new Response(bytes).body`, and `shotRequest` cleanly generalises the existing `request` helper without redundancy. All nine test assertions are tightly scoped and the security-relevant guard paths (`..` traversal and out-of-prefix rejection) each get their own isolated case with explicit `'bad key'` body checks.

Nits (4)

  • The `placeholder=failed` and `placeholder=auth` tests omit a `cache-control` assertion while `placeholder=loading` explicitly checks `'public, max-age=60'`; if those two cards set a different or absent header the tests silently miss it (`visual-shot.test.ts` ~lines 175-186).
  • The custom-namespace success case verifies only status and content-type, unlike the default-namespace test which also validates response body bytes via `arrayBuffer`; a routing bug that garbles the stream would pass undetected.
  • Add `cache-control` assertions to the `failed` and `auth` placeholder tests to lock in the expected header contract for all three cards, matching the completeness of the `loading` test.
  • Extend the custom-namespace test with a byte-level `arrayBuffer` assertion (mirroring the default-namespace case) to confirm the full read path works, not just the routing decision.

🟩 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

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 27, 2026
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 - worth 0.5x multiplier. gittensor Gittensor contributor context size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants