[cristim]

What and why

PR1b of the #799 R2 route plan (typed constants, line-item selection). Stacked on #1054 (PR1a, recommendation projection); the go pin bump (#1053) is already on main and in #1054's history, so no extra base is needed.

• SelectLineItem(rec, items) (id, SelectStatus) is exported. It reuses sameResource and the ambiguity logic that MatchRecommendation had inline, and MatchRecommendation now calls it. The handler (PR3) selects the item first, then makes a scoped comparison call.
• ProjectContract projects the published insurance.LineItem and Comparison into the R1 identity types (adapted from the read-only WIP). vendorAlternatives(entries) skips IsCurrent candidates, so the current offer is never an alternative.
• Typed CandidateStatus and Reason* constants replace the string literals in match and alternatives. validPayment uses insurance.Payment*.

Size: 367 added lines, every file under 500.

Tests

• SelectLineItem: none, one, ambiguous, unsupported recommendation, nil.
• MatchRecommendation with nil or zero amounts (recommendation, line item, current row, all-zero) gives MatchUnavailable. This pins existing behaviour.
• IsCurrent candidate excluded, through ProjectContract and MatchRecommendation. It fails without the filter (mutant below).
• Per-field projection of candidates and payment options.

GOWORK=off: go test -race ./internal/archera/, go vet ./..., go build ./..., gofmt, gocyclo at most 10 all pass. Local lint is broken, so CI is authoritative.

Mutations

18 synthetic mutants, run against synthetic fixtures with go test -overlay on scratch copies: every contract field mapping (swap, drop, constant), the IsCurrent filter, the selector's ambiguity check, validity check and returned ID, a dropped payment constant, and the candidate status. 16 killed. 2 survived (item payment, current payment: the fixture used the same value as the mutant), so TestProjectContractCarriesPaymentOptions was added and both are now killed. One constant-value mutant did not apply and was not run. No real vendor responses were used.

Refs #799

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.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 15 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 74 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-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 8be54857-feac-4458-a314-b9084b04f7ab

📥 Commits

Reviewing files that changed from the base of the PR and between d47d516 and 7a9721e.

📒 Files selected for processing: 6

• internal/archera/recommendation_alternatives.go
• internal/archera/recommendation_alternatives_test.go
• internal/archera/recommendation_contract.go
• internal/archera/recommendation_contract_test.go
• internal/archera/recommendation_match.go
• internal/archera/recommendation_match_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 f27a848

Verdict: no blocking or major findings. Two minor findings; the first has a one-test fix. Scope is the #1056 diff against the #1054 head (aeb2e93).

Findings

1. Minor, recommendation_contract.go:18 with recommendation_contract_test.go: a mutant that hardcodes CurrentType: "savings_plan" survives. No test projects a row whose current commitment_type is not savings_plan. The matcher-level test covers sameCurrentBasis, but not the mapping from OfferEntry.CommitmentType. Fix: add a current_type case to TestProjectContractPreservesMissingOrContradictoryEvidence that sets c.Rows[0].Current.CommitmentType = "ri" and expects MatchUnavailable.
2. Minor and not in the plan, recommendation_contract.go:36-42: contractLineItem blanks Type for an unselected item or an offer/line payment_option mismatch. It does not cross-check ContractSpec.PaymentOption, ContractSpec.CommitmentType or ContractTerm against Offer.Type, the line payment and DurationSeconds. The test deliberately treats commitment_type as opaque, and the decoder validates each field's own enum. I could not show an exploitable mismatch, so this is a question rather than a defect. If the vendor can contradict itself, the cross-check belongs here.

Notes, not defects:

• AmountProvenance and EconomicStatus are now typed Reason, though they are not reasons. That is a naming smell only.
• An unselected twin line item no longer counts toward ambiguity, because its Type is blanked. This is consistent with "unselected is not the resource" and is tested (unselected).

