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

[cristim]

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; the vendor text is cleaned, the API key is masked, and the whole message is withheld if the key is still readable once combining marks, variation selectors, fillers, spaces and invalid bytes are ignored.
• Success-path table and JSON output is not key-masked (owner ruling); only error text is masked.

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 for viewing an Archera commitment plan comparison. It makes no Archera request unless run and supports table or JSON output.
  • Results show plan totals, line-item details, hypothetical options, deltas, product-support status, and disclosures. Unknown values are identified, and monetary amounts use exact decimals.
  • Configure the command with ARCHERA_API_KEY and organization and plan IDs supplied by flags or environment variables.
• Documentation
  • Added a guide to configuration, output formats, and interpreting comparison results.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds the opt-in archera-comparison command. It fetches an Archera plan comparison and renders table or JSON output. The command resolves configuration, sanitizes errors, and includes tests and documentation for its behavior.

Changes

Archera Plan Comparison

Layer / File(s)	Summary
Comparison output contract and rendering cmd/archera_render.go, cmd/testdata/archera_comparison.json, cmd/testdata/archera_json.golden, cmd/testdata/archera_table.golden, cmd/archera_comparison_test.go	Defines the JSON output DTO and maps comparison data to JSON or table output. Sanitizes vendor text and formats known monetary values as exact decimal strings. Fixtures and tests cover output fields, formatting, and sanitization.
Command request and configuration cmd/archera_comparison.go, cmd/archera_comparison_test.go, README.md, docs/cli/archera-comparison.md, CHANGELOG.md, go.mod	Adds the default-off command. It reads the API key from the environment and resolves organization and plan IDs from flags or environment variables. Tests cover configuration, requests, and output behavior. Documentation and the changelog describe the command.
Bounded error sanitization and key detection cmd/archera_comparison.go, cmd/archera_comparison_test.go, cmd/archera_comparison_key_test.go	Sanitizes and bounds error messages. The command withholds messages when key-readability checks indicate that the key may remain visible. Tests cover obfuscated keys, error paths, and truncation boundaries.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 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 or table renderer
  Command->>Client: Request comparison with resolved settings
  Client->>API: Fetch plan comparison
  API-->>Client: Return comparison data or error
  Client-->>Command: Return comparison result
  Command->>Renderer: Render selected format

Loading

---

Merge Risk: ⚪ Minimal · up to 1194b

No concrete merge-blocking issue was established. The timing concern remains unverified, and the available evidence does not show that the command exposes the API key.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 4 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main change: adding a read-only archera-comparison command.
Linked Issues check		Issue [#2156] is open and directly linked. The reviewed head includes the explicit default-off archera-comparison command, pkg/insurance pin 73d3366903bc, environment-only ARCHERA_API_KEY, and…
Out of Scope Changes check		The command, renderer, tests, fixtures, dependency update, README entry, documentation, and changelog entry support Issue [#2156]. The added error-redaction and output tests support the required crede…

---

Full details: Docstring Coverage

Explanation

Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 4 files. (1 skipped: 1 unsupported.)

---

• Fix all pre-merge checks with AI

✨ 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]

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]

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.

[cristim]

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]

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

[cristim]

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]

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.

[cristim]

Gate finding F1 at 8f95325 (credential handling, blocking): the API key can reach the error output

Reproduced with a probe test in a git archive copy (GOTOOLCHAIN=go1.26.9, httptest only, synthetic key):

1. Strip reassembles a masked key. pkg cleanMessage (client.go, pin 73d3366) masks the key only on an exact match and only strips unicode.IsControl. If the vendor message carries the key with U+200B between characters, pkg misses it, then archeraSafeError -> archeraSanitizeN (cmd/archera_render.go:40, new Cf strip) removes the U+200B and rebuilds the key:
   err="archera API returned HTTP 403: bad key test-key-SYNTHETIC-0000"
2. Decode errors are not masked at all. Key sent back verbatim in an enum field (hypothetical_totals[0].payment_option):
   err="hypothetical_totals[0]: payment_option: unsupported value \"test-key-SYNTHETIC-0000\""

pkg masks a vendor-echoed key on the HTTP path, so vendor echo is an in-scope threat; the CLI now has two paths around it. Fix: in runArcheraComparison (cmd/archera_comparison.go:83-85), mask key after sanitizing (e.g. archeraSafeError(err, key) doing strings.ReplaceAll(sanitized, key, "[redacted]") as the last step), and add both cases above as tests (ZWSP-interleaved key in a 403 message; key as an enum value in the body).

