fix(sources): never end a run early and call it Completed (ADR D72) (#264)

* fix(sources): never end a run early and call it Completed (ADR D72)

The runner's stuck-cursor guard (#263) bounds a source that makes no progress. This is
the opposite failure direction, which the §6 audit found in four places: a source that
stops early and reports the run Completed with rows missing. Nothing downstream can tell
that apart from a genuinely complete run — no exception, no partial capture, no warning —
which makes it the worst outcome the pipeline can produce.

- Elasticsearch answers a partial search with HTTP 200: it sets `timed_out`, or reports
  failed shards under `_shards`, and returns what the responsive shards had. Neither
  field was inspected, so the short page simply ended pagination. Both are now checked
  before the hits are read, matching what GraphQL (D63) does with a 200 carrying `errors`
  and this source's own "full page with no sort values" guard.

- `records.Count == pageSize` is no longer how "is there more?" is decided in OData's
  `Skip` and the HTTP source's `Page`/`Offset`. Inferring it from a full page is wrong
  whenever the service caps the page below what was requested — Dynamics, SAP Gateway and
  Business Central all clamp, and many REST APIs silently reduce an over-max limit, so
  against any of them the FIRST page comes back short and the run stopped there. They now
  page until a response comes back empty: one extra request per run, and this class of
  truncation becomes structurally impossible. NextLink and the cursor strategies are
  unaffected, since they follow a real token.

- HubSpot and Airtable clamp the page size to the 100 their APIs accept rather than
  sending the engine's 1000 default and failing the first request (maintainer decision,
  D72). Safe because both derive HasMore from the server's own continuation token, so
  clamping only means more requests — had they inferred it from a full page, clamping
  alone would have been unsafe, which is exactly the bug above.

Also clears CodeQL cs/linq/missed-select (alert 292), opened on master by the
Link-header parsing in #262: the loop guards its split on the way in instead of
assigning then checking, which is what read as a missed `.Select`.

Eight tests, each verified to fail against the unfixed code. Three existing tests encoded
the old full-page semantics and are updated rather than deleted; a new one covers the
actual defect — a service that caps the page no longer truncates the report.

* refactor(sources): drop the page-size argument BuildPageResult no longer uses

Author: thiagoluga

DECISIONS.md

@@ -1149,3 +1149,38 @@ special cases.
 the stuck-source tests, and reverting *only* the adapter (keeping the guard) fails the streaming
 test. The echoing fake throws past 50 reads on purpose — without the guard it loops forever, and a
 hanging test is worse than a failing one, both for CI and for anyone bisecting later.
+
+### D72, continued — the source-level half: never truncate quietly
+
+The runner's guard bounds a source that makes no progress. It cannot help with the opposite failure
+direction, which the §6 audit found in four places: a source that **stops early** and reports the
+run as `Completed` with rows missing. Nothing downstream can tell that apart from a genuinely
+complete run — no exception, no partial-artifact capture, no warning — so it is the worst outcome
+the pipeline can produce. All four are resolved the same way: fail, or keep going, but never
+silently deliver less than was asked for.
+
+**Elasticsearch partial searches now fail.** A partial search is HTTP **200** with fewer hits than
+the shards hold: the cluster sets `timed_out`, or reports failed shards under `_shards`, and returns
+what the responsive shards had. Neither field was inspected, so the short page ended pagination.
+This matches what GraphQL (D63) already does with a 200 carrying `errors`, and this source's own
+"full page with no sort values" guard. It does turn a previously-silent success into a hard failure
+— that is the point; a report missing an unknown number of rows is not a success.
+
+**`records.Count == pageSize` is no longer how "is there more?" is decided.** OData's `Skip` and the
+HTTP source's `Page`/`Offset` strategies have no server token to follow, so it can only be inferred
+— but inferring it from a *full* page is wrong whenever the service caps the page below what was
+requested. Dynamics, SAP Gateway and Business Central all clamp `$top`/`limit`, and many REST APIs
+silently reduce an over-max value; against any of them the **first** page comes back short and the
+run stopped there. These now page until a response comes back **empty**. The cost is one extra
+request at the end of a run; the benefit is that this class of truncation is structurally impossible
+rather than merely unlikely. (`NextLink` and the cursor strategies are unaffected — they follow a
+real token.)
+
+**HubSpot and Airtable clamp the page size instead of failing.** Both cap at 100 while the engine
+defaults to 1000, so a source built with defaults failed its very first request until the author
+happened to call `.PageSize(100)` — a default configuration that could not work.
+**Decision (maintainer, 2026-08-05): clamp.** A report author should not have to know each
+provider's ceiling, and a page size is a throughput hint, not a promise about how many rows arrive
+at once. Both derive `hasMore` from the server's own continuation token, so clamping only means more
+requests — it cannot truncate. (Had they inferred it from a full page, clamping alone would have
+been unsafe; that is precisely the bug fixed in the paragraph above.)

