Skip to content

feat(insurance): add read-only archera-comparison command - #2166

Open
cristim wants to merge 4 commits into
mainfrom
feat/2156-archera-comparison-impl
Open

cristim wants to merge 4 commits into
mainfrom
feat/2156-archera-comparison-impl

Conversation

@cristim

@cristim cristim commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds ri-helper archera-comparison: an explicit, read-only, default-off comparison of one Archera commitment plan using pkg/insurance (pkg pin 73d3366, already on main). It never purchases, is independent of --purchase, and makes no Archera request unless run.

  • Config: ARCHERA_API_KEY from the environment only (no key flag); ARCHERA_ORG_ID / ARCHERA_PLAN_ID from env or --org-id / --plan-id (read inside RunE only, so --help never shows them). Missing setting: exit 1 naming the setting, no value echoed.
  • insurance.Config and *insurance.Client are RunE locals; nothing holds them in a package var or struct. Production passes hc=nil (hardened client, pinned origin, no redirects, no retries).
  • Output via cmd.OutOrStdout() only (AppLogger not used), so --format json is exactly one document. Exact decimal money (smallest round-tripping precision, no float, no division), unknown = unknown/null, 730-hour monthly vs one-time upfront, discount rate verbatim, vendor strings stripped of control chars and capped at 256 bytes, product support supported/unknown only, caveat and plan-wide note, disclosures verbatim from pkg/common (non_gating_disclosure, sponsorship_disclosure).
  • Vendor errors: sanitized HTTP message plus Retry-After (client caps at 24h); no retry.

Evidence (synthetic, offline)

All tests use httptest behind a RoundTripper that asserts https://api.archera.ai before rewriting; synthetic key and UUIDs; no live Archera calls. Covered: default-off (no request on --help), each missing setting, documented GET path/header/no query, table and JSON output (premium-inclusive, unknown, fallback reason, lease attached, supported vs unknown product), control-char stripping, 401/403 (body echoing the key)/429 (Retry-After 30)/invalid UUID/redirect (second URL never hit), no key-like flag, %v %+v %#v %s of CLI structs before and after a call, key grep on stdout/stderr/errors. make lint 0 issues; go test ./cmd ok (457s).

No filter flags (defaults only), so no ContractTerms seam is needed yet.

Closes #2156

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added the optional, read-only archera-comparison command to compare an Archera commitment plan. Results are available in table or JSON format; the command makes no request unless run.
    • Comparison output includes plan totals, line-item details, deltas, support status, and disclosures, with unknown values clearly identified.
  • Documentation
    • Added a guide covering configuration, output formats, and how to interpret comparison results.

Explicit, default-off comparison of an Archera commitment plan using
pkg/insurance. Key from ARCHERA_API_KEY only (no flag); org and plan IDs
from env or flags. Table or JSON output via cmd.OutOrStdout, exact decimal
money, unknown never rendered as 0, vendor strings sanitized, disclosures
verbatim. Never purchases and makes no request unless run.

Closes #2156

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/few Limited audience effort/m Days type/feat New capability labels Oct 10, 2026
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →Review in Change Stack →

Warning

Review limit reached

  • Run on-demand review

This review includes 9 billable files and costs up to $2.25.

  • 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 54 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 89 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-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: d991fc50-f5e4-4c28-b705-8946a541324e

📥 Commits

Reviewing files that changed from the base of the PR and between 722808a and 222ef76.


📒 Files selected for processing (9)
  • CHANGELOG.md
  • README.md
  • cmd/archera_comparison.go
  • cmd/archera_comparison_test.go
  • cmd/archera_render.go
  • cmd/testdata/archera_comparison.json
  • cmd/testdata/archera_json.golden
  • cmd/testdata/archera_table.golden
  • docs/cli/archera-comparison.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 9d6739c3-14de-48ce-bd78-3f8e37787e66




📥 Commits

Reviewing files that changed from the base of the PR and between 0ab66ab and 722808a.





