Remediate feature-review HIGH: report the Supply run outcome after Push to Calendar #141
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Maniacs_Mansion/MealQuest#141
Reference in New Issue
Block a user
Remediation of the Feature 17 feature-level review
Fixes the HIGH finding and its companion MEDIUM from the read-only feature-level review of #140 (recorded there as comment 10389). Maestro task
e75db0f0-817f-4eef-84b2-266e793c018a.F17-01 (HIGH) — a failed Supply run was reported as a successful push.
POST /plans/{weekStart}/push-calendaranswers 200 withsupply_run(generated|archived|failed, witherror.codeplan_changed|supply_run_failed), but the client parsed only the plan and dropped it, soPlanPageshowed a successful Calendar sync with no mention of the list that was never made.web/src/plan/api.tsnow parsessupply_runin the server's own shape and rejects any other shape rather than treating it as success, and the Plan page tells the household each outcome:generated— made this week's Supply run from the plan.archived— it is in the Supply archive and was left as it was.plan_changed— an error banner: the plan is on Calendar but no list was made, with Retry to push again (re-reading the edited plan first, as a refused push does).supply_run_failed— an error banner with "Make the shopping list", using the existing generate hook and endpoint.A push whose Supply run failed is kept in the mutation cache like a failed push, so leaving the week and coming back still shows it.
F17-02 (MEDIUM) — the stale list. A successful push never refreshed the week's Supply run query, so a cached 404 or the previous list could persist for the 30-second freshness window. The push now refreshes that query using the Supply run screen's existing exported
supplyRunKey(one key builder, not a duplicate).Not in this PR: F17-03 (archived posts joining the current dinner name) is MEDIUM and is deferred to Feature 22 task
22f80eb6, because the fix changes migration0016, which has already been reviewed as safe for the live Feature 16 database — changing it would invalidate that review in the deploy path. No Go and no migration files are touched here.Quality floor at
9616941b7310a0b7601de1530a8241c071169085:npm run typecheckPASS;npm testPASS 49 files / 877 tests (up from 872 —PlanPage.test.tsxdrives all four outcomes andapi.test.tsxcovers returning to a week whose Supply run failed);npm run buildPASS including the colour and fonts checks;gofmt -l .empty;go vet ./...clean. The new tests were confirmed to fail without the fix.The branch was pushed by the developer, whose token cannot open pull requests; this one was opened with the orchestrator's token and the developer's report is otherwise unmodified.
The push client parsed only the plan from POST /plans/{weekStart}/push-calendar and dropped supply_run, so a Supply run that was not made was shown as a successful push (F17-01). Parse supply_run as the server sends it and tell the household every outcome on the Plan: made, left as it was in the Supply archive, or not made, with its recovery: push again for plan_changed (reading the edited plan again first), or make the shopping list again through the existing generate endpoint for supply_run_failed. A push whose Supply run failed stays in the mutation cache like a failed push, so leaving the week before it settles still shows it on coming back. Read the week's Supply run again when the push answers, using the Supply run screen's supplyRunKey, so a cached 404 or the earlier list is not shown for the freshness window (F17-02). Maestro task e75db0f0-817f-4eef-84b2-266e793c018a. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>Independent Codex gpt-6-sol review: APPROVED for this exact commit.
Read-only review of the remediation —
APPROVEDat9616941b7310a0b7601de1530a8241c071169085The reviewer confirms the defect is closed and did not rerun the gates:
supply_runresponse is parsed strictly — a missing or unrecognised outcome throws rather than being read as success, which is exactly the failure mode F17-01 was about.plan_changedpath re-reads the plan before another push; the existing generate endpoint and the exportedsupplyRunKeyare reused rather than duplicated.One MEDIUM finding, to be fixed before merge — F17-R1 /
web/src/plan/PlanPage.tsx:375: the page treats a successful "Make the shopping list" action as resolving the failed push only while the page stays mounted. Make the list, visit the Supply run screen, and return, and the recovery action's success state is gone while the retained push still sayssupply_run_failed— so the page again tells the household the list could not be made, although it exists. It must be settled when generation succeeds, with a test covering that navigation.A second round on this branch is being implemented now; the head will move and this pull request will be re-reviewed at the new head before the approver merges it into the feature branch.
Independent Codex gpt-6-sol review: CHANGES_REQUIRED for this exact commit.
Second read-only review —
CHANGES_REQUIREDatc589c02cdc72218df6b88b972424c976a5fb21efConfirmed fixed: F17-R1's after-success defect (the navigation test would have caught it), and round one is intact — strict
supply_runparsing, all four outcomes reaching the household, theplan_changedre-read.settle()targets the push on screen when generation began and cannot remove a newer push;reset()still refuses a pending push. The developer's decision to settle the push rather than follow the week's Supply run query was judged sound: an older Supply run can exist and the returned list has no revision or timestamp proving it belongs to this push.New HIGH — F17-R2 /
web/src/plan/api.ts:283(sweep(false)on week navigation). A push whose Supply run failed is a settled mutation. Press "Make the shopping list" and then select another week while the request is in flight, andsweep(false)removes that push before generation finishes. If generation then fails, the rejection does not callsettle()— but the push is already gone, so returning to the week shows no failure and no recovery action although its Supply run was never made. The existing navigation test leaves for the Supply run screen and only navigates after generation has succeeded, so it cannot catch this.An unresolved failed Supply run outcome must survive week navigation until a successful generation settles that specific push or a replacement push supersedes it — it must not be swept merely because its recovery is still in flight. Round three is being implemented now; a test will navigate away while the recovery request is pending, fail it, return, and assert the failure and recovery are still shown.
Independent Codex gpt-6-sol review: APPROVED for this exact commit.
Third read-only review —
APPROVEDat445750cecb203587a93d3b1f4a09ba5940d2bbc9F17-R2 is closed. The latest
sweep(false)preserves the latest unmade push across week navigation, so a failed in-flight recovery leaves the alert and action available on return; a successful recovery settles that exact push through the captured promise, including after navigation, and cannot remove a newer push.unmade()is now the shared predicate for both failure and retention, and pending-push reset refusal and the sweeping of other settled failures are unchanged.Retention was judged acceptable for a single-household PWA: one unmade push per unresolved week, each entry small and representing a real unresolved outcome. Earlier commits remain intact — strict parsing, all four outcome messages, the Supply run refresh, the
plan_changedre-read, andsettle()targeting only the on-screen push.One MEDIUM remaining, being fixed before merge — F17-R3 /
web/src/plan/PlanPage.tsx:385: the Plan page clears the retained unmade push only when its own "Make the shopping list" request succeeds; the Supply run screen makes that same list through the shared endpoint without settling it. Return to Plan after making the list that way and it can still say the Supply run could not be made and offer the action again, although the list exists. The reconciliation is moving to the generate mutation for the week so both screens get it and there is no second mechanism.The reviewer also noted that the success-after-navigation test does not independently prove settlement (sweeping the push also removes the alert); that assertion is being strengthened so settlement cannot silently regress.
Independent Codex gpt-6-sol review: APPROVED for this exact commit.