feat(insurance): decode plan line-item identity

[cristim]

What

Decode plan line-item identity: account_id, offer.provider_offering_id and selected_commitment from the Public Archera CommitmentPlanLineItem schema.

Why

Comparison rows join to line items by line_item_id; this adds the typed decoder that join needs (#434).

Test evidence

Synthetic httptest fixtures only; no live Archera calls.

• cd pkg && gofmt -l . clean, go vet ./insurance/... clean
• cd pkg && go test ./insurance/... ok; go test -race -short ./insurance/... ok
• cd pkg && go build ./... ok
• Local golangci-lint could not run (built with go1.26, repo needs go1.27); relying on CI lint.

Stack

PR 2 of 3. Base: feat/434-insurance-selected-amount (PR 1). Followed by the pagination PR.

Refs #434

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 16 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: ade53c5d-7c1a-4cad-a3dd-a7bae044f3b7

📥 Commits

Reviewing files that changed from the base of the PR and between a45b889 and 55abc0a.

📒 Files selected for processing: 2

• pkg/insurance/line_items.go
• pkg/insurance/line_items_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review at f767908

Verdict: changes requested (one medium decode gap, plus test gaps where mutations survive).

Source of truth: the live Public OpenAPI schema (the two local spec files named in the brief were absent; I fetched the schema from the vendor's unauthenticated openapi endpoint and parsed it). Checked CommitmentPlanLineItem_Exclude_CoveredServices, PublicCommitmentOffer, PurchasePlanContractSpec.

Findings

1. Medium, line_items.go:180-194: offer.provider_offering_id is the purchase identity key (issue feat(insurance): decode plan line items and selected comparison amounts #434) but an empty string is accepted (probed: "provider_offering_id": "" decodes without error). offer.type and contract_spec.commitment_type get an empty check (line_items.go:190, 155); this field should too. Fix: reject empty with errMissing and add a test.
2. Low, line_items.go:199: duration_seconds accepts zero and negative values (probed). Schema only says number, so this is defensible, but a nonpositive duration cannot be a real term; consider nonnegativeRat or a positive check.
3. Low (test gap, mutants survived): fixtures use distinct values for instance_type/instance_family and tenancy/product_description but TestDecodeLineItemsIdentity (line_items_test.go:26-46) never asserts InstanceType, InstanceFamily, Tenancy, ProductDescription, Region or PlanType. Swapping instance_type<->instance_family and tenancy<->product_description targets passes all tests. Also untested: duration_seconds required (mutated to optional, survived) and is_selected: null rejection (made nullable, survived, silently yielding false).
4. Info: LineItem.ID, OfferID, PlanID keep the vendor's case while equality checks use EqualFold; a consumer joining to comparison line_item_id must compare case-insensitively (no normalization exists on either side). Required-but-undecoded schema fields (lease_menu_item_id, display_name, upfront_cost, ...) are not presence-checked; acceptable for the identity scope.

Verified correct against the schema: names; nullable fields (account_id, offer az/region/offering_id/instance_*/tenancy/product_description/plan_type/is_multi_az/is_flexible) stay nil and the key must be present (required-nullable); offering_class and payment_option are optional and nullable; line-item and offer payment_option use "No Upfront"/... while contract_spec.payment_option uses snake_case, and the cross-spelling is rejected; contract_term and contract_spec.term are checked against a set identical to the schema enum (compared item by item); UUID validation; selected_commitment is *big.Rat and rejects quoted numbers; selected_quantity is an integer (1.5 rejected); no vendor text is echoed in decode errors.

Mutation results (scratch copy, each reverted; 23 mutants, suite run per mutant): killed: swap region/az, swap multi_az/flexible, account_id/region/plan_type non-nullable, accept snake-case payment on line item, drop contract_spec payment/term check, drop contract_term check, drop offering_class check, drop offer.id match, drop plan_id match, drop duplicate check, drop negative quantity, nonneg->required, drop type/commitment_type empty checks, drop provider parse, swap id/offer_id, ignore top-level payment_option. Survivors: swap instance_type/family, swap tenancy/product_description, duration optional, is_selected nullable (see 3).

[cristim]

Independent adversarial review: CLEAN at f767908724f795038c700c64797ff0a5dba3cd79 (claude-go-pr-merge-2; non-author reviewer, diff 829ae14e..f7679087).

• DecodeLineItems/decodeLineItem (line_items.go:78-170) require UUID id, plan_id, offer_id; plan_id must equal the requested plan (tenant isolation) and offer.id must equal offer_id; duplicate IDs rejected case-insensitively. Nullable offer fields and account_id stay nil, not zero. Unknown provider, term, payment option or offering class fail closed; selected_commitment via nonnegativeRat; selected_quantity nonnegative int64.
• Mutation: disabling the plan_id match fails TestDecodeLineItemsContractBreaks.
• Non-blocking: account_id is *string with no format check (empty string accepted); unknown top-level keys are ignored (tolerant decoding; identity fields stay strict).

Local verification (macOS; httptest/JSON fixture evidence only, no live Archera calls): (cd pkg && GOWORK=off go test -race -short ./...) pass at 829ae14, f767908 and b0f66cd; at b0f66cd make build, make vet and go build ./... in all five modules (go.work) pass. No Archera schema or fixture exists in the repo, so field names and pagination parameters were not checked against the vendor schema (gap recorded).

Full CI has not run on this head because its base is #435's branch; after #435 merges this PR is retargeted to main and must go green there (a pure rebase with empty git range-diff keeps this verdict). CR skipped: quota (review skipped on non-default base). Labels mirror #434.

[cristim]

Review at f767908 addressed in e385da8 (new commit, no amend):

1. Empty offer.provider_offering_id now fails with the existing missing-field error; test case "empty provider offering" added.
2. offer.duration_seconds <= 0 is rejected; tests for zero and negative.
3. TestDecodeLineItemsIdentity now asserts InstanceType, InstanceFamily, Tenancy, ProductDescription, Region, PlanType (nil in the base fixture, plus a non-nil compute case) and IsSelected. New cases: null duration_seconds, null is_selected (rejected, not decoded as false).
4. No doc comment said UUID joins are case-insensitive; added one on LineItem (ID/PlanID/OfferID keep vendor case, compare with EqualFold). No behavior change.

Mutants run in a scratch copy, each reverted: swap instance_type/family, swap tenancy/product_description, swap region/az, plan_type to tenancy, is_selected nullable, drop empty provider_offering_id, allow zero duration, allow negative duration: all killed. "duration_seconds optional" alone and "requiredRat to ratOrNil" alone survive because they are redundant with each other (readFields already rejects null on a non-nullable field, requiredRat rejects it again); applying both together is killed by the null-duration test.

Local: gofmt, go vet, go test and go test -race -short on ./insurance/... pass, go build ./... ok. Synthetic fixtures only.

[cristim]

Independent review at e385da8

Verdict: no actionable findings. Pass-1 findings are resolved.

Resolution check (diff b0f66cd..e385da8 on line_items.go and tests; full suite passes at the #437 tip):

• Empty provider_offering_id is now rejected (line_items.go:195-197). The schema (PublicCommitmentOffer.provider_offering_id: {type: string}) does not forbid empty, so this is deliberate fail-closed strictness for the join key. Accepted.
• duration_seconds must be positive (line_items.go:207-209). The schema only says number, so this is also stricter than the documented contract. Low residual risk: the term enum contains zero_day; if a zero-duration offer ever exists, the decode fails closed rather than misreads. Accepted, worth a note in the PR.
• Test gaps closed: instance_type/family, tenancy/product_description, region, plan_type and is_selected are now asserted; null duration and null is_selected are rejected by test. Case-insensitive join is documented on LineItem.

Mutants rerun (scratch, reverted): swap instance_type/family, swap tenancy/product_description, is_selected nullable, drop empty-provider_offering_id check, drop positive-duration check, positive->nonnegative: all killed.

Redundancy: duration_seconds optional (requiredRat -> ratOrNil) still survives alone. Reason: readFields already rejects absent or null non-nullable keys before requiredRat runs, so the requiredRat nil branch is unreachable here. It is the shared helper's own contract, used elsewhere, so this is acceptable defence in depth, not dead code to remove from this PR.

[cristim]

Delta review: CLEAN at e385da8b973524333cc3d1d995db3fd5a3bddf0d (claude-go-pr-merge-2, same non-author reviewer as the f7679087 verdict).

• git range-diff 740dfbd..f7679087 740dfbd..e385da8b: 829ae14 and f767908 are =; only new commit e385da8.
• e385da8 rejects empty offer.provider_offering_id (line_items.go:195-197) and duration_seconds <= 0 (:207-209); new cases at line_items_test.go:98-100. Mutations: removing the provider-offering check and relaxing <= 0 to < 0 both fail TestDecodeLineItemsContractBreaks. No repo schema or fixture shows either value as legitimate; if Archera ever sends one the decode fails loud (vendor schema not checkable locally).
• Local verification (macOS, fixture/mock only): (cd pkg && GOWORK=off go test -race -short ./insurance/...) ok at e385da8.

Still after #435; then retarget to main and CI there. CR skipped: quota (non-default base).

[cristim]

Automated review of e385da8: no blocking findings and CI is green. Ready to merge.
The new line-item decoder reuses the existing helpers correctly. It checks required fields, nulls, UUID matching, term and payment enums, and duplicate IDs without regard to case. Tests are broad and CI is green. I found no blocking bugs.

• (optional) pkg/insurance/line_items.go:104: The duplicate line item ID error has no row index, unlike the other decode errors, which say line_items[i]. Adding the index would make failures easier to locate.
• (optional) pkg/insurance/line_items.go:126: Ranging over a map makes the UUID validation order random, so with several bad IDs the reported field name varies between runs. A fixed-order slice would give stable errors.

Posted by a read-only review sweep. It stays silent on this commit and re-reviews only after a new push.

[cristim]

#435 merged as 4566498. Retargeted this PR to main and rebased with git rebase --onto origin/main 829ae14e. git range-diff 829ae14e..e385da8b 4566498..bdcb2911: 1: f767908 = 1: 4d79d24, 2: e385da8 = 2: bdcb291, and the PR diff is unchanged (2 files, +420). The CLEAN verdicts (f7679087 and the delta at e385da8b) stand for bdcb29115f8c3b78acc04294e7d01fe064cbcd8f. Local check (macOS, fixture/mock): (cd pkg && GOWORK=off go build ./... && GOWORK=off go test -race -short ./insurance/...) pass. Full CI now runs against main.

[cristim]

First full CI against main (bdcb2911) failed pre-commit: scripts/test-git-secrets-allowlist.sh (file-argument scan of all tracked files) flagged pkg/insurance/line_items.go:40. gofmt aligned the grouped LineItemOffer fields with a run of more than 40 spaces, which matches the git-secrets AWS secret-key pattern [^A-Za-z0-9/+=]{40}[^A-Za-z0-9/+=].

Fix: new commit 2b7cc51 "style(insurance): declare line-item offer fields one per line" (no behaviour change). Delta review CLEAN at 2b7cc516 (same non-author reviewer): all 17 fields keep the same names, types and order. There are no unkeyed LineItemOffer{ literals in any module, no 40-char non-alphanumeric runs remain in pkg/insurance, and #437 does not touch this file. Local (macOS, fixture/mock): (cd pkg && GOWORK=off go test -race -short ./insurance/...) ok; bash scripts/test-git-secrets-allowlist.sh 44 PASS, 0 FAIL; all pre-commit hooks pass.

Main then moved to 512e42f. Rebased: git range-diff 4566498..2b7cc516 512e42f..0b854cfb is = for all three commits. New head 0b854cfb2d5db1c7b578bd76bce03c5d4bcd007e. Waiting for CI.

[cristim]

CI green on 0b854cfb (pre-commit 38936218527, Build & Test 38936218652), but main moved to a45b889 first. Rebased: git range-diff 512e42f..0b854cfb a45b889..55abc0a2 is = for all three commits. New head 55abc0a29612730b44ae7bbc021efe6c0ec14787; local (cd pkg && GOWORK=off go test -race -short ./insurance/...) pass (fixture/mock).

[cristim]

Merge evidence at 55abc0a29612730b44ae7bbc021efe6c0ec14787 (claude-go-pr-merge-2, owner-authorized): independent review CLEAN at f7679087, delta CLEAN at e385da8b, delta CLEAN at 2b7cc516 (git-secrets struct-format fix). 55abc0a2 is a pure rebase of those commits onto a45b889 (range-diff =). No open threads. CI green on 55abc0a2 (pre-commit 38938880461, Build & Test 38938880464). Local verification (macOS, fixture/mock, no live Archera) recorded above. Labels mirror #434. Stack 2/3; #437 is retargeted to main next. CR skipped: quota. CR waived: quota, adversarial review + local verification + green CI