fix: group a NULL analytics group_key as unattributed instead of failing the query (#1347)

[sakibsadmanshajib]

Closes #1347.

What was broken

GET /api/v1/accounts/current/analytics/errors?group_by=api_key&window=24h answered 500 on the console overview page, the first page a user sees after signing in. The card degraded honestly rather than showing a false zero, which is the right failure direction, but the rows it could not scan are exactly the ones an operator most wants to see.

err="usage: scan error summary row: cannot scan into dest[0] (col: group_key):
     cannot scan NULL into *string"

usage_events.api_key_id is nullable and is the only nullable grouping column: endpoint and model_alias are both NOT NULL (supabase/migrations/20260330_02_usage_accounting.sql), which is why group_by=model and group_by=endpoint answered 200 on the same account. One NULL in the grouped column failed the scan for the whole result set, so a single unattributable row took the entire summary down.

Where the NULLs actually come from

Three sources, none of them a data defect:

1. Ordinary chat traffic, which is the dominant one on the demo box today. A request billed to an account through a signed-in session carries no API key at all. The qa-tester workspace has 14 such rows in the last 24 hours, all of them successful chat_completions calls, and that is what was breaking its overview card. This is broader than the issue framing of a pre-auth error.
2. An error recorded before a key was resolved, as the issue describes.
3. A key deleted after the fact, since the foreign key is ON DELETE SET NULL.

The sibling call, checked

The issue notes analytics/usage?group_by=model answers 200 and infers the errors summary is the only affected path. That inference holds for the grouping column, not for the endpoint. GetUsageSummary and GetSpendSummary carry the identical hole on the same nullable column, and the new test shows all three failing with the same error before this change:

GetErrorSummary must tolerate a NULL group key: usage: scan error summary row: ... cannot scan NULL into *string
GetUsageSummary must tolerate a NULL group key: usage: scan usage summary row: ... cannot scan NULL into *string
GetSpendSummary must tolerate a NULL group key: usage: scan spend summary row: ... cannot scan NULL into *string

All three are fixed here rather than only the one the issue names, because all three route through the same shape and the other two would have surfaced the same 500 the first time anyone grouped them by API key. Checked the rest of the package too: ListEvents and ListAttempts already scan api_key_id into *uuid.UUID, so no other unsafe scan remains.

The fix

The scan destination is nullable, and a NULL renders as an explicit unattributed bucket rather than being dropped. Dropping those rows would trade a loud 500 for a quiet undercount, which is the worse failure. The sentinel cannot collide with a real group key, which renders as a UUID.

The console half, which the live capture forced

The plan was to leave the front end alone: the overview card sums error_count without reading the key, and the analytics table renders group_key verbatim, so the bucket reads correctly there with no change.

Capturing the proof disproved that for one widget. Top API keys by spend labels any group key missing from the account key list as "Deleted key", so with the summary now returning the bucket, that row rendered as Deleted key / unattrib on the qa-tester workspace. Nothing was deleted; that spend is chat traffic that never carried a key. It now reads Unattributed / no key on record, wording that is true under all three causes of a NULL, since nothing in the row says which one applies.

The contract constant lives in a new dependency-free apps/web-console/lib/control-plane/contract.ts rather than in client.ts, which imports next/headers and the server Supabase client: cache-metrics.ts is a pure module and a value import from client.ts would drag that graph into it. TestUnattributedGroupKeyMatchesTheConsole reads the console file and pins the Go and TypeScript literals to each other, so a rename on either side fails loudly instead of silently restoring the deleted-key label.

Tests that can actually fail

TestSummariesByAPIKey_NullKeyGroupsAsUnattributed seeds one attributed row and one genuinely NULL row, then asserts the fixture itself contains exactly one NULL api_key_id before asserting anything else, so a fixture that quietly stopped producing a NULL fails loudly rather than passing. It also asserts the attributed row keeps its own UUID and its own token, credit and count values, so a fix that collapsed every row into the bucket, or corrupted the attributed one, would not pass either.

Four mutations were run, each against the real substrate (live Postgres with the full migration chain for the Go ones, a rebuilt image for the console one):

1. Scan destination reverted to the non-nullable type: RED on all three subtests, with the production error string verbatim.
2. Sentinel replaced with an empty string: RED on all three, proving the assertions pin the bucket and not merely the absence of an error.
3. Console constant renamed to unattributed_traffic: RED on the cross-language guard.
4. Console label branch removed: RED on the vitest case, expected Deleted key to be Unattributed.

The vitest case keys its fixture by the wire literal rather than by the constant the code compares against, so it cannot pass by asserting that a value equals itself.

Verification

• go test -tags integration ./internal/usage/... -count=1 against live Postgres: pass, whole package.
• go test ./apps/control-plane/... -count=1 -short: pass.
• npm run test:unit (web console): 902 pass. The one failing suite, ci-web-e2e-secret-free, fails identically on main inside the Docker image, which carries no .github/ directory; it passes in CI.
• npm run build (web console): pass.
• node tools/lint-no-token-in-proof-captures.mjs: pass.
• ./internal/usage/... is already in the CI live-Postgres package list, so the live test runs on every PR rather than skipping silently.

Live before and after against real demo data, same account and same bearer, one control-plane deployed and unfixed and one built from this branch against a restore of the same database: 500 and 200 respectively, the 200 carrying four attributed key buckets plus {"group_key":"unattributed","error_count":0,"total_requests":14}. Full procedure in docs/proof/analytics-null-group-key-1347/capture-log.txt; screenshots posted below.

Buglog entry

{"id":"bug-2026-08-29-analytics-null-group-key","date":"2026-08-29","title":"Analytics summaries returned 500 when a grouped column was NULL","error_message":"usage: scan error summary row: cannot scan into dest[0] (col: group_key): cannot scan NULL into *string","root_cause":"GetErrorSummary, GetUsageSummary and GetSpendSummary scanned group_key into a non-nullable string. usage_events.api_key_id is nullable (NULL for chat traffic with no API key, for errors recorded before a key is resolved, and for keys deleted under ON DELETE SET NULL), so one unattributable row failed the scan for the entire result set and the console overview card rendered its error state on the first page after sign-in.","fix":"Scan group_key into a nullable destination in all three summaries and map NULL to an explicit unattributed bucket rather than dropping the row. The console Top API keys widget no longer labels that bucket Deleted key. Pinned by TestSummariesByAPIKey_NullKeyGroupsAsUnattributed, TestUnattributedGroupKeyMatchesTheConsole and a console unit case, each proven red by its own mutation.","tags":["control-plane","usage","analytics","postgres","null-scan","console-overview","web-console"],"files":["apps/control-plane/internal/usage/repository.go","apps/control-plane/internal/usage/repository_live_test.go","apps/control-plane/internal/usage/wire_contract_test.go","apps/web-console/lib/analytics/cache-metrics.ts","apps/web-console/lib/control-plane/contract.ts"]}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Warning

Review limit reached

Next included review available in 23 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 17225489-0bd9-4d02-831c-a032e3a84ee3

📥 Commits

Reviewing files that changed from the base of the PR and between 0c418db and 221ed5b.

📒 Files selected for processing (6)

• apps/control-plane/internal/usage/wire_contract_test.go
• apps/web-console/__tests__/console-cache-visibility.test.tsx
• apps/web-console/lib/analytics/cache-metrics.ts
• apps/web-console/lib/analytics/overview-fetch.ts
• apps/web-console/lib/control-plane/contract.ts
• docs/proof/analytics-null-group-key-1347/capture-log.txt

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7232854e-6446-4cb3-9bde-48e17763f418

📥 Commits

Reviewing files that changed from the base of the PR and between 37d49fc and 0c418db.

📒 Files selected for processing (5)

• apps/control-plane/internal/usage/repository.go
• apps/control-plane/internal/usage/repository_live_test.go
• apps/web-console/__tests__/console-cache-visibility.test.tsx
• apps/web-console/lib/analytics/cache-metrics.ts
• apps/web-console/lib/control-plane/client.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Analytics summaries now map NULL API-key grouping values to unattributed. The web console uses the same key to display these rows as “Unattributed” instead of “Deleted key”.

Changes

Analytics unattributed grouping

Layer / File(s)	Summary
Nullable summary grouping apps/control-plane/internal/usage/repository.go	The usage, spend, and error summaries scan nullable grouping values and map NULL values to UnattributedGroupKey.
Live summary validation apps/control-plane/internal/usage/repository_live_test.go	Live PostgreSQL tests seed attributed and NULL-key events and verify all three summaries return the expected groups and metrics.
Console unattributed rendering apps/web-console/lib/control-plane/client.ts, apps/web-console/lib/analytics/cache-metrics.ts, apps/web-console/__tests__/console-cache-visibility.test.tsx	The console shares the unattributed key and labels matching rows “Unattributed” with the suffix “no key”.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 0c418

Analytics summaries now show API-key-less activity as “unattributed” instead of failing the entire result, preserving visibility for affected accounts. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue #1347. All three analytics summaries handle NULL group_key values, preserve them under the explicit unattributed bucket, and include a live-Postgres regression test with an a…
Out of Scope Changes check	✅ Passed	The changes are within scope. The additional usage and spend summary fixes address the same nullable grouping defect, and the client labeling update supports the new unattributed bucket required by th…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: mapping NULL analytics group keys to an unattributed bucket instead of failing the query.

Full details: Linked Issues check

Explanation

The changes satisfy issue #1347. All three analytics summaries handle NULL group_key values, preserve them under the explicit unattributed bucket, and include a live-Postgres regression test with an actual NULL value. Console labeling also distinguishes unattributed rows from deleted keys.

Full details: Out of Scope Changes check

Explanation

The changes are within scope. The additional usage and spend summary fixes address the same nullable grouping defect, and the client labeling update supports the new unattributed bucket required by the fix.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/analytics-null-group-key

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sakibsadmanshajib]