docs/STATUS-AND-BACKLOG.md

@@ -199,14 +199,19 @@ rather than decided:
   emitted a constant cursor — every file-backed source would have failed at page 2 — so the adapter
   now emits its page count rather than the runner special-casing a sentinel. Original description:
   no page cap, no "the cursor did not change" guard, no "zero rows but still more" guard.
-- **`records.Count == pageSize` ends a run early when the server caps the page.** OData's `Skip`
+- ~~**`records.Count == pageSize` ends a run early when the server caps the page.**~~ **FIXED
+  (ADR D72)** — `Skip`/`Page`/`Offset` now page until a response comes back **empty**, so this class
+  of truncation is structurally impossible rather than merely unlikely. Costs one extra request per
+  run. Original description: OData's `Skip`
   strategy (`ODataBatchSource`) and the HTTP source's `Page`/`Offset` strategies infer "more data"
   from a full page. Services that clamp `$top`/`limit` below the engine's 1000 default (Dynamics, SAP
   Gateway, Business Central; many REST APIs silently reduce an over-max `limit`) return a short first
   page → the run stops there and reports **`Completed`** with partial data. `Skip` is opt-in and
   `NextLink` is the default, which limits blast radius. A fix means either honouring `@odata.nextLink`
   in `Skip` mode too, or paging until a page returns zero rows — both change termination semantics.
-- **Elasticsearch treats a partially-failed search as a short page.** ES returns **HTTP 200** with
+- ~~**Elasticsearch treats a partially-failed search as a short page.**~~ **FIXED (ADR D72)** — both
+  fields are inspected before the hits are read, and a partial search now fails loudly. Original
+  description: ES returns **HTTP 200** with
   `timed_out: true` / `_shards.failed > 0` and fewer hits; neither field is inspected, so the report
   silently ends early as `Completed`. GraphQL already fails loudly on 200-with-`errors`; the ES
   equivalent would be consistent, but it turns today's silent success into a hard failure.
@@ -252,11 +257,12 @@ rather than decided:
   index, so a misconfigured `headerRow` produces N rows of all-nulls reported as success instead of
   failing loudly; (c) an interior blank row is returned as `[]` and materialized as a phantom
   all-default row. (a) is the clearest and most contained.
-- **HubSpot and Airtable default to a page size their API rejects.** Both send the engine's 1000
-  default as `limit`/`pageSize`, but both providers cap at 100 (recorded in `DECISIONS.md`), so a
+- ~~**HubSpot and Airtable default to a page size their API rejects.**~~ **DECIDED AND FIXED
+  (ADR D72)** — the maintainer chose **clamping**: an author should not need to know each provider's
+  ceiling. Safe because both derive `hasMore` from the server's continuation token, so clamping only
+  means more requests. Original description: Both send the engine's 1000
+  default as `limit`/`pageSize`, but both providers cap at 100, so a
   source built with defaults fails its very first request until the author calls `.PageSize(100)`.