[cristim]

Gate verdict at 8f95325: REQUEST CHANGES (finding F1 above, credential handling, plus two credential/security-sink test gaps below).

Evidence at this SHA (git archive, GOTOOLCHAIN=go1.26.9, httptest only, no live Archera/AWS calls):

• go test -race -count=3 -run Archera ./cmd: ok, 0 DATA RACE (race fixed: zone set once in test init()).
• go test -count=1 ./cmd: ok 112.967s. golangci-lint run: 0 issues. CI green, mergeStateStatus CLEAN, labels match feat(insurance): explicit Archera comparison command using pkg/insurance #2156.
• Independent probes pass: all 24 vendor string fields fed ESC+C1+U+202E+U+2066+U+200B+U+2028/9+300B come out clean and <=256 bytes in DTO and table; a 403 message with U+202E/U+200B/U+2028 is stripped (HTTP 403: badevilxy); JSON disclosures equal common.ArcheraNonGatingDisclosure/ArcheraSponsorshipDisclosure verbatim; no USD/$.
• Fixed since 222ef76 and verified killed: key to real os.Stdout/os.Stderr/cobra streams/errors, flag-vs-env precedence, plan-ID guard (exact message), Currency: nil, rows/hypotheticals/line items truncated to one, .UTC()->.Local(), every field mapping (financials, totals, offer, delta, hypothetical, line item), every sanitizer call site, Cf/Zl/Zp/IsControl removal, money precision/float/nil->0, unknown->0, disclosures/caveat/premium note.

Mutation sweep: 140 mutants, 123 killed, 17 survived.

Blocking (credential handling / security sink):

1. F1 (posted above): key rebuilt by the Cf strip from a ZWSP-interleaved vendor echo, and unmasked in decode errors.
2. log.Println(key) in RunE SURVIVES. log's default logger captured os.Stderr at package init, so swapping os.Stderr in captureRealStd (archera_comparison_test.go:131) does not see it. Also log.SetOutput(errW) there and restore it.
3. archeraSafeError with stripping removed (cap only) SURVIVES (archera_comparison.go:121). pkg %q already escapes decode-error values and pkg strips IsControl from HTTP messages, so no test reaches the CLI strip. Add a 403 whose message carries U+202E/U+200B/U+2028 and assert they are gone (current code does strip them; the test is what is missing).

Non-blocking (equivalent or cosmetic, one list): money via big.Float.Text (equivalent: the Cmp round-trip guard forces the exact value); support if true (equivalent: unknown has empty Source/Evidence); Provider constant "aws" (fixture all aws); per-site uncapped OfferID/CommitmentType/PlanID/Reason/row LineItemID (strip kept; caps verified by probe); sanitizer byte cut mid-rune (len-only test); key into a package var (verified absent by inspection); RetryAfter >=0; Retry-After wording; http.DefaultClient (pkg forces no-redirect); cobra.NoArgs; context.Background().

[cristim]

