Repository navigation
refactor(review): encode resource measurement invariants in the descriptor type - #1243
ShryukGrandhi wants to merge 2 commits into
Conversation
…iptor type Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
|
PR author is not in the allowed authors list. |
squad-robert
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
Summary
parseHunkReviewResourceDescriptorrejects descriptors that carry only one ofbyteLengthanddigest, butReviewResourceDescriptorV1declared both as independent optionals. TypeScript therefore accepted half-measured descriptors the protocol refuses, and callers ofisMaterializedReviewResourcestill needed!assertions after the check.Changes
ReviewResourceMeasurement({ byteLength: number; digest: string } | { byteLength?: never; digest?: never }) incore/review/resources.tsand compose it into each resource descriptor type. The descriptor types become intersection type aliases because an interface cannot extend a union.isMaterializedReviewResourcea type predicate that narrows to the measured branch.ReviewResourceStore.measureandHunkSessionBrokerState.assembleResource.Publication, measuring, and chunk reading behave exactly as before. Only the types changed.
Testing
resources.test.tsnow uses@ts-expect-erroron the half-measured cases and adds a test that a materialized descriptor narrows tonumber/string. Before the fix,bun run typecheckfailed on those lines (unused@ts-expect-error, andnumber | undefinednot assignable tonumber). After the fix, it passes.bun run typecheck,bun run lint,bun run format:check, andbun run deps:checkall 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.tsfails the same way on unmodifiedmain.test/session/daemon.test.tsandstatusRouting.test.tspass 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