-  Loud, but the default configuration is non-functional. Clamping vs. failing with a clear message is
-  a product call.
 - ~~**API: `POST /reports/{name}/preview` is the one data-plane endpoint that doesn't scrub driver
   exceptions.**~~ **FIXED** (routes through `SchemaProblem` like its siblings). Original description: It catches only `ConfigurationException`, so a bad filter value surfaces the raw
   `SqlException`/`PostgresException` (host, port, database) as a 500 — its siblings all route through

src/Sources/NeoReports.Sources.Airtable/AirtableBatchSource.cs

@@ -78,9 +78,19 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
         return new BatchResult<T>(records, cursor, hasMore);
     }
 
+    /// <summary>
+    /// The largest page Airtable accepts. The engine's own default is 1000, and sending that made the
+    /// very first request fail until the author happened to call <c>.PageSize(100)</c> — a default
+    /// configuration that could not work. The maintainer chose clamping over failing (ADR D72): a
+    /// report author should not have to know each provider's ceiling, and a page size is a
+    /// throughput hint, not a promise about how many rows arrive at once.
+    /// </summary>
+    private const int MaxPageSize = 100;
+
     private Uri BuildRequestUri(AirtableCursorState state, int pageSize)
     {
-        var queryParams = new List<(string Key, string Value)> { ("pageSize", pageSize.ToString(CultureInfo.InvariantCulture)) };
+        int effectivePageSize = Math.Min(pageSize, MaxPageSize);
+        var queryParams = new List<(string Key, string Value)> { ("pageSize", effectivePageSize.ToString(CultureInfo.InvariantCulture)) };
 
         if (state.Offset is { Length: > 0 } offset)
             queryParams.Add(("offset", offset));

src/Sources/NeoReports.Sources.Elasticsearch/ElasticsearchBatchSource.cs

@@ -83,6 +83,8 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
         {
             using JsonDocument document = await JsonDocument.ParseAsync(responseBody, cancellationToken: cancellationToken).ConfigureAwait(false);
 
+            EnsureSearchWasComplete(document.RootElement);
+
             JsonElement hits = JsonRecords.GetArray(document.RootElement, "hits.hits");
             var records = new List<T>(context.PageSize);
             JsonElement? lastHit = null;
@@ -154,4 +156,43 @@ private byte[] BuildSearchBody(ElasticsearchCursorState state, int pageSize)
 
         return stream.ToArray();
     }
+
+    /// <summary>
+    /// Rejects a search that Elasticsearch answered only partially.
+    /// <para>
+    /// A partial search is <b>HTTP 200</b> with fewer hits than the shards actually hold: the cluster
+    /// sets <c>timed_out</c>, or reports failed shards under <c>_shards</c>, and returns whatever the
+    /// responsive shards had. Read as an ordinary short page, that ends pagination and the report is
+    /// delivered as <c>Completed</c> while silently missing rows — the worst outcome available, since
+    /// nothing downstream can tell it apart from a genuinely complete run.
+    /// </para>
+    /// <para>
+    /// This is the same posture GraphQL (D63) already takes toward a 200 carrying <c>errors</c>, and
+    /// the same direction as this source's own "full page with no sort values" guard: fail loudly
+    /// rather than truncate quietly (ADR D72).
+    /// </para>
+    /// </summary>
+    private static void EnsureSearchWasComplete(JsonElement root)
+    {
+        if (JsonRecords.TryGetField(root, "timed_out", out JsonElement timedOut)
+            && timedOut.ValueKind == JsonValueKind.True)
+        {
+            throw new HttpSourceException(null, null,
+                "Elasticsearch reported 'timed_out': the search returned only the hits it had gathered " +
+                "before the timeout, so the report would silently be missing rows. Raise the search " +
+                "timeout, or narrow the query.");
+        }
+
+        if (JsonRecords.TryGetField(root, "_shards.failed", out JsonElement failed)
+            && failed.ValueKind == JsonValueKind.Number
+            && failed.TryGetInt32(out int failedShards)
+            && failedShards > 0)
+        {
+            JsonRecords.TryGetField(root, "_shards.total", out JsonElement total);
+            string totalText = total.ValueKind == JsonValueKind.Number ? total.GetRawText() : "?";
+            throw new HttpSourceException(null, null,
+                $"Elasticsearch reported {failedShards} of {totalText} shards failed, so the response " +
+                "covers only part of the index and the report would silently be missing rows.");
+        }
+    }
 }

