Repository navigation
test(api): pin the future-only start_date guard on Add Purchases (refs #50) - #1028
Conversation
LeanerCloud#873 already refuses a planned-purchase start_date that is not after the current instant ("start_date must be in the future"), but nothing tested it. Add an injectable handler clock (nil means time.Now, so production is unchanged) and regression tests through the real HandleRequest path: yesterday, a long past date and today are refused with 400, tomorrow is accepted, a malformed date keeps its format error, and the UTC-midnight boundary and a non-UTC clock show the comparison is in UTC. Document the 400 and the midnight-UTC rule for start_date in openapi.yaml. Refs LeanerCloud#50 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 4 billable files and costs up to $1.00. Or wait 38 minutes for your next included review. View limit details
Comment |
cristim
left a comment
There was a problem hiding this comment.
Independent review of 533ee46: draft: blocks ready
No BLOCKING code findings. The only open items are gate items: this draft has no ready-for-review flip yet and CI had not finished when I checked.
What actually changes
Production behavior is unchanged. internal/api/handler_plans.go:554-560 swaps time.Now() for h.clock() in the guard that #873 already added. handler.go adds an unexported now field that is nil in every production constructor, so clock() returns time.Now() (handler_plans.go:565-570). The openapi.yaml hunks are docs only. The title is accurate in effect, but the PR does touch 3 non-test files.
Findings
- NON-BLOCKING:
internal/config/store_postgres.go:2094still saysparseCreatePurchasesRequest accepts a past start date. That has been false since #873. Fix it here or in the frontend follow-up. - NON-BLOCKING: #50 acceptance criteria say "No regression for
today" and the issue's suggested server fix accepts today (start_date must be today or later). This PR keeps #873's stricter rule (today refused, tomorrow UTC is first accepted). The PR body records the timezone decision, which satisfies "decision recorded", but I suggest also posting it on #50 and correcting the criterion there, so the frontend follow-up implements the matching rule. - NON-BLOCKING: the UI still lets a user pick today in local time (
frontend/src/plans.tsmin), which the server refuses. The PR names this as the remaining #50 work.
Issue #50: Refs vs Closes
Criteria 1, 2 and 5 (server 400, backend test, timezone decision) are met. Criterion 3 (frontend inline check plus plans-range-validation.test.ts) is not. Keeping #50 open via "Refs" is reasonable, and a closing keyword would be wrong here. Labels on the PR (priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/bug, triaged) match #50 exactly.
What I attacked
- Routes that can set a start date:
parseCreatePurchasesRequest(handler_plans.go:663) is the only client-supplied date. Plan create and update deriveRampSchedule.StartDateserver-side (handler_plans.go:348,393; update preserves the stored value). Ladder amend has its own future check (handler_ladder_amend.go:95).updatePlanNextExecutionDateTx(:738) only runs after the guard, in the same transaction. No other API path takes a date. The guard is enforced on the single path that matters. - Timezone:
time.Parse("2006-01-02", ...)yields UTC midnight regardless of serverTZ. Comparison is an instant comparison, so the clock's zone is irrelevant. Today is refused, the date at its exact midnight is refused, and tomorrow is accepted up to 23:59:59 UTC. Empty, zero or malformed strings fail the parse with 400 before the guard. - Ran
go test ./internal/api -run 'StartDate|HandlerClock|CreatePlannedPurchases': 18 passed.StartDateMustBealso passes underTZ=Pacific/Kiritimati. - Fail-before: the tests cannot compile on origin/main (no
nowfield), and main's production guard is identical, so I proved the tests have teeth with mutations on the PR head (each reverted, worktree clean afterwards):- guard disabled: 6 subtests fail;
!Afterchanged toBefore: the exact-midnight case fails;- clock ignored (
time.Now()): both "tomorrow accepted" cases fail; - day-truncation accepting today: 4 subtests fail;
ParseInLocation(..., time.Local)underTZ=America/Los_Angeles: 3 subtests fail.
- openapi.yaml vs handler: the 400 text and the
start_datedescription match the messages and behavior.Errorschema exists and matches the{"error": ...}body asserted in the tests. - Merge:
git merge-treeof the head onto current origin/main reports no conflicts (main has since changed the same openapi block for the 409 codes; it merges cleanly). - Size: +159 additions in total, under the 400 cap; no file near 500 lines.
Gate status (origin/main Review gate)
- Independent adversarial review naming the SHA: this comment.
- Findings resolved: none blocking. CodeRabbit skipped the draft; there are no CR threads.
- CI green on SHA: not yet.
CI - Build & Testwas queued andpre-commitin progress when I checked; noaction_requiredruns seen. Re-check after the PR leaves draft. - Local verification on macOS: the real router path with a pinned clock, covered by the tests and probes above. Gaps: Postgres integration tests were not run by me; the
SavePurchaseExecutionwrite path here is mocked. - Verdict and evidence recorded on the PR: this comment.
Blocks ready
- Mark the PR ready for review (it is a draft).
- Wait for CI to finish green on this SHA. Any new commit restarts the gate.
|
Gate record for 533ee46 (origin/main CLAUDE.md "Review gate"):
Merging at 533ee46 with --match-head-commit. |
Refs #50
Summary
#50 asked for the server to refuse a past
start_dateonPOST /api/plans/{id}/purchases. That guard already exists: #873 (5134af4) added it invalidatePurchaseScheduling:No test covered it, though, and the API docs didn't mention it. This PR keeps #873's rule exactly as it is and pins it with tests.
now func() time.Timefield onHandler, read throughh.clock(). When it is nil, which is the case for every production constructor, it falls back totime.Now, so nothing changes in production. The guard is the only caller.internal/api/handler_plans_start_date_test.go. They go through the realHandleRequestrouter path with the clock pinned.start_date.The check stays where #873 put it, after the plan lock.
Timezone rule: why today is refused
start_dateis a date-only value parsed as midnight UTC, and the guard needs it to be strictly after the current instant. So today's UTC date is already in the past and is refused, and tomorrow (UTC) is the earliest accepted date. This PR keeps that on purpose. From the #873 review thread:and on keeping the check under the lock:
Accepting today would create executions that are already due. The scheduler would fire them on its next tick and skip the veto window, which is the harm #50 describes.
Endpoints checked
POST /api/plans/{id}/purchases: the only endpoint that takes a client start date for planned purchases. Covered here.RampSchedule.StartDateto now, and updates keep the stored value.scheduled_datethat is not in the future (handler_ladder_amend.go:95). Not touched.Verification
Run locally on macOS at 533ee46, with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.New tests, clock fixed at 2026-03-10 12:00 UTC unless noted:
03/11/2026invalid start_date format...Every 400 case also asserts that no execution row and no plan update were written. A separate test checks that the clock falls back to
time.Now.The new tests also pass with
TZ=America/Los_Angeles,TZ=Pacific/KiritimatiandTZ=UTC.Mutation probes (fail-before). Each mutation was applied to
handler_plans.go, the tests were run, and the file was restored:!Afterchanged toBeforeParseInLocation(..., time.Local)withTZ=America/Los_Angelestime.Now())CI-equivalent checks:
go build ./...andgo vet ./...: clean.go test ./internal/api/... ./internal/mocks/... -count=1: ok.--allow-parallel-runnerson./internal/api/...: 0 issues.gocyclo -over 10onhandler.goandhandler_plans.go: clean.git diff --check: clean, and openapi.yaml parses.--scan --cachedon the changed files in a freshgit initrepo with an isolated HOME: clean.bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.Not run locally: the Postgres integration tests and gosec, which isn't installed here. CI covers them.
Frontend follow-up (not in this PR)
frontend/is left alone because UI work is in progress there. Two changes remain for #50:frontend/src/plans.ts:2252:startDateInput.min = toLocalDateInputValue(now)lets the user pick today in their local timezone, and the server always refuses that. West of UTC late in the day, it can even allow a date that has already passed in UTC.minshould be tomorrow's date in UTC.handleAddPurchases: add the inline check Enforce future-only purchase start_date server-side (defense in depth for #1249) #50 asks for, with an error that says the date must be after today (UTC) before the request goes out, and a regression test inplans-range-validation.test.ts.#50 should stay open until those land, which is why this PR says Refs rather than Closes.
🤖 Generated with Claude Code