Repository navigation
fix(purchase): refuse unverified ramp sizing and adoption - #905
Conversation
|
@coderabbitai review |
|
|
@coderabbitai review |
|
cristim
left a comment
There was a problem hiding this comment.
Independent review of c3c1d17: changes requested
Scope: git diff 17a337704f38...c3c1d171e58d (base tip is the head of #904; stack consistent, #905 is +372, 3 production files, +46/-19 production lines).
Findings
-
ACTIONABLE (owner-decision, PR body must say it): every existing multistep ramp stops permanently, with no recovery path.
internal/config/store_postgres_ramp_baseline.go:76returnsErrRampLedgerUntrustedfor anylegacymultistep plan, and migration 000124 makeslegacypermanent (a plan is never promoted back). All plans created before the migration, plans created by an old Lambda during the drain window, and any plan whose shape is edited after its first freeze (the trigger degradestrustedtolegacy, andrampBaselineStalealso clears the baseline,store_postgres.go:761) land there. Failing scenario: user attaches step 1 of a new ramp, edits the service term before approving; the plan is nowlegacy, and step 2 is refused forever. Nothing in the code can reset it (grep: no other writer oframp_ledger_state). The error text says "reconcile prior commitments before attaching another step", but no reconcile action exists; the only way out is delete and recreate the plan, which detaches the ledger rows (plan_idSET NULL) and the new plan starts with an empty ledger. The PR body's "Deploy note" only covers the drain window, not that in-flight ramps halt. Fix: state the halt and the recovery procedure in the PR body and error text, or add an explicit admin re-trust path as a follow-up issue before this merges to production. -
ACTIONABLE: the nonempty-root refusal is not recorded and not tick-quiet.
plan_step_attach.go:93-95returns the rawErrRampLedgerUntrusted(not wrapped inerrExecutionNotNotifiable), sosendPlanNotification(notifications.go:~170) logsFailed to create executionat Error on every notification tick, and the pending/notified row stays untouched with no durable reason on it (the test asserts the row stayspending/notified). The new/bare path does record a failed row with the reason (failStep), so the two paths differ. Also the check runs before the date-passed check, so an overdue untrusted root never reaches the quiet skip. Fix: wrap witherrExecutionNotNotifiable(stillerrors.Is-able toErrRampLedgerUntrusted) and consider stamping the reason on the row, or accept and document the log-only surfacing. -
NIT:
plan_step_attach.go:210-212(!wrotebranch) is unreachable afterGetPlanRampBaselineTxpasses under the same plan lock (its predicate equals the UPDATE's WHERE). Mutation probe: replacing it with_ = wroteleaves all config and purchase retention tests green. Harmless fail-closed defence; keep or drop.
What I attacked
- Build and vet clean. Integration (real Postgres 16 via testcontainers):
go test -tags integration -run 'TestCumulativeAttach|TestRampStep|Cumulative' ./internal/purchase/ok;-run 'RampRetention|Retention|RampBaseline|TargetMet' ./internal/config/ok. - Fail-before/pass-after (mutation on production code in a scratch worktree, reverted): disabling the
GetPlanRampBaselineTxtrust check fails all 6 subcases ofTestCumulativeAttach_UntrustedHistoryCannotSizeOrNotifyand both subcases ofTestCumulativeAttach_AuthorizedLegacyQuantityRemainsImmutable. Removing the nonempty-root guard atplan_step_attach.go:93fails exactly the 2 nonempty pending/notified subcases. Surviving:!wrotebranch (finding 3). - TOCTOU: the trust read (
GetPlanRampBaselineTx) and the freeze (SetPlanRampBaselineTx) both run in the claim transaction afterLockPurchasePlanTx ... FOR UPDATE(store_postgres.go:~715), and every plan writer, including old Lambdas, needs that row lock, so state, baseline and current_step cannot change between check and attach. Authorized scheduled/approved executions are intentionally not re-checked at purchase time (they consume their frozen recommendations; the immutable7 tests cover it). No race found. - Double/skipped purchase: a refused step never writes recommendations, suppressions, tokens or emails (asserted: zero sends, zero suppressions, zero resolver calls). The refused bare step records a failed row with
NoSubmission, so no later tick creates a second root. Single-step and immediate plans skip the check (total > 1). - Surfacing: bare/new step is durable (failed row with reason). Nonempty root is log-only (finding 2).
Gate status
- Independent review: this comment, at c3c1d17.
- Findings resolved: no, 1 and 2 open.
- CI on SHA: not applicable until retarget (base is a branch, not main).
gh pr checksshows only Trivy, gosec and a rate-limited CodeRabbit run, which is not CI coverage. - CodeRabbit: rate limited, no findings.
- Labels: match #631 (effort/xl, impact/many, priority/p1, severity/high, triaged, type/feat, urgency/this-sprint).
- Local real-path evidence: PR body claims local integration runs; I reproduced them above (macOS, Postgres 16 testcontainer, not the 17 the plan mentions). Real-account verification: none, and none claimed.
- Verdict recorded: this comment. A new commit restarts the gate.
Mint fresh ramp lineage at creation and freeze it atomically. Preserve authorized quantities while refusing new ramp sizing or notification adoption from unverifiable histories.
Delta of 686b9f02 over #907: the planstep and config code is identical on main. With the retention runtime (#905) a completed protected ramp has an untrusted baseline, so the exact-count test must assert the refusal instead of reading the baseline; without this TestEqualCountAttachThree fails on main plus #905. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Retargeted to main (its base #904 merged as 94a058c) and rebased. The PR's own commit is a pure rebase: Added c9c92ed (cherry-pick of the claude-cc-631 test-only commit 43354cb3). Without it, main's TestEqualCountAttachThree (from #907) fails once this PR is applied, because a completed protected ramp now has an untrusted baseline. Local proof with Gate status: the head moved, so the gate restarts. My review findings on c3c1d17 are still open: (1) legacy ramps halt permanently with no recovery path, which needs a fix or an explicit owner decision; (2) the untrusted refusal logs Error every tick and records no durable reason. Note that c9c92ed asserts the behaviour in finding 1. |
c3c1d17 to
c9c92ed
Compare
|
Automated review of
Posted by a read-only review sweep. It stays silent on this commit and re-reviews only after a new push. |
…essions When the retention ledger refuses to size a step whose root execution row already carries recommendations, fail the row like the other refusals (Error and NoSubmission recorded, later ticks skip quietly) instead of returning the raw sentinel, which logged an error every tick. The pinned failure now expects the row's real recommendation state, and a won failure releases the row's suppressions in the same transaction as the stamp, since the step provably bought nothing. The CAS still commits before the stamp; that pre-existing gap is recorded in known_issues/36 and #1052. Refs #631 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fix for review finding 2 pushed as 00eeaeb (fast-forward from c9c92ed). Finding 1 (the permanent What changed
Review Local verification (macOS, testcontainers Postgres 16, the repo's test image)
Refs #631 |
cristim
left a comment
There was a problem hiding this comment.
Independent delta review of 00eeaeb (previous: c9c92ed): changes requested (finding 1 only, owner decision pending; no new defects in the delta)
Scope: git diff c9c92ed8..00eeaeb8 (4 files, +79/-6). OLD is an ancestor of NEW (fast-forward). git merge-tree origin/main HEAD shows no conflicts.
Previous findings
- Permanent
legacyledger state, no recovery path: NOT RESOLVED (owner decision). Nothing in the delta touches it; the author states it awaits the owner. It stays blocking until the owner decides or the recovery path / PR-body disclosure lands. - Untrusted-root refusal logged Error every tick with no durable reason: RESOLVED.
plan_step_attach.go:99-103routesErrRampLedgerUntrustedfor a nonempty root throughfailStep(->claim.emptyRoot,notifications.go:248-251->failExistingPlanStep), which stampsfailed/NoSubmission/reason.adoptableRootRowonly adopts pending/notified, so the next tick is a quiet skip. Proof: the extendedTestCumulativeAttach_UntrustedHistoryCannotSizeOrNotifyfails on OLD production code with NEW tests (both nonempty subcases: "Target error should be in err chain") and passes on NEW. !wroteunreachable branch (NIT): unchanged, still non-blocking.
Delta attack (money path)
- Cannot fail a step that should have run: the failed row is only reached after
GetPlanRampBaselineTxreturnedErrRampLedgerUntrustedunder the plan row lock, the same condition that already refused to notify it. A refused step never reached an approval token/recommendation write, so failing it forgoes no purchase. The CAS (store_postgres.go:1229-1230) isexecution_id = $1 AND status IN (pending, notified), so an approved/scheduled/in-flight row is untouched (lost race returnserrExecutionNotNotifiable). - Pin change (
notifications.go:301): the CAS is still keyed on oneexecution_id, so it cannot fail another root's step. The recommendations pin now equals what the caller read (len(exec.Recommendations) == 0); the other callers (failDueManualStep, bare path) pass empty roots, so they still pin empty. A row whose recommendations changed since the read fails the CAS and is skipped. - Suppression release (
notifications.go:310-318): runs only after a won CAS, in one tx with the stamp, keyed by execution id (store_postgres_suppressions.go:139). The newTestFailExistingPlanStep_LostRaceKeepsSuppressionsshows an approved row keeps its suppressions; the retention test shows another execution's suppression is untouched. It passes on OLD as well, so it guards the new release rather than proving the fix. - Residual: CAS commits before the stamp/release tx, so a crash between leaves a failed row without a reason and with suppressions until expiry. It fails closed (no second root, nothing bought). Recorded in
known_issues/36_plan_step_fail_cas_not_atomic.mdand #1052. NON-BLOCKING. - Order change: the untrusted check now precedes the date-passed skip for nonempty roots; an overdue untrusted root is failed durably instead of skipped. Intended.
Tests run (macOS, testcontainers Postgres, Docker available)
- NEW:
go build ./...,go vet ./internal/purchase/,go test -tags integration -p 1 -run 'TestCumulativeAttach|TestFailExistingPlanStep|TestEqualCountAttach|TestRampStep' ./internal/purchase/ok;go test ./internal/purchase/ok. - OLD production + NEW tests: the two nonempty subtests fail as above. Worktree restored afterwards.
- I did not re-run the author's 7 mutation probes.
Gate status
- Independent review: this comment at 00eeaeb; delta clean, finding 1 open (owner decision).
- CI on 00eeaeb: run 39037632588 (CI - Build & Test) in progress at review time; pre-commit pass; no
action_requiredruns seen. Not waited for. - CodeRabbit: in progress / rate limited, no findings posted on this SHA.
- Labels: match #631 set (effort/xl, impact/many, priority/p1, severity/high, triaged, type/feat, urgency/this-sprint).
- Local evidence: macOS integration reproduced as above; no real-account verification claimed.
Migration 000127 marks a protected multi-step ramp 'new' again when its retention ledger is 'legacy' but it is still at step 0, has no frozen baseline and has no surviving purchase execution rows. Such plans then freeze a baseline at their first step like any new plan. The ledger guard trigger is disabled only around that update, inside the migration transaction. Mid-ramp legacy plans are left for a follow-up change. The untrusted-ledger refusal now says what is wrong instead of naming a reconcile action that does not exist. Refs #631 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge driver note on 8c0123d: it adds migration |
cristim
left a comment
There was a problem hiding this comment.
Independent delta review of 8c0123d (previous: 00eeaeb): no actionable findings (residual below needs an explicit owner acceptance)
Scope: git diff 00eeaeb8..8c0123d1 (6 files: migration 000127 up/down, error text, new migration integration test, two test tweaks). OLD is an ancestor of NEW. git merge-tree origin/main HEAD is conflict-free. The branch lacks main's 000126 but 000127 > 000126, so golang-migrate applies in order.
Previous finding 1 (permanent legacy): PARTIALLY RESOLVED
- RESOLVED for pre-existing never-started ramps: 000127 up promotes them to
new, and the new test shows step 1 then sizes and freezes a baseline. - NOT addressed, by design: a ramp that started, or has any execution row, stays
legacyfor good. This includes the original failing scenario (attach step 1, edit the term before approving: an execution row exists, the trigger degrades the plan, step 2 is refused). There is still no re-trust path; only delete and recreate. Per the owner decision (no backward compatibility) I treat this as accepted, NON-BLOCKING, but it needs the owner's explicit acceptance recorded on the PR. - NON-BLOCKING: the new
ErrRampLedgerUntrustedtext (store_postgres_ramp_baseline.go:35) says the history is "from before the retention ledger". That is inaccurate for plans degraded after the ledger exists by a post-attach shape edit. Suggest neutral wording.
Attack on migration 000127
- Rows touched: only
legacyANDramp_ledger_protected(total_steps > 1, current_step < total) ANDcurrent_step = 0ANDramp_baseline IS NULLAND nopurchase_executionsrow of any status for that plan_id. A plan with any executed, approved, in-flight, failed or canceled step has a row and is skipped. Tenant crossing: the predicate is per plan row, with a correlated NOT EXISTS on the sameplan_id; no join across tenants. The test covers: has-execution, mid-ramp, single-step, frozen-baseline and trusted plans all unchanged. Residual (documented in the SQL, accepted): pre-000124 retention cleanup may have deleted completed rows of a plan whose step never advanced, so "no rows" is not proof of "never bought". A concurrent old-Lambda execution insert during the UPDATE is not blocked (FK takes ROW SHARE only), but such a plan simply has a visible execution row that the new ledger counts. - DOWN is a deliberate no-op (
SELECT 1). It is not an inverse, but it loses nothing: the promoted plans are indistinguishable from freshly creatednewplans, and demoting them would re-block sizing. Safe after new rows were written. 000124's own down drops the column anyway. - Idempotent: promoted rows no longer match
legacy, so a re-run touches 0 rows. Empty table: no-op. Locking:DISABLE/ENABLE TRIGGERtakes SHARE ROW EXCLUSIVE onpurchase_plans(blocks plan writes, not reads) for the migration;purchase_plansis small and the NOT EXISTS probe usesidx_purchase_executions_plan_id(000002). Only theguard_ramp_ledger_statetrigger is disabled;retain_ramp_ledger_executionstays live. The test assertstgenabled = 'O'afterwards. The migration runner applies the file as one statement batch, so a failure rolls back both toggles; I did not independently confirm the golang-migrate transaction semantics beyond the test passing. - Drain-window caveat: an old Lambda that inserts a plan after 000127 gets
legacyagain (column default, INSERT guard). Same deploy-note class as before; not a defect in this migration. - Fail-before/pass-after (testcontainers Postgres):
TestMigration_NeverStartedLegacyRampsBecomeNewpasses on NEW; with the up file replaced bySELECT 1;it fails ("expected new, actual legacy"). Worktree restored. Also ranTestCumulativeAttach|TestFailExisting|TestEqualCount|TestRampStepon NEW: ok.go build ./...ok.
Gate status
- Independent review: this comment at 8c0123d; the delta is clean; finding 1 residual awaits the owner's acceptance.
- Migration numbering: #1020 (owner hold) also adds a 000127 migration. Gate item, not a code defect: whichever lands second must renumber (coordinator proposes #905 keeps 000127, #1020 -> 000128, #1022 -> 000129).
- CI on 8c0123d: run 39042104138 (CI - Build & Test) in progress, pre-commit success; no
action_requiredruns seen. Not waited for. CodeRabbit: nothing posted on this SHA. - Labels: unchanged, match #631. Local evidence: macOS testcontainers integration, no real-account verification claimed.
|
Parking this PR. On 2026-10-17 the owner cut #631 to a UI-first ship on the existing save path. Under that cut, #905 is finished only if its review findings are small. Finding 1 (a permanent State at head 8c0123d (CI green)
Merge-order note for whoever resumes: the deferred #631 M3 branch must renumber its receipts migration above this PR's migrations. Refs #631 |
|
Gate record for 8c0123d (origin/main CLAUDE.md "Review gate"):
Merging at 8c0123d with --match-head-commit. |
…loud#905 LeanerCloud#905 merged its own 000127 (never_started_legacy_ramps_to_new), so this PR's migration moves to 000128. The down step in its test now targets the real predecessor instead of a hard-coded 126. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#905 merged 000127 and LeanerCloud#1020 moves to 000128, so this PR's migration becomes 000129 (and still merges after LeanerCloud#1020). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1020) * fix(config): let an account with only terminal executions be deleted Migration 000053 made purchase_executions.cloud_account_id ON DELETE RESTRICT, so one completed/failed/canceled/expired execution blocked the account delete forever, and the handler answered with a 409 asking the operator to cancel pending purchases that did not exist. DeleteCloudAccount now detaches the account's terminal executions (cloud_account_id = NULL, status in the new TerminalExecutionStatuses allowlist) in its transaction before the DELETE, keeping them as history. Live rows stay attached, so the FK still refuses the delete. The deleteAccount preflight adds CountLiveExecutionsForAccount and answers a 409 with reason "active_executions" for approved, running, scheduled or paused rows, after the existing pending/notified 409. Migration 000127 gives purchase_executions its own updated_at trigger that keeps updated_at on a detach-only UPDATE, so deleting an account does not pull old failures back into the plan health window or restart the retention clock of canceled rows. Closes #265 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(db): renumber executions-detach migration to 000128 after #905 #905 merged its own 000127 (never_started_legacy_ramps_to_new), so this PR's migration moves to 000128. The down step in its test now targets the real predecessor instead of a hard-coded 126. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(api): re-run the delete preflight after an FK-violation race When DeleteCloudAccount hits SQLSTATE 23503 because a row raced past the preflight, re-query so the client gets the structured pending (with counts) or active 409 for that row. Fall back to the bare pending 409 if the re-query fails or no longer sees a blocker (CodeRabbit on #1020). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…n state (#1022) * fix(scheduler): record partial multi-subscription sweeps in collection state tolerateIncompleteSweep kept the rows of a partial sweep but recorded the incompleteness only in a WARN log, so the freshness API (and therefore the dashboard) rendered 1-of-10 subscription coverage exactly like full coverage. The helper now returns a sanitized *sweepGap (nil = complete) instead of a bool. It is threaded through the per-account collectors, fanOutPerAccount, the three provider collectors and collectAllProviders into a new recommendations_state.last_collection_partial JSONB column (migration 000128), served as last_collection_partial on GET /api/recommendations/freshness. The payload carries per-provider counts (incomplete accounts, attempted/failed scopes, failed details) and a closed set of reason categories; raw provider errors and subscription/project IDs stay in the log, because the state row is global while accounts are access-scoped. UpsertRecommendations and ReplaceRecommendations reset the column, so the next complete collection clears the marker. Hard failures are unchanged: they still fail the provider and populate last_collection_error. Tests now drive the partial/complete/hard-error cases for AWS, Azure and GCP (ambient) plus AWS/GCP registered accounts, closing audit finding A16-007's per-provider coverage gap. Closes #87 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(purchase): migrate to 000128 before the Azure pricing recollection upsert UpsertRecommendations now also resets recommendations_state.last_collection_partial (migration 000128, #87), so the upsert failed on the v111 schema this test pinned before re-collecting. Bump the pin to 128, following the test's existing precedent for 000111. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(db): renumber recommendations-state-partial migration to 000129 #905 merged 000127 and #1020 moves to 000128, so this PR's migration becomes 000129 (and still merges after #1020). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
What and why
Runtime half of ramp-ledger retention. Ramps now get fresh lineage at creation, frozen atomically. Authorized quantities are preserved, but new ramp sizing or notification adoption is refused when the history cannot be verified. This stops the system from acting on a ledger that retention could not protect.
Stack position
2 of 2. Base:
fix/631-retention-ledger-db(the migration PR). Retarget tomainafter that PR merges.Verification (local, macOS)
go build ./...andgo test -race ./internal/config/... ./internal/purchase/...pass.plan_step_retention_integration_test.go,store_postgres_ramp_retention_integration_test.go) run against a real local Postgres with migrations applied.Deploy note
Retention needs an old-Lambda drain window: roll out the migration first and let Lambdas from before this change drain before relying on the refusal behaviour.
Refs #631
🤖 Generated with Claude Code
Summary by CodeRabbit