Skip to content

refactor(review): encode resource measurement invariants in the descriptor type - #1243

Open
ShryukGrandhi wants to merge 2 commits into
modem-dev:mainfrom
ShryukGrandhi:fix/issue-769-encode-review-resource-measurement-invar
Open

ShryukGrandhi wants to merge 2 commits into
modem-dev:mainfrom
ShryukGrandhi:fix/issue-769-encode-review-resource-measurement-invar

Conversation

@ShryukGrandhi

Copy link
Copy Markdown
Contributor

Summary

parseHunkReviewResourceDescriptor rejects descriptors that carry only one of byteLength and digest, but ReviewResourceDescriptorV1 declared both as independent optionals. TypeScript therefore accepted half-measured descriptors the protocol refuses, and callers of isMaterializedReviewResource still needed ! assertions after the check.

Changes

  • Add ReviewResourceMeasurement ({ byteLength: number; digest: string } | { byteLength?: never; digest?: never }) in core/review/resources.ts and compose it into each resource descriptor type. The descriptor types become intersection type aliases because an interface cannot extend a union.
  • Make isMaterializedReviewResource a type predicate that narrows to the measured branch.
  • Drop the non-null assertions in ReviewResourceStore.measure and HunkSessionBrokerState.assembleResource.
  • Wire parsing is unchanged: it still rejects half-measured descriptors before returning the validated record.
  • Add an empty Changeset because this is maintenance-only.

Publication, measuring, and chunk reading behave exactly as before. Only the types changed.

Testing

  • resources.test.ts now uses @ts-expect-error on the half-measured cases and adds a test that a materialized descriptor narrows to number/string. Before the fix, bun run typecheck failed on those lines (unused @ts-expect-error, and number | undefined not assignable to number). After the fix, it passes.
  • bun run typecheck, bun run lint, bun run format:check, and bun run deps:check all pass.
  • bun test packages/hunk/src/core packages/hunk/src/app packages/hunk/src/session test/session: all pass except a few timing-sensitive broker/daemon tests. A different test fails on each run under load. brokerServer.test.ts fails the same way on unmodified main. test/session/daemon.test.ts and statusRouting.test.ts pass when run alone.
  • bun test ./test/review-conformance: 118 pass.
  • bun run test:session-broker-node: 4 pass.

Tested on macOS (arm64) only.

Fixes #769

…iptor type

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@squad-robert squad-robert left a comment

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.

One nonblocking test gap in resources.test.ts:63–74: measured is initialized with both fields, so the new union already narrows it to the measured branch before the guard runs. I changed only the predicate's return annotation to boolean in a compiler-host negative control; this test still typechecked. A probe receiving an arbitrary descriptor correctly failed to narrow under that same control.

Could you move the narrowing assertions into a helper accepting descriptor: ReviewResourceDescriptorV1, then call it with the measured fixture? That would make the test protect the predicate's contract rather than the initializer's narrowing.

The implementation's type behavior worked in my independent checks against base 33682090 and exact head 24fd3761: both half-measurements were rejected for all three resource kinds, and an unknown descriptor narrowed to concrete measurements. The unchanged author regression source produces the four expected type errors on base and passes on head.

Runtime behavior also held in the cases exercised: 14 independent wire/producer/broker cases passed on both revisions, including zero-byte content, measured-size budgets, and size/digest mismatches. The focused review/broker/session/conformance/boundary run passed 788 tests on each, and the real-Node adapter passed all four on each. Head typecheck and changed-file lint/format passed. Linux x64, Bun 1.4.4, TypeScript 5.9.3; no full PTY/TTY or non-Linux claim. These results are for the recorded base/head, not a rebase onto today's newer main.

This comment was generated by Pi using GPT-6.1 Sol

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>

@squad-robert squad-robert left a comment

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 narrowing-test gap is resolved at 46f95e78. readTestMeasurements now receives an arbitrary ReviewResourceDescriptorV1, so its assignments depend on the predicate rather than the fixture initializer. I repeated the same compiler-host negative control: changing only the predicate's return annotation to boolean now produces the two expected assignment errors in this test; the original test incorrectly survived that control.

Fresh matched checks against base 33682090: the updated author regression source produces four intended type errors on base and none on head; the independent all-kind type probe produces ten → zero. Both revisions pass the same 14 runtime probes, 788 focused review/broker/session/conformance/boundary tests, and four real-Node adapter tests. Head full typecheck and changed-test lint/format pass. The follow-up changes only this test; the production diff is unchanged from my earlier review.

No remaining actionable defect found in the exercised contract. These are Linux/Bun 1.4.4 results for the recorded base and exact new head, not current-main rebase or non-Linux certification.

This comment was generated by Pi using GPT-6.1 Sol

This branch has not been deployed

No deployments
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.

Encode review resource measurement invariants in the descriptor type

2 participants