Checked against the published types (pkg 3def3f89, insurance/line_items.go, types.go)

• Field mapping: ProviderOfferingID comes from Offer.ProviderOfferingID. Mutants that alias it to Offer.OfferingID or to Offer.ID are both killed.
• Nullable fields stay unproven: AccountID, Region, InstanceFamily and PlanType map nil to "". The recommendation requires non-empty source, region and family and plan type EC2Instance, so "" can never equal them. DurationSeconds and SelectedCommitment stay nil, and sameAmount and sameResource reject nil. A nil candidate or current PaymentOption gives "", which validPayment rejects. A nil Region on the current row stays nil.
• IsCurrent filter: vendorAlternatives drops current entries in order, and ComparisonIdentity.Candidates is exactly that filtered list. Matcher output is index-aligned with vendorAlternatives(row.Candidates). PR2 must iterate vendorAlternatives, not the raw Candidates, as the plan states. TestProjectContractExcludesTheCurrentOfferFromAlternatives and the every-field test cover it.
• SelectLineItem against the old inline loop: identical logic (first match remembered, second match is ambiguous, no match is none). The ID is taken after comparisonIndex, as before. comparisonIndex still runs first, so error and unproven ordering is unchanged. The validRecommendation re-check in the selector is redundant but harmless. All R1 matcher tests pass unmodified apart from the typed constants, and the new selector table covers none, one, ambiguous, nil and invalid.
• Typed fields: Reason, CandidateStatus and SelectStatus are string-kind types, so their JSON is byte-identical. The only JSON tags in the package are in dto.go, which does not use them. Nothing outside internal/archera references these types.
• validPayment now uses insurance.Payment*. The constants are no_upfront, partial_upfront and all_upfront, the same strings as before; dropping AllUpfront is killed.
• gocyclo is at most 10 on non-test code, and files are 31 to 242 lines.

Mutation results (15 mutants, scratch copy, each reverted)

• 12 killed: the ProviderOfferingID alias to OfferingID, the alias to Offer.ID, source, duration, commitment, the IsSelected check, the payment-mismatch check, the filter removed, the current amount, the current flag, current payment, and the SelectLineItem ambiguity check.
• 1 killed for the wrong reason: LineItemID mapped through ToUpper failed to compile, so it counts as invalid. It is not evidence.
• 1 killed: validPayment without all-upfront.
• 1 survived: CurrentType hardcoded (Finding 1).

Local verification (synthetic fixtures; the contract test decodes vendor-shaped JSON through the real insurance.DecodeLineItems and DecodeComparison, with no real vendor data)

GOWORK=off go test -race ./internal/archera/ passes (143 tests). go vet ./internal/archera/ is clean.

This is a review verdict only, not an approval or merge.

[cristim]

Finding 1 resolved: added a current_type case ("ri") to the contradictory-evidence table. The hardcoded CurrentType mutant is now killed (verified in a scratch overlay, reverted).

Finding 2: the ContractSpec cross-checks are deliberately out of scope. Per the plan, the offer-level fields are the identity proof.

[cristim]

Nit on the unselected twin: no test pinned it before (the existing case only covered an unselected item alone). Added TestProjectContractUnselectedTwinDoesNotCreateAmbiguity: a selected item associates (SelectOne) even with an identical unselected twin, and the twin alone selects nothing. Dropping the IsSelected check in contractLineItem makes it fail. Unselected items are plan alternatives, not current commitments, so excluding them before the ambiguity check is intended.

[cristim]

Independent review at 80ee82c

Final verdict: no remaining findings from me. Delta since f27a848 is two test-only commits.