📒 Files selected for processing (6)
  • CHANGELOG.md
  • README.md
  • cmd/archera_comparison.go
  • cmd/archera_comparison_test.go
  • cmd/archera_render.go
  • docs/cli/archera-comparison.md




Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.






📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The change adds the opt-in archera-comparison command. It retrieves a plan comparison and renders it as a table or JSON. The change also adds tests and documentation for command configuration, output, and error handling.

Changes

Archera Plan Comparison

Layer / File(s) Summary
Comparison output contract and rendering
cmd/archera_render.go
Defines the comparison output DTO and converts comparison data into JSON or table output. Sanitizes vendor text and formats known monetary values as exact decimal strings; unknown values remain unknown.
Command configuration and request flow
cmd/archera_comparison.go
Adds the command, resolves the API key from the environment and organization and plan IDs from flags or environment variables, then requests and renders the comparison.
Command tests and documentation
cmd/archera_comparison_test.go, docs/cli/archera-comparison.md, README.md, CHANGELOG.md
Adds tests for command requests, output, configuration, errors, redirects, and credential handling. Adds command documentation and changelog and README references.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Command as archera-comparison command
  participant Client as insurance.Client
  participant API as Archera API
  participant Renderer as JSON/table renderer
  Command->>Client: Request comparison with command context
  Client->>API: Send comparison request
  API-->>Client: Return comparison data or error
  Client-->>Command: Return comparison result
  Command->>Renderer: Build and render selected output
Loading







Merge Risk: ⚪ Minimal · up to 72280

The supplied evidence establishes no issue that should block merging. The reported test results and insurance-client behavior remain unverified here.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 14.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 3 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely identifies the main change: adding the read-only archera-comparison command under the insurance feature.
Linked Issues check Passed Issue #2156 is open and directly linked. The PR adds the default-off archera-comparison command, uses pkg/insurance, reads the API key from ARCHERA_API_KEY, accepts organization and plan IDs fro…
Out of Scope Changes check Passed The changed files support issue #2156. They add the command, rendering code, focused offline tests, the required dependency pin, and related README, CLI documentation, and changelog entries. The chang…




Full details: Docstring Coverage

Explanation

Docstring coverage is 14.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 3 files. (3 skipped: 3 unsupported.)









✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR





🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR








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

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

@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Gate review (independent, Opus) at 722808a: CHANGES REQUESTED, not merged

Local evidence (git archive of the head, GOTOOLCHAIN=go1.26.9, GOWORK=off, macOS; fixture/httptest only, no live Archera calls):

  • go build ./..., go vet ./..., make lint (0 issues): pass. go test ./cmd: ok (497s).
  • Real binary, no key set: exit 1, stderr ARCHERA_API_KEY is not set (read from the environment only), stdout empty; --help shows no key flag and no env value.
  • pkg pinned at 73d3366 (from main), labels mirror feat(insurance): explicit Archera comparison command using pkg/insurance #2156, docs/README/CHANGELOG match behaviour.

Security properties: hold. Mutations killed by the PR's tests: unknown-as-supported (JSON and table), disclosures replaced, 2-decimal money, JSON written to os.Stdout, key fallback, missing-setting message echoing the key, Retry-After hidden, one automatic retry, api-key flag, unexported insurance.Config field in archeraOpts (%v leak caught), control-char strip removed, 256-byte cap removed. Money through float64 is killed only by my extra test (12345678901234567.01); the PR's own money test values are all float-exact.

