fix(ai-cost): invoice connector follow-up — key an invoice on its own identity - #2657
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ 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 |
Gregory91G
added
component:ingestion
Open
4 tasks
`seat_unit_amount` returns None for anything that is not an int, so if the hosted-invoice surface ever serialises the unit amount as a float or a digit string, every seat price in the run becomes NULL while `chain_status` still reads `ok` — indistinguishable from a vendor that prices no seats, and invisible to the coverage check, which only looks at rows that are not `ok`. A seat-pricing line whose amount arrives as another type now degrades its invoice to `failed`, which is the state the coverage check already watches and the remediation already explains. Absence keeps its own meaning: a subscription line the vendor left unpriced is a state this connector reports, not a shape it failed to read, so only a changed type degrades. The warning carries the offending type by name. A `StripeChainError` is authored here and names what the response got wrong, so its message is logged; anything else still contributes its type and status alone, because a request error stringifies its URL and the hosted-invoice hop carries the token in that URL. A bare `failed` row would otherwise send the operator to the egress and Stripe-Version checks the remediation names, neither of which is the fault. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
A draft has no hosted invoice URL — the vendor issues one only at finalisation — so there is nothing invariant to identify it by, and its row can only be keyed on what the wrapper reports: created timestamp, payment intent, total and due date. Those are exactly the fields that move when the draft is finalised. The payment intent is created at that moment and a draft's total can still change until then, so the same invoice would be keyed one way as a draft and another once finalised: the draft's row stays in the class beside the real one, and a sum over `invoice_net_cents` counts that invoice twice. Nothing would surface it, because a draft carries no lines and the coverage check only looks for missing lines. Skipping them is the cheaper guard than keeping their money off the row, and it is also the truthful one: a draft is not invoiced, and this is the invoiced layer. Its total is provisional by definition. Two things fall out. `no_hosted_url` now means only what it should — the vendor offered no hosted URL for an invoice it has finalised, which is a vendor-side change rather than a legitimate state — so the coverage check no longer needs to exclude drafts and reports every status again. And drafts stay out of the drift ratio, where two of them beside one malformed URL could have tipped a healthy run into refusing to write. The count of skipped drafts is logged: an operator comparing the vendor's list against ours should not have to guess why the totals differ. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
A run walks every invoice the organisation has, and each one costs two requests to `invoicedata.stripe.com` — the host this connector's own README names as its known risk, since it appears in no Stripe documentation and carries no contract. Nothing paced them. A quarter-second sits between consecutive chains, and only between them: the hops inside one chain stay back to back, because the ephemeral key that authorises the last one is short-lived. The reference implementation paces the same boundary by the same amount. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
An invoice's own row was keyed on what the wrapper reports — created timestamp, payment intent, total and due date. Everything but the first can move over an invoice's life, and when the key moves the old row stays: silver has no deletion path, because class_ai_invoice filters its input with `_version > max(_version)` and delete+insert deletes only keys present in the incoming set. The invoice's money is then counted twice, and nothing surfaces it. The hosted invoice URL carries an identity that does not move. Its `live_…`/`test_…` segment is base64 of `acct_<account>,_<entity>,<rotating>`, and only the trailing field is re-issued on every list call. `stable_invoice_ref` decodes the first two into `invoice_ref`, `invoice_identity` puts it on every row of an invoice, and `unique_key_parts` keys on it — falling back to the wrapper's fields only where the vendor offered no URL and there is nothing else to key on. The ref reaches the class through `invoice_metrics_json` rather than as a column of its own: the staging model's header reserves the class contract for facts every contributor can supply, and vendor extras go into the JSON. A run whose URLs stop decoding is refused before the first chain call. Re-keying a whole run onto the mutable fields would write a second copy of every invoice that already has a row, and nothing downstream can delete the originals — the same reasoning as the existing unparsable-URL guard, at the same ratio. Two unit premises stopped being true and are replaced by the stronger claim they were reaching for: comparing a with-URL failure to a without-URL one no longer describes one invoice, and two invoices issued in one second are separated by their identities rather than by their totals. The e2e recovery fixture keyed its failed sync on the fallback while the recovered sync keyed on the identity, so the pair had stopped describing one invoice — a chain only fails once the URL has decoded, so both syncs carry the ref. The second-instance fixture gets one for the same reason: left without, it would exercise the fallback while every other invoice beside it exercises the identity, and nothing would say so. The bronze column needs no migration: reconcile_bronze_schema.py derives its ADD COLUMN statements from the snapshot and heals warm tables before the migrations run. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
…end convention The header said delete+insert was chosen because the same unique_key arrives twice. True, but it reads as a local preference, and the conventions check prescribes append for staging and delete+insert for silver — so the next reader to run that check sees a violation with no argument against it. The argument is the invariant: an invoice's row is REWRITTEN as its chain gets further, so a later sync has to replace the row an earlier one wrote rather than stand beside it. Appending leaves both until a background merge collapses them, which makes the replacement unobservable. The models the convention was written for restate a value under a key that never moves, which union_by_tag already resolves by _version — not this case. Comment only. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
Gregory91G
force-pushed
the
invoice-identity-key
branch
from
August 19, 2026 10:40
8189a3d to
cae0a15
Compare
hello1101n
approved these changes
Aug 19, 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
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.
Follow-up to #2432, under #2429.
Why. An invoice's money can be counted twice, silently: nothing downstream deletes the row a re-keyed invoice leaves behind, and the coverage check only looks for missing lines.
What changed. The invoice row keys on the identity decoded out of its hosted URL —
stable_invoice_refbase64-decodes thelive_…segment toacct_…,_<entity>, which survives the rotation the vendor applies on every list call — instead of on the wrapper's total, payment intent and dates, which move.The rest of #2432's review. A changed seat-price type degrades the invoice instead of blanking every price; drafts are not emitted; chains are 250 ms apart. One commit each.
The wrapper's fields stay as a fallback. An invoice the vendor offers no URL for has no identity, and a collidable key beats losing it.
No migration for the new bronze column.
reconcile_bronze_schema.pyheals warm tables from the snapshot before migrations run — the route #2431 and #2545 took.Out of scope. Skipping the chain for invoices already enriched — it needs the identity map carried in Airbyte state, and is tracked under #1607.
Verified.
metrics/test_ai_invoice_silver.py11 passed from wiped volumes; connector suite 77 passed;check-field-parity.py0 failures; the upgrade path exercised on a throwaway cluster built from the previous snapshot. The snapshot was reproduced by hand — theconnectors-ddllane re-dumps it on every PR.