Visual proof

Console overview for the qa-tester workspace, same account and same session in both shots, differing only in which control-plane the console calls. First: the deployed unfixed control-plane, Recent errors renders its failure state. Second: this branch, the card renders 11 real errors. Third: the analytics page grouped by API key on this branch, where the new bucket appears as unattributed on the chart and as Unattributed with the suffix no key in Top API keys by spend, rather than as a deleted key. Full procedure and the before and after API responses are in docs/proof/analytics-null-group-key-1347/capture-log.txt.

[sakibsadmanshajib]

Visual proof

Re-captured from the branch head after review. Top API keys by spend, row 3, now reads Unattributed with the suffix no key on record, replacing the earlier no key: the same bucket also collects rows whose key was genuinely deleted under ON DELETE SET NULL, and nothing in the row says which case it is, so the suffix has to hold for both. This supersedes the third image in the previous proof comment.

[sakibsadmanshajib]

Adversarial review, streams and dispositions

Three streams ran against this branch. Every finding is listed with what happened to it.

CodeRabbit CLI (coderabbit review --base main --committed)

Ran twice, and completed both times. The GitHub-side CodeRabbit check reports "Review rate limited", so the local CLI run is the one that carries weight here.

First pass, on the Go-only commit: no findings.

Second pass, after the console change: 2 findings, both Major.