Findings (blocking):

  1. cmd/archera_comparison_test.go field-mapping coverage: 32 of 41 single-field mutations in cmd/archera_render.go survive the Archera tests, including swapping CommitmentCostTotal/CloudProviderCost (L156-157), GrossSavings->CoveredOnDemandCost (L159), CoveredOnDemandCost (L161), totals/offer UpfrontCost dropped (L166, L196), all four delta_vs_current fields (L198-201), BreakevenDays (L195), offer IsCurrent/OfferID/CommitmentType/Provider/Region/ContractTerm/PaymentOption/LeaseAttached (L183-191), PlanID, FetchedAt UTC, all hypothetical fields incl. Totals replaced by current (L222-227), line item LineItemID/ActualTerm/ActualPaymentOption/ActualCommitmentType (L230-233), row LineItemID and Current replaced by candidate 0. Cause: the fixture reuses one financials object and identical offers everywhere, and the assertions cover only premium, net, discount rate, reason. Fix: give every money field and every offer/row/hypothetical a distinct value and assert each JSON field path (and the table line) exactly.
  2. TestArcheraVendorErrorsAndRedaction "403 echoing key" (L295) is vacuous: bodies use {"detail": ...} but the pinned client reads message (insurance/client.go:233), so no vendor text ever reaches the error. With {"message":"bad key <key> sent"} the error contains [redacted] and no key (verified locally), so production is safe; the test should use the message shape and assert [redacted] so it can fail.
  3. Minor: the caveat line can be deleted from the table with no failure; the flag check only lists exact names (an archera-token flag survives).

CI: Integration Tests fail on TestRecommendationCompletenessCommand/savingsplans-ec2instance/valid (DescribeRegions count), outside this diff; needs a rerun or its own fix before merge. mergeStateStatus BLOCKED.

@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Line-number correction for the gate comment above (same findings, head 722808a): archera_render.go financials L160-165, totals upfront L170, offer fields L188-205 (deltas L201-204), hypothetical fields L230-235, line item fields L240-244; test case "403 echoing key" is archera_comparison_test.go:296.

Every money field in the fixture now carries a unique value (hundreds digit
is the block, units digit the field) and table and JSON output are compared
against golden files, so a swapped field mapping changes the output. One
value is a long decimal that float64 cannot hold exactly. The vendor error
cases use the real "message" body shape and assert the key is redacted. The
credential-flag check matches any flag containing key, token, secret or
password.

Refs #2156

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Gate re-review (independent, Opus) at 7082bb8: CHANGES REQUESTED, not merged

The diff since 722808a touches only test files and testdata; the renderer is unchanged. Local run (git archive, go1.26.9, GOWORK=off, macOS, httptest fixtures only, no live calls): build and vet pass; full go test ./cmd -count=1 ok (501s). CI is green on this head, and mergeStateStatus is CLEAN.