• git diff f27a848e 80ee82ca -- ':!*_test.go' is empty. Only recommendation_contract_test.go changed (+20 lines).
• Finding 1 resolved: the new current_type case sets the current CommitmentType to ri and expects unavailable. Re-running my mutant that hardcodes CurrentType: "savings_plan" now fails TestProjectContractPreservesMissingOrContradictoryEvidence.
• Finding 2 (the unselected twin) is pinned by TestProjectContractUnselectedTwinDoesNotCreateAmbiguity. It shows the twin's Type is empty and SelectLineItem returns one match. Removing the IsSelected check fails the new test.
• GOWORK=off go test -race ./internal/archera/ passes (145 tests) on synthetic fixtures.

This is a review verdict only, not an approval or merge.

[cristim]

Independent review verdict: CLEAN at 80ee82c

Reviewer: a fresh Claude agent (Sonnet 5.5, high effort), not the author, with a cold brief. It reviewed aeb2e936..80ee82ca in full, including the two test-only commits, and re-checked the head just before reporting. Coordinator: claude-cc-reviewer.

Blocking findings: none.

What was verified

• The refactor keeps behaviour. MatchRecommendation was diffed before and after the SelectLineItem extraction. No case that used to match is now refused, and no case that used to be refused now matches. The unavailable reasons are unchanged:

  • no match gives identity_unproven;
  • an ambiguous match gives ambiguous_line_items;
  • a basis mismatch gives comparison_basis_unproven.

  Nil and zero amounts still give unavailable, and the new test pins this.

• The selector cannot pick the wrong item. sameResource compares provider, source account, offering, plan type, region, instance family, payment, type, duration and a positive equal amount. Items that differ only in an ignored field (OfferID, SelectedQuantity) are both matched, so the result is ambiguous and the selector refuses rather than picking one. comparisonIndex rejects duplicate IDs. An unselected twin is ignored, and a test pins that.

• ProjectContract was checked field by field against pkg/insurance line_items.go:18-56 and types.go:139-209. Nothing is missing or swapped, the units are consistent, and the payment slugs match types.go:74-76.

• vendorAlternatives never offers an IsCurrent candidate. With no current item it returns nil, and the output order is deterministic.

• Typed constants: every value is byte-identical to the old literal, and nothing outside internal/archera uses them.

• Mutation probes: 26 mutants were run on scratch copies, and 17 were killed. All 9 survivors are covered by the non-blocking notes below.

Local verification (fixture-based, GOWORK=off)

• go build ./..., go vet ./... and go test -race ./internal/archera/... pass at 80ee82c. The full go test ./internal/... passes at f27a848; the commits after it change tests only.
• golangci-lint v2.10.1 reports 0 issues.

Non-blocking notes (for PR3)

1. No test pins the string values of Reason* and CandidateStatus, so all 8 string mutants survived. Add a golden or literal test once PR3 serializes them.
2. A mutant that sources the payment from offer.PaymentOption instead of li.PaymentOption survived, because the fixtures set both to the same value. The two differ only when offer.PaymentOption is nil.
3. A non-current candidate with the same OfferID as the current row would still be listed. Whether vendor data can contain that is unproven.
4. SelectLineItem can return SelectOne with an empty ID. MatchRecommendation still reports unavailable in that case, but PR3 should treat an empty ID as unusable.

Gate:

• The PR was retargeted to main after feat(archera): project stored savings plan recommendations into match identities #1054 merged (d47d516).
• Labels are mirrored from feat(insurance): per-recommendation Archera quotes (blocked on an Archera API mapping) #799.
• CodeRabbit posted no reviews or comments. It is optional under the review gate, because this independent review and exact-revision verification cover the SHA.
• Merge waits on full CI being green at 80ee82c against main.

[cristim]

Pure rebase onto main after #1054 squash-merged (d47d516). Done with gh pr update-branch --rebase, and the already-merged #1054 commit dropped out. git range-diff aeb2e936..80ee82ca origin/main..7a9721ea shows all three commits as = (f27a848→c6408f09, 6a2f183→f9c29e49, 80ee82c→7a9721ea). The CLEAN verdict at 80ee82c carries to 7a9721e. Full CI is now running at this head. (claude-cc-reviewer)