Gate finding F3 at 2d42cf8 (credential handling, blocking per the cross-repo ruling from the mcp #59 gate)

F1 is fixed for Cc/Cf/Zl/Zp and for literal \uXXXX/\xNN escapes: a ZWSP/WJ/SHY/RLO/LS-split key and a plain key are [redacted] in HTTP, decode and transport errors, and literal backslash escapes leave visible backslashes with no reassembly. Errors wrapping context.DeadlineExceeded/Canceled all go through archeraSafeError (cmd/archera_comparison.go:86), so the plain key there is masked too (NB2 clean).

Still leaking (NB1): characters that survive archeraSanitizeN keep the key split, so the exact-match mask misses it, but the terminal still displays the key. Synthetic key, probe in a git archive copy, through archeraSafeError and through a real 403 via runArchera:

• VS16 U+FE0F: archera API returned HTTP 403: k t️e️s️t️-️k️e️y️-️S️Y️N️T️H️E️T️I️C️-️0️0️0️0
• The same happens with VS17 U+E0100, CGJ U+034F, Mn U+0301, Me U+20DD, the Hangul fillers U+115F/U+1160/U+3164/U+FFA0, and invalid UTF-8 \xff (shown as U+FFFD). Each leaks in the transport, ctx-wrapped and HTTP paths.

Fix (the mcp ruling, fail closed): build a skeleton of the cleaned message with Cc/Cf/Mn/Me/Zl/Zp/Zs, variation selectors (FE00-FE0F, E0100-E01EF), CGJ, the Hangul fillers and invalid bytes removed. If the skeleton contains the trimmed key, withhold the whole message (e.g. archera API returned HTTP 403: [message withheld: contained the API key], keeping the status and Retry-After). Keep the existing mask for the plain case. Add the classes above to TestArcheraErrorsMaskObfuscatedKey.

Accepted as documented limits (non-blocking): success-path table/JSON output is not key-masked (coordinator ruling: document in the PR body). Keys that themselves contain \ or " would be escaped by pkg %q and miss the mask (Archera keys are not expected to contain them).

[cristim]

Gate verdict at 2d42cf8: REQUEST CHANGES (F3 above, plus one credential test gap).

Evidence at this SHA (git archive, GOTOOLCHAIN=go1.26.9, httptest only, no live Archera/AWS calls; heavy runs throttled per the coordinator's rules):

• go test -race -count=3 -run Archera ./cmd: ok, 0 DATA RACE. go test -count=1 ./cmd: ok 87.7s. golangci-lint run: 0 issues. CI green, mergeStateStatus CLEAN, labels match feat(insurance): explicit Archera comparison command using pkg/insurance #2156.
• F1 is fixed for the key split by ZWSP/WJ/SHY/RLO/LS and by literal \u/\x escapes, and for a plain key, across the HTTP, decode and transport (incl. ctx-wrapped) paths. The escape strip creates no new reassembly. log.Println/slog.Info key mutants are now KILLED (TestArcheraRunDoesNotLogKey). Turning off the bidi (Cf) strip is KILLED.

Mutation sweep at this head: 126 mutants run, 113 killed, 13 survived. The 22 render-only table/sanitizer-site mutants were not re-run: archera_render.go is byte-identical to 8f95325 and the test diff is additions only, so their earlier results carry over (all killed except the per-site cap mutants listed below).

Blocking:

1. F3 (comment above): a key split by VS/CGJ/Mn/Me/Hangul fillers/invalid UTF-8 is not masked and still displays as the key. Fail closed per the mcp #59 ruling.
2. Mask-after-cap SURVIVES (archera_comparison.go: mask applied after the 512-byte cap). The "straddles the cap" case in TestArcheraErrorsMaskObfuscatedKey asserts only that the full key is absent, so a truncated partial key prefix passes. The current order (mask, then cap) is right; the test should also assert that no 8+-character prefix of the key is in the output.

Non-blocking (equivalent/cosmetic): final-cap strip removal (equivalent: the text is already cleaned before masking); regex without \x (a control-split key then shows a visible literal \x01, and is also covered once F3's skeleton check lands); money via big.Float (equivalent through the Cmp round-trip); support if true (equivalent); Provider "aws" constant; sanitizer byte cut mid-rune; per-site uncapped ids (caps verified by probe); key in a package var (absent by inspection); RetryAfter >=0 and its wording; http.DefaultClient (pkg forces no-redirect); cobra.NoArgs; context.Background().

Documented limits (coordinator ruling): success table/JSON output is not key-masked; please state this in the PR body.

[cristim]

Fixes pushed at 70a511b for the gate findings at 2d42cf8.

• F3: archeraSafeError now cleans the text (Cc/Cf/Zl/Zp), masks the trimmed key, builds a skeleton of the full text (drops Cc, Cf, Mn, Me, Zl, Zp, Zs, Hangul fillers, invalid bytes; variation selectors are Mn) and withholds the whole message when the key is still readable, then caps. HTTP errors keep the status and Retry-After. Same order and skeleton semantics as cloud-commitments-mcp.
• Tests (synthetic key only): VS16/VS17/CGJ/U+0301/U+20DD/Hangul fillers/space/invalid UTF-8, split once and split everywhere, across HTTP, decode, transport, deadline-wrapped and cancel-wrapped errors; a cap-boundary sweep asserting no key fragment of 4+ bytes survives; a withhold-at-every-cap-offset sweep; a trimmed-key test.
• Mutants (each fails the new tests, real code passes): mask after cap (killed by the prefix sweep), no skeleton check, skeleton without Mn/Me, without Hangul fillers, without invalid bytes, check after cap, untrimmed key. A variation-selector-only removal is equivalent (Mn covers them), so that class was dropped from the skeleton.
• PR body now states that success-path table/JSON output stays unmasked (owner ruling).
• Go pin stays at 73d3366: insurance.Secret is in cloud-commitments-go PR feat(frontend/recs): On-Demand Monthly column should use provider on_demand_cost directly (no reconstruction) #330 (source-breaking Config.APIKey change), a separate follow-up bump.

[cristim]

Gate verdict at 70a511b: REQUEST CHANGES (F4, F5; both credential handling, blocking)

F3 from the previous round is addressed for the cases it named. Two bypasses remain. Evidence: git archive 70a511b2 into a scratch dir, a probe test file added next to cmd/archera_comparison_test.go reusing setupArchera/runArchera/archeraTestKey, go test ./cmd -run TestProbeF -count=1 -v (go1.27.1 darwin/arm64, httptest only, no live Archera calls). Both probes FAIL at this SHA.

F4: key prefix survives the pinned lib's 200-byte cut

The pinned cloud-commitments-go/pkg@v0.0.0-20261008151559-73d3366903bc insurance/client.go:237 does truncate(cleanMessage(msg, apiKey), 200). cleanMessage drops only unicode.IsControl runes and masks only the exact key. A key split by U+200B (Cf, not a control) passes the lib's mask, the 200-byte cut lands mid-key, then the CLI drops the U+200B, so the exact-key mask and the full-key skeleton check in archeraSafeError (cmd/archera_comparison.go) both miss the surviving prefix.

Probe: vendor 400 body {"message": "<186 x a>" + "test​-key-SYNTHETIC-0000"}. Actual CLI error:

archera API returned HTTP 400: aaaa...aaaatest-key-SY...
--- FAIL: TestProbeF4KeyPrefixAcrossLibTruncation
    F4: output contains key fragment "test-key-SY" (11 of 23 chars)

(The unsplit key straddling the cut is masked by the lib before truncation, so the split is required; that is exactly the vendor-controlled input F1/F3 covered.)

Fix (test first):

1. Add TestArcheraHTTPErrorKeyPrefixAtLibTruncation: httptest 400 whose message is padding + key split by U+200B after byte 4, placed so the 200-byte cut falls mid-key. Assert no 8+ character substring of archeraTestKey appears in stdout, stderr or the error, and that the error contains the withheld marker. Confirm it fails at 70a511b.
2. In archeraSafeError, after masking, withhold when the skeleton of the message contains the skeleton of any substring of the key of length >= 8 (sliding window over the key; a truncated tail is the case that matters, but check every window so a cut at either end is covered). Make the threshold a named constant.

F5: U+2800 and unassigned code points bypass archeraSkeleton

archeraSkeleton uses a deny list (Cc, Cf, Mn, Me, Zl, Zp, Zs, Hangul fillers, RuneError). U+2800 BRAILLE PATTERN BLANK is So and renders blank; unassigned code points (Cn, e.g. U+0378, U+E0080) are in none of the listed tables. A key interleaved with any of them is neither masked nor withheld:

--- FAIL: TestProbeF5SkeletonBypass
    F5 U+2800: key spaced out by invisible rune not withheld: "vendor said: t⠀e⠀s⠀t⠀-⠀k⠀e⠀y⠀-⠀S⠀Y⠀N⠀T⠀H⠀E⠀T⠀I⠀C⠀-⠀0⠀0⠀0⠀0⠀"
    F5 U+0378 unassigned: ... not withheld
    F5 U+E0080 unassigned tag-block: ... not withheld

A deny list will keep losing to the next blank-rendering code point.

Fix (test first):

1. Add TestArcheraSkeletonDropsNonKeyRunes: table of separators {U+2800, U+0378, U+E0080, plus one visible non-key rune such as ·}, each interleaved between every key character and passed through archeraSafeError; assert the result contains the withheld marker and no 8+ character key fragment. Confirm it fails at 70a511b.
2. Replace the deny list with an allow list derived from the key: build the skeleton of the message from only the runes that occur in the key (a set built per call), and compare against the key itself. This subsumes the current Cc/Cf/Mn/Me/Z*/filler cases, so the existing F1/F3 tests must stay green unchanged. Combine with F4's windowed check.

Gate status

Not merging. A new commit restarts the gate; the next gate re-runs the probes above against the new head.

[cristim]

Addressed gate F4 and F5 in the new commit (head e2035fb, main merged in).

• F4: archeraSafeError now withholds the whole message when any run of min(len(key), 8) consecutive key characters is readable after skeleton normalisation, on every path (HTTP, decode, transport, deadline, cancel). This covers the pkg 200-byte cut.
• F5: the skeleton keeps only runes that occur in the trimmed key, so any inserted character (U+2800, unassigned code points, anything unlisted) is a separator. The category lists are gone.
• Tests: TestArcheraLongKeyNeverReadableOnAnyPath uses a 41-char synthetic key, 30 separators, split once and between every character, HTTP with a pad sweep across the pkg cut, plus decode, transport, deadline and cancel. It fails on 70a511b and passes now.
• Mutants killed: allow-list removed, threshold 8 to 64, whole-key-only check, check after cap, mask after cap.
• golangci-lint 0 issues; full ./cmd tests pass.

Synthetic keys only. Not merged; re-gate needed.

[cristim]

Gate verdict at e2035fb: REQUEST CHANGES (G1, G2 blocking credential handling; G3, G4 test gaps; G5 low). Not merged.

Evidence: git archive e2035fb7 into scratch dirs; probe file next to cmd/archera_comparison_test.go reusing setupArchera/runArchera/archeraLongKey; httptest only, synthetic keys, no live Archera calls. Heavy runs under the heavy-run lock, nice, -p 2.

F4/F5 from the last round are fixed. My F4 probe (U+200B-split key across the pkg's 200-byte cut, HTTP path) and F5 probe (U+2800, U+0378, U+E0080 between every key character) both PASS at this SHA and FAILED at 70a511b. TestArcheraLongKeyNeverReadableOnAnyPath covers the 41-char key, 30 separators, the pad sweep across the cut, and the decode/transport/deadline/cancel paths. Other cases that behave correctly: short 5-char key spaced by invisible runes (withheld), repeated-character key (withheld), exactly 8 consecutive key chars (withheld), 7 (shown, by design), escaped ⠀ in a quoted decode value (withheld).

CI: all checks complete, success except Trivy/gosec NEUTRAL; mergeStateStatus CLEAN. Labels match #2156. Local: GOTOOLCHAIN=go1.26.9 golangci-lint run ./... 0 issues; go test -count=1 ./cmd ok (57.9s).

G1 (blocking): a key character used as the separator defeats the allow-list

The skeleton keeps every rune that occurs in the key, so a separator taken from the key's own alphabet stays in the skeleton and breaks every 8-window. archeraLongKey contains -, x, 0. End to end through the HTTP path (401, message "bad key " + key with "-" after every char):

err="archera API returned HTTP 401: bad key P-R-B---s-Y-n-T-h-3-t-1-c---Q-Z-9-x---7-K-d-2---m-W-8-v---L-p-4-n---H-t-6-r---0-0-"

Same result with x and 0 as the separator (PxRxBx-xsxYx..., P0R0B00-0s0Y0...). The key is readable to anyone at a glance, which makes this the same kind of bypass as F5. The only difference is that the separator character comes from the key's own alphabet.

Fix (test first):

1. Add to TestArcheraLongKeyNeverReadableOnAnyPath's separator table: "-", "x", "0", and "--" (all occur in archeraLongKey). Confirm they fail at e2035fb.
2. Withhold when any 8-window of the key occurs in the message skeleton as a subsequence whose gaps are each at most 2 runes, not only as a contiguous substring. A plain substring is the gap-0 case. A small DP or greedy scan per window is enough at these sizes, and the gap bound keeps ordinary text from matching.

G2 (blocking): case-changed key is shown

strings.ToLower(archeraLongKey) through the HTTP path:

err="archera API returned HTTP 401: bad key prb-synth3t1c-qz9x-7kd2-mw8v-lp4n-ht6r-00"

Upper case behaves the same way. That output leaves only letter case to guess. The same holds for the fullwidth form (ＰＲＢ－ｓＹｎＴｈ３ｔ１ｃ…, not withheld), which a terminal shows as the key.

Fix (test first): add lower, upper and fullwidth (r + 0xFEE0 for printable ASCII) echoes to the same table, then fold the message and the key the same way before building the alphabet and skeleton: NFKC (golang.org/x/text/unicode/norm) then strings.ToLower, or, without the new import, unicode.SimpleFold-based lowering plus mapping U+FF01..U+FF5E down by 0xFEE0.

G3 (test gap): threshold boundary is untested

Mutants archeraFragmentLen = 9 and = 10 SURVIVE, because the readability helper in the test only flags 10+ char fragments. Add a direct test: key spaced by U+2800 with exactly 8 consecutive key chars echoed, which must be withheld, and 7, which must not be. That kills 9/10 and pins the 7-char behaviour.

G4 (test gap): window position is untested

Mutants skip-first-window (loop from i=1), skip-last-window (i+n < len(ks)) and prefix-window-only SURVIVE. Add a test where only the key's first 8 chars and, separately, only its last 8 chars are echoed (spaced by an invisible rune so the exact mask does not catch them). Both must be withheld.

G5 (low): key containing a non-printing rune or inner space

archeraKeyReadable builds the alphabet from the raw trimmed key, while the message is cleaned first. Key abcd efgh ijkl echoed as abcd⠀efgh⠀ijkl is shown, and key abcdefg​hijklmn echoed as itself prints abcdefghijklmn. Real Archera keys are unlikely to look like this. Fix: run the key through the same clean as the message and drop non-graphic runes and spaces from the key skeleton before windowing. Add one test for each.

Mutation sweep (21 mutants, go test ./cmd -run Archera)

Killed: mask after cap, no readable check, allow-list removed, keep Mn/Me, keep Hangul fillers, keep invalid bytes (U+FFFD), keep U+2800, check after cap, untrimmed key, threshold 16, whole-key only, max instead of min, suffix-window only, no \uXXXX escape strip, no exact mask, withheld message drops HTTP status. The author's 7 previous-round mutants map onto these and stay killed. Survived: threshold 9, threshold 10, skip first window, skip last window, prefix-window only (G3/G4). Baseline restored and passing.

The next gate re-runs these probes and mutants against the new head.

[cristim]

Addressed gate G1-G5 at 0230025.

• G1: key windows (min(len,8)) now match as an ordered subsequence of the message skeleton with at most 2 skipped runes between neighbors; gap 0 is the old substring check. Separators '-', 'x', '0', '--' are in the path test.
• G2: key and message are folded (NFKD + lower case) before the alphabet and skeleton are built. Lower, upper, fullwidth echoes (also with key-alphabet separators) are tested.
• G3/G4: direct tests for 8 withheld vs 7 shown, on middle, first and last windows (spaced by U+2800); the path helper now flags 8+ characters.
• G5: the key skeleton drops non-graphic runes and spaces. Tests for inner-space and format-rune keys.
• Negative tests: ordinary vendor text is not withheld (long and short key); gap bound test (2 withheld, 3 not).
• Mutants killed: allow-list removed, threshold 9/10/64, gap 3/1/0, fold removed, lower removed, NFKD removed, skip first/last/prefix-only window, key non-graphic/space kept, check after cap, mask after cap, whole-key only, gap ignoring order.
• golangci-lint ./... 0 issues; full ./cmd tests pass. x/text is now a direct dependency (was indirect).

Synthetic keys only. Not merged.

[cristim]

Gate verdict at 0230025: REQUEST CHANGES (H1 hang, H2 gap-3 key-alphabet separators; both blocking). Not merged.

Head is 0230025 (lint-only commit on top of f668042). Evidence: git archive into scratch dirs, probe test files next to the archera tests, httptest only, synthetic keys, no live Archera calls. Heavy runs under the heavy-run lock, nice, -p 2.

What passes

• G1/G2 probes (key archeraLongKey with -, x, 0, -- after every char; lower, upper, fullwidth, fullwidth with -): all withheld at 0230025, all shown at e2035fb.
• Folding edges: ligature in key or message, accent stripped, ß vs SS, Ⅻ vs XII, Turkish İ, combining mark after every char: all withheld. A reversed key is shown, which is correct.
• False positives: none. 8 ordinary vendor messages (401/404/429/500 with UUIDs, request IDs, timestamps, URLs, IPs) against 6 key shapes (the 41-char synthetic key, 32 hex, 64 hex, base62-40, arch_live_..., UUID): nothing withheld.
• CI complete: all success except Trivy/gosec NEUTRAL; mergeStateStatus CLEAN. Labels match feat(insurance): explicit Archera comparison command using pkg/insurance #2156. Local GOTOOLCHAIN=go1.26.9 golangci-lint run ./... 0 issues; go test -count=1 ./cmd ok (71.1s).

H1 (blocking): archeraHasGappedRun is quadratic, so a hostile body hangs the CLI

For each next key rune the matcher scans every message position and, for each, walks cur from the start until it finds a predecessor within the gap: O(M x |cur|) per step, for every window. The decode path echoes the rejected vendor value in full (insurance/quote.go unsupported value %q; the pkg reads up to maxResponseBytes = 8 << 20), and the check runs on the whole text before the 512-byte cap.

A 200 response whose payment_option is "p"*N/2 + "r"*N/2, end to end through runArchera, gave:

value size	time
4 KB (unit)	2.8 ms
64 KB (unit)	545 ms
256 KB (decode path)	9.9 s
1 MB (decode path)	2m22s

Extrapolated to the pkg's 8 MB limit: about 2.5 hours.

Fix (test first):

1. Add TestArcheraHostileLargeDecodeValueIsBounded: decode path with a 1 MB value shaped as above, require that it finishes within 1 s (or use testing.Benchmark/an elapsed bound with headroom for CI), and assert the error is still capped. Confirm it fails at 0230025.
2. Make the matcher linear: keep a boolean "reachable at position i for run index k" and, for each next rune, mark j reachable if any of j-1-g (g ≤ archeraMaxGap) was reachable. That is O(M x n) per window. Also bound M: only text that can reach the output matters, so check archeraSanitizeN(msg, archeraMaxErrorSize + slack) where slack covers one fully-gapped window (for example archeraFragmentLen*(archeraMaxGap+1) runes, times the max rune width). Keep TestArcheraErrorsWithholdSplitKeyAtAnyCapOffset green.

H2 (blocking): three or more key-alphabet separator runes still hide the key

The gap bound of 2 closes - and --, but the same G1 class reopens at gap 3. HTTP path, 401, archeraLongKey with the separator after every char:

sep "---":  archera API returned HTTP 401: bad key P---R---B-------s---Y---n---T---h---3---t---1---c-------Q...
sep "0x0":  archera API returned HTTP 401: bad key P0x0R0x0B0x0-0x0s0x0Y0x0n0x0T0x0h0x030x0t0x010x0c0x0...
sep "-- -": archera API returned HTTP 401: bad key P-- -R-- -B-- ---- -s-- -Y-- -n-- -T-- -h-- -3-- -t-- ...

These are still readable at a glance. Raising the gap only moves the boundary, and false positives grow with it.

Fix (test first):

1. Add "---", "0x0", "-- -" and a 5-rune key-alphabet separator to the separator table in TestArcheraLongKeyVisibleDisguisesAreWithheld. Confirm they fail at 0230025.
2. Detect a repeated separator instead of bounding its length. For each occurrence of a window's first rune, take the text between it and the next occurrence of the window's second rune (up to some bound, for example 16 runes) as the candidate separator s. Withhold if the window spelled with s between every neighbour occurs in the skeleton. Keep the gap-≤2 subsequence check for irregular single insertions. Both checks are linear per start once H1's bound is in place. Over-withholding stays low because s has to repeat exactly 7 times.

Notes (non-blocking)

• Confusables are not folded: Cyrillic Т inside the key, or dotless ı for i, is shown. NFKD does not map these. This is acceptable residual risk unless you want a confusables table. I'd record it in the PR body.
• Mutants: 35 run, 31 killed. They include every previous-round mutant, threshold 9/10, gap 3/1/0, fold removed, lower removed, NFKD removed, NFD instead of NFKD, fold on one side only, key graphic/space filters, gap off by one, skip first/last/prefix-only window, and first rune skipped. Four survive:
  • Keeping Mn/Me, Hangul fillers or U+FFFD in the message skeleton survives because every test inserts one invisible rune per gap, and gap ≤2 absorbs it. Adding a 3-rune invisible separator (for example "́́́") to the table kills these.
  • gap-ignores-order, which lets a predecessor sit up to 3 runes after the current rune, survives. It only causes over-withholding, so a test that a locally scrambled fragment is shown would pin it. Low priority.

The next gate re-runs these probes, the 1 MB timing and the mutants against the new head.

[cristim]

Addressed H1 and H2 at 1194b60.

• H1: the window match is now a linear DP and only looks at the displayable prefix plus a slack (cap + 8174 bytes). TestArcheraHostileLargeDecodeValueIsBounded (1 MB p/r value, decode path) took 143s before and passes in 0.03s. The whole archera test file runs in about 3s.
• H2: a separator of up to 16 key-alphabet runes repeated between every key character is detected ('---', '0x0', '-- -', '-x0-9' added to the disguise test; failed before). The gap<=2 subsequence check stays for irregular insertions. Confusable letters from other scripts remain a documented residual risk.
• Small mutant check on the new code: separator bound, separator rule removed, gap rule removed, first/last window, M bound removed, cap-only bound, slack too small: all killed.
• golangci-lint ./... 0 issues; full ./cmd tests pass.

Synthetic keys only. Not merged.

[coderabbitai]

🔇 Additional comments (3)

cmd/archera_comparison.go (1)

171-171: LGTM!

Also applies to: 184-273

go.mod (1)

72-72: LGTM!

cmd/archera_comparison_key_test.go-178-192 (1)

178-192: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

The 1-second wall-clock assertion can make CI flaky.

go test -race is used in CI, and the race detector slows code 5–20×. A full runArchera round trip also decodes about 1 MiB of JSON through httptest. On a loaded runner, this test can go over one second even though the matcher is bounded. Use a looser bound such as 10 seconds. You can also test the bounded behavior without timing, by calling archeraKeyReadable directly on the large input.

Proposed fix

-	assert.Less(t, elapsed, time.Second)
+	assert.Less(t, elapsed, 10*time.Second)

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: af7d58a5-c158-46e4-8cd9-407bd5a1e557

📥 Commits

Reviewing files that changed from the base of the PR and between e2035fb and 1194b60.

📒 Files selected for processing (3)

• cmd/archera_comparison.go
• cmd/archera_comparison_key_test.go
• go.mod

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.

[cristim]

Gate verdict at 1194b60: APPROVE. Merging at this SHA with --match-head-commit.

Evidence: git archive 1194b60a into scratch dirs; probe test files next to the archera tests; httptest only, synthetic keys, no live Archera calls. Heavy runs under the heavy-run lock, nice, -p 2.

H1 (hang) fixed. The decode path with a p*N/2 + r*N/2 value took 9.0 ms at 256 KB and 25.5 ms at 1 MB, against 9.9 s and 2m22s at 0230025. The error is still capped at 512 bytes. Hostile 1 MB bodies aimed at both matchers stayed between 12.6 and 14.5 ms with 41- and 64-char keys:

• p/r halves
• p- repeated
• prb- repeated
• a 7-char key prefix repeated
• p + 13 - + r repeated
• a single rune repeated

H2 fixed. HTTP path, separator after every key char: ---, 0x0, -, -- -, -x0-9, and a 16-rune - separator are all withheld.

Still passing: F4 (U+200B-split key across the pkg's 200-byte cut), F5 (U+2800, U+0378, U+E0080), G1 (-, x, 0, --), G2 (lower, upper, fullwidth, fullwidth with -), the 8-vs-7 boundary, short and repeated keys, inner-space and format-rune keys, and the folding edges (ligatures, accents, ß, Ⅻ, İ, combining marks).

False positives: none. 8 ordinary vendor messages against 6 key shapes (41-char synthetic, 32 hex, 64 hex, base62-40, arch_live_..., UUID): nothing withheld.

Checks: CI complete, all success except Trivy/gosec NEUTRAL; mergeStateStatus CLEAN. Labels match #2156. Local GOTOOLCHAIN=go1.26.9 golangci-lint run ./... 0 issues; go test -count=1 ./cmd ok (70.0s).

Residual risk (non-blocking, adversarial-only shapes; follow-ups):

• Irregular separators that alternate between lengths, with one of them over the gap bound, are shown. Examples: -/--- (P-R---B-----s-Y---n...), --/----, 0x0/x0x0, and 3/4/5 dashes. No accidental vendor format produces these. A follow-up could collapse runs of a single repeated key-alphabet rune before the gapped check.
• A separator longer than archeraMaxSep (17 dashes) is shown. The key then spans about 700 bytes, so the 512-byte cap shows only part of it.
• Confusables (Cyrillic Т, dotless ı) are not folded, as documented in the code.
• Mutant survivors from the single sweep at 0230025 (owner policy: follow-ups only):
  • Message skeleton keeping Mn/Me, Hangul fillers or U+FFFD: no test uses a 3-rune invisible separator.
  • gap-ignores-order: over-withholding only.