Adversarial review of merged PRs -- findings tracker [platform part] #348

Split from LeanerCloud/cloud-commitments-cli#1448 (cross-component issue, monorepo split)

An independent adversarial review swept all 8 high-stakes subsystems (407 of 581 merged PRs) against current committed main, reviewing each subsystem holistically (not per-diff). Every finding below was confirmed to persist in current main at review time (old-PR flaws already fixed by later PRs were excluded). This issue tracks the full set; each fix lands as its own follow-up PR with a regression test.

This tracker covered findings across the whole pre-split monorepo. Below is only the subset whose files now live in cloud-commitments-platform (internal/, frontend/, iac/). The pkg//providers/* subset got its own tracker in cloud-commitments-go: LeanerCloud/cloud-commitments-go#119 (filed alongside this issue; will be linked below).

Critical / High

• feat(db): add generated availability_zone column and 9-column recommendations index #808 RI resale priced at ~40% — purchase_history.term is years but handler_marketplace.go treats it as months; supplied schedules rejected. Frontend consent modal (history.ts:1210) mirrors it.
• H1–H4 (exchange daily cap) manual path fails open on unparseable PaymentDue; daily ledger sums quoted not paid; failed ledger-save silently drops spend; over-cap re-quote bounded by per-exchange cap not daily headroom. (handler_ri_exchange.go, pkg/exchange/auto.go) — this repo's part is handler_ri_exchange.go; the pkg/exchange/auto.go half is cloud-commitments-go's.
• Savings#2 revoked/refunded commitments still counted ACTIVE across dashboard/inventory/snapshots/KPIs (no revoked_at IS NULL on read paths).
• Savings#3 by_service.current_savings fabricated as full potential for services with no commitments (competing fix(purchase): claim the exact managed schedule root #908 fixes; tests cement it).
• Purchase F1 approval bypass — sweep/SQS execute pending/notified with no approval + no AutoPurchase gate.
• Purchase F2/F3 dead auto-create (not-found≠error); pre-fire delay/revoke window fails open on config-read error.
• Migration#1 dirty auto-heal marks a rolled-back (never-applied) migration as applied → silent schema divergence (the 000074 class). Superseded — see A04-018 below, which found this premise no longer holds against current main.

Medium

• feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277 cancel writers still emit legacy 'cancelled'; canceled_by written by no code; RI-exchange writers legacy — completes the expand-contract before fix(exchange): show unknown-outcome auto-exchanges as needing reconciliation in the notification email #1278. (This repo's part: internal/config/store_postgres.go, internal/purchase/approvals.go, internal/api/handler_purchases.go legacy "cancelled"/canceled_by handling. The pkg/ladder/store.go writer is cloud-commitments-go's scope.)
• RBAC F1 /api/history exempts empty-AccountID rows → leaks other users' in-flight purchases + emails (PII).
• IaC Fargate task role can spoof SES from any verified identity (missing ses:FromAddress condition the Lambda role has).
• Savings#4 provider chip filters recs but not commitment KPIs (mixed-provider coverage ratio).
• Migration#2 TestMigrations_FullStackIdempotent is a false guard (2nd run is a version-table no-op).
• Purchase F4 reaper stamps "safe to retry" on non-idempotent Azure SP rows → duplicate SP; pre-empts idempotent re-drive.

Medium/Low — to be scheduled

• Savings#5 coverage denominator sums ~6 per-cell variants (no dedup) → coverage % understated
• Savings#6 home per-service range chart mixes alternatives across cells
• Migration#3 000032...down.sql fails on realistic term/payment-variant data (chained-rollback dead end)
• Purchase F6 non-atomic CAS+save can strand a scheduled purchase with NULL scheduled_execution_at
• Savings#7–feat(purchase): Archera referral attribution on purchase execution #10, auth low (JWKS http.DefaultClient not hardened; recovery-code modulo bias), RBAC F2 (API-key constraints not enforced — being fixed).

Notable clean subareas (traced, no defects) — this repo's part

auth/OIDC (RS256-pinned inbound + subject pinning, constant-time everywhere, HKDF CSRF, fail-closed MFA/login, race-free last-admin trigger, MaskToken); approval tokens + atomic claim + submit idempotency + ramp-step + creator-scope + delayed-fire; snapshot SUM-then-AVG rollups; active-only SQL reads; account-scope on listing endpoints; IaC IAM bootstrap-vs-runtime split + PassRole/ExternalId/OIDC scoping (except the Fargate SES gap).

From the 2026-09-02 codebase audit — this repo's part

A05-003 (medium)

Detail and a narrowing for the Purchase F1 line, from finding A05-003. The mechanism is that enforceFourEyesPolicy (internal/purchase/approvals.go:167) documents itself as the universal choke point but runs only inside ApproveAndExecute. claimAndExecute (manager.go:177-193) calls executeAndFinalize directly, so both of its callers, the SQS execute_purchase worker (messages.go:136) and the cron sweep processOneExecution (manager.go:678), commit money without reaching the gate. The four-eyes tests cover ApproveAndExecute and ProcessMessage's approve branch only. Narrowing worth recording: the human self-approval scenario is not reachable today, because executableByScheduler rejects PurchaseSourceWeb (manager.go:635) and the only two writers of CreatedByUserID both stamp Source=cudly-web. What is reachable is a system-created row (nil creator, empty Source, AutoPurchase plan) executing on the cron tick where checkDifferentApprover would have denied it fail-closed. So the fix is to decide whether dual control binds AutoPurchase and say so at the gate, moving the check into executeAndFinalize alongside armedRedriveRefusal if it does.

A04-018 (low)

The Migration#1 entry in this tracker ("dirty auto-heal marks a rolled-back migration as applied") is premised on a Force that no longer happens, so it needs re-deriving before anyone works it. maybeAutoHealDirty (internal/database/postgres/migrations/migrate.go:381-421) calls no Force at all; it returns an error on every dirty row. The names and comments were not updated with it: the gate is still CUDLY_MIGRATION_AUTOHEAL, autoHealEnabled still defaults it to true (:428), and the call-site comment at :114-118 still promises the dirty flag is cleared "at the CURRENT recorded version so the subsequent Up() re-applies any pending migrations, letting a cold start self-recover". So the live risk has inverted: rather than a wrong auto-heal, an operator who leaves the gate at its true default expects self-recovery and gets a permanently dirty database until they discover CUDLY_FORCE_MIGRATION_VERSION. Renaming to maybeRefuseDirty / CUDLY_MIGRATION_DIRTY_CHECK and rewriting the comment would make the tracker item resolvable as no-longer-applicable. Audit finding A04-018.

Acceptance criteria

• Each unchecked item above lands as its own follow-up PR in this repo with a regression test that reproduces the real failure mode (not a narrower unit that stays green with the bug present).
• Cross-repo items (H1-H4, feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277) are coordinated with the matching cloud-commitments-go issue so both halves land together where the fix requires it (e.g. H1-H4's ceiling fix needs pkg/exchange/auto.go and internal/api/handler_ri_exchange.go in sync).
• Migration#1 is re-derived per A04-018 (rename/re-document maybeAutoHealDirty/CUDLY_MIGRATION_AUTOHEAL) rather than fixed against its original, now-stale premise.