Skip to content

feat(skip): add static layout reuse proof model#1674

Merged
james-elicx merged 9 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/726-skip-02-proof-model
May 29, 2026
Merged

feat(skip): add static layout reuse proof model#1674
james-elicx merged 9 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/726-skip-02-proof-model

Conversation

@NathanDrake2406
Copy link
Copy Markdown
Contributor

What this changes

Adds the proof and planner vocabulary that decides whether a client reuse manifest entry for a retained static layout is reusable. Nothing renders the decision yet, so no RSC payload is omitted and no response cache control changes.

Why

Skip transport correctness depends on the server proving the retained client layout is indistinguishable from the target server layout. That proof has to live next to the cache model so the planner can reject anything that does not match exact artifact compatibility, payload hash, route id, or invalidation. Landing the planner without the manifest plumbing or encoder change keeps this slice reviewable in isolation.

Approach

  • skip-cache-proof: static layout reuse planner with per-entry budget, exact artifact-compatibility check, payload hash, route id, and invalidation proof. Preserves rejection traces and uses canonical parsed entry ids.
  • static-layout-client-reuse-proof: payload hash, route id, and artifact compatibility helpers used by the planner.
  • client-reuse-manifest: extends the rejection code union with the layout-safety rejection codes the planner emits, and adds the enabled skip disposition variant so future slices can carry an authorized result without retyping the union.
  • Non-goals: no manifest plumbing into request handling, no observation tracker, no encoder change.

Validation

  • vp check: format, lint, type check passed.
  • vp test run tests/skip-cache-proof.test.ts tests/client-reuse-manifest.test.ts: 2 files, 38 tests passed.

Risks / follow-ups

Refs #726

Copilot AI review requested due to automatic review settings May 28, 2026 16:56
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 28, 2026

Open in StackBlitz

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

commit: 9b06fe3

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a static-layout skip transport plan layer on top of the client-reuse manifest cross-check, enabling verified public layout entries to opt into a skipStaticLayout disposition while preserving render-and-send fallback paths for oversized, over-budget, or rejected manifests.

Changes:

  • Introduce createClientReuseSkipTransportPlan and SKIP_STATIC_LAYOUT_VERIFIED disposition, extending crossCheckClientReuseManifestEntryWithCache to emit skip dispositions on exact compatibility matches.
  • Extract static-layout client-reuse proof helpers (route id, payload hash, layout-scoped compatibility) into a dedicated module and re-export from skip-cache-proof.ts.
  • Expand ClientReuseManifestRejectionCode with layout-observation codes and broaden ClientReuseManifestSkipDisposition to the new discriminated union; add tests covering the planner.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/vinext/src/server/skip-cache-proof.ts Adds skip transport planner, static-layout disposition, exact-compatibility check, and re-exports proof helpers.