I reran my mutation set against the PR's Archera tests: 48 of 50 mutants were killed. These include every money/offer/row/hypothetical/line-item field mapping, money via float64 (both goldens fail), deleting the caveat, an archera-token flag, and the security set (disclosures, unsupported shown as supported, Retry-After, retry, key flag, unexported Config field, strip/cap removal). Two mutants survive:

  1. offer.name_unsanitized (security regression in the tests): ArcheraOfferName: e.GuaranteedDisplayName (raw, without archeraSanitizePtr, archera_render.go:194) passes. At 722808a this mutant was killed, because the old fixture carried \u001b[31m in the offer name. The new testdata/archera_comparison.json has no control character in any vendor string, so assert.NotContains(t, out, "\x1b") (archera_comparison_test.go:203) can never fail, and no vendor field's sanitizer wiring is checked end to end. Fix: put an ESC and a C1 (\u009b) character into at least the offer name and one other vendor string in the fixture, and update the goldens.
  2. fetched_at_utc: c.FetchedAt.Local().Format(...) passes, because archeraFetchedRE (L171) masks the timestamp. On this machine (CEST) the output then shows local time labelled Z. Fix: before masking, parse the captured value and assert it is within a few seconds of time.Now().UTC(), or assert the hour against UTC.

@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Correction to the re-gate comment above: the offer-name mapping is archera_render.go:196, not :194.

cristim and others added 2 commits October 10, 2026 05:47
…omparison

The fixture now injects ESC and C1 characters (and an over-256-byte name)
into the vendor string fields, the goldens pin the sanitized values, and a
direct DTO test covers the fields the decoder would reject. The printed
fetch time is asserted to be the current instant in UTC with the process
zone shifted. Pkg-supplied product support text is no longer sanitized.

Refs #2156

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Gate finding 1 (blocker) at 222ef76: data race from mutating time.Local in tests; CI Unit Tests and Integration Tests fail on it.

cmd/archera_comparison_test.go:116-118 (runArchera) assigns time.Local = time.FixedZone(...) and restores it in t.Cleanup. time.Local is an unsynchronized package global read by every time.Now()/formatting call, including the httptest.Server connection goroutines (net/http/server.go:1850 setState -> time.go:1360). Under go test -race (Makefile lines 33/37/41, CI run 38021943815) this is reported as race detected during execution of test and fails TestArcheraComparisonTable, TestArcheraComparisonJSON, TestArcheraVendorErrorsAndRedaction/401 (stack at archera_comparison_test.go:117). Not a flake: it is deterministic whenever a server goroutine from the current or previous test touches time after the write.

Suggested fix: set the shifted zone once before any goroutine starts (in TestMain in cmd/main_test.go, before m.Run()), or re-exec the test binary in a subprocess with TZ=Asia/Kolkata, and drop the per-test write/restore. Keep the "ends in Z and within 2 minutes" assertion so the .Local() mutant is still killed.

@cristim

cristim commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Gate re-review at 222ef76: REQUEST CHANGES (blocker is finding 1 above: the time.Local data race that fails CI Unit Tests and Integration Tests under -race; reproduced locally with go test -race -count=3 -run Archera ./cmd: 6x WARNING: DATA RACE, 5 Archera tests FAIL).

Since 66b752d the PR's own files are unchanged except CHANGELOG lines from main. Other evidence at this SHA (git archive, GOTOOLCHAIN=go1.26.9): full go test -count=1 ./cmd without -race ok 437.676s; golangci-lint run no issues; labels match #2156.

Independent mutation sweep, 73 mutants over every DTO field mapping, sanitizer call sites, money formatting, disclosures/caveat/premium note, Retry-After, format check, output writer, settings/key handling, loops: 65 killed, 8 survived. The .Local() FetchedAt mutant the author did not run is KILLED (TestArcheraComparisonTable, archera_comparison_test.go:187). All sanitizer removals (offer name, ids, region, commitment types, payment, reason, plan id) are killed.

Survivors (finding 2, should fix alongside the race):

  1. cmd/archera_comparison.go:100 adding fmt.Fprintln(os.Stderr, "using key", key) SURVIVES. The no-echo assertions only inspect the cobra out/err buffers, so a key leak to the real process stdout/stderr is undetected. Capture os.Stdout/os.Stderr (pipe) in the redaction test and assert the key is absent.
  2. archera_comparison.go:57-62 swapping flag/env precedence survives: no test sets both --org-id/--plan-id and the env var.
  3. archera_comparison.go:107 removing the plan-ID guard survives: pkg's own error also contains "plan ID", so assert the full message naming --plan-id or ARCHERA_PLAN_ID.
  4. archera_render.go Currency: nil survives (no fixture or DTO test sets a currency).
  5. Truncating dto.Rows to 1 survives: the fixture has a single row; add a second row.

Non-blocking: dropping .UTC() is equivalent (pkg client.go:202 already returns time.Now().UTC()); passing http.DefaultClient instead of nil survives but pkg client.go:100-101 forces no-redirect on any client, and the code passes nil by inspection; cobra.NoArgs -> ArbitraryArgs survives (trivial). archeraSanitize uses unicode.IsControl, which keeps bidi format characters (U+202E etc., category Cf), so a vendor id can visually reorder the rest of its table line; consider also dropping unicode.Is(unicode.Cf, r).

Product-support Source/Evidence left unsanitized: accepted, they come from pkg constants in AssessProductSupport, not vendor data.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(insurance): explicit Archera comparison command using pkg/insurance

1 participant