fix(skip): centralize final static-layout skip rejection

[NathanDrake2406]

What this changes

• centralizes the final static-layout skip rejection in getStaticLayoutObservationSkipRejection()
• makes isAppLayoutObservationUnsafeForStaticReuse() delegate to that same helper
• adds SKIP_LAYOUT_DYNAMIC_USAGE_OBSERVED and includes dynamicUsageObserved in the rejection trace fields
• keeps the render-layer gate small: read the observation, ask the shared helper, and fall back to render-and-send when it rejects

Why

#1437 and #1717 already provide the primary correctness path: runtime-unsafe layouts are classified dynamic before they enter the static skip set, and the cache cross-check still protects against payload-divergent reuse.

This PR keeps the last transport-boundary invariant. Immediately before omitting a layout from the RSC payload, the server re-checks that the observation is still safe. That keeps the omission boundary conservative even if classification logic later drifts or misses a signal.

Tests

• table-driven helper coverage for every unsafe observation shape, especially dynamicUsageObserved
• focused render-level regression proving the layout stays in the payload when final verification rejects dynamicUsageObserved

Validation

• vp test run tests/app-layout-param-observation.test.ts tests/app-page-render.test.ts
• vp check packages/vinext/src/server/app-layout-param-observation.ts packages/vinext/src/server/app-page-render.ts packages/vinext/src/server/client-reuse-manifest.ts tests/app-layout-param-observation.test.ts tests/app-page-render.test.ts

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1722

commit: 48e5ca3

[james-elicx]

/bigbonk review

[ask-bonk]

Review: centralize final static-layout skip rejection

LGTM overall. This is a clean, well-tested refactor. I verified behavioral parity, types/lint, and the affected tests locally.

What I checked

• Behavioral parity preserved. The 11 conditions in the old isAppLayoutObservationUnsafeForStaticReuse() boolean chain map 1:1 onto STATIC_LAYOUT_OBSERVATION_SKIP_RULES (incomplete, paramScope, observed, dynamicUsage, requestApis, finiteRevalidate, cacheLife, unstableCaches, cacheTags, cacheableFetch, dynamicFetch). The delegating boolean now returns getStaticLayoutObservationSkipRejection(observation) !== null, which is equivalent.
• Rejection shape matches the existing seam. The manually-constructed object in rejectStaticLayoutObservation() matches SkipCacheCrossCheckRejected (kind: "rejected", rejection: { code, entryId, fields }, skipDisposition), consistent with rejectSkipCacheCrossCheck() in skip-cache-proof.ts.
• Defense-in-depth rationale is real. The primary path classifies unsafe layouts as "d" via isLayoutObservationDynamic in app-page-execution.ts:422/454, so they never enter the "s" skip set. The new render-boundary check is a redundant last invariant, exactly as the PR describes.
• Test design is good. The new regression test stubs isLayoutObservationDynamic: () => false to simulate classification drift while the probe records dynamicUsageObserved: true, proving the transport-boundary gate independently rejects. The table-driven helper test asserts a distinct code per unsafe shape.
• vp test run tests/app-layout-param-observation.test.ts tests/app-page-render.test.ts → 45 passed.
• vp check on the three changed source files → clean (format + lint + types).
• New SKIP_LAYOUT_DYNAMIC_USAGE_OBSERVED code is added to the union and is now actually produced.

Minor / non-blocking

• The PR description lists tests/app-page-dispatch.test.ts under Tests/Validation ("drops the broad contract-only dispatch pile"), but that file is not in the changeset. Either the dispatch test edits were dropped from this PR or the description is stale — worth reconciling so the PR body matches reality.
• See the inline note on the unreachable-looking !observation branch.

[ask-bonk]

Minor: getLayoutObservation is typed to always return an AppLayoutParamAccessObservation (never null/undefined) — for an unknown layout it returns a default object with completeness: "unknown", which would itself yield SKIP_LAYOUT_PARAMS_OBSERVATION_INCOMPLETE. So this !observation branch only fires when layoutParamAccess itself is undefined (the optional-chain short-circuit). That's a legitimate case (no tracker → reject incomplete), but the dual meaning is a little subtle. Consider a clarifying comment that this guards the missing-tracker case rather than a missing observation, since the tracker never returns a missing observation.

[ask-bonk]

Nice that the rule codes are constrained to Extract<ClientReuseManifestRejectionCode, "SKIP_LAYOUT_${string}"> via the satisfies clause — adding a typo'd code or one outside the SKIP_LAYOUT_ family will fail to compile. One gap: there's no compile-time guarantee that every observation field that can make a layout unsafe has a corresponding rule (the list is hand-maintained). If a new unsafe field is added to AppLayoutParamAccessObservation later, nothing forces a matching rule here. Not actionable for this PR, but worth a follow-up note or test if the observation shape grows.