packages/vinext/src/server/static-layout-client-reuse-proof.ts New module with proof field list, payload hash, route id, and layout-scoped compatibility helpers.
packages/vinext/src/server/client-reuse-manifest.ts Adds verification entry-budget constant, layout-observation rejection codes, and union skip disposition type.
tests/skip-cache-proof.test.ts Covers planner happy path, rejections, oversized/over-budget manifests, invalid budgets, and verified-id trust.
knip.ts Whitelists the new proof + planner modules pending later runtime wiring.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +305 to +313
if (maxEntriesToVerify !== undefined && manifest.manifest.entries.length > maxEntriesToVerify) {
return createRenderAndSendPlan({
entryRejections: manifest.entryRejections,
manifestRejection: createEntryCountExceededRejection(
manifest.manifest.entries.length,
maxEntriesToVerify,
),
});
}
Comment on lines +82 to +83
export const STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET =
CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET;
Comment on lines +266 to +271
const skipDisposition = isExactArtifactCompatibility(
input.artifact.compatibility,
entry.artifactCompatibility,
)
? createStaticLayoutSkipDisposition([entry.id])
: createDisabledSkipDisposition();
Comment on lines +30 to +33
const artifactCompatibility: Record<string, string | number | null> = {};
for (const field of ARTIFACT_COMPATIBILITY_PROOF_FIELDS) {
artifactCompatibility[field] = input.artifactCompatibility[field];
}
Comment on lines +98 to +107
| "SKIP_LAYOUT_CACHE_LIFE_OBSERVED"
| "SKIP_LAYOUT_CACHE_TAGS_OBSERVED"
| "SKIP_LAYOUT_CACHEABLE_FETCHES_OBSERVED"
| "SKIP_LAYOUT_DYNAMIC_FETCHES_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVATION_INCOMPLETE"
| "SKIP_LAYOUT_PARAMS_PRESENT"
| "SKIP_LAYOUT_REVALIDATE_PRESENT"
| "SKIP_LAYOUT_REQUEST_API_OBSERVED"
| "SKIP_LAYOUT_UNSTABLE_CACHE_OBSERVED"
Comment on lines +264 to +267
it("falls back without verifier work for oversized manifests", () => {
const manifest = parseClientReuseManifestHeader('{"entries":[]}', {
limits: { ...DEFAULT_CLIENT_REUSE_MANIFEST_LIMITS, maxManifestBytes: 8 },
});
Add the proof/planner vocabulary that decides whether a verified
static layout entry from a client reuse manifest is reusable.
Nothing renders this decision yet, so no payload is omitted.

- skip-cache-proof: static layout reuse planner with per-entry budget,
  exact artifact-compatibility check, payload hash, route id, and
  invalidation proof. Preserves rejection traces. Returns the existing
  disabled disposition when not authorized.
- static-layout-client-reuse-proof: static layout payload hash, route
  id, and artifact compatibility helpers used by the planner.
- client-reuse-manifest: extends the rejection code union with the
  layout-safety rejection codes the planner emits, and adds the
  enabled skip disposition variant so future slices can carry an
  authorized result without retyping the union.

This slice is the proof vocabulary only. The planner is callable but
not invoked from the render path; encoder behavior is unchanged.
@NathanDrake2406 NathanDrake2406 force-pushed the nathan/726-skip-02-proof-model branch from e4b67bf to 1de3566 Compare May 28, 2026 17:01
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thorough review of the static layout reuse proof model. The design is clean — the planner is well-separated from the cross-check logic, the trust boundary between verifier-provided skip IDs and the planner's own entry IDs is correctly maintained, and the test coverage is good. A few substantive issues below, one of which is a correctness concern for the entry-budget check.

Copilot already flagged some of these; where I agree, I've elaborated on the fix rather than repeating the observation.

throw new RangeError("maxEntriesToVerify must be a non-negative safe integer");
}

if (maxEntriesToVerify !== undefined && manifest.manifest.entries.length > maxEntriesToVerify) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Budget check only counts parsed entries, not total wire input size.

manifest.manifest.entries contains only entries that survived parse-time validation (public, known kind, valid schema, etc.). manifest.entryRejections holds the rest. An attacker can send 100 entries where 99 are private (rejected at parse time) and 1 is valid — the budget check sees entries.length === 1 and allows verification, even though the server already did O(100) work in the parser.

The budget is meant to bound total server-side work per manifest. The comparison should use the original wire entry count (parsed + rejected), or at minimum manifest.manifest.entries.length + manifest.entryRejections.length.

Suggested change
if (maxEntriesToVerify !== undefined && manifest.manifest.entries.length > maxEntriesToVerify) {
if (maxEntriesToVerify !== undefined && manifest.manifest.entries.length + manifest.entryRejections.length > maxEntriesToVerify) {

Note: the fields in the rejection should also reflect the total count so the trace is accurate:

manifestRejection: createEntryCountExceededRejection(
  manifest.manifest.entries.length + manifest.entryRejections.length,
  maxEntriesToVerify,
),


const invalidationRejection = crossCheckInvalidationProof(entry, input.artifact.invalidation);
if (invalidationRejection) return invalidationRejection;
const skipDisposition = isExactArtifactCompatibility(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triple-checking artifact compatibility is redundant and fragile.

By this point we have already:

  1. collectArtifactCompatibilityProofMismatches(artifact.compatibility, proof.candidateArtifactCompatibility) — field-level proof check (line 218)
  2. evaluateArtifactCompatibility(artifact.compatibility, entry.artifactCompatibility, ...) — compatibility-map-aware evaluation (line 228)

Now isExactArtifactCompatibility(artifact.compatibility, entry.artifactCompatibility) does a third field-level walk over the same pair. The semantic gap is that check (2) can return compatible via a compatibility map even when fields differ (canary/rollback scenario), while this check requires exact field equality. That's the right policy for skip — but the triple walk is easy to get out of sync.

Consider deriving skip eligibility from the evaluation result instead. For example, evaluateArtifactCompatibility could return a kind: "exact" discriminator when all fields match without needing the compatibility map, and you could check that here. Alternatively, at least add a comment explaining why the third walk is intentional and when it differs from check (2).

}>;

export const STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET =
CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This re-export creates two names (STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET and CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET) for the same value, but the planner doesn't use either as a default — callers must pass maxEntriesToVerify explicitly, and no test uses this constant.

If the intent is for callers to pass this as maxEntriesToVerify, consider wiring it in as the default inside createClientReuseSkipTransportPlan so the budget is enforced even when callers forget:

const maxEntriesToVerify = input.maxEntriesToVerify ?? STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET;

Or if the intent is for the caller to always be explicit, drop the re-export so there aren't two names for the same thing.

}

return createClientReusePayloadHash(
JSON.stringify({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hash stability depends on field list ordering.

ARTIFACT_COMPATIBILITY_PROOF_FIELDS drives the iteration that populates the artifactCompatibility object before JSON.stringify. While V8 preserves insertion order for non-integer string keys (and the iteration order is deterministic within a single engine run), JSON.stringify key order is formally implementation-defined. If ARTIFACT_COMPATIBILITY_PROOF_FIELDS is reordered in a future commit, every hash silently changes.

Since this is a cross-process/cross-deployment hash used for proof identity, consider serializing as a sorted [key, value] array instead:

const pairs = ARTIFACT_COMPATIBILITY_PROOF_FIELDS
  .map(f => [f, input.artifactCompatibility[f]] as const)
  .sort(([a], [b]) => a < b ? -1 : a > b ? 1 : 0);
return createClientReusePayloadHash(
  JSON.stringify({ pairs, layoutId: input.layoutId, ... }),
);

Alternatively, add a comment + test asserting the field order is load-bearing and must not be reordered.

Comment on lines +98 to +107
| "SKIP_LAYOUT_CACHE_LIFE_OBSERVED"
| "SKIP_LAYOUT_CACHE_TAGS_OBSERVED"
| "SKIP_LAYOUT_CACHEABLE_FETCHES_OBSERVED"
| "SKIP_LAYOUT_DYNAMIC_FETCHES_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVATION_INCOMPLETE"
| "SKIP_LAYOUT_PARAMS_PRESENT"
| "SKIP_LAYOUT_REVALIDATE_PRESENT"
| "SKIP_LAYOUT_REQUEST_API_OBSERVED"
| "SKIP_LAYOUT_UNSTABLE_CACHE_OBSERVED"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ten new SKIP_LAYOUT_* rejection codes are added to the union, but nothing in this PR (or the planner) emits or consumes any of them. The planner only produces SKIP_ENTRY_COUNT_EXCEEDED, SKIP_MANIFEST_TOO_LARGE, and the entry-level cross-check codes already defined.

Adding unused union members bakes them into the type surface before the producing code exists. If the observation tracker in slice 3 needs different names, these become dead variants. Consider landing these codes alongside the producers that emit them, or at minimum add a doc comment here noting they are forward declarations for the observation tracker.

},
skippedEntryIds: [],
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test for kind: "absent" manifest.

The planner has three entry branches for manifest.kind: "absent" (line 290), "rejected" (line 293), and "parsed". The "rejected" branch is covered by the oversized-manifest test, and "parsed" by most other tests, but the "absent" branch (when parseClientReuseManifestHeader(null) returns { kind: "absent" }) is never exercised.

Consider adding a test case:

it("falls back without verifier work for absent manifests", () => {
  const manifest = parseClientReuseManifestHeader(null);
  let verifierCalled = false;
  const plan = createClientReuseSkipTransportPlan({
    manifest,
    verifyEntry() {
      verifierCalled = true;
      throw new Error("absent manifests must not enter skip verification");
    },
  });
  expect(verifierCalled).toBe(false);
  expect(plan).toEqual({
    kind: "renderAndSend",
    entryRejections: [],
    skipDisposition: { code: "SKIP_MODEL_DISABLED", enabled: false, mode: "renderAndSend" },
    skippedEntryIds: [],
  });
});

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 28, 2026

Review posted. Here's a summary of the 6 comments I left:

  1. Entry budget counts only parsed entries (correctness bug)skip-cache-proof.ts:305. The budget check uses manifest.manifest.entries.length which excludes parse-rejected entries. An attacker can pad a manifest with 99 private entries (rejected at parse) + 1 valid entry, slipping under the budget while the server already did O(100) parse work. Should use entries.length + entryRejections.length.

  2. Triple artifact compatibility walk is redundantskip-cache-proof.ts:266. By line 266, artifact compatibility has already been checked twice (proof mismatch + evaluation). The third isExactArtifactCompatibility walk is intentional (exact-only for skip, unlike compatibility-map-aware evaluation), but fragile. Suggested deriving skip eligibility from the evaluation result or documenting why the third walk is necessary.

  3. Re-exported budget constant not wired as defaultskip-cache-proof.ts:83. STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET is exported but never used as a default in the planner. Should either wire it as the default for maxEntriesToVerify or drop the re-export.

  4. Hash stability depends on field list orderingstatic-layout-client-reuse-proof.ts:36. JSON.stringify key order is implementation-defined. If ARTIFACT_COMPATIBILITY_PROOF_FIELDS is reordered, hashes silently change. Suggested serializing as sorted [key, value] pairs or adding a test/comment asserting order is load-bearing.

  5. Ten unused rejection codes added prematurelyclient-reuse-manifest.ts:98-107. The SKIP_LAYOUT_* codes have no producers in this PR. Suggested landing them with the observation tracker that emits them, or documenting them as forward declarations.

  6. Missing test for kind: "absent" manifest branchtests/skip-cache-proof.test.ts:296. The absent branch in the planner is never exercised. Provided a concrete test case.

github run

@NathanDrake2406 NathanDrake2406 requested a review from Copilot May 29, 2026 05:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment thread packages/vinext/src/server/skip-cache-proof.ts
Comment thread packages/vinext/src/server/skip-cache-proof.ts
Comment thread packages/vinext/src/server/static-layout-client-reuse-proof.ts Outdated
Comment thread packages/vinext/src/server/client-reuse-manifest.ts
Comment thread tests/skip-cache-proof.test.ts
@NathanDrake2406 NathanDrake2406 requested a review from Copilot May 29, 2026 07:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment on lines +98 to +111
// Forward declarations — emitted by the render-observation tracker in a
// later slice. The planner never produces them, but the rejection code
// union must carry them so the tracker's entry rejection is assignable to
// ClientReuseManifestRejectionCode without a cast.
| "SKIP_LAYOUT_CACHE_LIFE_OBSERVED"
| "SKIP_LAYOUT_CACHE_TAGS_OBSERVED"
| "SKIP_LAYOUT_CACHEABLE_FETCHES_OBSERVED"
| "SKIP_LAYOUT_DYNAMIC_FETCHES_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVED"
| "SKIP_LAYOUT_PARAMS_OBSERVATION_INCOMPLETE"
| "SKIP_LAYOUT_PARAMS_PRESENT"
| "SKIP_LAYOUT_REVALIDATE_PRESENT"
| "SKIP_LAYOUT_REQUEST_API_OBSERVED"
| "SKIP_LAYOUT_UNSTABLE_CACHE_OBSERVED"
Comment on lines +135 to +143
function isExactArtifactCompatibility(
artifactCompatibility: ArtifactCompatibilityEnvelope,
entryCompatibility: ArtifactCompatibilityEnvelope,
): boolean {
return (
collectArtifactCompatibilityProofMismatches(artifactCompatibility, entryCompatibility)
.length === 0
);
}
Comment on lines +316 to +325
const totalWireEntries = manifest.manifest.entries.length + manifest.entryRejections.length;
if (totalWireEntries > maxEntriesToVerify) {
return createRenderAndSendPlan({
entryRejections: manifest.entryRejections,
manifestRejection: createVerificationBudgetExceededRejection(
totalWireEntries,
maxEntriesToVerify,
),
});
}
Comment on lines +12 to +21
export const ARTIFACT_COMPATIBILITY_PROOF_FIELDS: readonly (keyof ArtifactCompatibilityEnvelope)[] =
[
"schemaVersion",
"graphVersion",
"deploymentVersion",
"appElementsSchemaVersion",
"rscPayloadSchemaVersion",
"rootBoundaryId",
"renderEpoch",
];
Comment on lines +340 to +350
if (verification.kind === "verified" && verification.entryId !== entry.id) {
entryRejections.push({
code: "SKIP_CACHE_ENTRY_ID_MISMATCH",
entryId: entry.id,
fields: {
verifierEntryId: verification.entryId,
manifestEntryId: entry.id,
},
});
continue;
}
…axWireEntriesToVerify, short-circuit exact check
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good follow-up addressing the first-round review comments — budget now counts wire entries, skipIneligibleEntryIds provides observability for the bridged-entry path, SKIP_VERIFICATION_BUDGET_EXCEEDED is properly distinguished from parse-time rejections, ARTIFACT_COMPATIBILITY_PROOF_FIELDS moved to artifact-compatibility.ts, and the hash serializes pairs directly. A few remaining items below, one correctness issue with a misleading comment and one defensive-coding gap.

// ARTIFACT_COMPATIBILITY_PROOF_FIELDS order is load-bearing for hash
// determinism. The artifact compatibility section is serialized as a sorted
// array of [field, value] pairs — no object key-order dependency — so the
// hash is invariant across runtimes and immune to field-list reorderings.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment says "immune to field-list reorderings" — it isn't.

createCanonicalProofPairs iterates ARTIFACT_COMPATIBILITY_PROOF_FIELDS in array order and produces [[field0, val0], [field1, val1], ...]. If the array is reordered, the pairs array in the JSON changes, and the hash changes silently. The pairs aren't sorted — they just follow the field list's declaration order.

The comment on line 23 in artifact-compatibility.ts ("Order is load-bearing") gets this right. This comment contradicts it.

Suggested change
// hash is invariant across runtimes and immune to field-list reorderings.
// array of [field, value] pairs — no object key-order dependency — so the
// hash is invariant across runtimes. The pair order follows
// ARTIFACT_COMPATIBILITY_PROOF_FIELDS declaration order and is load-bearing;
// reordering the field list silently changes every hash.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The comment now states that pair order follows ARTIFACT_COMPATIBILITY_PROOF_FIELDS declaration order and is load-bearing, matching the comment in artifact-compatibility.ts.

// The planner must not trust a verifier that returns a verified result
// for a different entry than the one it was given. This is an internal
// invariant, not a hostile-client defence.
if (verification.kind === "verified" && verification.entryId !== entry.id) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verification.kind === "verified" guard is redundant and masks future variant drift.

After the if (verification.kind === "rejected") { ... continue; } on line 333, only "verified" remains (the union is two-variant). The explicit verification.kind === "verified" check on this line is harmless today, but if a third kind is ever added to SkipCacheCrossCheckResult, control will silently fall through to the skipDisposition.enabled access below without narrowing. Consider an else + assertNever pattern:

if (verification.kind === "rejected") {
  entryRejections.push(verification.rejection);
  continue;
}
if (verification.entryId !== entry.id) {
  // ... mismatch rejection ...
  continue;
}

With "rejected" already handled and continued, TypeScript narrows verification to SkipCacheCrossCheckVerified without the redundant kind check. If a third variant is added later, the narrowing will break at compile time, which is the desired behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Removed the redundant guard; TypeScript now narrows automatically after the rejected branch continues.

verifyEntry: (entry: ClientReuseManifestEntry) => SkipCacheCrossCheckResult;
}>;

export const STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the rename to maxWireEntriesToVerify and the default on line 312, this re-export is now wired as the fallback. The dual-name concern from the prior review is resolved — but the names have drifted further apart (STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET vs CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET). The "STATIC_LAYOUT" prefix suggests this is layout-specific, but it's just an alias for the generic manifest budget.

Minor: consider dropping this alias and importing CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET directly in callers, since the indirection adds no value now that the default is wired inline on line 312.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Dropped the STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET alias; the planner now imports CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET directly and uses it as the default fallback.

return mismatchedFields;
}

function isExactArtifactCompatibility(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isExactArtifactCompatibility is semantically collectArtifactCompatibilityProofMismatches(...).length === 0 with early exit. That's fine for performance, but the two functions iterate the same field list with the same comparison and could drift if one is updated without the other. Consider implementing isExactArtifactCompatibility in terms of the collect function (the array is 7 elements — the allocation is negligible on a verification path), or at least add a cross-reference comment so future maintainers know to update both.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. isExactArtifactCompatibility now delegates to collectArtifactCompatibilityProofMismatches(...).length === 0. The array is 7 elements and the allocation is negligible on a verification path; the drift risk is eliminated.

return {
kind: "renderAndSend",
entryRejections: options.entryRejections ?? [],
...(options.manifestRejection ? { manifestRejection: options.manifestRejection } : {}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the conditional spread ...(options.manifestRejection ? { manifestRejection: options.manifestRejection } : {}) exists to omit the key entirely when absent, which is correct for the renderAndSend variant since manifestRejection? is optional. But the toEqual assertion in the oversized-manifest test (line 453) asserts the key is present. If the intent is that absent manifests genuinely omit the key (not just set it to undefined), the test coverage is correct. Just noting that the conditional spread and the optional type are both necessary for this to work — if someone changes it to manifestRejection: options.manifestRejection ?? undefined, the toEqual tests would break.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. The conditional spread is intentional: the optional key must be genuinely absent (not undefined) for the renderAndSend variant. The test coverage in the oversized-manifest branch asserts the key is present when a rejection exists, and absent in the no-rejection branches. If someone changes the spread to unconditional, the toEqual assertions will fail — which is the desired guard.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 29, 2026

Review posted. Here's a summary of the 5 comments:

  1. Misleading comment about hash reorder immunity (static-layout-client-reuse-proof.ts:36) — The comment claims the hash is "immune to field-list reorderings," but createCanonicalProofPairs iterates ARTIFACT_COMPATIBILITY_PROOF_FIELDS in declaration order without sorting. Reordering the field list silently changes every hash. The comment in artifact-compatibility.ts correctly says "Order is load-bearing" — this comment contradicts it. Suggestion provided.

  2. Redundant verification.kind === "verified" check masks future variant drift (skip-cache-proof.ts:341) — After the "rejected" branch continues, only "verified" remains in the union. The explicit kind check is redundant and would silently fall through if a third variant is ever added. Suggested removing the redundant guard so TypeScript's narrowing catches new variants at compile time.

  3. STATIC_LAYOUT_SKIP_VERIFICATION_ENTRY_BUDGET re-export adds no value (skip-cache-proof.ts:83) — Now that the budget is wired as the default on line 312, this alias of CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET with a different prefix ("STATIC_LAYOUT" vs "CLIENT_REUSE_MANIFEST") is just indirection. Consider dropping it and importing the original directly.

  4. isExactArtifactCompatibility duplicates collectArtifactCompatibilityProofMismatches logic (skip-cache-proof.ts:134) — Both iterate the same field list with the same comparison. The early-exit version is fine for performance (7 fields), but they could drift. Suggested implementing one in terms of the other or adding a cross-reference comment.

  5. Conditional spread for manifestRejection is subtly load-bearing (skip-cache-proof.ts:158) — Noting that the conditional spread and toEqual test assertions depend on the key being genuinely absent (not undefined). Not a bug, just flagging the coupling.

github run

…dant guard, drop budget alias, dedupe exact-compatibility check
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shape after two rounds of review. The budget fix, skipIneligibleEntryIds observability, distinct SKIP_VERIFICATION_BUDGET_EXCEEDED code, field list homing in artifact-compatibility.ts, and pair-array serialization all landed cleanly. The overall design — planner separated from cross-check, trust boundary on verifier entry IDs, exact-equality gate for skip — is solid.

Three items below, none blocking.


const maxWireEntriesToVerify =
input.maxWireEntriesToVerify ?? CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET;
if (!Number.isSafeInteger(maxWireEntriesToVerify) || maxWireEntriesToVerify < 0) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxWireEntriesToVerify: 0 silently accepts empty manifests instead of rejecting.

The validation allows 0 (non-negative safe integer), and the budget check is totalWireEntries > 0, which is false when the manifest has 0 entries. So the loop runs zero iterations and the planner returns renderAndSend — which is fine functionally, but a caller passing 0 probably intended "reject all manifests" rather than "allow empty ones through".

Not a correctness issue today (an empty parsed manifest always produces renderAndSend with no skipped entries), but worth a test to pin the behavior so it doesn't surprise future callers:

it("accepts empty manifests with budget 0 (no entries to verify)", () => { ... });

Or, if the intent is that 0 means "no manifests allowed", change > to >= or require > 0.

"rootBoundaryId",
"renderEpoch",
];
type ClientReuseSkipTransportPlan =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: ClientReuseSkipTransportPlan, CreateClientReuseSkipTransportPlanInput, and SkipCacheCrossCheckResult are all unexported. That's fine for this slice since the planner isn't invoked yet, but the next slice that wires it into request handling will need to import the plan type. Worth exporting them now (or at least marking them with a comment that they're intentionally module-private until slice 3) so the next PR doesn't have to touch this file just to add export.

// ARTIFACT_COMPATIBILITY_PROOF_FIELDS order is load-bearing for hash
// determinism. The artifact compatibility section is serialized as an
// array of [field, value] pairs in declaration order — no object key-order
// dependency — so the hash is invariant across runtimes. Reordering the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 33-37 says the hash is "invariant across runtimes" and then immediately says "Reordering the field list silently changes every hash." These are contradictory in the same sentence. The hash is invariant across JS runtimes (no object key-order dependency), but variant across field-list orderings. The two clauses need a clearer break.

Suggestion:

Suggested change
// dependency — so the hash is invariant across runtimes. Reordering the
// dependency — so the hash is invariant across JavaScript runtimes.
// However, the pair order follows ARTIFACT_COMPATIBILITY_PROOF_FIELDS
// declaration order and is load-bearing — reordering the field list
// silently changes every hash.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 29, 2026

Review posted. Here's a summary of the 3 comments:

  1. maxWireEntriesToVerify: 0 edge case (skip-cache-proof.ts:308) — The validation allows 0 and the > comparison means an empty manifest passes through. A caller passing 0 probably intended "reject all manifests," not "allow empty ones." Worth pinning with a test or tightening the comparison.

  2. Unexported plan types (skip-cache-proof.ts:60) — ClientReuseSkipTransportPlan, CreateClientReuseSkipTransportPlanInput, and SkipCacheCrossCheckResult are all module-private. The next slice that wires the planner into request handling will need them. Worth exporting now or adding a comment noting they're intentionally private until slice 3.

  3. Contradictory comment about hash invariance (static-layout-client-reuse-proof.ts:36) — The comment says the hash is "invariant across runtimes" then immediately says "reordering the field list silently changes every hash." These need a clearer break — the hash is invariant across JS runtimes but variant across field-list orderings. Provided a suggestion.

None blocking — the prior review rounds caught the substantive issues and they've been addressed well.

github run

@james-elicx james-elicx merged commit e8c188c into cloudflare:main May 29, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants