From 24fd37615355ccbba9f74881ca514484a48f7ab1 Mon Sep 17 00:00:00 2001 From: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:19:41 -0700 Subject: [PATCH 1/2] refactor(review): encode resource measurement invariants in the descriptor type Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com> --- .../review-resource-measurement-type.md | 2 + packages/hunk/src/app/review/resourceStore.ts | 3 +- .../hunk/src/core/review/resources.test.ts | 17 +++++++ packages/hunk/src/core/review/resources.ts | 51 +++++++++++-------- packages/hunk/src/session/broker/state.ts | 6 +-- 5 files changed, 52 insertions(+), 27 deletions(-) create mode 100644 .changeset/review-resource-measurement-type.md diff --git a/.changeset/review-resource-measurement-type.md b/.changeset/review-resource-measurement-type.md new file mode 100644 index 000000000..a845151cc --- /dev/null +++ b/.changeset/review-resource-measurement-type.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/hunk/src/app/review/resourceStore.ts b/packages/hunk/src/app/review/resourceStore.ts index 423e5a3d5..bc7acd908 100644 --- a/packages/hunk/src/app/review/resourceStore.ts +++ b/packages/hunk/src/app/review/resourceStore.ts @@ -305,8 +305,7 @@ export class ReviewResourceStore { const digest = this.digest(bytes); if ( isMaterializedReviewResource(descriptor) && - (descriptor.byteLength !== bytes.byteLength || - !reviewDigestsEqual(descriptor.digest!, digest)) + (descriptor.byteLength !== bytes.byteLength || !reviewDigestsEqual(descriptor.digest, digest)) ) { return reviewResourceFailure( "resource-integrity", diff --git a/packages/hunk/src/core/review/resources.test.ts b/packages/hunk/src/core/review/resources.test.ts index 66b394e4a..d96522ad7 100644 --- a/packages/hunk/src/core/review/resources.test.ts +++ b/packages/hunk/src/core/review/resources.test.ts @@ -6,6 +6,7 @@ import { REVIEW_PATCH_CONTENT_TYPE, REVIEW_RESOURCE_CHUNK_BYTES, reviewResourceId, + type ReviewResourceDescriptorV1, } from "./resources"; const fileKey = "file:0123456789abcdef"; @@ -50,12 +51,28 @@ describe("resource materialization state", () => { test("is only complete when both measurements are present", () => { expect(isMaterializedReviewResource(descriptor)).toBe(false); + // @ts-expect-error A length without a digest is not a descriptor the type admits. expect(isMaterializedReviewResource({ ...descriptor, byteLength: 10 })).toBe(false); + // @ts-expect-error A digest without a length is not a descriptor the type admits. expect(isMaterializedReviewResource({ ...descriptor, digest: "a".repeat(64) })).toBe(false); expect( isMaterializedReviewResource({ ...descriptor, byteLength: 10, digest: "a".repeat(64) }), ).toBe(true); }); + + test("narrows a materialized descriptor to concrete measurements", () => { + const measured: ReviewResourceDescriptorV1 = { + ...descriptor, + byteLength: 10, + digest: "a".repeat(64), + }; + if (!isMaterializedReviewResource(measured)) { + throw new Error("expected a materialized descriptor"); + } + const byteLength: number = measured.byteLength; + const digest: string = measured.digest; + expect({ byteLength, digest }).toEqual({ byteLength: 10, digest: "a".repeat(64) }); + }); }); describe("isReviewResourceRange", () => { diff --git a/packages/hunk/src/core/review/resources.ts b/packages/hunk/src/core/review/resources.ts index 9da096f4d..f8fda1e74 100644 --- a/packages/hunk/src/core/review/resources.ts +++ b/packages/hunk/src/core/review/resources.ts @@ -52,35 +52,40 @@ export const REVIEW_CANONICAL_FILE_CONTENT_TYPE = export const REVIEW_PATCH_CONTENT_TYPE = "text/x-diff; charset=utf-8" as const; export const REVIEW_SOURCE_CONTENT_TYPE = "text/plain; charset=utf-8" as const; +/** + * Measurements are set together once the producer has materialized the content, and never + * apart: a descriptor that declares one without the other cannot be verified and is a bug. + */ +export type ReviewResourceMeasurement = + | { byteLength: number; digest: string } + | { byteLength?: never; digest?: never }; + interface ReviewResourceDescriptorBase { id: string; generation: string; fileKey: string; - /** - * Set together once the producer has materialized the content, and never apart: a - * descriptor that declares one without the other cannot be verified and is a bug. - */ - byteLength?: number; - digest?: string; } -export interface ReviewCanonicalFileResourceV1 extends ReviewResourceDescriptorBase { - kind: "canonical-file"; - contentType: typeof REVIEW_CANONICAL_FILE_CONTENT_TYPE; -} +export type ReviewCanonicalFileResourceV1 = ReviewResourceDescriptorBase & + ReviewResourceMeasurement & { + kind: "canonical-file"; + contentType: typeof REVIEW_CANONICAL_FILE_CONTENT_TYPE; + }; -export interface ReviewPatchResourceV1 extends ReviewResourceDescriptorBase { - kind: "patch"; - contentType: typeof REVIEW_PATCH_CONTENT_TYPE; -} +export type ReviewPatchResourceV1 = ReviewResourceDescriptorBase & + ReviewResourceMeasurement & { + kind: "patch"; + contentType: typeof REVIEW_PATCH_CONTENT_TYPE; + }; -export interface ReviewSourceResourceV1 extends ReviewResourceDescriptorBase { - kind: "source"; - contentType: typeof REVIEW_SOURCE_CONTENT_TYPE; - side: ReviewSide; - /** Identity of the content behind the reader, so a stale read is detectable. */ - sourceIdentity: string; -} +export type ReviewSourceResourceV1 = ReviewResourceDescriptorBase & + ReviewResourceMeasurement & { + kind: "source"; + contentType: typeof REVIEW_SOURCE_CONTENT_TYPE; + side: ReviewSide; + /** Identity of the content behind the reader, so a stale read is detectable. */ + sourceIdentity: string; + }; export type ReviewResourceDescriptorV1 = | ReviewCanonicalFileResourceV1 @@ -88,7 +93,9 @@ export type ReviewResourceDescriptorV1 = | ReviewSourceResourceV1; /** Whether one descriptor has been measured, and can therefore be verified when read. */ -export function isMaterializedReviewResource(descriptor: ReviewResourceDescriptorV1) { +export function isMaterializedReviewResource( + descriptor: ReviewResourceDescriptorV1, +): descriptor is Extract { return descriptor.byteLength !== undefined && descriptor.digest !== undefined; } diff --git a/packages/hunk/src/session/broker/state.ts b/packages/hunk/src/session/broker/state.ts index 0758fe771..821de5e1c 100644 --- a/packages/hunk/src/session/broker/state.ts +++ b/packages/hunk/src/session/broker/state.ts @@ -502,11 +502,11 @@ export class HunkSessionBrokerState extends SessionBrokerState< key: { sessionId: string; generation: string; resourceId: string }, descriptor: ReviewResourceDescriptorV1, ): Promise { - const measured = isMaterializedReviewResource(descriptor); + const measured = isMaterializedReviewResource(descriptor) ? descriptor : undefined; const reservation: ReviewResourceReservation = this.resources.reserve( key, measured - ? descriptor.byteLength! + ? measured.byteLength : Math.min(REVIEW_RESOURCE_CHUNK_BYTES, reviewResourceCeiling(descriptor.kind)), ); try { @@ -516,7 +516,7 @@ export class HunkSessionBrokerState extends SessionBrokerState< digest: nodeReviewDigest, maxBytes: reviewResourceCeiling(descriptor.kind), ...(measured - ? { expected: { byteLength: descriptor.byteLength!, digest: descriptor.digest! } } + ? { expected: { byteLength: measured.byteLength, digest: measured.digest } } : {}), }); From 46f95e78c83be2a3aa3438ecc2bb1071a423c014 Mon Sep 17 00:00:00 2001 From: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com> Date: Sat, 31 Oct 2026 07:24:00 -0700 Subject: [PATCH 2/2] test(review): check measurement narrowing through the type predicate Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com> --- .../hunk/src/core/review/resources.test.ts | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/hunk/src/core/review/resources.test.ts b/packages/hunk/src/core/review/resources.test.ts index d96522ad7..977e10119 100644 --- a/packages/hunk/src/core/review/resources.test.ts +++ b/packages/hunk/src/core/review/resources.test.ts @@ -60,18 +60,20 @@ describe("resource materialization state", () => { ).toBe(true); }); - test("narrows a materialized descriptor to concrete measurements", () => { - const measured: ReviewResourceDescriptorV1 = { - ...descriptor, - byteLength: 10, - digest: "a".repeat(64), - }; - if (!isMaterializedReviewResource(measured)) { + /** Reads measurements through the predicate alone, so only its narrowing can typecheck them. */ + function readTestMeasurements(resource: ReviewResourceDescriptorV1) { + if (!isMaterializedReviewResource(resource)) { throw new Error("expected a materialized descriptor"); } - const byteLength: number = measured.byteLength; - const digest: string = measured.digest; - expect({ byteLength, digest }).toEqual({ byteLength: 10, digest: "a".repeat(64) }); + + const byteLength: number = resource.byteLength; + const digest: string = resource.digest; + return { byteLength, digest }; + } + + test("narrows a materialized descriptor to concrete measurements", () => { + const measured = { ...descriptor, byteLength: 10, digest: "a".repeat(64) }; + expect(readTestMeasurements(measured)).toEqual({ byteLength: 10, digest: "a".repeat(64) }); }); });