1. cache-metrics.ts, deleted-key rows and no-key rows presented alike. Fixed in commit b377813. The suffix read "no key", which is a claim about history and is false for a key deleted under ON DELETE SET NULL; it now reads "no key on record", which holds under all three causes. The reviewer's alternative, a provenance field on the response, is not implementable from the data: the column is NULL in all three cases and nothing records which one happened, so such a field would have to invent a distinction the database does not hold. That reasoning is in the commit message.
2. docs/proof/.../capture-log.txt, remove the account identifier. Rebutted. qa-tester@hive.test is a synthetic fixture address on the RFC 2606 reserved .test TLD. It is not a mailbox, not routable, and not attached to a person. It already appears in docs/proof/public-auth-origin-2026-08-21/browser-login.log, docs/proof/chat-w0-surface-gaps/README.md, .wolf/buglog.jsonl and vendor/open-webui/src/lib/hive/displayName.ts on main. Naming the workspace is also what makes the capture checkable: an operator can go look at the same 14 rows. The repository hygiene rule this would engage bans real credentials and PII, and this is neither.

Antigravity (agy --model gemini-3.1-pro-high --effort high)

First pass, on the Go-only commit: 2 findings.

1. The live test asserted every value on the unattributed bucket but only the existence of the attributed one. Fixed in commit 0c418db. Both summaries now assert its token counts, credits and entry count.
2. The fixture guard's comment overstated what the guard buys. Fixed in the same commit. The bucket lookups would already fail if the row stopped being NULL; the guard stays because it fails pointing at the fixture instead of at the fix, and the comment now says that.

