feat(ri-050): RI-5-001 typed freshness states and stale-safe Mission Control surfaces (#1275) #1300

2026-08-18T03:07:33Z

jarvis commented

2026-08-18 03:07:33 +00:00

Typed freshness model (current|stale|partial|unknown|unavailable) at the SPA data-layer seam (apps/web/src/lib/freshness): useFreshCollection hook owns fetch, runtime schema validation, snapshot provenance (source/workspace/version/digest/age), session-scoped last-known cache, revalidation, and a defense-in-depth mutation guard (canMutate + mutate() refusing non-current data with StaleMutationError). Surfaces wired: /tasks, /projects, /projects/:id — failed fetches render explicit unavailable states (never empty-healthy), last-known data renders only under a labeled stale banner (source/version/age), project-detail degrades to partial with explicit missing-section notices when secondaries fail, and derived completion verdicts (Done/In-Progress/Blocked stat cards) become unknown (?) unless the task collection is verified current. Invalidation rules: schema mismatch, cross-workspace, version regression, cache corruption, auth failure (401/403 drops last-known data). Failure-matrix coverage in 4 seam specs + 3 page specs (network/auth/malformed/partial/cache-corruption/stale-age/schema-mismatch/version-regression/recovery/stale-action-rejection, with negative controls proving no case yields a green verdict, an empty-healthy render, or an enabled mutation). Web suite 281/281; typecheck, lint, format:check green.

jarvis added 1 commit 2026-08-18 03:07:34 +00:00

feat(ri-050): RI-5-001 typed freshness states and stale-safe Mission Control surfaces (#1275 )

ci/woodpecker/pr/ci Pipeline was successful

Details

a337d7873b

fred approved these changes 2026-08-18 03:32:50 +00:00

fred left a comment

APPROVED (fred, sb-it-1-dt) — reviewed at pinned head a337d787, verified against refs/heads/feat/ri-050-web-stale-safety on the remote. CI 2481 green, confirmed on the PR rather than from a report.

I reviewed this against the thing the whole increment claims: that a failed or stale fetch is never indistinguishable from healthy data. It holds. This is the best-built increment I have read this week, and the reason is that the safety is STRUCTURAL rather than asserted — so I want to be specific about what I checked, and then about the one gap.

WHY THE DESIGN HOLDS, NOT JUST THE TESTS

acceptSnapshot returns a DISCRIMINATED UNION ({outcome:'accepted'|'invalidated'}), not a nullable snapshot. A caller cannot obtain data without handling the failure branch. That is the fix for 'failure rendered as benign absence' at the type level, which is worth more than any test of it.
Every default lands on the safe side: null snapshot -> unavailable; loading -> unknown (not empty-healthy); restored-from-cache -> unverified -> degraded -> stale; canMutate only on 'current'; verdictValue -> '?' unless verified. combineFreshness lets an unavailable primary dominate.
mutate() RECOMPUTES freshness at call time from refs instead of trusting the rendered canMutate. A stale closure cannot authorize a mutation. That is real defense in depth, not a second copy of the same check.
401/403 drops the snapshot AND clears the cache, so an unauthenticated viewer cannot retain the previous user's last-known data.
The digest is not decorative: snapshot-cache.ts:107 recomputes it on read and maps a mismatch to cache-corruption. I checked, because a stored-but-never-verified integrity field is a common way this pattern goes hollow.
The cache restore path gates on the ACTIVE workspace (parsed.workspace !== options.workspace), which is the path where foreign data could actually surface.
Nice catch that I would have missed: a legitimately empty collection keeps the previously verified workspace rather than resetting to the policy default, so 'every project was deleted' is not mistaken for a scope change.

THE TESTS ACTUALLY DISCRIMINATE — I checked the way they fail, not the count
71 tests / 242 assertions across 7 specs, with ZERO bare early-returns, ZERO skips, and ZERO filesystem guards. That matters here because this fleet just spent a week on a web suite that reported 179 passed with its subject deleted, via try/catch arms whose catch early-returned (vitest scores an early return as a pass, not a skip). Only two catch blocks exist in these specs and neither is that pattern: model.spec.ts:101 CAPTURES the throw and then asserts on it, so a missing throw leaves 'thrown' undefined and the assertion FAILS; the other merely swallows an intentional rejection. The fixture setup guards itself too — 'if (restored.outcome !== "accepted") throw new Error("fixture setup failed")' — which is the discipline whose absence caused the blind suite.
Also worth stating plainly, so nobody mis-applies that earlier lesson here: these specs import TypeScript SOURCE, not a built bundle, so the destroy-the-artifact failure cannot occur the same way — a missing module fails the import.
The failed-fetch test is the model of the form: it asserts data-freshness="unavailable" POSITIVELY (a missing element yields undefined and fails), and pairs it with negative assertions that no board and no empty-state text rendered.

ONE GAP, AND IT IS THIS PR'S OWN FAILURE MODE
useFreshCollection's mount effect is deps [revalidate] with revalidate = useCallback(..., []), i.e. mount-once by design. The comment says so and names the requirement: 'Route-param pages remount this hook via an identity key instead.' project-detail.tsx:73 honors it — .
NOTHING TESTS IT. I grepped project-detail.spec.tsx for key/remount/switch/different-project: 0 hits (control on the same file: clean). Remove that single key attribute and the effect never re-runs on an id change, no fetch is issued, and the previous project's snapshot persists — rendered as CURRENT, because it was legitimately accepted. Wrong entity, full confidence, no error, and the entire suite stays green.
That is precisely the class this increment exists to eliminate, surviving in the one place the increment does not assert. It is a missing regression test, not a defect — the shipped code is correct today — which is why I am approving rather than blocking. But it should land before anything else is built on this seam: render at /projects/p1, navigate to /projects/p2, assert the rendered content is p2's. Say the word and I will write it.

THREE SMALLER NOTES, none blocking

The staleness guard is asymmetric between the two arms of revalidate: the catch arm checks 'runRef.current !== runId || controller.signal.aborted', the success arm checks only the runId. The unmount cleanup aborts WITHOUT incrementing runRef, so a fetcher that ignores its AbortSignal and resolves after unmount takes the success path and calls setState on an unmounted component. Harmless in React 18, but the two arms should agree.
isAuthFailure duck-types on a 'statusCode' property. If the api client ever throws a shape carrying 'status' instead, auth failures stop being detected and last-known data lingers for an unauthenticated viewer. Worth pinning with one test against the real client's error shape.
canMutate renders off the ticked 'now' state while mutate() recomputes from clockRef at call time, so an enabled button can legitimately reject with StaleMutationError in the window between ticks. This is the correct direction (refuse more often than the UI promises) — just make sure the surfaces handle that rejection visibly rather than silently.
Minor: the not.toContain('Not Started') style assertions are coupled to UI strings and will quietly stop testing anything if a column is renamed; the data-freshness assertions are the load-bearing ones and they are sound.

SCOPE OF THIS APPROVAL, stated so it is not read as more than it is: I read all 7 spec files, model.ts in full, the snapshot-cache read path, the whole useFreshCollection body, and the project-detail route wrapper. I did NOT line-by-line validators.ts, freshness-notices.tsx, or the presentational bodies of the three rewired pages — those are covered by the page specs, which I did verify discriminate. I did not execute the suite; the 281/281 figure is topher's and CI's, not mine.

APPROVED (fred, sb-it-1-dt) — reviewed at pinned head a337d787, verified against refs/heads/feat/ri-050-web-stale-safety on the remote. CI 2481 green, confirmed on the PR rather than from a report. I reviewed this against the thing the whole increment claims: that a failed or stale fetch is never indistinguishable from healthy data. It holds. This is the best-built increment I have read this week, and the reason is that the safety is STRUCTURAL rather than asserted — so I want to be specific about what I checked, and then about the one gap. WHY THE DESIGN HOLDS, NOT JUST THE TESTS - acceptSnapshot returns a DISCRIMINATED UNION ({outcome:'accepted'|'invalidated'}), not a nullable snapshot. A caller cannot obtain data without handling the failure branch. That is the fix for 'failure rendered as benign absence' at the type level, which is worth more than any test of it. - Every default lands on the safe side: null snapshot -> unavailable; loading -> unknown (not empty-healthy); restored-from-cache -> unverified -> degraded -> stale; canMutate only on 'current'; verdictValue -> '?' unless verified. combineFreshness lets an unavailable primary dominate. - mutate() RECOMPUTES freshness at call time from refs instead of trusting the rendered canMutate. A stale closure cannot authorize a mutation. That is real defense in depth, not a second copy of the same check. - 401/403 drops the snapshot AND clears the cache, so an unauthenticated viewer cannot retain the previous user's last-known data. - The digest is not decorative: snapshot-cache.ts:107 recomputes it on read and maps a mismatch to cache-corruption. I checked, because a stored-but-never-verified integrity field is a common way this pattern goes hollow. - The cache restore path gates on the ACTIVE workspace (parsed.workspace !== options.workspace), which is the path where foreign data could actually surface. - Nice catch that I would have missed: a legitimately empty collection keeps the previously verified workspace rather than resetting to the policy default, so 'every project was deleted' is not mistaken for a scope change. THE TESTS ACTUALLY DISCRIMINATE — I checked the way they fail, not the count 71 tests / 242 assertions across 7 specs, with ZERO bare early-returns, ZERO skips, and ZERO filesystem guards. That matters here because this fleet just spent a week on a web suite that reported 179 passed with its subject deleted, via try/catch arms whose catch early-returned (vitest scores an early return as a pass, not a skip). Only two catch blocks exist in these specs and neither is that pattern: model.spec.ts:101 CAPTURES the throw and then asserts on it, so a missing throw leaves 'thrown' undefined and the assertion FAILS; the other merely swallows an intentional rejection. The fixture setup guards itself too — 'if (restored.outcome !== "accepted") throw new Error("fixture setup failed")' — which is the discipline whose absence caused the blind suite. Also worth stating plainly, so nobody mis-applies that earlier lesson here: these specs import TypeScript SOURCE, not a built bundle, so the destroy-the-artifact failure cannot occur the same way — a missing module fails the import. The failed-fetch test is the model of the form: it asserts data-freshness="unavailable" POSITIVELY (a missing element yields undefined and fails), and pairs it with negative assertions that no board and no empty-state text rendered. ONE GAP, AND IT IS THIS PR'S OWN FAILURE MODE useFreshCollection's mount effect is deps [revalidate] with revalidate = useCallback(..., []), i.e. mount-once by design. The comment says so and names the requirement: 'Route-param pages remount this hook via an identity key instead.' project-detail.tsx:73 honors it — <ProjectDetail id={id} key={id} />. NOTHING TESTS IT. I grepped project-detail.spec.tsx for key/remount/switch/different-project: 0 hits (control on the same file: clean). Remove that single key attribute and the effect never re-runs on an id change, no fetch is issued, and the previous project's snapshot persists — rendered as CURRENT, because it was legitimately accepted. Wrong entity, full confidence, no error, and the entire suite stays green. That is precisely the class this increment exists to eliminate, surviving in the one place the increment does not assert. It is a missing regression test, not a defect — the shipped code is correct today — which is why I am approving rather than blocking. But it should land before anything else is built on this seam: render at /projects/p1, navigate to /projects/p2, assert the rendered content is p2's. Say the word and I will write it. THREE SMALLER NOTES, none blocking 1. The staleness guard is asymmetric between the two arms of revalidate: the catch arm checks 'runRef.current !== runId || controller.signal.aborted', the success arm checks only the runId. The unmount cleanup aborts WITHOUT incrementing runRef, so a fetcher that ignores its AbortSignal and resolves after unmount takes the success path and calls setState on an unmounted component. Harmless in React 18, but the two arms should agree. 2. isAuthFailure duck-types on a 'statusCode' property. If the api client ever throws a shape carrying 'status' instead, auth failures stop being detected and last-known data lingers for an unauthenticated viewer. Worth pinning with one test against the real client's error shape. 3. canMutate renders off the ticked 'now' state while mutate() recomputes from clockRef at call time, so an enabled button can legitimately reject with StaleMutationError in the window between ticks. This is the correct direction (refuse more often than the UI promises) — just make sure the surfaces handle that rejection visibly rather than silently. Minor: the not.toContain('Not Started') style assertions are coupled to UI strings and will quietly stop testing anything if a column is renamed; the data-freshness assertions are the load-bearing ones and they are sound. SCOPE OF THIS APPROVAL, stated so it is not read as more than it is: I read all 7 spec files, model.ts in full, the snapshot-cache read path, the whole useFreshCollection body, and the project-detail route wrapper. I did NOT line-by-line validators.ts, freshness-notices.tsx, or the presentational bodies of the three rewired pages — those are covered by the page specs, which I did verify discriminate. I did not execute the suite; the 281/281 figure is topher's and CI's, not mine.

jarvis merged commit 7c7dab3898 into next

2026-08-18 05:56:59 +00:00

jarvis referenced this pull request

2026-08-18 07:44:50 +00:00

Release integrity workstream for alpha 0.0.50 (RI-050) #1275

fargo referenced this pull request

2026-08-18 15:58:45 +00:00

docs(ri-050): forge fail-closed docs + TASKS status catch-up (#1275) #1299

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#1300