Adversarial review of all high-stakes merged PRs — findings tracker #1448

Adversarial review of all high-stakes merged PRs — findings tracker

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.

🔴 Critical / High — fix-forward PRs in progress

• C1 EC2Instance SP purchased for the wrong instance family/region — CE parser discards SavingsPlansDetails; lookupOfferingID returns the lexicographically-smallest offering across families; uses client region not the rec's. (parser_sp.go, savingsplans/client.go)
• feat(marketplace): sell/cancel Standard RIs on AWS Marketplace (closes #292) #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)
• 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 Merge per-service savings charts into one range chart with current-savings underlay; fix unpopulated current_savings #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).
• Provider H1/H2 Azure Search returns VM recs mislabeled as Search (missing resourceType); Synapse filters non-existent 'SQLDatabaseDTU' (+ database 'SqlDatabase', cosmosdb 'CosmosDb' casing) — being fixed against SDK enums.

🟡 Medium — fix-forward in progress

• fix(db): rename status value cancelled->canceled (expand-contract, US spelling) #1277 cancel writers still emit legacy 'cancelled'; canceled_by written by no code; RI-exchange writers legacy — completes the expand-contract before fix(db): contract migration -- drop 'cancelled' from CHECK constraints and drop cancelled_by column cloud-commitments-platform#47.
• 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.
• Provider M1/M2 Azure covered-cost derivation discarded by ExpandPaymentVariants (fabricates &0.0); AWS SP parser coerces unparseable money to 0 (RI path fails loud).

🟠 Medium/Low — to be scheduled (not yet in a fix PR)

• Exchange M2 exchange target offering ignores payment option + nondeterministic (ec2/client.go FindConvertibleOffering)
• Exchange M3 EC2 RI term silently defaults to 1yr on unknown term (ec2/client.go getDurationValue)
• Exchange M4 findBestFit can recommend MORE capacity than owned (reshape increases spend)
• Exchange M6 GetOfferingDetails reads upfront from marketplace PricingDetails → $0 for standard offerings
• Savings#5 coverage denominator sums ~6 per-cell variants (no dedup) → coverage % understated
• Savings#6 home per-service range chart mixes alternatives across cells
• Provider M3 GCP compute OnDemand/Commitment are full-term totals in a monthly field (12×/36× inflated)
• Provider M4 GCP page cap counts skipped non-ACTIVE recs → region yields zero
• 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
• Exchange L1–L5, Savings#7–bug(ui): Navigation bar overflows and clips on mobile and tablet viewports #10, provider LOW batch (MemoryDB unreachable/name, hardcoded Engine:"redis", SavingsPlanDetails.Coverage mislabel, Azure retail-pricing silent truncate, region-name aliases, memorydb pagination no ctx cap), auth low (JWKS http.DefaultClient not hardened; recovery-code modulo bias), RBAC F2 (API-key constraints not enforced — being fixed).

🔎 Needs LIVE cloud verification (cannot confirm without real credentials)

• H5 (exchange) RI utilization keyed by CE SUBSCRIPTION_ID group key but matched against EC2 ReservedInstancesId — if mismatched, the entire reshape/auto-exchange pipeline silently no-ops ("all RIs well-utilized"). No utilization_test.go / fixture. Verify one real GetReservationUtilization key shape; extract RI id from reservationARN if needed.
• Provider H3 (GCP) recommender IDs likely wrong: compute uses google.billing.CostInsight.commitmentRecommender (an insight type, not a recommender; commit 1778a2f7b changed AWAY from the documented google.compute.commitment.UsageCommitmentRecommender); cloudsql/memorystore use Performance recommenders (recommend cost increases, mislabeled as CUD savings). If invalid, GCP CUD recs silently show "none". Verify each via gcloud recommender recommendations list --recommender=....
• Provider H2 residuals confirm Azure SqlDataWarehouse/SQLDatabases/CosmosDB filter values against a live subscription (SDK-enum check done in the fix PR; live behavior still worth confirming).

✅ Notable clean subareas (traced, no defects)

pkg/ladder (allocation/tranche/guardrails); 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; no double-amortization; snapshot SUM-then-AVG rollups; active-only SQL reads; account-scope on listing endpoints; AWS pagination/tenancy/RDS-enum; Azure httpclient.New() on all clients + fabricated-price refusal; IaC IAM bootstrap-vs-runtime split + PassRole/ExternalId/OIDC scoping (except the Fargate SES gap).

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A09-004 (critical)

H1-H4 has a third site the bullet does not name: pkg/exchange/exchange.go:303-308. resolvePaymentDue substitutes a fresh zero big.Rat whenever PaymentDueUSD is nil, which happens whenever the AWS response's PaymentDue is empty (the parse at :260 is gated on a non-empty raw string). checkInitialQuote (:313) and checkReQuote (:330) then compare that zero against MaxPaymentDueUSD and always pass, so an exchange AWS declined to price reaches AcceptReservedInstancesExchangeQuote (:372) with no effective ceiling. The doc comment asserts the only cause of a nil is a genuinely zero-cost exchange; nothing verifies that. Finding A09-004.

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.

A07-022 (medium)

Exchange M2 has a sibling on the ListTargetOfferings path. normalizeTargetOfferingsParams (providers/aws/services/ec2/client.go:874-878) calls convertEC2PaymentOption and discards the error with if ... err == nil, so an unrecognised payment option (for example the display form "All Upfront" rather than "all-upfront") leaves offeringType at its zero value. The SDK omits an empty offeringType, so AWS returns offerings for every payment option and ListTargetOfferings presents partial-upfront and all-upfront exchange targets for a request that asked for one specific option, with no signal to the caller that its input was rejected. The path is live from internal/api/handler_ri_exchange.go:154. Audit finding A07-022.

A07-023 (medium)

Exchange M2 undersells this one: alongside the payment option and the first-result nondeterminism, FindConvertibleOffering (providers/aws/services/ec2/client.go:794-818) does not paginate at all. It issues a single DescribeReservedInstancesOfferings with a six-entry Filters[] request, MaxResults: 20, no OfferingType, and returns ReservedInstancesOfferings[0]. The comment on findOfferingID at :481-487 documents that this exact Filters[] shape is the one AWS answers with empty pages plus a NextToken on sparse offering sets, so a first empty page yields 'no convertible offering found' for an instance type that does have one. Live callers are internal/server/ladder_write.go:64 and internal/server/handler_ri_exchange.go:63. Whatever fixes the payment-option match should also build the request from typed fields the way describeInputFromQuery does and page with the existing isLastEC2Page helper. Audit finding A07-023.

A09-010 (medium)

Exchange M4 has a neighbour in the same loop. sizeOrder ends with "metal" (pkg/exchange/reshape.go:382-387) and normalizationFactors["metal"] == normalizationFactors["24xlarge"] == 192 (:378-379), and the loop at :694-699 keeps the LAST index whose factor fits, so any normalizedUsed >= 192 selects index 17, "metal". analyzeRI then composes family + "." + targetSize at :486 with no per-family existence check, producing targets such as m5.metal, t3.9xlarge and r5.3xlarge. resolveOffering in auto.go fails the lookup and the recommendation is silently skipped, while the reshape dashboard shows a target no operator can act on. Dropping "metal" from sizeOrder (keeping it in normalizationFactors for parsing existing RIs) and preferring the smaller-indexed size on a factor tie fixes both. Audit finding A09-010.

A09-011 (medium)

A second route into the M4 outcome, upstream of findBestFit. normalizationFactors["metal"] = 192 (pkg/exchange/reshape.go:379) is one family-independent entry for a quantity that is family-dependent: m5.metal is 24xlarge-equivalent (192), but i3.metal is 16xlarge-equivalent (128) and m5zn.metal is 12xlarge-equivalent (96). resolveNormFactor (:432-437) falls back to that table whenever ri.NormalizationFactor == 0, feeding normalizedPurchased at :461 and findBestFit at :481 with no family discrimination, so an i3.metal at 50% utilization computes normalizedUsed 96 instead of 64 and the target is sized 50% too large: more capacity than owned, which is M4's stated failure. Per the fail-loud rule the entry should come out so resolveNormFactor returns 0 and analyzeRI skips the RI unless the caller supplied the real per-family factor from ec2:DescribeInstanceTypes. Audit finding A09-011.

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.