Skip to content

feat(review): vision-verify screenshot-table PRs with the local VLM - #4691

Merged
JSONbored merged 2 commits into
mainfrom
feat/screenshot-table-vision-verify
Jul 10, 2026
Merged

feat(review): vision-verify screenshot-table PRs with the local VLM#4691
JSONbored merged 2 commits into
mainfrom
feat/screenshot-table-vision-verify

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • screenshot-table-gate.ts's existing check is deterministic (no AI) and only verifies markdown STRUCTURE — a table with image-bearing cells exists. It has no way to catch a contributor pasting two identical images, an unrelated screenshot, or otherwise gaming the gate. This adds an advisory-only vision check on top, never a gate blocker.
  • Two stages, cheapest first:
    1. Free deterministic check: fetch both images (base64), compare bytes directly — identical bytes need no AI call at all.
    2. Bounded AI-vision call (BYOK or self-host env.AI_VISION, reusing Stretch: evaluate spare GPU headroom for a local vision-language model in visual-review #4335's existing binding) for pairs that survive stage 1 — asks whether the two images still look near-identical (a re-encoded duplicate a byte comparison would miss) or plausibly unrelated to the PR's stated title.
  • New URL extraction (extractTableRowImageUrls in screenshot-table-gate.ts) handles bare ![]() cells, <img src> tags, and the PR template's own clickable-thumbnail convention ([![alt](img-url)](link-url) — correctly targets the inner image URL, not the outer link).
  • Every extracted URL is routed through isSafeHttpUrl before fetching — unlike every existing fetchShotContentBlock caller (which only ever fetches the bot's own server-constructed shot URLs), these come from contributor-supplied PR body text.
  • No new config field. Gated on the existing screenshotTableGate.enabled (the deterministic gate must already be opted into) plus the same reputation/BYOK/aiReviewAllAuthors settings feat(review): advisory-only AI-vision analysis of before/after visual captures #4111's sibling visual-vision check already uses — mirrors that precedent instead of inventing a second per-repo toggle, which keeps this a much smaller, better-precedented change than a brand-new .gittensory.yml field would require (DB migration + Drizzle schema + settings resolver + OpenAPI + yml schema, none of which feat(review): advisory-only AI-vision analysis of before/after visual captures #4111 needed either).

Closes #4366

Design notes

  • Mirrors visual-findings.ts's pure-decision-logic / live-caller split exactly: src/review/visual/screenshot-table-vision.ts (gate, prompt, response parsing, finding construction — no I/O) + runScreenshotTableVisionForAdvisory in processors.ts (fetch, byte-compare, BYOK/self-host resolution, the actual AI call).
  • Strictly advisory: SCREENSHOT_TABLE_VISION_FINDING_CODE is absent from isConfiguredGateBlocker's allowlist, so this can never become a gate blocker — same pattern as visual_regression_finding, verified by a regression test asserting gate.conclusion stays "success" even under a maximally strict policy.
  • packages/gittensory-engine/src/review/screenshot-table-gate.ts is a hand-duplicated twin (engine-parity:drift-check, scripts/check-engine-parity.ts) — updated with the identical pure extraction functions (import-path-normalized per that script's own convention). The AI-calling code stays out of that package entirely, matching its "pure, deterministic only" scope.
  • Bounded to the first 2 qualifying table rows (mirrors MAX_VISION_ROUTES in the sibling feat(review): advisory-only AI-vision analysis of before/after visual captures #4111 check) so a long table can't translate into unbounded fetch/vision spend.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • 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 a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries
  • npm run engine-parity:drift-check — the hand-duplicated engine-package twin was updated in the same commit and verified in sync.

If any required check was skipped, explain why:

  • No UI/MCP/OpenAPI/wrangler-binding/build-pipeline surface touched (actionlint/test:workers/build:mcp/test:mcp-pack/ui:* not applicable — no package.json, Dockerfile, or wrangler.jsonc changes this time). test:coverage was run targeted on the exact touched/related test files (297 tests across 8 files, all green — screenshot-table-vision pure module 100% stmt/branch/func, screenshot-table-gate.ts 100% stmt, 99.2% branch (162/162, 124/125), the new processors.ts wiring exercised by 14 dedicated tests) rather than the full unsharded suite. Also ran npm --workspace @jsonbored/gittensory-engine run build and its own test suite (349 tests) directly since this PR touches that workspace package.

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. (Vision prompt explicitly forbids mentioning rewards/payouts/wallets/hotkeys/coldkeys/trust scores, matching VISUAL_VISION_SYSTEM_PROMPT's own convention.)
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session surface touched)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — internal advisory-finding path only, no external API surface)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes)
  • Visible UI changes include a UI Evidence section below. (N/A — no visible UI change; this only ever adds an advisory finding to the existing "Visual findings" collapsible)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A)

UI Evidence

N/A — no UI/frontend/docs-visible change.

Notes

  • Manual validation against real past PRs (both genuine and gamed screenshot tables) — one of this issue's deliverables — wasn't run as a separate step here: the feature is off until an operator has both screenshotTableGate.enabled: true for a repo AND a configured vision provider (BYOK or self-host AI_VISION), neither of which is currently set for any of the 3 gate repos, so there's no live PR traffic to validate against yet. The unit/wiring test suite covers the same scenarios (identical images, unrelated images via a mocked vision response, SSRF-unsafe URLs, reputation/quota gating) that manual validation would otherwise spot-check.

…4366)

