[cristim]

Refs #631. Supersedes #1082, preserving its original branch and review history.

Characterize persisted ramp-step completion and notification clocks across 16 manual/automatic, purchased/target-met and existing/absent-root cases, including a second notification tick. Whole-row comparisons now preserve historical timestamps across the first completion and notification transitions. Production code is unchanged. The baseline fixture also checks the transaction error before its success flag.

Verified against this source locally on macOS with Docker PostgreSQL 16 and migration 130:

• All 16 cases passed with race detection and zero skips.
• An isolated timestamp-only mutation failed at the intended whole-row assertion; removing the added assertions made that same probe pass.
• go build ./..., integration-tagged purchase-package lint with golangci-lint 2.10.1 and all 12 applicable normal commit hooks passed.
• Two independent reviewers completed two adversarial passes and approved exact head b790891cbb22b4db9db5037085efc1ece527147d after checking its committed tree and local proof.

Provider completion is simulated and email delivery uses a counted fake sender. This does not prove live vendor behavior or exact pipeline clock equality. The veto/gate, failure, post-send and remaining UI/API slices stay open in #631. All 21 Build & Test jobs and pre-commit passed. CodeRabbit completed its exact-head review with no actionable comments; its test-docstring warning is justified in the verification comment. Platform merge is held pending the existing development deployment decision.

Summary by CodeRabbit

• Tests
  • Added integration coverage for purchase-plan completion and upcoming notifications across automatic and manual plans, with target-met and target-unmet scenarios.
  • Checks verify completion state and persisted clock behavior, notification eligibility and side effects, and that repeating completion or notification calls does not trigger additional writes or sends.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: test/631-completion-history-audit
• File: .coderabbit.yaml
• Commit: d25194b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 3135e247-d1c6-4a9e-9040-f9932f0c75d5

Commits

Reviewing files that changed from the base of the PR and between b790891 and d25194b.

Files selected for processing: 1

• internal/purchase/plan_step_clock_characterization_integration_test.go

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 2 reviews per hour.

Walkthrough

The pull request adds an integration test for purchase-plan completion and upcoming notifications. The test checks persisted plan state and notification behavior across plan types, completion outcomes, and step-three date cases.

Changes: Purchase plan completion and notifications

Layer / File(s)	Summary
Completion fixture and state checks internal/purchase/plan_step_clock_characterization_integration_test.go	The fixture snapshots plan and database state and simulates prior-step completions. The test checks the plan clock, existing step-three data, and duplicate-completion behavior.
Notification scenario checks internal/purchase/plan_step_clock_characterization_integration_test.go	The test varies plan type, step-two outcome, and step-three date. It checks notification timing, step-three creation or retention, suppression and cooldown state, and repeat-run behavior.

Priority: Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Other

Merge Risk: Minimal · up to d2519

This PR adds test coverage for purchase-step completion and notification timing and does not change runtime behavior. No merge-blocking risk was found in the supplied context.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…	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 describes the added integration test coverage for completion clocks and preservation of historical row data.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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]

archera_reviewer_a

APPROVED b790891 by archera_reviewer_a

Two adversarial source passes and retained fix reviews cover the full main-based change. Verified the exact committed tree against the reviewed index and proven source, all 16 native cases passing without skips, the intended timestamp-only failure and passing assertion-deletion probe, pinned lint/build results and all 12 applicable normal hooks. Historical original-head evidence remains distinguished from the current proof. Production code is unchanged.

Local approval only. Public CI, CodeRabbit review and the existing merge/deployment decision remain separate gates.

[cristim]

archera_reviewer_b

APPROVED b790891 by archera_reviewer_b

Two adversarial source passes and retained fix reviews cover the full main-based change. Independently matched the committed tree, file modes and source to the approved staged artifacts and audited the 16-case native run, exact timestamp assertion failure, passing deletion probe, lint/build and normal-hook evidence. No remaining actionable local finding. Process cleanup assessment relies on the inspected guard and terminal execution receipts; no independent process-list reproduction was available in this review.

Local approval only. Public CI, CodeRabbit review and the existing merge/deployment decision remain separate gates.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

resume-cc-audit

Verification at b790891cbb22b4db9db5037085efc1ece527147d:

• All 21 jobs in Build & Test, including integration tests, passed. Pre-commit passed as well.
• CodeRabbit's completed full review covers the base-to-head change and reports no actionable comments. Both independent exact-head approvals and local proof remain recorded above.
• The docstring coverage warning concerns integration-test helpers and the test function. Their names and assertions describe their behavior; additional docstrings are not warranted under the repository's comment convention. No production API is introduced.

No merge or deployment was performed. Merge remains held pending the existing development deployment decision. Remaining #631 acceptance gaps are recorded in the parent issue; this characterization does not close it.

Follow-up issues filed: none. The warning is justified above, and remaining parent-issue work is already tracked in #631.

[cristim]

Session: claude-cc-platform

