Narrow a VIN decode to one trim, and say how confident it is - #315
Open
zer0stars wants to merge 15 commits into
Open
Narrow a VIN decode to one trim, and say how confident it is#315zer0stars wants to merge 15 commits into
zer0stars wants to merge 15 commits into
Conversation
A VIN decode narrows a multi-trim template to one trim and reports how confident the match is, instead of asserting one flat record for every configuration. Adds template-shaped catalog reads, a pure trim matcher, and additive proto fields for trim, template version and match quality.
GetDefinitionByID/GetDefinitionByIDFresh become GetTemplateByID and GetTemplateByIDFresh, reading t/<id>.json instead of definitions/<id>.json. No fallback to the old path: a 404 now fails loudly rather than being served as the pre-migration flat record. Attribute values decode as map[string]any so typed values (fuel_tank_capacity_gal: 15.8) stay typed instead of getting stringified. Updates every caller of the renamed methods to consume the new Template shape directly, without shimming it back into the old model.
bulk-update-powertrain read templates (typed attributes) then wrote them back through Update(), which stringified everything and hit the legacy definitions/<id> route -- undoing the point of the template migration and mixing the new no-fallback read path with the old flat write path in one runnable subcommand. Powertrain is now a per-trim value derived during extraction anyway, so a flat bulk override no longer matches the model. Deleted the subcommand, its main.go registration, Update() (its only caller), and DeviceDefinitionUpdateInput (Update()'s only consumer). fetchDoc/fetchDocFresh/fetchDocFrom/catalogDoc and workerPutBody stay: they're still live behind GetDeviceDefinitionByID/GetDefinition (add_vin), Create (decode_vin, create_dd), and the manifest reader.
GetTemplateByID/GetTemplateByIDFresh returned one generic error for both a genuine 404 and any other catalog failure, so get_ds_by_id.go and upsert_vin_decode.go both collapsed the two into NotFoundError. A catalog outage then looked exactly like a missing vehicle, and the plausible response to that spurious 404 -- creating the definition -- would write a duplicate for a vehicle that already exists, at the worst possible moment. Add ErrTemplateNotFound, a typed sentinel returned only for a literal 404; every other failure (bad status, transport error, decode failure) stays a distinct error. Both callers now branch on errors.Is(err, ErrTemplateNotFound) rather than treating any error as not-found, with a test on each side. Also: make TestTemplateHasNoLegacyFields actually exercise the Template type instead of asserting on its own fixture, and add a no-fallback test for the Fresh/worker-backed read path, which previously only had the CDN path under direct test.
…tage decode_vin.go's main Handle() flow treated any GetTemplateByID error -- outage, timeout, 500, decode failure, not just a genuine 404 -- as "this definition doesn't exist" and fell through to Create() with a nil template. On the VIN-decode hot path, at decode volume, a catalog outage meant every decode for an already-existing vehicle attempted to write a duplicate definition. Only errors.Is(err, gateways.ErrTemplateNotFound) (or a nil error with a nil template) is now allowed to fall through to the create-if-missing path; any other error aborts the decode and returns before any writes. Checked the other two GetTemplateByID call sites in this file (hydrateResponseFromVinNumber, vinInfoFromKnown) -- neither calls Create or any other write on failure, so neither carries this risk. Added a test proving Create() is never called when the catalog fails for a non-not-found reason, and confirmed it fails against the pre-fix code before restoring the fix.
Extraction emits only manufacturerCode selectors, and decode_vin could only ever supply styleName, so no trim could match and match_quality was permanently model-only. Drivly is the one provider that carries a manufacturer code (DrivlyVINResponse.ManufacturerCode); it was being dropped during normalization to VINDecodingInfoData. Carry it through and forward it into MatchSignals so drivly-decoded VINs -- the population the templates were built from -- can actually resolve.
The plan's riskiest assumption is that manufacturerCode reaches a decode as a usable signal at all: trim selectors are keyed on it, only drivly supplies it, and the plan says Task 3 "must report how often it happens on real decodes". Nothing did. The share of decodes landing on model-only, and which providers they come from, was inferable only by reading one decode response at a time.
MatchTrim resolves both and nothing could read either. match.by ("which
selectors fired") is in the resolved contract and was computed and dropped.
hardwareTemplateId is the template-default-with-trim-override resolution the
contract requires be settled server-side "so callers never reimplement the
fallback" -- it has two rulings and three tests behind it, and its only
observable effect was inside those tests.
Also documents what an empty match_quality means. The three values all assert
the matcher ran; empty means it could not run, which is distinct from
model-only and was previously undocumented.
Additive only: fields 16 and 17, nothing renumbered or removed. Regenerated
with the Makefile's pinned protoc-gen-go v1.30.0.
Every VIN is answered from vin_numbers from its second decode onward, so hydrateResponseFromVinNumber -- not the freshly-decoded path -- is where most production traffic lands. It never ran the matcher. trim, template_version, match_quality and match_candidates were all left unset, emitting an empty match_quality that the response contract does not define, and powertrain was read from template-level attributes alone. That last part is the old bug wearing a new hat. A template only carries powertrain_type at the top level when every trim agrees on it; on exactly the multi-trim templates this migration exists for, it lives on the trims. The lookup therefore missed and fell through to a make/model heuristic, so the same VIN returned "Hybrid LE / HEV / exact" on its first decode and a heuristic guess with no stated confidence on every decode after. The signals are rebuilt from what was already persisted: drivly_data is the marshalled DrivlyVINResponse the live path read manufacturerCode from, and the style row holds the name processDeviceStyle wrote. No heuristic fallback, matching the live path -- the two paths disagreeing is the defect being fixed, not a behaviour to keep on one side of it. Proven red by reverting: the exact-match test fails on the old code with "Unexpected call to GetManufacturerNameByID", direct evidence that the cached path reached for the make/model heuristic instead of the trim. Also plumbs match_by and hardware_template_id on the freshly-decoded path, and threads the request context into the cached path instead of context.Background().
A trim declaring both manufacturerCode and styleName matched only when both agreed, but that was established by reading the code -- and reading the code is what has repeatedly missed defects in this migration. Were it ever to degrade to an OR, a VIN with the right style name and the wrong OEM code would silently resolve to a trim it is not. Covers both halves matching, each half alone, and a declared selector whose signal is absent entirely.
GET /device-definitions/{id} now returns a vehicle template -- typed
attributes and a trims array -- where it returned the flat tableland model
with metadata.device_attributes. Its swagger annotation still advertised the
old shape, so the one place a consumer would look to discover the break said
nothing had changed. docs/swagger.json is generated and still stale; that is
recorded as a follow-up.
GetTemplateByIDFresh's comment describes read-modify-write callers that no
longer exist: its only caller was the bulk powertrain tool, deleted with the
legacy Update() path earlier on this branch. Say plainly that it has no
production caller and name the migration expected to adopt it.
…ploy Headline is the worker-route blocker: the new worker serves /t/:id and four /admin routes and 404s everything else, while dd-api's Create, Delete and manifest reads still speak /definitions/<id> and /manifest.json. Nothing here breaks because nothing here deploys, but dd-api cannot run against the new worker until those migrate. Also records the pre-existing panic in bulk_validate_vin's dead code, the attributes GetDeviceStyleByID now omits for multi-trim templates, the stale generated swagger, and the deferred minors the ledger flagged.
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
1 participant
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.
Stacked on #314. Retarget to
mainonce that merges.Why
toyota_camry_2020in production declarespowertrain_type: ICEwhile carryingthe hybrid's fuel economy, tank size and OEM code — one flat record standing
for several real cars. A sibling pipeline now emits that model-year as ten
trims: three HEV at 13.2 gal against seven ICE at 16.
This makes a VIN decode narrow a template to one trim and say how confident
it was.
What changes
t/<id>.json— the template shape — with no fallback todefinitions/<id>.json. A missing template means the import hasn't run, andthat must fail loudly rather than silently serving the pre-migration record.
manufacturerCode,styleNameor aVIN pattern. Several matches is
ambiguous, not "take the first" — itemits only the attributes every candidate agrees on and names the candidates.
Nothing matching is
model-only, which carries no trim's values at all.DecodeVinResponsegainstrim,template_version,match_quality,match_candidates,match_by,hardware_template_id— fields 12–17,additive only, nothing renumbered.
quality/source-labelled counter, so how often decodes land onmodel-onlyis measurable rather than assumed.Bugs caught in review
error covered a real 404 and a 500 alike. The plausible response to a spurious
404 is to create the definition — so
decode_vinwrote a duplicatedefinition on every decode during an outage, on the hot path. Now a typed
sentinel, checked by identity.
manufacturerCode;buildFromDrivlydropped that field duringnormalisation. No trim could ever match — every decode would have answered
model-onlywhile looking healthy. Each half was correct alone; the gap wasonly visible where they met.
Handlereturnsearly for any VIN already in
vin_numbers— most production traffic — andthat path never ran the matcher. The same VIN resolved
Hybrid LE / HEV / exactonce, then an unqualified guess forever after, withmatch_qualityemitting empty.
Merge-ready, not deploy-ready
docs/superpowers/2026-08-28-dd-api-followups.mdrecords the gate:Create()still
PUTs/definitions/<id>andmanifest()stillGETs/manifest.json,and the new worker serves neither. Every catalog miss and every search sync
will fail — loudly, not corruptingly — until those migrate. Nothing here
worsens it.
🤖 Generated with Claude Code
https://claude.ai/code/session_016knorHPbUjvAv85qppPwx7