fix(ai-cost): scope the invoice layers' incremental boundary to one source - #2621
Merged
Conversation
…ource #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>
|
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 18, 2026
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 #2620. Follow-up to #2607, which #2432 crossed with in flight.
#2607 replaced the table-wide
_version > max(_version)on the AI silver classes withsilver_incremental_watermark, and taught the convention checker to forbid the old form.#2432 landed the invoice path a few hours later from a branch that predated that fix, so
class_ai_invoiceis the last AI class carrying the shape #2607 removed. This restores itto the convention.
For the record, since the ordering is what caused this: #2607 is not a new connector — it
fixes the existing Claude Team path. #2432 did not disagree with it; it was simply written
first and merged second.
The defect, in #2607's own words
That precondition is this class's stated purpose — "Unified vendor invoices across AI
vendors", with the staging model documenting itself as defining the contract future
vendors emit into.
Nothing is being lost today: one contributor means the table-wide maximum is its own.
This is preventive.
Two layers
silver/ai/class_ai_invoice.sqlis the obvious half. The other isclaude_team__ai_invoice.sql, whose boundary is the same shape overcollected_at.Fixing only the class would change 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_idis part of everyunique_key, and whycheck_connectionrefuses an empty one with "would collide unique_keys across connectorinstances".
Both layers now take the shared macro rather than a second implementation of it. Staging's
empty-table guard goes with the change:
coalesceto the epoch already admits an instancethe table has never seen, which is exactly what the guard stood in for.
Test plan
Automated, run:
metrics/test_ai_invoice_silver.py+metrics/test_ai_seat_extra_usage_history.py—13 passed against a stand built from wiped volumes.
the class already holds — the shape a per-table boundary silently drops — and reverting
either layer alone fails it with
assert [] == ['in_SECOND']. Both reverts were run overthe full module: with a
-kfilter the earlier fixtures never run, staging is empty, andthe empty-table guard hides the bug.
ruff check+ruff format --checkat the pinned version — clean.connectors-ddlsnapshot unchanged, and expected to be: the column list is untouchedand the snapshots hold bronze DDL only.
Manual, needs a stand — please tick these yourself:
staging.claude_team__ai_invoiceandsilver.class_ai_invoicekeep accepting rows on the next scheduled sync (the boundary changes shape, not meaning,
so an in-place upgrade needs no full refresh).
Not in this PR
assert_ai_staging_rows_reach_silver— the completeness check #2607 added to report exactlythis loss — cannot run: its
ref()calls sit inside a loop overmaterialised_models_for_tag, which is empty at parse time, so dbt records no dependenciesand fails the test at run time with "unable to infer all dependencies … ref() is placed
within a conditional block". Reproduced against a live stand with
dbt test --selector data_quality.class_ai_invoiceis deliberately not added to its list: a fourth entry would only widenthe set of stands whose scheduled data-quality run turns red. Written up in #2620 for whoever
owns the check —
-- depends_on:hints cannot be written for refs chosen at run time, so itneeds a design call rather than a one-liner.