fix(ai-cost): stop the Claude Team path losing data it already has - #2607
Merged
Conversation
… instance A class table is written by every connector that feeds it, and each runs on its own schedule. `_version` is a timestamp whose meaning differs per producer — extraction time here, write time elsewhere — so a single `max(_version)` over the whole table lets whichever producer commits first raise the boundary above another producer's rows. Those rows are then below it forever: nothing re-reads them, and until now nothing reported the loss either. The boundary moves to `max(_version)` per (tenant, source instance), the shape already merged for the crm classes and class_person_attribution_claims. The column, its values and its role in ReplacingMergeTree are untouched — only the group the maximum is taken over. `coalesce(max_version, 0)` is load-bearing: a source absent from the target has no boundary row, and comparing against NULL would drop every row of every new source. assert_ai_staging_rows_reach_silver reports what staging can prove the class is missing, per source. The convention checker prescribed the table-wide form and would have kept reintroducing it. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
Claude Team staging dropped every row whose seat status was not active. The vendor restates that status for every day it re-reads, so it describes the seat as of the read and not as of the metric date: the day a person's seat is deactivated, the filter deleted their entire history. Nothing about a seat's present state licenses deleting the work it recorded. The value is carried in the new class-contract column seat_status, appended last so the contract grows without a rebuild, and emission is gated on the activity counters instead — the same gate every other contributor to the class already uses, and the one the contract asks for. Sources with no seat lifecycle concept emit NULL, which reaches gold as 'unknown'. Also documents what conversation_count actually holds: for the dev-agent tools the vendors publish no separate conversation counter, so the connectors map their session or thread count into it and the value equals session_count on those rows. That is the intended mapping rather than a duplication to collapse, and it does not hold on the assistant class, where the two differ per surface. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
… in silver seat_status reaches gold as a dimension of the ai_usage source, so the activity of people who have since left can be told apart rather than silently mixed in or, as before, deleted. prs_with_cc_count and prs_total_count have been in silver unread since the class gained them. They are served as ai.prs_with_assistant and ai.prs_total, emitted only where the vendor supplies a value so an organisation without the vendor's code-host connection returns no value rather than a zero that would assert "no pull requests involved the assistant". ai.prs_total is context for the other measure, not a goal of its own: it is the vendor's count over the vendor's own window, which need not be the day it is reported against. ai.dev_conversations gains the explanation its number needed — for the agent tools it counts sessions or threads, because no vendor publishes a separate conversation counter. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
identity_resolution_coverage listed every evidence relation except the seat one, so the share of billed money that reaches no person was measured nowhere. It is the one relation whose unresolved rows are money rather than activity, which makes its match rate the most consequential of the set. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
The code-metrics response carries org-level pull-request attribution and top-user rankings beside the per-user array. They were read and discarded on every sync; the vendor keeps no history, so a value not captured on the day it was published is gone. claude_team_code_metrics_org keeps them as their own stream — one request and one row per day, with the per-user array stripped so the roster is not stored twice. Copying them onto every user row would multiply one organisation fact by the roster and make any sum wrong, which is why they are not folded into the existing stream. disabled_reason, disabled_until and account_name join the overage extras blob, where a zero spend can now be told apart from a seat the vendor blocked. full_refresh=false guards claude_team__ai_overage. That model is the only place a past month's closing spend exists — the endpoint keeps no history — so a rebuild from Bronze deletes those months rather than reproducing them, and reconcile-connectors dispatches the rebuild automatically on a MAJOR bump. The README's key for the overage stream still described the pre-#2528 shape. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
Audit D1 said to keep the status filter and add a check counting non-active rows. That reading missed that the filter is retroactive, so it is replaced by the decision the code now implements. D5 kept the PR counters out of the decomposition; the counters themselves are now served, while anything per-PR stays #1660's and FR-9's. PRD OD-5 and the research note both rested on "a deactivated person keeps an overage row but loses activity rows". That half no longer holds — the activity rows stay. Their conclusion is unchanged for the other reason: a seat with no usage has no activity row to inherit a state from. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
hello1101n
approved these changes
Aug 17, 2026
Gregory91G
enabled auto-merge
August 17, 2026 13:47
This was referenced Aug 18, 2026
Gregory91G
added a commit
to Gregory91G/insight
that referenced
this pull request
Aug 18, 2026
constructorfabric#2607 added the column to the class contract but to no migration, so only a fresh cluster got it: the DDL snapshot is CREATE TABLE IF NOT EXISTS, and dbt appends it (on_schema_change=append_new_columns) only when the silver model runs — which happens at connector-sync time, after the deploy. The migrate hook builds gold in between, and ai_metric_evidence reads seat_status, so upgrading an existing installation failed the hook and rolled the release back. The staging heal is not optional. A class unions its staging members positionally, and each member gains the column on its own connector's sync, so an install with two AI vendors gets `UNION different number of columns` on the first sync after the upgrade — a sync failure traded for the deploy failure. Healing all four contributors in one deploy closes that window. AFTER _version, matching the position the column holds in the staging projections and in the DDL snapshot; MODIFY converges an instance where an out-of-band ALTER placed it elsewhere. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
hello1101n
pushed a commit
to hello1101n/insight
that referenced
this pull request
Aug 18, 2026
…ade (constructorfabric#2629) constructorfabric#2607 added the column to the class contract but to no migration, so only a fresh cluster got it: the DDL snapshot is CREATE TABLE IF NOT EXISTS, and dbt appends it (on_schema_change=append_new_columns) only when the silver model runs — which happens at connector-sync time, after the deploy. The migrate hook builds gold in between, and ai_metric_evidence reads seat_status, so upgrading an existing installation failed the hook and rolled the release back. The staging heal is not optional. A class unions its staging members positionally, and each member gains the column on its own connector's sync, so an install with two AI vendors gets `UNION different number of columns` on the first sync after the upgrade — a sync failure traded for the deploy failure. Healing all four contributors in one deploy closes that window. AFTER _version, matching the position the column holds in the staging projections and in the DDL snapshot; MODIFY converges an instance where an out-of-band ALTER placed it elsewhere. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
This was referenced Aug 19, 2026
cyberantonz
pushed a commit
to cyberantonz/insight
that referenced
this pull request
Aug 25, 2026
…ource (constructorfabric#2621) constructorfabric#2607 replaced the table-wide `_version > max(_version)` on the AI silver classes with `silver_incremental_watermark`, because a class is written by every connector feeding it and each producer stamps `_version` from its own clock: whichever commits first raises the boundary above another producer's rows, and those rows stay below it forever, silently. `class_ai_invoice` landed from a branch that predated that fix, so it is the only AI class still carrying the pattern. The same boundary sits one layer down, in `claude_team__ai_invoice`, and fixing only the class would have changed nothing observable: a second instance's rows are dropped at staging before silver ever sees them. Two instances of one connector do write to one staging table — that is why `insight_source_id` is part of every unique_key and why `check_connection` refuses an empty one. Both layers now take the same macro, rather than a second implementation of it, and staging's empty-table guard goes with it: a `coalesce` to the epoch already admits an instance the table has never seen, which is what the guard stood in for. The e2e gains a second source instance read BEFORE everything the class already holds — the shape a per-table boundary silently drops. Reverting either layer alone fails it. `assert_ai_staging_rows_reach_silver` deliberately does NOT gain this class. Its `ref()` calls sit inside a loop over `materialised_models_for_tag`, which is empty at parse time, so dbt records no dependencies and the test errors with "unable to infer all dependencies" on any deployment where a listed class has a materialised contributor. Adding a fourth entry would only widen the set of stands where a scheduled data-quality run turns red. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
cyberantonz
pushed a commit
to cyberantonz/insight
that referenced
this pull request
Aug 25, 2026
….2.0 deferred (constructorfabric#2666) No schema change of its own. A MAJOR descriptor version is the only thing that makes reconcile-connectors dispatch a one-shot `dbt --full-refresh` scoped to `tag:claude-team+`, and three earlier changes each needed rows to re-materialise and each deliberately shipped as MINOR instead. Four things only that rebuild can do. Rows written before 2.2.0 carry an empty seat_status and reach gold as `unknown`, because staging re-reads a three-day window and never revisits them. The seats the pre-2.2.0 `status = 'active'` filter dropped at staging time are still in Bronze — the vendor restates that status as of the read rather than as of the metric date, so deactivating a seat removed that person's whole history — and only a full re-read brings them through. The roster rows admitted before 2.2.0's activity gate violate what assert_ai_dev_usage_rows_active states. And the rows the old table-wide silver watermark stranded return only when staging re-emits them: constructorfabric#2607 moved the boundary to per-(tenant, source), which stops the loss without undoing it. Safe now and not before. `full_refresh=false` on claude_team__ai_overage landed in 2.2.0, and that model is the only place a past month's closing spend exists — the endpoint keeps no history. It had to be on main before anything could read a MAJOR version, or the rebuild would delete the months it protects. The rebuild now skips it while class_ai_overage re-materialises from it. The version is set by hand rather than by CI: claude-team is declarative and carries no `images:` block, so the build's automatic minor bump never sees it. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
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
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.
Closes #2606. Part of #2479. Depends on #2528 (merged) — the Bronze key it added is what makes the overage half of this safe.
Six findings from reading the Claude Team path end to end. Each is its own commit.
The seat status was a filter; it is now a column
WHERE status = 'active'in Claude Team staging looked like a seat-state gate. It is retroactive: the endpoint restates the status for every day it re-reads, so the value describes the seat as of the read and not as of the metric date. The day a person's seat is deactivated, the filter deletes their entire history — and a rebuild removes it from silver altogether. Nothing about a seat's present state licenses deleting the work it recorded.The value now rides in the class-contract column
seat_status, appended last so the contract grows without a rebuild, and emission is gated on the activity counters instead — the same gate every other contributor to the class already uses. Sources with no seat lifecycle concept emit NULL and read asunknownat gold, where the value becomes a dimension of theai_usagesource.Audit decision D1 said the opposite (keep the filter, add a check counting non-active rows); the spec now records why that reading was wrong.
One incremental boundary for several producers
Silver classes carried
WHERE _version > (SELECT max(_version) FROM {{ this }}). A class is written by every connector that feeds it, andunion_by_tagrebuilds it from all feeders on whichever connector's run fires._versionis a timestamp, but its meaning differs per producer — extraction time for the AI connectors, write time elsewhere — so a single maximum over the whole table lets whichever producer commits first raise the boundary above another producer's rows. Those rows sit below it forever.The boundary moves to a maximum per (tenant, source instance) — the shape already merged for the crm classes and
class_person_attribute_claims._version, its values and its role inReplacingMergeTreeare untouched; only the group the maximum is taken over.coalesce(max_version, 0)is load-bearing: a source absent from the target has no boundary row, and comparing against NULL would drop every row of every new source.This fixes the future only. Rows already below a boundary stay below it, so recovering them is a separate, deliberate operation.
assert_ai_staging_rows_reach_silveris new and reports what staging can prove a class is missing, per source — there was no check for this at all.check-dbt-conventionsprescribed the table-wide form and would have kept reintroducing it.Counts that stopped in silver
prs_with_cc_countandprs_total_counthave been in the class unread since it gained them. They are served asai.prs_with_assistantandai.prs_total, emitted only where the vendor supplies a value, so an organisation without the vendor's code-host connection returns no value rather than a zero asserting "no pull requests involved the assistant".ai.prs_totalis context for the other measure and not a goal of its own: it is the vendor's count over the vendor's own window, which need not be the day it is reported against.Seat spend joins identity coverage
identity_resolution_coveragelisted every evidence relation except the seat one — the only relation whose unresolved rows are money rather than activity.Vendor fields that reached nothing
The code-metrics response carries org-level pull-request attribution and top-user rankings beside the per-user array; they were read and discarded on every sync, and the vendor keeps no history.
claude_team_code_metrics_orgkeeps them as their own stream: one request and one row per day, per-user array stripped so the roster is not stored twice. Copying them onto every user row would multiply one organisation fact by the roster and make any sum wrong.disabled_reason,disabled_untilandaccount_namejoin the overage extras blob, where a zero spend can now be told apart from a seat the vendor blocked.A rebuild that deletes months
full_refresh=falseguardsclaude_team__ai_overage. That model is the only place a past month's closing spend exists — the endpoint keeps no history — so a rebuild from Bronze deletes those months rather than reproducing them, and reconcile-connectors dispatches exactly that rebuild on a MAJOR descriptor bump. Silver still rebuilds freely: it reads the staging model, not Bronze.Also
conversation_countis documented for what it holds. For the dev-agent tools no vendor publishes a separate conversation counter, so the connectors map their session or thread count into it and the value equalssession_counton those rows. That is the intended mapping rather than a duplication to collapse, and it does not hold on the assistant class, where the two differ per surface.ai.dev_conversationsgains the explanation its number needed.The connector README described the pre-#2528 shape of the overage key.
Descriptor versions
claude-team
2.1.0→2.2.0, cursor / chatgpt-team2.0.0→2.1.0, claude-enterprise3.0.0→3.1.0. All MINOR, deliberately not MAJOR: a MAJOR dispatches the one-shotdbt --full-refresh, and the guard that makes it safe arrives in this same change — it has to be on main before any MAJOR bump reads it. Rows written before this version keep an emptyseat_statusand read asunknown, which is what they are.Test plan
Automated, run on this branch:
dbt parse— cleandbt compileof the touched models — the compiled SQL carriesseat_status, both new measures and theai_cost_metric_evidencebranchcargo test -p analytics --bins— 420 passed, 0 failedcargo fmt --checkandcargo clippy -- -D warnings— cleansource.sh validateandvalidate-strictfor ai/claude-team — manifest valid;discoverreturns five streamsNeeded the CI rig, now run on this PR:
e2e-bronze-to-api,aishard — 4m20s, green, including the newai_pr_attribution.test.yaml. The other three shards are green too, andMetric coverage gatepasses, so both new metric keys carry the period / peer / timeseries / breakdown coverage that gate requiresconnectors-ddlsnapshot and field-parity gate — 6m31s, green: a warehouse built from scratch by the real pipeline reproduces both edited snapshots, and every staging contributor still lines up with its silver union targettests/stand— theapi-smokejob of the Stand E2E lane, greenBuild runner imagewas cancelled on the first attempt at its 30-minute ceiling while the merge queue was saturated; re-running the same commit passed, so the timeout was contention rather than a defect. Every other check is green.