refactor(ai): remove legacy workers-ai framing from live review features#3270
Merged
Conversation
Workers AI has no live binding anywhere today (hosted or self-host — see CONVERGENCE_RUNBOOK.md); Codex/Claude Code via the self-host provider adapter is what actually serves every AI feature now. Update the four live AI call sites (ai-review.ts, ai-summaries.ts, ai-slop.ts, planner.ts) so their user/log-facing text and defaults reflect that: - Replace the "Workers AI binding is not configured" / "workers_ai_failed" strings with provider-neutral wording. - Drop the non-functional @cf/... default model in ai-summaries.ts in favor of the configured provider's own default. - Thread real per-call usage (provider/effort/tokens/cost) into recordAiUsageEvent for ai-summaries.ts, ai-slop.ts, and planner.ts via the shared coerceAiUsage helper (already exported from ai-review.ts), matching the tracking ai-review.ts's own review path already has. - Reword stale comments/docstrings that framed Workers AI as the live default reviewer instead of the legacy last-resort fallback.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 Gittensory is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3270 +/- ##
=======================================
Coverage 94.10% 94.11%
=======================================
Files 274 274
Lines 30092 30094 +2
Branches 10985 10985
=======================================
+ Hits 28319 28322 +3
Misses 1127 1127
+ Partials 646 645 -1
🚀 New features to boost your workflow:
|
24 tasks
This was referenced Jul 5, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 5, 2026
#3281) * fix(observability): report actual orb ai token usage on the dashboards The Codex usage dashboard's durable SQL panels filtered on model LIKE '%codex%' and only surfaced estimated_neurons, even though migration 0109 added real provider/effort/input_tokens/output_tokens/ total_tokens/cost_usd columns to ai_usage_events months ago: - Rewrite every durable panel in codex-usage.json to filter on provider = 'codex' instead of pattern-matching the model string. - Replace the "Estimated neuron usage by day" panel with a real "Durable token usage by day" panel (input/output tokens), add a new "Durable cost by day" panel, and add effort/token/cost columns to the recent-events table (keeping estimated_neurons alongside for reference, not as the only metric). - Add grafana/dashboards/orb-ai-usage.json: a provider-neutral dashboard over ai_usage_events showing every configured provider (Codex, Claude Code, OpenAI-compatible, Ollama, Anthropic BYOK) broken down by provider/model/effort/repo/PR, plus an "events missing real usage" counter to track which features still haven't threaded real usage through (see #3270). - Clarify claude-usage.json's description: it's Claude Code's own OTEL/session telemetry, not the durable ai_usage_events DB attribution — cross-references the new orb-ai-usage dashboard for that. All three dashboard JSON files are auto-provisioned from grafana/dashboards/ (no config wiring needed for the new file) and validated by scripts/validate-observability-configs.mjs. * fix(observability): coalesce empty aggregates and tighten the missing-usage filter - Wrap every sum(input_tokens|output_tokens|total_tokens|cost_usd) in coalesce(..., 0) across codex-usage.json's and orb-ai-usage.json's SQL panels, matching the existing coalesce(sum(estimated_neurons), 0) convention already used in sumAiEstimatedNeuronsSince — an empty result set now renders 0, not a blank/missing data point. - Tighten orb-ai-usage.json's "Events missing real usage" panel to filter status = 'ok' instead of a bare feature exclusion. Verified against the actual status literals ever written to ai_usage_events (quota_exceeded/disabled/unavailable never reach a DB write at all; ai_key_change audit rows use "set"/"replace"/"delete") that the old filter was overcounting: quota_exceeded and BYOK key-lifecycle rows never carry provider/usage data by design, not because a feature is missing usage tracking. Reworded the panel + dashboard descriptions to match what the tightened query actually measures. - Verified all four rewritten SQL queries against a real sqlite3 instance seeded with rows matching each status/provider combination. * fix(observability): exclude ai_key_change rows from the missing-usage counter explicitly The "Events missing real usage" panel's query relied on an implicit invariant in a different file (recordAiKeyChange's status param is typed "set"|"replace"|"delete", never "ok") to keep ai_key_change audit rows out of the count, even though the panel's own description already promised that exclusion explicitly. Add the feature <> 'ai_key_change' filter directly to the query so the panel is self-contained and doesn't silently over-count if that invariant ever changes elsewhere. Verified behavior-neutral against a seeded sqlite3 table: identical result (1) for realistic data with and without the new filter, and the new filter correctly excludes a row that violates the current type invariant (an ai_key_change row with status='ok'), which the old query would have wrongly counted. * fix(observability): catch provider-attributed rows with zero real usage too The "Events missing real usage" panel only checked provider IS NULL, missing a second reachable gap: coerceAiUsage (ai-review.ts) extracts provider independently of input_tokens/output_tokens/total_tokens/ cost_usd, so a raw provider response like {"usage": {"provider": "codex"}} with no parseable token counts yields a stored row with provider='codex' but all four numeric columns at their default. The AI reviewer's literal suggested fix (checking those four columns IS NULL) is dead code against this schema — migration 0109 declares them NOT NULL DEFAULT 0, and recordAiUsageEvent's finiteNumber() coercion means they can never be SQL NULL, only 0. Verified this with an independent two-agent analysis (both traced the same schema/ coercion/extraction code) plus two adversarial agents that built a scratch sqlite3 table from the actual migration SQL and ran the fixed predicate against 16 seeded rows total across both verification passes, confirming: the original provider-IS-NULL case still counts, the new provider-set-all-zero case now counts, a provider-set-with- real-nonzero-usage case does NOT count (no false positive), and ai_key_ change/quota_exceeded rows stay excluded. Also empirically confirmed a literal NULL insert into these columns is rejected by the schema itself. Query is now: status = 'ok' AND feature <> 'ai_key_change' AND (provider IS NULL OR (input_tokens = 0 AND output_tokens = 0 AND total_tokens = 0 AND cost_usd = 0)). Description updated to explain why a zero-value check, not an IS NULL check, is the correct "no data" signal for this schema. * fix(observability): recover quota-blocked codex reviews in the status/recent-events panels The status-breakdown and recent-events panels filtered strictly on provider = 'codex', but a quota-blocked ai_review_pr call never gets provider attribution — record() short-circuits before any provider ever runs, so `actualUsage` (and therefore `provider`) is never passed. That call's `model` field, however, is computed unconditionally via reviewerModelLabel(env, input) inside record() regardless of status, so it always reflects the configured/intended reviewer (e.g. "codex", "codex:o4-mini", "codex+claude-code") even when the call never executed. Filtering by provider alone therefore made every quota-blocked codex review invisible to these panels — a real regression, not the "disabled"/"unavailable" rows the review comment also named (those two statuses never reach a DB write at all, confirmed by tracing every return path in runGittensoryAiReview, so no filter change could have dropped them). Fixed the "Successful review records" (for consistency with the panel below), "Review record status", and "Recent Codex-attributed review events" panels to match 'codex' as a reviewer-name token in EITHER provider OR the "+"-joined model label — reusing the exact matching pattern already established (and covered by test/unit/selfhost-grafana-reporting.test.ts) for the pre-existing model-string-based filter this PR replaced. Left the token/cost sum panels (10, 13) on the strict provider filter, since an unattributed row contributes 0 to a sum regardless of which filter is used, and documented why in both the dashboard and panel descriptions to preempt an "inconsistent filters" flag on a future pass. Verified via two independent agents tracing the exact code paths (early-return statements, record()'s model/provider computation) plus two adversarial agents running the corrected predicate against 9-10 seeded sqlite3 rows each, covering: the flagged quota_exceeded/codex case, cross-provider exclusion, dual-reviewer-join and provider-model override label formats, substring false-positive safety, cross-feature scoping, and no double-counting. Final end-to-end check against all three fixed panels' exact query text confirms the expected row/count results.
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
CONVERGENCE_RUNBOOK.md's resource inventory;wrangler.jsonchas had noaiblock since the self-host cutover, and production runsAI_PROVIDER=codex). The four live AI call sites (ai-review.ts,ai-summaries.ts,ai-slop.ts,planner.ts) still carried Workers-AI-branded error strings, a dead@cf/...model default, and stale comments describing Workers AI as the default/live reviewer."Workers AI binding is not configured."/"workers_ai_failed"with provider-neutral wording ("AI provider is not configured."/"ai_summary_failed") in the three files that still had it.ai-summaries.ts's non-functional@cf/meta/llama-3.1-8b-instruct-fp8-fastdefault (the self-host adapter'sresolveModelalready discards@cf/-prefixed ids, so this string was dead weight) in favor of an empty default that lets the configured provider's own default win.ai-review.tsalready threads real per-call usage (provider/effort/input/output/total tokens, cost) intorecordAiUsageEventviacoerceAiUsage/aggregateActualUsage(migration 0109's columns) —ai-summaries.ts,ai-slop.ts, andplanner.tsdid not. ExportedcoerceAiUsagefromai-review.tsand wired it into all three so every AI feature records real usage, not just the estimated-neurons proxy, whenever the configured provider reports it.No behavior change for anyone with a configured
AI_PROVIDER(the overwhelming majority of real traffic) — this is copy/tracking-fidelity cleanup, not a routing change. A follow-up PR covers the remaining comment-only Workers AI references across the rest of the review stack, another covers a couple of stale UI strings, and a fourth updates the Grafana dashboards that still key offmodel LIKE '%codex%'/estimated_neuronsinstead of the new columns.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 typechecknpm run test:coveragelocally — every line/branch I touched is covered (verified viagit diffline-by-line cross-referenced against v8's uncovered-branch report); pre-existing gaps in these files are untouched by this diff.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitiesSafety
UI Evidencesection. — N/A, no visible UI changes (backend-only diff).Notes
ai-slop.ts's advisory path andplanner.ts's@gittensory plancommand are both live (wired intosrc/queue/processors.ts), not dead code — an earlier pass of this audit mistakenly concluded otherwise due to a NUL byte inprocessors.tsthat makes plaingrepsilently miss matches there; re-verified withgrep -a.ai-review.ts/ai-summaries.ts/ai-slop.ts's existingAI_SUMMARIES_ENABLED/AI_PUBLIC_COMMENTS_ENABLEDgating untouched — despite the summaries-sounding name, this is the established, consistently-tested shared master kill-switch across all three AI features (present since the originalai-review.tsintroduction in feat(ai-review): dual-AI maintainer review + BYOK (Phase C) #652), not a bug.BEST_REVIEW_MODELS/RELIABLE_FALLBACK_MODELS's@cf/...values themselves, the shared daily-neuron-budget gating mechanism, or BYOK usage tracking (callAiProviderdoesn't capture usage today, matchingai-review.ts's own existing BYOK path) — those are pre-existing designs/gaps out of scope for a Workers-AI-copy cleanup.