fix(capture): keep sessions token columns nullable (absent = NULL, not 0) - #318
Open
khustup2 wants to merge 3 commits into
Open
fix(capture): keep sessions token columns nullable (absent = NULL, not 0)#318khustup2 wants to merge 3 commits into
khustup2 wants to merge 3 commits into
Conversation
|
Thanks for the PR. Before we can merge, please read our CLA and sign it by posting the comment below. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
📝 WalkthroughWalkthroughPer-turn token and cache columns are now emitted for every captured event. Present counts are written as values and absent counts as SQL ChangesTurn usage persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
The four per-turn token columns (input_tokens, output_tokens, cache_read_input_tokens, cache_creation_input_tokens) were designed as nullable BIGINT so an absent count (never produced) stayed distinct from a measured 0 (PRD-060a a-AC-6). That distinction is not representable on pg-deeplake: it maps a scalar SQL column to a NON-NULLABLE deeplake type (SQL nullability is ignored, `pg_to_deeplake_type` never sees attnotnull), so a capture INSERT that OMITTED a token column was rejected at flush with Failed to flush inserts: Invalid value for column 'cache_creation_input_tokens'. Reason - 'Invalid Operation: None value for scalar type' (sqlstate XX000). This surfaced in prod as a steady internal-error stream from a single org whose sessions were silently failing to persist. Collapse absent -> 0: - usageColumns always emits all four counts (the measured value, else 0) for every event; this also keeps a batched multi-row INSERT column-consistent. - The catalog columns become BIGINT NOT NULL DEFAULT 0, matching every other scalar in the array and staying heal-safe on a populated table. The reader stays null-tolerant: an all-zero session still reports ROI "absent" via roi-savings' totalTokens===0 guard, so dashboard semantics are unchanged. No DB migration is needed -- the always-emit writer supplies a value even on an existing non-nullable column.
khustup2
force-pushed
the
fix/sessions-token-columns-zero-fill
branch
from
July 16, 2026 16:25
f09cf70 to
515a536
Compare
…t 0) Reverses the zero-fill direction. The four per-turn token/cache columns (input_tokens, output_tokens, cache_read_input_tokens, cache_creation_input_tokens) stay nullable BIGINT so "token data absent" (the count was never produced) stays DISTINCT from a measured 0 — e.g. a real cache_read_input_tokens = 0 means "nothing read from cache", which must not be confused with "no usage data". The capture writer still emits all four columns on EVERY turn so a batched multi-row INSERT stays column-consistent (BUG-03), but now writes the measured value when present and SQL NULL when absent, instead of a silent 0. Schema goes back to plain `BIGINT` (no DEFAULT); it remains additive / heal-safe on a populated table. Tests updated to assert NULL for absent counts and a real 0 only for a measured 0.
khustup2
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-5: Update the v0.22.1 release note to state that absent
token/cache usage counts are persisted as 0, backed by BIGINT NOT NULL DEFAULT 0
columns, rather than SQL NULL. Preserve the distinction that measured zero
remains a valid measured value.
In `@src/daemon/runtime/capture/capture-handler.ts`:
- Around line 922-940: Restore zero-fill for absent token counts in usageColumns
so every value uses numeric 0, and update the related contract documentation; in
src/daemon/runtime/capture/capture-handler.ts:712-717 and 922-940 document and
implement absent/measured-zero collapse. Restore BIGINT NOT NULL DEFAULT 0 in
src/daemon/storage/catalog/sessions-summaries.ts:57-67, restore downstream
zero-fill and invalid-count behavior in src/hooks/normalize.ts:272-273 and
312-313, and update the listed capture tests in
src/.../capture-token-usage.test.ts:233-278 and
capture-transcript-model.test.ts:253-253 to expect "0". Update
src/daemon/storage/catalog/sessions-usage-columns.test.ts:30-36 to assert the
non-null default and matching heal/create SQL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f910e344-1e53-4057-88d5-87a2bf38b2e4
📒 Files selected for processing (8)
CHANGELOG.mdsrc/daemon/runtime/capture/capture-handler.tssrc/daemon/storage/catalog/sessions-summaries.tssrc/hooks/claude-code/transcript.tssrc/hooks/normalize.tstests/daemon/runtime/capture/capture-token-usage.test.tstests/daemon/runtime/capture/capture-transcript-model.test.tstests/daemon/storage/catalog/sessions-usage-columns.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/claude-code/transcript.ts
Comment on lines
+3
to
+5
| ## v0.22.1 — 2026-07-16 | ||
|
|
||
| Fixed a bug where token/cache usage counts could be omitted from stored session rows, causing storage write failures. All four counts are now always written on every turn — the measured value when present, or SQL NULL when absent — kept distinct from a measured 0. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the zero-fill behavior accurately.
This says absent counts are persisted as SQL NULL, but PR #318 changes the contract to write 0 for absent counts and use BIGINT NOT NULL DEFAULT 0. Update the release note to avoid misleading users about session-row semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 3 - 5, Update the v0.22.1 release note to state
that absent token/cache usage counts are persisted as 0, backed by BIGINT NOT
NULL DEFAULT 0 columns, rather than SQL NULL. Preserve the distinction that
measured zero remains a valid measured value.
Comment on lines
+922
to
+940
| * The per-turn token/cache columns EVERY event contributes to its `sessions` row | ||
| * (PRD-060a a-AC-5 / a-AC-6). All four columns are ALWAYS emitted so a batched | ||
| * multi-row INSERT stays column-consistent, but each carries the MEASURED value only | ||
| * when the assistant turn produced it — otherwise SQL NULL. | ||
| * | ||
| * NULL = "token data absent", kept DISTINCT from a measured 0 (a-AC-6): a real | ||
| * `cache_read_input_tokens = 0` means "nothing read from cache", which must not be | ||
| * confused with "the count was never produced". The zod boundary already guaranteed | ||
| * every present count is a non-negative integer, so no re-validation is needed here. | ||
| */ | ||
| function usageColumns(event: CaptureEvent): ReadonlyArray<readonly [string, number]> { | ||
| if (event.kind !== "assistant_message" || event.usage === undefined) return []; | ||
| const u = event.usage; | ||
| const cols: Array<readonly [string, number]> = []; | ||
| if (u.input !== undefined) cols.push(["input_tokens", u.input]); | ||
| if (u.output !== undefined) cols.push(["output_tokens", u.output]); | ||
| if (u.cacheRead !== undefined) cols.push(["cache_read_input_tokens", u.cacheRead]); | ||
| if (u.cacheCreation !== undefined) cols.push(["cache_creation_input_tokens", u.cacheCreation]); | ||
| return cols; | ||
| function usageColumns(event: CaptureEvent): ReadonlyArray<readonly [string, ColumnValue]> { | ||
| const u = event.kind === "assistant_message" ? event.usage : undefined; | ||
| const cell = (n: number | undefined): ColumnValue => (n === undefined ? val.raw("NULL") : val.num(n)); | ||
| return [ | ||
| ["input_tokens", cell(u?.input)], | ||
| ["output_tokens", cell(u?.output)], | ||
| ["cache_read_input_tokens", cell(u?.cacheRead)], | ||
| ["cache_creation_input_tokens", cell(u?.cacheCreation)], | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore zero-fill; SQL NULL recreates the capture failure this PR is meant to fix.
usageColumns() now sends raw NULL into pg-deeplake scalar fields. The PR objective and the changed catalog-test comment both state that these scalars reject None; existing healed tables also retain their prior non-nullable columns because this PR includes no migration. Emit val.num(n ?? 0), retain BIGINT NOT NULL DEFAULT 0, and update the contracts/tests to assert literal 0 for absent counts.
src/daemon/runtime/capture/capture-handler.ts#L922-L940: zero-fill every absent count instead of emittingval.raw("NULL").src/daemon/runtime/capture/capture-handler.ts#L712-L717: document absent and measured-zero collapse.src/daemon/storage/catalog/sessions-summaries.ts#L57-L67: restoreBIGINT NOT NULL DEFAULT 0.src/hooks/normalize.ts#L272-L273: restore the downstream zero-fill contract.src/hooks/normalize.ts#L312-L313: state invalid counts become absent and persist as0.tests/daemon/runtime/capture/capture-token-usage.test.ts#L233-L278: expect all absent token columns to be"0".tests/daemon/runtime/capture/capture-transcript-model.test.ts#L253-L253: expect"0".tests/daemon/storage/catalog/sessions-usage-columns.test.ts#L30-L36: assertBIGINT NOT NULL DEFAULT 0and matching heal/create SQL.
📍 Affects 6 files
src/daemon/runtime/capture/capture-handler.ts#L922-L940(this comment)src/daemon/runtime/capture/capture-handler.ts#L712-L717src/daemon/storage/catalog/sessions-summaries.ts#L57-L67src/hooks/normalize.ts#L272-L273src/hooks/normalize.ts#L312-L313tests/daemon/runtime/capture/capture-token-usage.test.ts#L233-L278tests/daemon/runtime/capture/capture-transcript-model.test.ts#L253-L253tests/daemon/storage/catalog/sessions-usage-columns.test.ts#L30-L36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daemon/runtime/capture/capture-handler.ts` around lines 922 - 940,
Restore zero-fill for absent token counts in usageColumns so every value uses
numeric 0, and update the related contract documentation; in
src/daemon/runtime/capture/capture-handler.ts:712-717 and 922-940 document and
implement absent/measured-zero collapse. Restore BIGINT NOT NULL DEFAULT 0 in
src/daemon/storage/catalog/sessions-summaries.ts:57-67, restore downstream
zero-fill and invalid-count behavior in src/hooks/normalize.ts:272-273 and
312-313, and update the listed capture tests in
src/.../capture-token-usage.test.ts:233-278 and
capture-transcript-model.test.ts:253-253 to expect "0". Update
src/daemon/storage/catalog/sessions-usage-columns.test.ts:30-36 to assert the
non-null default and matching heal/create SQL.
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
2 participants
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.
What
Keep the four per-turn token/cache columns on
sessions—input_tokens,output_tokens,cache_read_input_tokens,cache_creation_input_tokens— as nullableBIGINT, and have the capture writer persist SQL NULL when a count is absent rather than a silent0.Why
The absent-vs-zero distinction is load-bearing: a measured
cache_read_input_tokens = 0("nothing was read from cache" — a real datapoint) must stay distinct from "no usage data was produced". Collapsing absent →0erases that signal.The reason absent counts previously caused storage write failures was a data-engine limitation in how omitted/NULL scalar values were handled on read; that is being addressed at the engine layer separately. On the client side the fix is: always emit all four columns (so a batched multi-row INSERT stays column-consistent), writing the measured value when present and
NULLwhen absent.Changes
sessions-summaries.ts: token columns areBIGINT(nullable, noDEFAULT). Additive / heal-safe on a populated table.capture-handler.ts(usageColumns): always emits all four columns per turn — measured value or SQLNULL.NULLfor absent counts and a literal0only for a genuinely measured0.Validation
Typecheck clean; SQL-safety audit passes; 304 capture + catalog tests green; lint clean on changed files.
🤖 Generated with Claude Code
Summary by CodeRabbit
0.0semantics.