[ask-bonk]

Review posted. Summary of my findings on PR #1722:

Verdict: LGTM — clean, well-tested defense-in-depth refactor.

Verified:

• Behavioral parity — all 11 conditions from the old boolean chain map 1:1 onto the new STATIC_LAYOUT_OBSERVATION_SKIP_RULES; the boolean now delegates to getStaticLayoutObservationSkipRejection(...) !== null.
• Rejection shape matches the existing SkipCacheCrossCheckRejected seam used by rejectSkipCacheCrossCheck.
• Defense-in-depth is genuine — the primary path already classifies unsafe layouts as "d" in app-page-execution.ts, so they never reach the skip set; the new render-boundary check is a redundant last invariant exactly as described.
• Test design — the regression test stubs isLayoutObservationDynamic: () => false to simulate classification drift while the probe records dynamicUsageObserved: true, proving the boundary catches it independently.
• vp test run → 45 passed; vp check → clean on all three source files.

Minor / non-blocking notes posted:

1. PR description lists tests/app-page-dispatch.test.ts but it's not in the changeset — stale description or dropped edits.
2. The !observation branch only fires for a missing tracker (the typed getter never returns missing) — suggested a clarifying comment.
3. No compile-time guarantee that every unsafe observation field has a matching rule — possible follow-up if the observation shape grows.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Re-review: centralize final static-layout skip rejection

LGTM. Re-reviewed at 48e5ca3. The two minor notes from the prior /bigbonk review have been addressed and the PR is in good shape.

Verified at this commit

• Behavioral parity preserved. The 11 conditions in the old isAppLayoutObservationUnsafeForStaticReuse() boolean chain map 1:1 onto STATIC_LAYOUT_OBSERVATION_SKIP_RULES. The boolean now correctly delegates via getStaticLayoutObservationSkipRejection(observation) !== null (app-layout-param-observation.ts:122-126).
• Type-safe rule codes. StaticLayoutObservationSkipCode = Extract<ClientReuseManifestRejectionCode, "SKIP_LAYOUT_${string}"> plus the satisfies clause means a typo'd or out-of-family code fails to compile.
• Rejection shape matches the existing seam. rejectStaticLayoutObservation() produces { kind: "rejected", rejection: { code, entryId, fields }, skipDisposition }, consistent with crossCheckClientReuseManifestEntryWithCache's return type and ClientReuseManifestEntryRejection (client-reuse-manifest.ts:145-148).
• Defense-in-depth is genuine. The primary classification path already marks unsafe layouts "d" so they never enter the "s" skip set; the new render-boundary check at app-page-render.ts:396-402 is a redundant last invariant, as described.
• !observation comment added. Commit 48e5ca3 adds the clarifying comment (app-page-render.ts:309-312) that the prior review requested — it correctly documents that the optional-chain guards the missing-tracker case, not a missing observation.
• PR description reconciled. The Tests/Validation section now lists only app-layout-param-observation.test.ts and app-page-render.test.ts, matching the changeset (the stale app-page-dispatch.test.ts reference is gone).
• New SKIP_LAYOUT_DYNAMIC_USAGE_OBSERVED is added to the union and is now actually produced and asserted.

Local validation

• vp test run tests/app-layout-param-observation.test.ts tests/app-page-render.test.ts -> 45 passed.
• vp check on the three changed source files -> clean (format + lint + types).

Non-blocking follow-up (unchanged from prior review)

• There is still no compile-time guarantee that every unsafe field on AppLayoutParamAccessObservation has a matching rule — the list is hand-maintained. If the observation shape grows, a new unsafe field could silently lack a rule. Worth a follow-up exhaustiveness test if the shape expands, but not actionable here.

[ask-bonk]

Re-review posted on PR #1722.

Verdict: LGTM (Approved) — defense-in-depth refactor is clean, well-tested, and the prior review's two minor notes have been addressed.

What I verified at 48e5ca3:

• Behavioral parity — all 11 conditions in the old boolean chain map 1:1 onto STATIC_LAYOUT_OBSERVATION_SKIP_RULES; the boolean delegates correctly.
• Type safety — Extract<..., "SKIP_LAYOUT_${string}"> + satisfies prevents typo'd/out-of-family codes from compiling.
• Rejection shape matches the existing crossCheckClientReuseManifestEntryWithCache seam.
• Defense-in-depth is real — primary path already classifies unsafe layouts "d"; this is a redundant last invariant.
• Both prior notes resolved — commit 48e5ca3 added the clarifying !observation comment, and the PR description no longer references the dropped app-page-dispatch.test.ts.
• Local validation — 45 tests passed; vp check clean on all three source files.

One non-blocking follow-up carried over: no compile-time exhaustiveness guarantee that every unsafe observation field has a matching rule. Not actionable for this PR.

github run