Repository navigation
feat!: phase 7c pagination (PAGE-1..PAGE-36) - #86
Merged
Merged
Conversation
Closed
Wahbeh-Mohammad
added
type:feature
Contributor
Author
Review: phase 7c implementation vs. design and planAutomated review pass: an independent Opus 5.5 reviewer read the branch (
Finding details (problem, evidence, required fix)1. The blocking pager throws NotSupportedException over SystemNetHttpClient, but the docs and checklist say it works through sync-over-asyncProblem. The blocking read calls Evidence. Scratch console app referencing the built bin/Release DLLs, with a stub HttpMessageHandler behind Required fix. Correct the blocking-pager text in pagination.md, the 2. The query splice strips every leading '?' from the raw query, rewriting an untargeted parameter whose name starts with '?'Problem. Evidence. Scratch probe calling the internal Required fix. Strip only the single delimiter, e.g. 3. The query splice's URL rebuild drops the zone id of a scoped IPv6 hostProblem. Evidence. Scratch probe (reflection on the built DLL): Required fix. Rebuild while keeping the original authority text, e.g. replace only the query span of Reviewer summaryPhase 7c mostly matches its design and plan. The engine (PageStep plus the two iterator shells), the strategies, the query splice, the Link parser and target, the fetcher form and the public surface follow the design's rulings, and every PAGE row has the evidence the checklist claims. All 135 test names the checklist cites exist in the right classes. The only Security change is the one added class, PaginationLinkOriginTests. Core's PublicAPI diff matches the design's list, and the SystemNet and STJ PublicAPI files, TestSupport and the props files are untouched. The only analyzer suppressions are test-only pragmas, each with a stated reason. Three findings survived verification; all were reproduced with a scratch console app outside the repo that references the built DLLs.
No blocker found. Reviewer gates: Run in the worktree under the resource limits:
Not run, to limit memory use: AOT publish, pack, reproducible pack, dependency audit, the tools solution, the coverage gate. Resolution (fix agent report)All three findings are fixed on Findingsfixed: The blocking pager throws NotSupportedException over SystemNetHttpClient, but the docs and checklist say it works through sync-over-async — The behaviour is unchanged. I corrected the text, added the pinning test and named the prerequisite.
fixed: The query splice strips every leading '?' from the raw query —
fixed: The query splice's URL rebuild drops the zone id of a scoped IPv6 host —
Checklist rows Commits (
|
QuerySplice reads and rewrites one query parameter by splicing the raw query: untargeted segments are copied verbatim, names match ordinally on the decoded name (fixes the as-built case-insensitive ?Page=1 match), the first match is replaced in place and later duplicates dropped, null removes, and the URL is rebuilt from GetComponents so no explicit default port is written. A lone surrogate in a name or value throws ArgumentException naming the parameter and never echoing the value (P7c-10). QuerySpliceTests ports the 22 cases of nodejs-sdk@c0ff3fd query-splice.test.ts. Dropped: the case pinning the WHATWG URL query encode set (`?tag=<raw>` rewritten to %3Craw%3E), which asserts a JavaScript host fact; its RFC 3986-legal half is kept. QuerySplicePropertyTests ports the three properties of query-splice.property.test.ts over a seeded System.Random (seed 20261009, 500 cases each, no package: P7c-18); the Node alphabet exclusion for the WHATWG encode set is a JavaScript host fact and is not ported.
…, PAGE-20) LinkHeaderParser.FindNext is a single-pass scanner over the joined header value: angle-bracket and quoted-string aware, quoted-pair escapes, rel quoted or unquoted, SP/HTAB token lists, case-insensitive next, first link-value wins, only the first rel parameter of a link-value counts (RFC 8288 section 3.3), and a malformed link-value is skipped, never thrown. tests/vectors/pagination/link-header.json holds the 15 cases of nodejs-sdk@c0ff3fd link-header.test.ts plus 24 .NET-specific cases. Node cases passing an array of header instances are joined with a comma, as the strategy joins them; none were dropped as JavaScript host facts. Divergence: Node reads every rel parameter of a link-value, this port reads the first.
…E-19, P7c-12)
LinkTarget.TryResolve is total: it rejects a target holding SP, controls, <, >
or " before resolving (System.Uri accepts "not a url" as a relative path),
resolves by RFC 3986 against the response URL, keeps the http(s)-with-host
guard, strips userinfo, and ends the stream on a target outside the template's
origin unless allowCrossOrigin is set. The origin compared is the template's,
not the response's, so a first page that redirected cannot launder an origin.
LinkTargetTests ports the PAGE-19 cases of nodejs-sdk@c0ff3fd strategies.test.ts.
Dropped: none as JavaScript host facts. Divergences kept as the .NET
expectation: Node follows an absolute cross-origin target ("used as-is") and
follows `<not a url>` as a relative path; this port ends the stream on both.
…AGE-2, PAGE-4) Breaking: Page<T>'s constructor requires the Request the walk sent for the page (P7c-7). PageInfo<T> is the strategy's one output (items plus a next request; null is the only end signal); IPageStrategy<in TPage, T>.Parse is the typed- envelope contract (P7c-2). The interface parameter is named `first`, not `template`, because CA1716 reserves `template` on an interface member. Test fixtures: PageFixtures (Envelope, scripted bodies and responders), EnvelopeSerde (a stateless "1,2,3|next" codec implementing both the sync and async members, which the shared ScriptedSerde does not) and EnvelopeBody (a valid-envelope body that counts and can fail its release). The three Page<T> constructor facts move from PageableTests to PageTests with the fourth argument.
…PAGE-5, PAGE-16..PAGE-20) PaginationStrategies gains Create, Cursor, PageNumber and the Link strategy (temporarily LinkHeaderNext: PublicApiAnalyzers RS0026 forbids a second overload with optional parameters, even across generic arities, so the rename to LinkHeader happens in the commit that removes the old one). Each built-in is an internal sealed class holding only its configuration. Names are validated at construction, including the lone-surrogate check (P7c-10). Behaviour: Cursor defaults to "cursor"; PageNumber ends on an empty page, reads the executed request's URL with invariant ASCII-digit parsing, honours startPage and ends at int.MaxValue (P7c-11); the Link strategy reads every header instance, resolves against the response URL and ends on a rejected or cross-origin target (P7c-12). Every next request is built from the template (P7c-8). PaginationStrategyParseTests (a temporary name; it replaces the old PaginationStrategiesTests when the old surface is removed) ports the 18 cases of nodejs-sdk@c0ff3fd strategies.test.ts and the type-level assertions of strategy.test.ts. Dropped: none as JavaScript host facts.
SingleUseAsyncEnumerable<T> and SingleUseEnumerable<T> back AsPages(): the second GetAsyncEnumerator/GetEnumerator throws InvalidOperationException (an int latch set with Interlocked.Exchange, so racing callers cannot both win), and the walk factory runs on the first call only, so building the view issues no exchange. The async view hands the token to the factory as well as the enumerator, so a walk that lacks [EnumeratorCancellation] still sees WithCancellation.
…4, PAGE-11, PAGE-13, PAGE-15, PAGE-27, PAGE-28, PAGE-32, PAGE-33) PageStep.FetchAsync sends one page, reads its body once, parses it, snapshots the items into an immutable list and closes the response before the page can be yielded, so no response is live at any yield (design section 10 entry 17). A non-fatal failure closes the response through Disposal with the failure as the primary, so a release failure lands on its ExceptionTrail (PAGE-13); a success-path release failure propagates unwrapped (PAGE-15, PAGE-32). A null response, a null envelope (DeserializationException naming the page type) and a null PageInfo are contract violations that go through the same close path (P7c-17). A response delivered after the token was cancelled is disposed and discarded (PAGE-33, P7c-14). With async false nothing awaits, so the blocking shell reads the returned task through SyncPath.GetCompletedResult. The blocking read opens the stream and reads it through BodyMaterializer under ReadAsBytes's 64 MiB cap rather than calling ReadAsBytes, because ReadAsBytes also closes the body (BODY-16): a release failure would then surface from the read, before the strategy ran, and never as a suppressed secondary of a parse failure. PageWalk bundles the walk's constants and validates them (null arguments, maxPages <= 0, a non-replayable template body). PageStepTests covers the matrix; the fatal-exception row is exercised with a hand-made OutOfMemoryException.
…surface removed (PAGE-1, PAGE-6..PAGE-10, PAGE-14, PAGE-25, PAGE-26, PAGE-31, PAGE-36) Breaking: - Pageable.Create takes an IAsyncHttpClient (an HttpPipeline still converts), an IPageStrategy<TPage,T> and RequestOptions? instead of an HttpPipeline, a DexpaceClientOptions and two delegates (PaginationStrategies.Create wraps the old delegates). maxPages <= 0 and a template with a single-use body throw at construction; a null envelope throws DeserializationException. - AsyncPageable<T>.AsPages() lost pageSizeHint, returns a single-use view and, with GetAsyncEnumerator, is no longer abstract: subclasses override the protected WalkPagesAsync. - PaginationStrategies' old Cursor/PageNumber/LinkHeader delegate factories and their private query helpers are removed; the Link factory takes the name LinkHeader (it was LinkHeaderNext beside the old overload). StrategyPageable is the iterator shell over PageStep: the token is checked per page (PAGE-26), the cap before each exchange (PAGE-9), and nothing is live at a yield. PageableTests is rewritten against the new factory and ports the 9 cases of nodejs-sdk@c0ff3fd paginator.test.ts and the 4 of cancellation.test.ts (none dropped as JavaScript host facts); PaginationStrategiesTests replaces the old file (git mv of the 2.2 tests). The page-view single-use latch, cancellation in flight, 10,000 synchronous pages, the same RequestOptions instance on every page and a pipeline as the client (PIPE-26) are covered.
…6, PAGE-7, PAGE-9, PAGE-14, PAGE-31) Pageable<T> is the blocking twin of AsyncPageable<T>: non-virtual AsPages() and GetEnumerator(), a protected abstract WalkPages(), a single-use page view. StrategyBlockingPageable runs the same loop over the same PageStep with async: false and reads each step through SyncPath.GetCompletedResult, so there is no sync-over-async in the pager and no second copy of the fetch, parse and close logic. CreateBlocking takes the IHttpClient seam, the same options and cap, and the CancellationToken it captures for the whole pageable. It has a different name from Create because an HttpPipeline converts to both seams and two Create overloads would be CS0121 for the most common caller. BlockingPageableTests runs over SyncFirstTransport (whose ExecuteAsync fails the test if called) and RecordingSyncTransport, and pins the 64 MiB per-page materialisation cap (BodyTooLargeException), which the blocking path inherits.
…PAGE-4, PAGE-11..PAGE-15, PAGE-27, PAGE-28, PAGE-32, PAGE-33)
Pins over the public API for rules PageStep and the two shells already implement,
each row run against Pageable.Create and Pageable.CreateBlocking: the page is
closed before its first item is delivered; an abandoned hand-driven enumerator
leaves disposed == fetched; a parse failure closes the response with the parse
error primary and a release failure on its ExceptionTrail; a success-path release
failure surfaces unwrapped and ends the walk; every path (drain, break, consumer
throw, parse failure, null envelope, null PageInfo) releases each fetched
response once; transport faults, eager throws, a null response and a consumer
throw surface as their own types; a late response after cancel is discarded.
Ported from the 13 cases of nodejs-sdk@c0ff3fd lifecycle.test.ts, adapted to
entry 17 ("closes the held page" becomes "no response is open at any yield";
SuppressedError becomes ExceptionTrail). Pins proven able to fail by temporarily
removing the parse-failure close (8 rows fail) and the success-path close (16 rows
fail); removing the late-cancel discard fails PageStepTests, not this suite,
because the cancelled read also closes the response.
…ession (PAGE-3, PAGE-14, PAGE-19, P7c-12) PaginationLinkOriginTests (Security, permanent, never loosen) drives the real stack, a BearerTokenAuthPolicy over a scripted transport, through a Link walk: a cross-origin, network-path, scheme-downgrade or other-port next target ends the walk after one exchange with no request reaching the other origin; a same-origin target is followed with the policy's own stamp; a userinfo-bearing same-origin target is followed with the userinfo removed; and a first page that redirected to another origin does not license following links there, because the guard compares the template's origin, not the response's. Proven able to fail by removing the comparison (6 rows fail) and by comparing against the response URL (the 2 laundering rows fail). PaginationArchitectureTests checks by reflection that pages, PageInfo and the fetched-page carrier are neither IDisposable nor IAsyncDisposable (entry 17) and expose no public setter or mutable collection, that AsPages/GetAsyncEnumerator/ GetEnumerator cannot be overridden, that the walk hooks are protected abstract, and that no public member leaks a Response except IPageStrategy.Parse and the Create adapter. The FetchedPage and PagingOptions rows skip until task 3.1. PaginationStrategiesTests gains the allowCrossOrigin Unit fact that pins what the XML doc warns about (per-call auth stamps the credential for the new origin).
…ions dotnet format --verify-no-changes rewrote the inline async lambdas of the matrix into an unreadable shape; named local functions keep the rows readable and the formatter quiet. No behaviour change.
…, PAGE-9, PAGE-14, PAGE-34, PAGE-35) FromFetchers drives a walk from caller-supplied async fetchers: the first runs exactly once per walk (a null result is an empty stream), the next is keyed by the previous page's NextLink when it is not blank, else its ContinuationToken, and a null page ends the stream. One mutable PagingOptions per walk is passed to every fetcher call, with NextLink and ContinuationToken written before each next call, and a fresh one per enumeration (PAGE-8, PAGE-35; P7c-16). The cap is checked after the yield so a cap of n never starts fetch n + 1. FetchedPage<T> is the positional record the spec shapes, with a Page that is validated at construction and by `with`. Ownership (P7c-15): a Page owns no response, so a fetcher disposes its own response (await using) and the engine never sees one; this inverts PAGE-34's "must not close" clause, a dated amendment to design section 10 entry 17. FetcherPageableTests ports the 12 cases of nodejs-sdk@c0ff3fd fetchers.test.ts, none dropped as JavaScript host facts; the Node cases that close a page the engine owns become the ownership rule. PaginationArchitectureTests no longer skips its FetchedPage and PagingOptions rows.
…18, PAGE-19, PAGE-20, PAGE-36) A three-page Link walk whose second page splits its links across two header lines (rel=prev first, rel=next second) and whose first page uses a query-only reference: the server sees /items, /items?page=2, /items?page=3 and SystemNetHttpClient surfaces both Link lines of page two as separate values. A cross-origin Link to a second loopback server ends the walk after one exchange and the second server sees nothing. A cursor walk hands the same RequestOptions instance to the real transport on every page. Pin proven able to fail by making the Link strategy read only the first header instance. Deviation from the plan: its row for a RequestOptions.Timeout that fires ServiceRequestTimeoutException is not built. SystemNetHttpClient accepts RequestOptions and reads none of it until phase 8b wires Timeout (TRANSPORT-5; SystemNetHttpClientTests.Options_are_accepted_and_ignored_until_8b), so there is no timeout to fire. The options-instance row above is PAGE-36's claim at the seam.
CheckPhase7cPaginationAsync runs inside the published NativeAOT binary: a three-page cursor walk through DexpacePipeline over an in-memory DelegateHttpClient, deserialized by the STJ serde with a source-generated JsonTypeInfo for a WidgetPage envelope in its own JsonSerializerContext (PaginationSmokeContext, so the shared context is not edited); item order, Page<T>.Request and per-page status; a second GetAsyncEnumerator on one AsPages view throws; a Link-header walk; and the same cursor walk through Pageable.CreateBlocking. No trim or AOT warning.
…ation.md) As built by phase 7c: the two views of a walk, pages as values, the three factories and why CreateBlocking is named apart, the built-in strategies and the custom-strategy rules (ported in structure from nodejs-sdk's write-a-paging- strategy.md), the cap, cancellation and the PAGE-33 race, the Link origin guard with the credential-stamping warning for allowCrossOrigin, the fetcher ownership rule, the blocking pager's 64 MiB note and PIPE-28 gap, and the migration table. pipelines.md, docs/README.md, the repository README and the Core package README point at it. Every code sample was compiled against the built assembly.
…and CLAUDE.md The checklist has one row per PAGE ID (33 built, 3 permanent simplifications under design section 10 entries 17 and 19, 0 deferred) with the evidence by test name, the cross-owner rows, the one added Security class, the deviations from the plan (the blocking read does not call ReadAsBytes because it closes the body; the RequestOptions.Timeout wire row cannot fire because the transport ignores options until 8b; CA1716 forced the `first` parameter name; the MTP/MSBuild flag clash) and the open rulings taken as designed. Dated corrections: design section 7.1 (As built, phase 7c), section 10 entries 17 and 18, new section 11 item 66 (the Link cross-origin reading of PAGE-19), the section 12 PAGE row, docs/first-release.md (link-cross-origin-guard, public-query-splice). CHANGELOG [Unreleased] lists the nine breaking changes and the additions; the roadmap gets a dated phase status note and the 7c checklist link; the CLAUDE.md layout line and built-phases list name the pagination surface.
Every_blocking_path_returns_an_already_completed_task left five faulted tasks unobserved; their finalizers raised TaskScheduler.UnobservedTaskException in whichever test was listening (LateResultTests failed with 'Expected 0, Actual 5'). The test now reads each completed task, so the fault is observed where it is made.
…(PAGE-21, PAGE-24)
Segments stripped every leading '?' from the raw query, so `p??a=1&page=1` lost the first character of an untargeted
parameter called "?a" and Get("a") matched it. It now drops only the delimiter.
Uri.GetComponents, Host and AbsoluteUri all drop the zone id of a scoped IPv6 literal; only IdnHost keeps it. Rebuild
restores it from there so a Cursor or PageNumber walk keeps aiming at the same interface.
…ystemNetHttpClient until 8b (PIPE-28) HttpResponseMessageBody has no OpenRead until phase 8b, so Pageable.CreateBlocking over the real transport, directly or through a pipeline, fails on the first page. The Integration test pins the failure so 8b's OpenRead is a reviewed flip to a passing walk.
… 8b adds HttpResponseMessageBody.OpenRead The 7c documents described the gap as sync-over-async inside the transport, which implied the pager works. Correct pagination.md, the Pageable<T> and CreateBlocking remarks, the checklist, the design (8b row, hand-off, R5), design 7 section 7.1, the roadmap note and the changelog; name HttpResponseMessageBody.OpenRead as the 8b prerequisite and record the three review corrections as checklist deviation 13.
…28, after 7b's OpenRead) Phase 7b gave HttpResponseMessageBody a synchronous OpenRead, so the pin that the blocking pager threw NotSupportedException over SystemNetHttpClient until 8b now fails. Flip it, as its comment asked, to a three-page cursor walk over the loopback server, directly and through PipelineBuilder().Build(transport).
…enRead Phase 7b built HttpResponseMessageBody.OpenRead before 7c merged, so CreateBlocking over the reference transport walks every page. pagination.md (the blocking section and the factory table), pipelines.md, the Pageable<T> and CreateBlocking remarks and the changelog bullet now say so. The frozen texts keep their history with dated notes: the 7c checklist (the paragraph under the totals, the PIPE-28 row, deviations 11 and 13), the 7c design (the 8b row, hand-off, R5, and R6 for 7a's ISerde stream decode), design 7.1, and the roadmap's 7c note, whose 8b hand-off is now the synchronous Execute and RequestOptions.Timeout only.
TaskScheduler.UnobservedTaskException is process-wide and every subscriber sees every raise. Phase 7a's TypedResponseTests leaves a fatal parse task unobserved on purpose and collects it in a GC loop; when that loop overlaps this test's, the raise was counted here too (reproduced on the rebased branch: the captured exception was TypedResponseTests' simulated OutOfMemoryException). Filter by the test's own exception, as TypedResponseTests already does.
The unscoped ActivityRecorder saw a "GET" span that a default-pipeline test in another collection (such as BearerPipelineTests) started on the same source while it listened, and Assert.Empty failed. A scoped recorder reports only its own trace, which is where an activity None started would land.
BearerPipelineTests is not in the "Instrumentation" collection, so its unscoped recorder also counted the attempt spans of default-pipeline tests running in parallel: the known flake 'Expected: 2, Actual: 3' on attempts.Count. A scoped recorder counts only this call's trace.
Wahbeh-Mohammad
force-pushed
the
81-phase-7c-pagination
branch
from
October 9, 2026 09:38
fa6df8b to
b0409c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 7c of the v1 roadmap: pagination, built to
docs/work/mvp/phase7/phase7c/2026-10-09-phase7c-pagination-design.mdand its plan.IPageStrategy<in TPage, T>returningPageInfo<T>;PaginationStrategies(Cursor,PageNumber,LinkHeader,Create) reworked with spec defaults.AsyncPageable<T>and the new blockingPageable<T>(Pageable.CreateBlocking). The page is closed before it is yielded; a release failure rides the parse error'sExceptionTrail; a response after cancel is disposed.AsPages(),maxPagesvalidated,Page<T>.Request, non-replayable template rejected at construction.Link: every header instance read, resolved against the response URL, cross-origin targets end the walk unlessallowCrossOrigin: true, userinfo stripped. NewSecurityclassPaginationLinkOriginTests.Pageable.FromFetchers,FetchedPage<T>,PagingOptions(PAGE-34,PAGE-35).tests/vectors/pagination/link-header.json,PaginationWireTests(loopback),PaginationArchitectureTests, AOT smokeCheckPhase7cPaginationAsync. Docs:docs/sdk-documentation/pagination.md, checklist, CHANGELOG, roadmap note, dated corrections.Checklist census
docs/work/mvp/phase7/phase7c/2026-10-09-phase7c-pagination-checklist.md: 36 rows (PAGE-1..PAGE-36): 33 ✅, 3 🚫 (PAGE-3,PAGE-29,PAGE-30, permanent simplifications under design §10 entries 17 and 19).Local gate (fix agent, 10.0.401): Release build 0 warnings,
dotnet format --verify-no-changes, 4844 tests (4843 passed, 1 skipped), Security Core 289/289 + SystemNet 32/32, coverage 97.51% aggregate, dependency audit, reproducible pack, NativeAOT publish + run, tools solution 438/438,scripts/knowledge verify-structure, housekeeping probe: all green.Breaking changes
Pageable.CreatetakesIAsyncHttpClient, anIPageStrategy<TPage, T>andRequestOptions?(PaginationStrategies.Create(selectItems, nextRequest)wraps the old delegates).PaginationStrategies.*gain the item selector and a second type parameter;Cursordefaults to"cursor";PageNumberloseshasMore, gainsstartPage;LinkHeaderlosesrel, gainsheaderNameandallowCrossOrigin.PageNumberends only on an empty page.LinkHeaderends on cross-origin / invalid targets, strips userinfo, reads every instance, resolves against the response URL.AsPages()losespageSizeHint, is non-virtual and single-use; subclasses overrideWalkPagesAsync.Page<T>'s constructor requires theRequest.maxPages <= 0and a non-replayable template body throw at construction.nullenvelope throwsDeserializationException.Open rulings
Taken as designed, recorded in the checklist: P7c-3 (
pageSizeHintremoved), P7c-6 (RequestOptionsper-call overrides), P7c-12 (cross-originLinkends the walk unless opted in), P7c-16 (mutablePagingOptions).Cross-PR note
The blocking pager over
SystemNetHttpClientis documented and pinned (PaginationWireTests.A_blocking_walk_over_the_real_transport_throws_NotSupportedException_until_8b_adds_OpenRead) as throwingNotSupportedExceptionbecause the transport body has no synchronousOpenRead. The 7b PR (#85) adds exactly thatOpenRead. Whichever of 7b / 7c merges second must flip the test and updatepagination.md, the XML remarks, the checklist (deviation 13) and the roadmap note.Review
Implemented, then independently reviewed against the design and plan with every finding verified, then fixed by a fresh agent — see the review comment below.
Closes #81.