Taking over this PR from resume-cc-audit / resume-cc-platform (Codex, out of quota); the old CI/CR watchers are no longer live. The user has since authorized merging cloud-commitments-platform once all gates pass, including the auto-deploy on push to main, which clears the deployment hold above. Before merging I will run a local build and tests at b790891cbb22b4db9db5037085efc1ece527147d, post the results here, and recheck CI, CodeRabbit and threads.

[cristim]

Status (claude-cc-platform): this PR's approvals at b790891c are from 2026-10-18, and main has moved a lot since. I re-ran its test on the PR merged onto current main (7e94bb2, merge-tree 3ac31ec9):

• TestB3C1CompletionNotification fails in every automatic_true case. The automatic_false cases pass.
• The failing assertion is the whole-struct config.PurchaseSuppression comparison at plan_step_clock_characterization_integration_test.go:71. Main's suppression rows have changed since this was written: the variant and alias work in fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411 and fix(config): canonicalise every AWS service alias in purchase suppressions, not only ec2/compute #1420, and the fail-atomically change in fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176.
• So the characterization needs re-baselining against current main before it can merge. That means a new commit, which restarts the review gate. Queued after fix(config): Settings Save from a stale tab reverts the global default term, payment and coverage #539.

[cristim]

New head d25194be (claude-cc-platform). This is a plain merge of origin/main (424d914, with no conflicts) plus a one-line test fix. No force-push.

