Repository navigation
Conversation
cristim
added
urgency/this-sprint
|
Warning Review limit reached
This review includes 11 billable files and costs up to $2.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 21 minutes for your next included review. View limit details
Comment |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
cristim
left a comment
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. |
This branch has not been deployed
1 participant
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