feat(review): route visual-vision to a local self-host provider (#4335) - #4353
Merged
Conversation
The visual-vision advisory (#4111) required a maintainer BYOK (anthropic/openai) key -- Workers AI is retired and the subscription CLIs (claude-code/codex) can't consume inline image bytes, so BYOK was the only option. Adds a dedicated AI_VISION binding (mirrors AI_EMBED's separation from the review chain) so self-host operators can point visual-vision at a local Ollama vision-language model instead, with BYOK still preferred when both are configured. evaluateVisualVisionGate's provider check is broadened additively (providerKey OR selfHostVisionAvailable) -- byte-identical behavior for any deployment that doesn't set AI_VISION.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | 6685eea | Commit Preview URL Branch Preview URL |
Jul 09 2026, 03:13 AM |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4353 +/- ##
==========================================
- Coverage 93.95% 93.95% -0.01%
==========================================
Files 399 399
Lines 36786 36796 +10
Branches 13440 13443 +3
==========================================
+ Hits 34561 34570 +9
Misses 1569 1569
- Partials 656 657 +1
🚀 New features to boost your workflow:
|
The BYOK-vs-self-host dispatch was a multi-line ternary assigning into visionText, with a separate `if (!visionText) return` below it. Both paths were exercised by tests (confirmed locally), but CI's coverage merge reported one branch of the `if` as never taken -- restructured into a plain if/else with the SAME behavior, which resolves the ambiguity cleanly (verified: both branches now hit in a full local coverage run).
10 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 10, 2026
…path rewrite #4335/#4353 rewrote runVisualVisionForAdvisory into a BYOK-vs-self-host dual path 42 minutes before this PR opened, so GitHub couldn't auto-merge the original diff. Reapplies the same BYOK daily-cap check + recordVisualVisionUsage accounting, scoped to only the BYOK branch (self-host consumes the operator's own resources and was never part of this spend surface). Moves the cap check back before the shot-fetching loop, matching this PR's own test name/intent ("...before fetching screenshots") -- my first pass had it after the loop. Also updates two tests for unrelated main drift since this PR opened: the "declines when no route crossed..." test now needs stubMinerCheckOnly() for #4513's install-wide reputation check, and both new tests need `mode: "live"` for the #token-bleed-spend-gate paused-mode field.
JSONbored
added a commit
that referenced
this pull request
Jul 10, 2026
* fix(review): meter visual vision BYOK calls * fix(review): reapply BYOK vision metering against the self-host dual-path rewrite #4335/#4353 rewrote runVisualVisionForAdvisory into a BYOK-vs-self-host dual path 42 minutes before this PR opened, so GitHub couldn't auto-merge the original diff. Reapplies the same BYOK daily-cap check + recordVisualVisionUsage accounting, scoped to only the BYOK branch (self-host consumes the operator's own resources and was never part of this spend surface). Moves the cap check back before the shot-fetching loop, matching this PR's own test name/intent ("...before fetching screenshots") -- my first pass had it after the loop. Also updates two tests for unrelated main drift since this PR opened: the "declines when no route crossed..." test now needs stubMinerCheckOnly() for #4513's install-wide reputation check, and both new tests need `mode: "live"` for the #token-bleed-spend-gate paused-mode field. * fix(review): distinguish a real provider failure from success in visual-vision's usage status recordVisualVisionUsage hardcoded status: "ok" even when visionResponse.failure was set (a genuine timeout/http_error/exception) -- the detail string already distinguished this from a completed-but-empty response, but that distinction was discarded at the status level. Now uses "error" for a genuine failure, matching runAgentSummary's existing convention (services/ai-summaries.ts) for the same shape of problem. Since countByokAiEventsForRepoSince/sumByokAiUsageForRepoSince gate on status = "ok", introducing "error" required deciding whether it should still count toward the daily BYOK cap. It must: excluding failed attempts would turn a flaky or misconfigured provider into a way to bypass the cap entirely via forced failures, defeating the whole point of #4363's own fix. Switched both functions from an exact "ok" match to an explicit BYOK_SPEND_ATTEMPT_STATUSES allowlist (["ok", "error"]). Caught during review: an exclusion-based filter (!= "quota_exceeded") is NOT safe here, because ai_usage_events is also reused for BYOK key-lifecycle audit rows (recordAiKeyChange's "set"/"replace"/"delete") whose model is also byok:<provider>-prefixed -- an exclusion would have silently started counting those as spend. Caught by a new regression test before it shipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AI_VISIONbinding (mirrorsAI_EMBED's existing separation from the review chain — same pattern, same reasoning) so self-host operators can point visual-vision at a local Ollama vision-language model instead. BYOK is still preferred when both are configured (bills the maintainer's own account, same convention as every other dual-path AI call site in this codebase).evaluateVisualVisionGate's provider check is broadened additively —providerKey OR selfHostVisionAvailable— so behavior is byte-identical for any deployment that doesn't setAI_VISION(existing BYOK-only deployments are unaffected).ollama/openai-compatibleHTTP provider path already forwards images correctly (toOpenAiMessageContentinsrc/selfhost/ai.ts), it just wasn't wired as an option for this specific call site.Closes #4335. Note on methodology: #4335 asked for a side-by-side quality comparison against the current cloud-based analysis on real past PR screenshots before adopting. That comparison turned out to be blocked — no BYOK vision key was configured for any repo in this deployment, so there was no real cloud-path baseline to compare against (the feature had never actually fired in production). Rather than leave this stalled, the maintainer made a direct decision to adopt local Ollama vision given the hardware is provisioned for it; this PR is that decision implemented, not the outcome of the originally-planned comparison. Closing #4335 on that basis rather than leaving an evaluation issue open for a question that's already been answered.
Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlint(vianpm run test:ci)npm run typecheck(vianpm run test:ci)npm run test:coveragelocally —visual-findings.tsgate change and the newrunSelfHostVisualVisionhelper/call-site branch inprocessors.tsboth fully covered (both BYOK-preferred and self-host-only paths, plus fail-safe: no binding, no.run, thrown error, empty response)npm run test:workers(vianpm run test:ci)npm run build:mcp/npm run test:mcp-pack(vianpm run test:ci)npm run ui:openapi:check/npm run ui:lint/npm run ui:typecheck/npm run ui:build(vianpm run test:ci) — no UI/API surface touchednpm audit --audit-level=moderate— 0 vulnerabilitiestest/unit/visual-findings.test.tsandtest/unit/visual-vision-wiring.test.ts)src/server.tsis excluded from Codecov (codecov.yml) — validated by the self-host integration workflow, not unit-coverable without booting a real server; the newAI_VISIONwiring there mirrors the existing (also-excluded)AI_EMBEDblock exactly.Ran
npm run test:ci(full unsharded suite) end-to-end — green.Safety
UI Evidencesection. — N/A, no UI change..env.exampledocuments every new var;selfhost-env-reference.tsregenerated vianpm run selfhost:env-reference).Notes
cf-typegenregeneration needed.npm run selfhost:env-referencewas run and its output committed (server.ts is a scanned source root for that generator).