Skip to content

fix(review): meter visual vision BYOK calls#4363

Merged
JSONbored merged 5 commits into
mainfrom
codex/fix-unmetered-byok-vision-calls
Jul 10, 2026
Merged

fix(review): meter visual vision BYOK calls#4363
JSONbored merged 5 commits into
mainfrom
codex/fix-unmetered-byok-vision-calls

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The visual-vision path (runVisualVisionForAdvisory) previously decrypted a repo BYOK key and called the external provider without checking the shared per-repo daily BYOK cap, recording usage, or caching, allowing repeated contributor actions to cause maintainer-paid provider calls.
  • This change closes that financial/resource-exhaustion hole by reusing the existing BYOK quota/accounting mechanisms used by other AI features.

Description

  • Enforce the per-repo BYOK daily limit before fetching screenshots or calling the provider by invoking countByokAiEventsForRepoSince and using AI_BYOK_DAILY_REPO_LIMIT/DEFAULT_BYOK_DAILY_REPO_LIMIT with clampNumber in runVisualVisionForAdvisory (src/queue/processors.ts).
  • Record every visual-vision BYOK attempt/result via a new helper recordVisualVisionUsage that calls recordAiUsageEvent with feature: "visual_vision" and model: byok:; provider usage/token/cost fields are forwarded when available so calls count toward the shared BYOK counter (src/queue/processors.ts`).
  • Record usage on both successful returns and no-output/provider-failure outcomes so later passes are counted toward the cap (callAiProvider usage is now recorded where applicable).
  • Add regression unit tests covering a zero AI_BYOK_DAILY_REPO_LIMIT (ensures no screenshot/provider calls) and verifying successful visual BYOK calls increment the repo/day counter (test/unit/visual-vision-wiring.test.ts).
  • Minor typecheck-safe stringification of provider failure info and import adjustments to bring needed helpers into scope.

Testing

  • Ran the focused unit suite npx vitest run test/unit/visual-vision-wiring.test.ts, and the new/modified tests passed.
  • Ran type checking with npm run typecheck and it passed with no errors.
  • Attempted the full coverage run with npm run test:coverage, but the run exposed unrelated long-running/hang behavior in the existing test/unit/queue.test.ts shard and was interrupted (investigation of the unrelated suite noise is out of scope for this patch).
  • npm audit --audit-level=moderate could not complete in this environment due to the registry returning 403 Forbidden.

Codex Task

@superagent-security

superagent-security Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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

@JSONbored JSONbored self-assigned this Jul 9, 2026
@JSONbored JSONbored added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 9, 2026
# Conflicts:
#	src/queue/processors.ts
#	test/unit/visual-vision-wiring.test.ts
…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.
@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 cad1ab7 Commit Preview URL

Branch Preview URL
Jul 10 2026, 08:26 AM

@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.11%. Comparing base (efcd25d) to head (00b52c5).
⚠️ Report is 17 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4363   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files         432      432           
  Lines       38348    38368   +20     
  Branches    13979    13988    +9     
=======================================
+ Hits        36091    36111   +20     
  Misses       1600     1600           
  Partials      657      657           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.61% <100.00%> (+<0.01%) ⬆️
src/queue/processors.ts 95.41% <100.00%> (+0.02%) ⬆️
🚀 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 Jul 10, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-10 08:54:45 UTC

2 files · 1 AI reviewer · 3 blockers · readiness 93/100 · CI green · unstable

⏸️ 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.
  • 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 PR closes a real gap: runVisualVisionForAdvisory previously called the BYOK vision provider without checking the shared per-repo daily BYOK cap or recording usage, so it now calls countByokAiEventsForRepoSince against AI_BYOK_DAILY_REPO_LIMIT/DEFAULT_BYOK_DAILY_REPO_LIMIT before fetching screenshots, and records every BYOK attempt (quota-exceeded, no-output/failure, and success) via a new recordVisualVisionUsage helper feeding recordAiUsageEvent. The self-host vision path is correctly left unmetered since it consumes local/operator resources rather than shared BYOK spend, and the new tests directly exercise both the zero-limit block and the counter-increment-on-success path. The one real wrinkle is that recordVisualVisionUsage is invoked with a hardcoded status of "ok" in both the success path (src/queue/processors.ts, final block) and the no-usable-output/provider-failure path (the `if (!visionText)` block), even though the detail string distinguishes a real failure from a genuine success — only the quota_exceeded call site passes a differentiated status.

Nits — 5 non-blocking
  • src/queue/processors.ts — recordVisualVisionUsage is called with the literal status "ok" for both the true-success path and the no-usable-output/provider-failure path (`visionResponse.failure ? ... : "no usable output"` still passes status "ok"); consider passing a distinct status like "no_output" or "provider_error" so any downstream consumer of the status field can't conflate a failed call with a successful one.
  • test/unit/visual-vision-wiring.test.ts — the new "adds no finding when the provider returns 200 with no usable text" test doesn't assert the BYOK counter incremented for that branch, leaving the (now status:"ok") no-output recording path's accounting effect unverified even though it's the branch most likely to be miscounted.
  • The PR description doesn't cite an issue number this fixes/closes — worth confirming it links to an eligible open issue per repo convention before merge.
  • The flagged "secret" at test/unit/visual-vision-wiring.test.ts:241 (`sk-ant-vision-key`) is a test fixture consistent with the pre-existing key literals used elsewhere in this same file, not a real credential.
  • Differentiate the status argument passed to recordVisualVisionUsage between the true success path and the no-output/provider-failure path instead of hardcoding "ok" for both (src/queue/processors.ts).

Concerns raised — review before merging

  • 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 (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.
Signal Result Evidence
Code review ❌ 3 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: 48 registered-repo PR(s), 40 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 334 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), 334 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.

🟩 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 the manual-review Gittensor contributor context label Jul 10, 2026
…al-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.
@JSONbored JSONbored merged commit 629ac82 into main Jul 10, 2026
7 checks passed
@JSONbored JSONbored deleted the codex/fix-unmetered-byok-vision-calls branch July 10, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

1 participant