src/Sources/NeoReports.Sources.Http/HttpBatchSource.cs

@@ -97,8 +97,8 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
             {
                 HttpPaginationStrategy.LinkHeader => BuildLinkHeaderResult(records, response, requestUri),
                 HttpPaginationStrategy.Cursor => BuildCursorResult(records, document.RootElement, state),
-                HttpPaginationStrategy.Page => BuildPageResult(records, state, context.PageSize),
-                HttpPaginationStrategy.Offset => BuildOffsetResult(records, state, context.PageSize),
+                HttpPaginationStrategy.Page => BuildPageResult(records, state),
+                HttpPaginationStrategy.Offset => BuildOffsetResult(records, state),
                 _ => throw new InvalidOperationException($"Unsupported pagination strategy '{_options.PaginationStrategy}'."),
             };
         }
@@ -211,8 +211,11 @@ private static IEnumerable<string> SplitLinkValues(string headerValue)
         {
             foreach (string link in SplitLinkValues(headerValue))
             {
-                string[] parts = link.Split(';');
-                if (parts.Length < 2)
+                // Guarded on the way in rather than assigned then checked: a bare `var parts =
+                // link.Split(...)` as the loop's first statement reads to CodeQL as a map that should
+                // have been a .Select, which this loop cannot be — it has two guards and an early
+                // return (alert cs/linq/missed-select, opened by the Link-parsing fix in #262).
+                if (link.Split(';') is not { Length: >= 2 } parts)
                     continue;
 
                 string urlPart = parts[0].Trim();
@@ -269,18 +272,25 @@ private BatchResult<T> BuildCursorResult(List<T> records, JsonElement responseRo
         return new BatchResult<T>(records, cursor, hasMore);
     }
 
-    private BatchResult<T> BuildPageResult(List<T> records, HttpCursorState state, int pageSize)
+    // Page and Offset have no next-page token to follow, so "is there more?" can only be inferred.
+    // Inferring it from a FULL page is wrong whenever the service caps the page below what was asked
+    // for — Dynamics, SAP Gateway and Business Central all clamp, and many REST APIs silently reduce
+    // an over-max limit. The short first page then reads as the last one and the run reports
+    // Completed with a fraction of the data. Paging until a page comes back EMPTY costs one extra
+    // request at the end of a run and cannot truncate (ADR D72).
+    private BatchResult<T> BuildPageResult(List<T> records, HttpCursorState state)
     {
         int currentPage = state.Page ?? _options.StartPage;
-        bool hasMore = records.Count == pageSize;
+        bool hasMore = records.Count > 0;
         string? cursor = hasMore ? HttpPagination.Encode(new HttpCursorState(Page: currentPage + 1)) : null;
         return new BatchResult<T>(records, cursor, hasMore);
     }
 
-    private BatchResult<T> BuildOffsetResult(List<T> records, HttpCursorState state, int pageSize)
+    /// <inheritdoc cref="BuildPageResult"/>
+    private static BatchResult<T> BuildOffsetResult(List<T> records, HttpCursorState state)
     {
         int currentOffset = state.Offset ?? 0;
-        bool hasMore = records.Count == pageSize;
+        bool hasMore = records.Count > 0;
         string? cursor = hasMore ? HttpPagination.Encode(new HttpCursorState(Offset: currentOffset + records.Count)) : null;
         return new BatchResult<T>(records, cursor, hasMore);
     }

src/Sources/NeoReports.Sources.HubSpot/HubSpotBatchSource.cs

@@ -78,9 +78,19 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
         return new BatchResult<T>(records, cursor, hasMore);
     }
 
+    /// <summary>
+    /// The largest page HubSpot accepts. The engine's own default is 1000, and sending that made the
+    /// very first request fail until the author happened to call <c>.PageSize(100)</c> — a default
+    /// configuration that could not work. The maintainer chose clamping over failing (ADR D72): a
+    /// report author should not have to know each provider's ceiling, and a page size is a
+    /// throughput hint, not a promise about how many rows arrive at once.
+    /// </summary>
+    private const int MaxPageSize = 100;
+
     private Uri BuildRequestUri(HubSpotCursorState state, int pageSize)
     {
-        var queryParams = new List<(string Key, string Value)> { ("limit", pageSize.ToString(CultureInfo.InvariantCulture)) };
+        int effectivePageSize = Math.Min(pageSize, MaxPageSize);
+        var queryParams = new List<(string Key, string Value)> { ("limit", effectivePageSize.ToString(CultureInfo.InvariantCulture)) };
 
         if (state.After is { Length: > 0 } after)
             queryParams.Add(("after", after));

src/Sources/NeoReports.Sources.OData/ODataBatchSource.cs

@@ -97,7 +97,7 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
             return _options.PaginationStrategy switch
             {
                 ODataPaginationStrategy.NextLink => BuildNextLinkResult(records, root, requestUri),
-                ODataPaginationStrategy.Skip => BuildSkipResult(records, state, context.PageSize),
+                ODataPaginationStrategy.Skip => BuildSkipResult(records, state),
                 _ => throw new InvalidOperationException($"Unsupported pagination strategy '{_options.PaginationStrategy}'."),
             };
         }
@@ -156,11 +156,15 @@ private static BatchResult<T> BuildNextLinkResult(List<T> records, JsonElement r
         return new BatchResult<T>(records, cursor, hasMore);
     }
 
-    private BatchResult<T> BuildSkipResult(List<T> records, ODataCursorState state, int pageSize)
+    // Skip has no server token to follow, so "is there more?" can only be inferred. Inferring it
+    // from a page that filled $top is wrong whenever the service caps the page below what was asked
+    // for — Dynamics, SAP Gateway and Business Central all clamp $top — because the short first page
+    // then reads as the last one and the run reports Completed with a fraction of the data. Paging
+    // until a page comes back EMPTY costs one extra request at the end and cannot truncate (D72).
+    private static BatchResult<T> BuildSkipResult(List<T> records, ODataCursorState state)
     {
         int currentSkip = state.Skip ?? 0;
-        int top = _options.TopValue ?? pageSize;
-        bool hasMore = records.Count == top;
+        bool hasMore = records.Count > 0;
         string? cursor = hasMore ? ODataPagination.Encode(new ODataCursorState(Skip: currentSkip + records.Count)) : null;
         return new BatchResult<T>(records, cursor, hasMore);
     }

tests/NeoReports.Sources.Airtable.UnitTests/AirtableBatchSourceTests.cs

@@ -45,6 +45,23 @@ private static async Task<List<T>> CollectAsync<T>(IBatchSource<T> source, int p
     private static HttpResponseMessage JsonResponse(string json, HttpStatusCode status = HttpStatusCode.OK) =>
         new(status) { Content = new StringContent(json, Encoding.UTF8, "application/json") };
 
+    [Fact]
+    public async Task The_engines_default_page_size_is_clamped_to_what_Airtable_accepts()
+    {
+        // The engine defaults to 1000, Airtable rejects anything over 100. Sending the default made
+        // the very first request fail until the author happened to call .PageSize(100) — a default
+        // configuration that could not work. The maintainer chose clamping over failing (ADR D72).
+        HttpClient client = StubHttpMessageHandler.CreateClient(
+            _ => JsonResponse("""{"records":[{"id":"rec1","fields":{"name":"Alpha","done":false}}]}"""),
+            out StubHttpMessageHandler handler);
+
+        var source = Source.Airtable("appXXX", "Projects", "token123", client).As<Project>();
+
+        await source.ReadBatchAsync(new BatchContext(Exec(), 1000, null, 1), CancellationToken.None);
+
+        handler.Requests[0].RequestUri!.ToString().ShouldBe("https://api.airtable.com/v0/appXXX/Projects?pageSize=100");
+    }
+
     [Fact]
     public async Task Paginates_via_offset_until_absent()
     {