• Cause: on current main, 14 of the 16 cases failed with a single-field diff in the whole-row config.PurchaseSuppression comparison: Service was "ec2", but main now writes "compute". Main stores AWS EC2 suppressions under the canonical slug (config.CanonicalSuppressionService, internal/config/suppressions_build.go:34, from the EC2-slug suppression work in fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411, fix(config): match purchase suppressions across the ec2 and compute slugs #1419, fix(config): match legacy EC2 tenancy and AZ shapes in purchase suppressions #1424 and fix(config): scope suppression tenancy matching to AWS EC2 #1426). Every other field of the row is unchanged (count, variant, engine, timestamps, IDs), so this is the expected value moving, not a regression.
• Change: Service: "ec2" becomes Service: "compute" at line 71. The comparison stays whole-row and nothing is loosened.
• Local results: all 16 cases pass with -race -tags integration, with no skips. go vet -tags integration ./internal/purchase/ and gofmt are clean. A mutation that set SuppressedCount to 21 failed the parent test and 14 subtests.
• Disclosure: the commit ran pre-commit with SKIP=cfn-lint, because that hook's Python environment can't be built locally. Every other hook ran and passed. CI's pre-commit job runs cfn-lint, and this PR touches no CloudFormation.

The approvals above are for b790891c, so this needs fresh reviews at d25194be.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE d25194b

Scope: the PR is test-only, adding one file, internal/purchase/plan_step_clock_characterization_integration_test.go (+268). The base is origin/main 7e94bb2, which is also the merge base. First parent: 0e81df0 and b790891 (the characterization tests), the plain merge of main 424d914, then the one-line re-baseline d25194b.

Findings: none blocking.

Attacked:

• The merge 424d914: git show --remerge-diff is empty, so the merge resolved with no conflict edits, and nothing outside the new test file changed.
• The re-baseline is correct, not just convenient. On main, suppressions_build.go:34 stores CanonicalSuppressionService(rec.Provider, rec.Service), and internal/config/types.go:783-784 maps AWS ec2 to compute (the same canonicalisation scheduler.go:1436/1482 and recommendation_identity.go:94 use). So an aws/ec2 recommendation is now stored as service "compute", and that is the value the whole-row assertion must pin. Every other field in the expected struct, the whole-struct comparison and the timestamp assertions are unchanged (the diff is that one field).
• Probe (head export, restored by an EXIT trap and checked with cmp against d25194b): putting Service: "ec2" back fails TestB3C1CompletionNotification on every subtest. The diff is exactly Service:"ec2" vs Service:"compute", with every other field equal. So the test really reaches the stored row and the assertion is exact.
• The test still asserts whole structs. It was not loosened to a partial match.

Verified locally (GOTOOLCHAIN=go1.26.10, cloud config pointed at /dev/null, heavy runs under the lock):

• gofmt -l on the file: clean. go vet -tags integration ./internal/purchase/: OK.
• go test -tags integration -race -count=1 -run TestB3C1 -v ./internal/purchase/: TestB3C1CompletionNotification (all 16 subtests), TestB3C1VetoHelpers, TestB3C1OverdueHelpers and TestB3C1NotifyBuckets PASS. None skipped.
• git merge-tree --write-tree origin/main d25194b: clean. No migration and no production code.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE d25194b
PR #1083 is OPEN. It is test-only: one new file, internal/purchase/plan_step_clock_characterization_integration_test.go (+268). Its merge base equals the current origin/main (7e94bb2).

Findings:

• low internal/purchase/plan_step_clock_characterization_integration_test.go:168-174 - in the target-met case, the step-2 row that CompleteTargetMetStepTx writes (store_postgres_ramp_target_met.go:72-76) is checked only for status, CompletedAt != nil, the skip reason and empty recs. Its completed_at value and its error note are never pinned. Failure scenario: a change that writes a wrong completion time or a wrong note for target-met steps still passes (probes M2 and M4 survived). The PR body only claims that historical timestamps are preserved, and that claim holds, so this is a coverage gap, not a false claim. Suggested fix: bracket CompletedAt between time.Now() taken before and after the call, and pin the note text exactly ("SIMULATED target already held: 40; 0 failed row(s) of unknown outcome counted as committed").
• low internal/purchase/plan_step_clock_characterization_integration_test.go:158-160 - after completion, the only plan clocks asserted are NextExecutionDate and CurrentStep. persistRampPosition stamps last_execution_date (store_postgres.go:815-816), and nothing checks it. Probe M6, which shifts it by -1h, survived. This clock belongs to the completion path the PR characterizes; bracketing it the same way would close the gap.
• nit PR body: "Docker PostgreSQL 16 and migration 130" may be stale after the merge from main. This is a claim only and does not affect the code.

Attacked:

• Production paths match the test's expectations (all at head):
  • NextExecutionDate == formula: persistRampPosition -> GetNextPurchaseDate (store_postgres.go:806-818). StartDate = formula-14d and the plan reaches step 2 at a 7-day interval.
  • Duplicate completion returns ErrRampStepCountedBySibling before any write (store_postgres.go:778-783). The test then asserts the snapshot is unchanged.
  • The target-met completion updates the root in place (store_postgres_ramp_target_met.go:68-82).
  • Root rows at 78/84/90h are all past vetoWindowEnd (now+72h, plan_step_attach.go:360-362). claimFromRows attaches to them and keeps their date (plan_step_attach.go:111-117, 339). Probe M3 confirms the test pins that date.
  • A no_root automatic plan mints a row at attachScheduledDate (plan_step_attach.go:331, 368-373). The test checks this at :231.
  • A no_root manual plan returns errPlanStepSkipped "manual plan has no scheduled root" (plan_step_attach.go:119-121), so nothing is notified, no recs are resolved and no suppressions are written.
• ec2 to compute: this is intended canonicalisation, not a masked regression.
  • BuildSuppressions keys on CanonicalSuppressionService (suppressions_build.go:34). That function maps "ec2" to "compute" through awsLegacyServiceSlugs (types.go:783-789, 811-822).
  • fix(config): match purchase suppressions across the ec2 and compute slugs #1419 (5f717aa) introduced the canonical write, which fixed an ec2-vs-compute double-buy.
  • The scheduler canonicalises both the suppression side and the rec side when it matches (scheduler.go:1436, 1482), so a stored "compute" still suppresses an "ec2" rec.
  • Variant nil in the expected struct is also correct: a Details-less AWS EC2 rec becomes a wildcard (suppressions_build.go:45-50).
• Vacuity: the run printed all 16 subtests and no skips. Each case asserts shouldNotifyPlan == true first (:206), so no case passes because the notification gate turned it away.
  • The two automatic_false/no_root cases do not weaken the suppression check. They compare the entire snapshot, including rawSuppressions and the plan row, against the post-completion state (:239). They also assert Notified/sends/resolver calls == 0.
  • Skipping the per-row suppression comparison is therefore correct: production writes no rows there, and whole-snapshot equality is the stricter check.
• Whole-row comparisons do catch timestamp drift on pre-existing rows. Mutation probes ran in the scratch copy; the tree was restored and diff -r against a fresh archive of the head was identical:
  • M1, step-1 completed_at +1us inside CompletePlanStep: KILLED (8 target_met_false subtests fail at the whole-row Equal).
  • M3, root scheduled_date +1s on attach (SetExecutionRecommendationsIfEmptyTx): KILLED (12 subtests).
  • M5, step-2 updated_at = NOW() inside CompletePlanStep: KILLED (8 subtests).
  • M2, target-met completed_at -1h: SURVIVED (see finding 1).
  • M4, target-met note altered: SURVIVED (see finding 1).
  • M6, plan last_execution_date -1h: SURVIVED (see finding 2).
• Run: go test -race -count=1 -tags integration -run TestB3C1CompletionNotification ./internal/purchase/ under lockf /tmp/claude/heavy.lock: PASS (16/16, 6.4s, no skips).
• Flake margins: the veto bracket has a 6h margin, the notification bracket has 12h on each side, and the cooldown uses a real clock. No race reports.
• Security and error bodies: not applicable, since the PR is test-only and changes no production code. No secrets; account IDs in the fixtures are placeholders.
• CI at review time: most checks pass; Unit Tests, Docker build, Security Scanning and CodeRabbit are still pending.

[cristim]

Local verification at d25194be1b5100c03f5568f82f348e27a68ad3c5 merged onto current main (7e94bb29; merge-tree d2bcc4c0; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/purchase
• Nothing was applied to any cloud.