It also confirmed, independently, that the pgx pointer-to-pointer scan is the right idiom, that the sentinel cannot merge distinct buckets under any group_by, and that no other unsafe string scan remains in the package.

Second pass, on the console change: 5 findings.

1. Importing a value from client.ts drags a server-only dependency graph (next/headers, the server Supabase client) into a module that promises to stay pure. Fixed in commit e9abbb3. This one was a genuine miss: the previous commit amended that module's doc comment to permit the import, which was rationalizing the defect rather than fixing it. The constant now lives in lib/control-plane/contract.ts, which imports nothing, and client.ts is byte-identical to main again.
2. The suffix is wrong for a genuinely deleted key. Same finding as CodeRabbit's first, already fixed.
3. The Go and TypeScript constants can drift with nothing to catch it. Fixed in e9abbb3. TestUnattributedGroupKeyMatchesTheConsole reads the console file and pins the two literals to each other. It needs no database and runs in the short leg. Proven red by renaming the console constant to unattributed_traffic.
4. The vitest case asserted a tautology, keying its fixture by the same constant the code compares against. Fixed in e9abbb3. The fixture now uses the wire literal and the constant is asserted against that literal separately.
5. The analytics table and charts render the raw lowercase unattributed. Rebutted. That column renders identifiers exactly as the API returns them: raw key UUIDs, model aliases, endpoint paths. Prettifying this one value while its neighbours stay raw is the inconsistency, and rendering it verbatim is what lets an operator line a table row up against a response body.

Local test runs

• go test -tags integration ./internal/usage/... -count=1 against real Postgres with the full migration chain: pass.
• go test ./apps/control-plane/... -count=1 -short: pass.
• npm run test:unit (web console): 902 pass. The one failing suite, ci-web-e2e-secret-free, fails identically on main inside the Docker image because that image carries no .github/ directory; it passes in CI, where the full repo is checked out.
• npm run build (web console): pass.
• node tools/lint-no-token-in-proof-captures.mjs: pass.

Every mutation used to prove a test can fail is recorded in the PR body and the commit messages: the scan destination reverted, the sentinel replaced with an empty string, the console label branch removed, and the console constant renamed. Four mutations, four reds.

[sakibsadmanshajib]

Third CodeRabbit pass, on the complete branch

Ran again after the console rework: 1 finding, Minor, and it is correct.

lib/analytics/overview-fetch.ts:197, the doc comment on fetchTopKeys claimed the function "already tells those apart via null". It does not. null means one thing only: the Promise.all failed. A successful fetch can still carry the unattributed bucket or a genuinely deleted key, and neither is distinguished by the return value.

The sentence predates this branch, but my earlier edit made it worse by extending its list of causes from two to three while leaving the claim intact. Fixed in 221ed5b: the comment now says the null return distinguishes exactly one of the three, the failed fetch, and that the bucket is told apart from a gone key by its group key rather than by this return value. Comments only, no behaviour change.

That closes every finding from all three streams. Running totals across the branch:

Stream	Runs	Findings	Fixed	Rebutted
CodeRabbit CLI	3	3	2	1
Antigravity gemini-3.1-pro-high	2	7	6	1

Nothing is left open. The two rebuttals are argued in the review-streams comment above: the .test fixture address in the proof log, and rendering the raw sentinel in the analytics table alongside raw UUIDs.