test(scheduler): verify incomplete savings plans persistence - #490
Conversation
Pin the published AWS completeness fix and cover malformed details, failed plan types, late-page survivors and conservative fallback eviction. Assert persisted financial values and account-scoped row identities. Refs LeanerCloud/cloud-commitments-go#170
cristim
added
urgency/this-sprint
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 43 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
Task C review and baselinePass 1: one finding, persisted identity assertion checked broad allowed values instead of exact survivor tuples. Author revised. Three clean runtime-script reviews before execution:
Matrix is24subcases (11RI+13SP),25test events including parent. Evidence is synthetic HTTP through actual SDK/scheduler/store into local PostgreSQL17.11, not live AWS or DockerPG16. Baseline executed: modules-parent exit 0 selects published AWS006ef. Full parent run session69886 exited1: all24subcases ran,15passed (all11RI plus4SP controls),9SP incomplete cases failed at successful-account/eviction-roster assertions. These are intended behavior failures, not compile/setup errors. They detect unauthorized eviction eligibility before persistence; no destructive baseline DELETE is claimed. Parent-controls session49901 exited0 with separately selected SP_valid, SP_empty, SP_clean_fallback, SP_empty_fallback all passing. Logs, exact argv/env/input hashes, result status and retained UUID database names are adjacent. Source hash unchanged; only planned test dirty. Fixed-pin proof remains pending root's repin. Fixed runner three clean reviews: (1) binds same reviewed test hash and exact three planned dirty files, requires published9962786, named fixed and broad check modes; (2) mutant removes only the AWS diagnostic Warnf call in an external scheduler overlay, preserves completeness/survivors/eviction, records mutant hash and runs mixed plus valid/empty controls; (3) all immutable-output, endpoint ownership, retained DB, lock, offline/synthetic env and timeout guards retained. Pinned golangci2.10.1 matches Makefile and installed version; lint explicitly includes integration-tagged changed test. No actionable findings. Fixed draft verification completeIndependently reviewed all three final draft files against base 4fe935e. Module diff changes only AWS006ef to published9962786 and its two sums; pkg and Azure/GCP pins unchanged, no replace. No actionable findings. The only dirty files are the approved test/go.mod/go.sum; diff-check passes. Fresh fixed modules provenance exit0. Fixed connected suite session75969 exit0: all24subcases PASS (25events including parent), package19.087s. Assertions exercise exact persisted key sets, costs, request tuples, count-bearing diagnostic output, account-filter isolation, partial old-row retention, clean-sibling eviction, and ambient/disabled-account boundaries. Diagnostic-only mutant session23838 exit1 as required: SP_mixed reaches the diagnostic assertion after successful exact persistence checks, then fails because output lacks Full repository race/short suite session96610 exit0, build53991 exit0, vet92916 exit0, pinned integration-tagged scheduler lint91765 exit0 with Final draft SHA256:
Draft verification approves these exact inputs for staging/normal hooks. Committed-SHA review and connected runtime remain pending the author's commit, as do remote CI/publication/merge gates. This evidence substitutes only native PostgreSQL helper creation for Docker; it does not assert live AWS, browser, deployed, or PostgreSQL16-container parity. Staged review gateBoth passes independently read the entire Read the five requested feedback memories: collection-result verification, SDK test network confinement, integration-tagged lint, cross-repository issue references, and installed push hooks. Collection outcomes and exact persisted rows are independently asserted. No issue references or hook changes occur in the diff. Test additions contain 0 comment lines out of 220 added lines (0%). Overall staged diff is 223 insertions and 14 deletions in three files. No unrelated production changes, Azure/GCP drift, new abstraction, or duplicated fixture was introduced. Evidence wording above now distinguishes the focused no-dial fixture from broad-suite proxy configuration. Pinned integration-tagged lint passed; separate Committed runner review before execution: three clean passes. Pass 1 verifies exact c47100d, clean tree, hashes for all three files, published pin, uncached race/integration selector. Pass 2 verifies the reused helper checks exact local PostgreSQL endpoint before additive UUID database creation and disables destructive cleanup; overlay is helper-only and synthetic no-dial SDK fixture is unchanged. Pass 3 verifies canonical lock, bounded timeout, offline modules, new exclusive committed-prefixed manifests/logs/results, and before/after source state recording. Only modules and fixed suite modes are allowed. Full committed three-file diff reread: no actionable findings; matches staged content. Exact committed verdictApproved c47100d, tree 112d4a2528ccc3c4a3d7a1267800470c6b2fb5e5, parent 4fe935e. All three committed file hashes match the reviewed and verified draft. Fresh full committed diff review found no actionable findings across all six dimensions, reuse and scope, including monetary values and account isolation. Approval is for this exact SHA and local verification gate; the author owns publication, CI and merge. Fresh The connected run proves exact persisted key sets and monetary fields, survivor retention, complete empty eviction, successful account roster, registered/ambient/disabled account boundaries, exact outbound request tuples, and count-bearing incomplete diagnostics against real migrations and local PostgreSQL17.11. Prior baseline and diagnostic-only mutant evidence uses identical test bytes and remains applicable. Broad race/short suite, build, vet and integration-tagged lint were not repeated because committed product bytes match their passing draft inputs. The author reports successful normal hooks and pinned linter config verification; those are attributed author observations rather than an independently rerun claim here. Final fresh git queries confirm exact HEAD/tree/parent above and empty porcelain status. Result manifest likewise records unchanged HEAD/test hash and clean status. All 24 newly created UUID databases are retained. No product edits, public writes, live-cloud credentials, or existing database deletion occurred in this final gate. This is synthetic SDK HTTP through production scheduler/store into native local PostgreSQL, not live AWS or Docker PostgreSQL16 proof. Sole Go runtime capacity is released to the author. |
PR490 independent reviewVerdict: no actionable findings at c47100d against base 4fe935e. Reviewed by independent gpt-6-astra under the user's explicit reviewer and synthetic-verification substitution. No product edits or public writes. Merge boundary update: coordinator reports main advanced to 115ff70 while this review ran. That object was not present locally at the last check. This verdict covers the exact head and original base only, not the combined candidate on new main. The new delta reportedly touches scheduler read-time filters and recommendation overrides. Inspect those exact changes and verify the merged candidate's connected scenario before treating this as merge approval; do not infer combined behavior from the 24/24 exact-head result. Follow-up source inspection after coordinator fetch: 115ff70 is now local. The complete PR diff is three files, 223 insertions and 14 deletions: published AWS provider pin and sums, plus scheduler integration tests. Prerequisite scheduler behavior is already in the base.
Source contract citations (paths below are relative to the Platform worktree, except SDK):
SDK source root: Independent reproduction, all with Go 1.26.6, race enabled, native PostgreSQL 17.11, real migrations and production scheduler/store, synthetic AWS HTTP responses:
The nine baseline failures occur at actual successful-account roster assertions ( Commands executed: Input SHA256:
Runner review: three passes checked input/revision binding, scope and safe writes; synthetic network and database confinement; exclusive lock, timeout, process-session cleanup and log/result integrity. Python syntax compiled. Existing source graph was consulted only for navigation; current source controlled the verdict. Limits: synthetic AWS payloads, not live cloud verification; this is connected scheduler/PostgreSQL integration, not browser UI or purchase testing. Full suite/build/vet/lint/hooks were not independently rerun because the brief records those checks on these same bytes and no new concern justified them. CI and public review posting remain the coordinator's responsibility. A fixed-run approval-review timeout occurred before process creation; the permitted single retry succeeded. All three execution sessions are terminal and their subprocess leaders were reaped; global process listing was sandbox-blocked. Final manifests verify unchanged clean product tree and the exact reviewed HEAD. |
PR490 moved-base candidate verificationBound candidate: staged Git tree Source review: the PR three-dot diff remains go.mod, go.sum and recommendation_completeness_integration_test.go only (223 insertions, 14 deletions). The 4fe935e to 115ff70 base delta changes relevant behavior only on recommendation reads: scheduler.GetRecommendationByID, filterRecsByResolvedConfigs, and config.ResolveAccountConfigsForRecs now apply global service policy to ambient rows. Collection, fallback completeness, conversion, account identity and database eviction are unchanged. No rebase or product correction is required. The exact-head review and baseline/mutation evidence remain in the sibling go170-pr490-cold-20261005 directory. Verification scope: TestAWSRecommendationCompletenessPersistence (24 subcases) plus TestGlobalFiltersPersistedAmbientAndRegistered (11 subcases). The latter checks persisted ambient and registered rows for enabled/disabled, minimum count, include/exclude engine/region/type, exclusion precedence, and registered-override precedence; visible rows also pass detail-view hidden-reason assertions. This targets the actual changed consumer boundary without repeating unrelated full-suite work. Runner: Result: PASS, exit 0. Exact counts are 37 RUN / 37 PASS / 0 FAIL, comprising 24 completeness subcases, 11 global-filter subcases and their two parent tests. No skips or race reports. Runtime package result: Verdict: no actionable findings for staged candidate tree Limits: synthetic AWS HTTP payloads with real SDK and PostgreSQL, not live cloud or UI/purchase verification. Coordinator must compare the eventual GitHub merge tree to this bound tree before extending this candidate proof to that merge. No new commit, source mutation, public write, or merge is performed by this reviewer. |
1 participant
Savings Plans collections can return usable rows with rejected details or failed requests. Platform now pins the published AWS completeness fix and verifies that those partial collections preserve their surviving offers without authorizing stale-row eviction.
The existing PostgreSQL regression covers 24 cases: 11 RI controls and 13 SP cases, including malformed details, failed plan types, late-page survivors, configured fallbacks, exact persisted financial values and account-scoped isolation. Only the integration test and AWS module pin/checksums change.
Refs LeanerCloud/cloud-commitments-go#170. This completes its Platform consumer task; the producer, MCP and CLI changes are already merged.
Independent reviewer:
gpt-6-astra, under the user's authorized local-verification alternative. Reviewed head:c47100d405b7e24aa2cc0034c125029f04e146da; base:4fe935e3dee6c0a7464cda279fbc308cbca728d5. Full committed diff review found no actionable findings. Both staged review passes were clean.Verification:
-count=1.Tests use synthetic AWS responses through the real SDK, scheduler and store into native PostgreSQL 17.11. A local overlay replaces only the test database-creation helper; production code, migrations and assertions remain unchanged. Fixture databases are retained. No live AWS or deployed-feature verification is claimed. The broader suite uses synthetic credentials and configured rejection proxies; its entire network boundary was not independently audited.
Added test comment ratio: 0%. Exact source hashes, commands, environments, parent-red proof, mutation proof and committed-head logs are recorded in the attached independent review.