[cristim]

What and why

#799 R2 step 1. archera.ProjectRecommendation projects a stored EC2 Instance Savings Plan recommendation into a RecommendationIdentity, and RecommendationIdentity.Equal compares two projections so a caller can detect that the row changed between two reads. The #945 squash dropped the nil-hourly check from validRecommendation, so the projection now rejects a nil, zero or non-number hourly itself and returns ErrRecommendationIdentity.

• Single json.Unmarshal into a struct (Details is written by Go marshal, so the duplicate-key token decoder is gone).
• Hourly token: at most 64 bytes, exponent in [-100,100] checked before big.Rat.SetString; quoted numbers, null, booleans and containers fail.
• Payment: no-upfront, partial-upfront, all-upfront map to insurance.Payment*; term 1 and 3 map to 31536000 and 94608000.
• RecommendationIdentity gains CloudAccountID so Equal sees it. Equal compares every projected field, using Rat.Cmp for the hourly amount, and is false for nil operands.

Callers: PR1b (selector and constants) and PR2 (DTO) consume it; this PR only adds the exported function.

Evidence (synthetic fixtures, no real account)

• Fail-before: on main, validRecommendation accepts an identity with a nil HourlyCommitment (scratch test failed on main, passes here via the projection).
• Tests: GOWORK=off go test -race ./internal/archera/, go vet ./..., go build ./..., gofmt, gocyclo <= 10 all pass locally. Local golangci-lint is broken by a toolchain mismatch; CI lint is authoritative.
• Mutation probes (24, hand-run via go test -overlay, synthetic fixtures): field-mapping swaps and drops, payment and term swaps, exponent bounds, length caps, quote acceptance, zero acceptance, nil-hourly acceptance, Equal field drops. 24 killed. Two initial survivors (a redundant first-byte check on the hourly token, and the Details size cap) led to removing the redundant check and adding a padded-valid-JSON test.

Refs #799

Stack: next is PR1b (typed constants, vendorAlternatives, SelectLineItem), then PR2.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• 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 37 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 73 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: 402c9192-54a9-48dc-9832-957211b1435f

📥 Commits

Reviewing files that changed from the base of the PR and between 150a604 and aeb2e93.

📒 Files selected for processing: 3

• internal/archera/recommendation_match.go
• internal/archera/recommendation_projection.go
• internal/archera/recommendation_projection_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 verdict: CLEAN at be20c0a, carried to aeb2e93 (pure rebase)

Reviewer: a fresh Claude agent (Sonnet 5.5, high effort), not the author, given a cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Rebase: I rebased onto main after #1053 (the go bump) merged. git range-diff shows 1: be20c0a4 = 1: aeb2e936, so the content is identical. CI is rerunning at the new head against the bumped go pin.

What was verified:

• Projection matches the writer. The writer, internal/scheduler/scheduler.go:1599-1616, goes through common.MarshalServiceDetails to SavingsPlanDetails (go pkg/common/types.go:685-710), and the projection reads the same JSON tags.
  • The term is stored as an int number of years. Only 1 and 3 map to seconds; 0, 2, 12, 36 and -1 are rejected.
  • Payment slugs are canonical (internal/config/validation.go:24), and their variants are rejected.
  • The 1e-07 and 1e+21 forms that Go emits both parse.
  • I found no stored row that fails to project or projects wrongly.
• Hourly parsing. 32 table cases plus a 30 s fuzz run (FuzzProject: no panic, and id.Equal(id) holds; exit 0).
  • 1e101, -0, 0, a leading +, 01, .5, quoted values, null, containers and tokens of 65 bytes or more are rejected.
  • Every case finishes in under 250 µs.
  • Hostile Details input (60k-deep arrays, 25k-deep objects) returns the error without panicking, and the 64 KiB cap holds.
• Equal.
  • 12 mutation probes were each killed. They covered: nil-vs-nil, Rat.Cmp, the nil-hourly check, the CloudAccountID and SourceAccountID assignments, both exponent bounds, Sign, the 64-byte cap, the Details cap, the validRecommendation call, and the term mapping.
  • 1.0 equals 1.00, which is tested and intended.
• feat(archera): associate vendor comparison alternatives #945 regression. On origin/main, validRecommendation accepts a nil HourlyCommitment; the projection now rejects it. This was confirmed with a scratch test.

Local verification (fixture-based, GOWORK=off, no cloud calls):

• go build ./... and go vet ./... pass.
• go test -race ./internal/archera/... -count=1 passes 118 tests.
• The full go test ./internal/... passes.
• golangci-lint v2.10.1 reports 0 issues.

Non-blocking notes:

• Duplicate or case-variant keys in Details are accepted, and the last one wins. This is acceptable only because Go's json.Marshal is the sole writer, as the PR body says.
• Equal(x, x) is true for an identity with a nil hourly. ProjectRecommendation never returns such an identity.
• The test for plan_type: {} does not pin plan_type, though validRecommendation already pins it.
• Equal compares the struct with ==, so a pointer field added later would be compared by address.
• The exported functions have no caller yet. The stacked PR1b and PR2 are the planned consumers.

Gate:

• Labels are mirrored from feat(insurance): per-recommendation Archera quotes (blocked on an Archera API mapping) #799.
• CodeRabbit was rate limited and posted no review or comments. It is optional under the review gate, because this review and exact-revision verification cover the SHA.
• Merging waits for CI to go green at aeb2e93.

[cristim]

Independent review at aeb2e93

Verdict: no blocking or major findings. Two minor nits, neither required. I started on be20c0a; the PR head is now aeb2e93 (a rebase onto the go-pin commit). git range-diff shows the commit unchanged and git diff be20c0a4 aeb2e936 -- internal/archera is empty. I re-ran the checks on aeb2e93.

Findings

1. Nit, recommendation_projection.go:15-24: the constant storedPartialUpfr is abbreviated and the other constants are inconsistent with it. The seven stored* constants are single-use. Rename it or inline them. This does not affect behavior.
2. Nit, recommendation_projection_test.go (hourly table): the accepted list covers 1E2, 1e-07, 1e100 and 1e-100, but not an explicit + exponent such as 1e+5. Go's own float marshal emits 1e+21, so it is a real stored shape. It passes today (probe below); a table entry would pin it.

I tried to refute both as bugs and could not. Neither changes behavior.

Checked against sources of truth

• JSON tags: SavingsPlanDetails in the pinned pkg (common/types.go:685-709) uses plan_type, hourly_commitment (float64), instance_family, region and offering_id. They match storedDetails. offering_id is omitempty, so a missing one fails loud, as intended.
• Writer: the scheduler's convertRecommendations stores Term as Atoi(TrimSuffix("1yr","yr")), which gives 1 or 3, and Payment through NormalizePaymentOption. For aws that is exactly no-upfront, partial-upfront and all-upfront (validation.go:50). The mapping matches. Details come from json.Marshal, so there are no duplicate keys, and JSONB normalizes them anyway.
• Quoted tokens: the leading-quote check was removed. I probed "0.5", "1e5", "0x1p-2", "1/2", " 0.5" and "0.5"e5. All return nil, because big.Rat.SetString rejects the quote. The removal is safe.
• Exponent edge cases through storedHourlyAmount:
  • Accepted: 1E5, 1e+5, 1e05, 1e-0, 1e+0100 (exponent 100) and 1e-0100.
  • Rejected: -0, 0.0, 0e100, 1e, 1e+, 1e++5, 1e5.5, 1E5 , and a 66-byte exponent-only token.
  • 007, .5, 0x10, 1_0 and 1p5 are accepted by SetString. They are not valid JSON, so they cannot arrive as json.RawMessage after Unmarshal. The function is private and the comment states the reliance.
• Details cap: the 64 KiB check runs before Unmarshal. The padded test is valid JSON plus whitespace, so it exercises the cap itself. The all-space case alone would fail at Unmarshal regardless.
• CloudAccountID: it is read nowhere in the R1 matcher (validRecommendation, sameResource, sameCurrentBasis). The struct gains one field, with no unkeyed literals, and the R1 tests pass. It is used only by Equal.
• gocyclo: ProjectRecommendation is 10 (the pre-commit threshold is >10). The files are 119 and 210 lines. YAGNI: Equal has a real caller in PR3 per the plan. Nothing else is speculative.

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

All 15 killed.

• Field tags: hourly, offering_id, instance_family, region and plan_type.
• Payment: partial and all-upfront wrongly mapped.
• Term: 3 years mapped to 1 year.
• Equal: CloudAccountID, SourceAccountID, InstanceFamily, TopLevelRegion and NativeDurationSeconds each ignored.
• Exponent bound moved to +1.
• Details cap raised 100x.

Not mutated (15-mutant cap): the 64-byte token cap and the Sign() <= 0 check. The bytes_65 and zero cases cover them by inspection.

Local verification (synthetic fixtures only, no real account or vendor data)

• GOWORK=off go test -race ./internal/archera/ passes (118 tests) at aeb2e93.
• GOWORK=off go vet ./internal/archera/ is clean.
• This was not run against real stored rows. The shape check above is against the provider struct tags and the scheduler code, not production data.

This is a review verdict only. It is not an approval and not a merge.