screenshot-table-gate.ts only checks markdown STRUCTURE (a table with
image markup exists) -- a contributor can satisfy it with two identical
images or a screenshot unrelated to the stated change. Adds an
advisory-only check on top, split into two stages:

1. A free, deterministic byte-identical comparison (before/after fetched
   as base64 -- identical bytes need no AI call at all).
2. A bounded AI-vision call (BYOK or self-host env.AI_VISION, reusing
   #4335's binding) for genuinely different pairs, judging whether they
   still look near-identical or plausibly unrelated to the PR's title.

Extracts image URLs from table cells (screenshot-table-gate.ts's
extractTableRowImageUrls, handling bare ![]() cells, <img src>, and the
PR template's [![alt](img-url)](link-url) clickable-thumbnail
convention) and routes every URL through isSafeHttpUrl before fetching
-- unlike every other consumer of fetchShotContentBlock, these are
contributor-supplied, not server-constructed.

No new config field: gated on the existing screenshotTableGate.enabled
plus the same reputation/BYOK/aiReviewAllAuthors settings #4111's
sibling visual-vision check already uses -- mirrors that precedent
instead of inventing a second toggle.

Mirrors visual-findings.ts's pure-decision-logic/live-caller split
exactly (screenshot-table-vision.ts + runScreenshotTableVisionForAdvisory
in processors.ts). Strictly advisory: the new finding code is absent
from isConfiguredGateBlocker's allowlist, so it can never become a gate
blocker.

Also updates the hand-duplicated packages/gittensory-engine copy of
screenshot-table-gate.ts (engine-parity:drift-check) with the same pure
extraction functions.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 10, 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 021e64e Commit Preview URL

Branch Preview URL
Jul 10 2026, 08:39 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 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.15%. Comparing base (c3b10f2) to head (021e64e).
⚠️ Report is 11 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4691      +/-   ##
==========================================
+ Coverage   94.14%   94.15%   +0.01%     
==========================================
  Files         437      438       +1     
  Lines       38531    38614      +83     
  Branches    14049    14080      +31     
==========================================
+ Hits        36274    36357      +83     
  Misses       1599     1599              
  Partials      658      658              
Files with missing lines Coverage Δ
...tensory-engine/src/review/screenshot-table-gate.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 95.37% <100.00%> (+0.05%) ⬆️
src/review/screenshot-table-gate.ts 100.00% <100.00%> (ø)
src/review/visual/screenshot-table-vision.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 10, 2026
@loopover-orb

loopover-orb Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Caution

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

🛑 Gittensory review result - fixes required

Review updated: 2026-07-10 19:35:24 UTC

7 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI failing · unstable

🛑 Suggested Action - Manual Review

  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Review summary
This adds an advisory-only, two-stage vision check (free byte-compare, then bounded BYOK/self-host AI call) on top of the existing deterministic screenshot-table-gate, to catch contributors gaming the structural check with duplicate or unrelated images. The finding type is correctly kept out of `isConfiguredGateBlocker`'s allowlist and a regression test locks that in. Wiring mirrors the precedent `visual-findings.ts`/`runVisualVisionForAdvisory` pattern closely (gate → reputation → BYOK/self-host resolution → parse → append), fetch URLs are routed through `isSafeHttpUrl` before any request (with tests for both non-HTTPS and private-host cases), and pair/row counts are bounded (slice to 2) so a long table can't cause unbounded fetch/spend. The PR closes a linked issue (#4366) and stays narrowly scoped to that.

Nits — 6 non-blocking
  • The new gate logic is duplicated byte-for-byte in both `packages/gittensory-engine/src/review/screenshot-table-gate.ts` and `src/review/screenshot-table-gate.ts` — confirm this is produced by an automated sync/build step and not something a future edit to one copy will silently drift from the other.
  • `recordScreenshotTableVisionUsage`'s status ternary in processors.ts (`visionText ? "ok" : response.failure ? "error" : "ok"`) has an untested arm (empty/blank `visionText` with no `response.failure`) — likely part of why codecov/patch is short of target; worth a dedicated test.
  • The byte-identical findings built via `Array.from({ length: identicalPairCount }, ...)` in processors.ts don't carry a row/pair index the way the AI-vision findings do, so two identical rows in one PR produce two indistinguishable "Possible screenshot-table issue: identical images" findings.
  • Magic numbers `400` (max tokens) and `200` (error-message slice length) in the new `runScreenshotTableVisionForAdvisory` block in processors.ts aren't named constants, unlike similarly-purposed values elsewhere in the file.
  • `runScreenshotTableVisionForAdvisory` nests fairly deep (gate resolution inside the `fetchedPairs.length > 0` branch inside the outer try); consider pulling the BYOK/self-host provider-resolution into a small helper for readability, similar to how `visual-findings.ts` keeps decision logic separate from the live caller.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Why this is blocked

  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

CI checks failing

  • codecov/patch — 83.72% of diff hit (target 99.00%)
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #4366
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 296 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 296 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: 48 PR(s), 296 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.

🟩 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

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 10, 2026
…ion PR

- Replace a fake API-key-shaped test fixture that tripped the secret-leak
  hard blocker (generic_secret_assignment) with an explicit fake-labeled
  value; scanned the full diff against the real detector to confirm clean.
- Add missing coverage the gate flagged: the engine-package twin of
  extractTableRowImageUrls had zero test coverage (a different file from
  the host copy despite mirrored content), a fetch-partial-failure path,
  a null-author path, an explicit aiReviewProvider match/mismatch path,
  the BYOK "200 with no usable text" ternary arm, and the catch-block
  error path were all untested.
- Mark one truly unreachable branch (extractCellImageUrl's fallback null)
  with a v8-ignore, matching this file's existing defensive-code convention.
- Fix a real nit: two identical-image rows in the same PR produced two
  indistinguishable findings; each now names its row number.
@JSONbored
JSONbored merged commit 269dbd7 into main Jul 10, 2026
8 checks passed
@JSONbored
JSONbored deleted the feat/screenshot-table-vision-verify branch July 10, 2026 20:33
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(review): vision-verify screenshot-table PRs with the local VLM

1 participant