/v1/timeline over-bound refusal is untested (new bound introduced by the pagination slice) #87

New issue

Closed

opened 2026-09-23 06:19:43 +00:00 by yuris · 0 comments

yuris commented

2026-09-23 06:19:43 +00:00

Owner

Context

The pagination slice introduced a new behaviour on the timeline route: a page-1 materialization larger than MAX_EXPANDED_INSTANCES (10,000) is refused with limit_exceeded instead of paging an unbounded result. The reviewer established that this bound is new (on main before that PR, /v1/timeline queried SQLite with LIMIT/OFFSET and enforced no ceiling), sanctioned it against the spec's "pages share a bounded materialized read result", and confirmed it is disclosed in the PR body.

It was verified by inspection only. The equivalent checks for tasks and agenda are tested; the timeline refusal is not, because generating 10,000 facts through the API is impractical inside the suite. The reviewer said so explicitly rather than implying coverage.

The code path, for precision:

// src/api/timeline.rs
let limit = (crate::calendar::MAX_EXPANDED_INSTANCES + 1) as u32;
...
if page.has_more || page.facts.len() > crate::calendar::MAX_EXPANDED_INSTANCES {
    return Err(ApiFailure::limit_exceeded(format!(...)));
}

Why it matters

Refusing a read that previously succeeded is a behaviour change, and it is currently the only newly-introduced refusal in the codebase with no test. Two failure modes are invisible today:

The refusal never fires (e.g. the limit is applied after materialization, or has_more is computed wrong) — a client with a large timeline gets a silently truncated page instead of a documented error.
The refusal fires one row early or late relative to the stated bound, so the documented threshold is wrong. Note the predicate is has_more || len > MAX, not len >= MAX — the boundary deserves pinning either way.

What to build

Seed the journal directly (bulk insert through the test's own database worker, not 10,000 API calls) so the materialization exceeds the bound, then assert the refusal: error code limit_exceeded, its documented HTTP status, and its exit code through exit_code_for — not merely the status.
Add the boundary control: a set just under the bound still pages successfully, and its as_of/cursor behaviour is unchanged. Pin which side of the boundary each case lands on, so the > vs >= question is answered by a test rather than by reading the code.
Keep it test-only: no production behaviour change. If the test reveals the predicate is off by one relative to the documented bound, that is a finding — fix the code and say so in the PR, and note that it is a behaviour change for clients near the boundary.

Acceptance criteria

A test exercises the over-bound timeline refusal and asserts code, status and exit code.
A boundary test pins the case just under the bound (still pages) and the case just over it (refused), so the threshold is unambiguous.
The test seeds data without 10,000 API round-trips and runs fast enough for the normal suite.
No production code changes unless the boundary is genuinely off by one; if it is, the change and its client-visible effect are stated in the PR body.

Notes

Reviewed by an independent reviewer agent; verdict APPROVE with no findings.
Boundary behaviour, measured through real HTTP requests: exactly 10,000 facts materialize and page (200, default limit 50, next_cursor and as_of present, page 2 continuing from the same snapshot instant); 10,001 facts are refused with limit_exceeded, 422, exit 2. The threshold is strictly exclusive and matches the documented contract, so no production change was needed and the PR is tests-only.
Seeding is a single recursive CTE insert straight into the events table, which the reviewer verified is the same table and shape the production read path queries — so the tests exercise the real pipeline rather than a bypass. The whole pagination test file runs in ~0.3 s.
The refusal exit code is asserted through the shared mapper (client::exit_code_for) against INVALID_INPUT, not a hard-coded literal.
Extra property confirmed: a small page limit is still refused, because the bound protects page-1 materialization rather than the requested page size.
Reviewer also checked every hunk of the shared pagination test file: no existing assertion weakened or moved.
Reported count: 800 passed / 0 failed (798 baseline + 2 new tests).

Blocked by

None. Related: the pagination slice (#45) which introduced the bound.

## Context The pagination slice introduced a **new** behaviour on the timeline route: a page-1 materialization larger than MAX_EXPANDED_INSTANCES (10,000) is refused with limit_exceeded instead of paging an unbounded result. The reviewer established that this bound is new (on main before that PR, /v1/timeline queried SQLite with LIMIT/OFFSET and enforced no ceiling), sanctioned it against the spec's "pages share a **bounded** materialized read result", and confirmed it is disclosed in the PR body. It was verified **by inspection only**. The equivalent checks for tasks and agenda are tested; the timeline refusal is not, because generating 10,000 facts through the API is impractical inside the suite. The reviewer said so explicitly rather than implying coverage. The code path, for precision: ```rust // src/api/timeline.rs let limit = (crate::calendar::MAX_EXPANDED_INSTANCES + 1) as u32; ... if page.has_more || page.facts.len() > crate::calendar::MAX_EXPANDED_INSTANCES { return Err(ApiFailure::limit_exceeded(format!(...))); } ``` ## Why it matters Refusing a read that previously succeeded is a behaviour change, and it is currently the only newly-introduced refusal in the codebase with no test. Two failure modes are invisible today: 1. The refusal never fires (e.g. the limit is applied after materialization, or has_more is computed wrong) — a client with a large timeline gets a silently truncated page instead of a documented error. 2. The refusal fires **one row early or late** relative to the stated bound, so the documented threshold is wrong. Note the predicate is has_more || len > MAX, not len >= MAX — the boundary deserves pinning either way. ## What to build - Seed the journal **directly** (bulk insert through the test's own database worker, not 10,000 API calls) so the materialization exceeds the bound, then assert the refusal: error code limit_exceeded, its documented HTTP status, and its exit code **through exit_code_for** — not merely the status. - Add the boundary control: a set just **under** the bound still pages successfully, and its as_of/cursor behaviour is unchanged. Pin which side of the boundary each case lands on, so the > vs >= question is answered by a test rather than by reading the code. - Keep it test-only: no production behaviour change. If the test reveals the predicate is off by one relative to the documented bound, that is a finding — fix the code and say so in the PR, and note that it is a behaviour change for clients near the boundary. ## Acceptance criteria - [x] A test exercises the over-bound timeline refusal and asserts code, status and exit code. - [x] A boundary test pins the case just under the bound (still pages) and the case just over it (refused), so the threshold is unambiguous. - [x] The test seeds data without 10,000 API round-trips and runs fast enough for the normal suite. - [x] No production code changes unless the boundary is genuinely off by one; if it is, the change and its client-visible effect are stated in the PR body. ## Notes - Reviewed by an independent reviewer agent; verdict APPROVE with no findings. - Boundary behaviour, measured through real HTTP requests: exactly 10,000 facts materialize and page (200, default limit 50, next_cursor and as_of present, page 2 continuing from the same snapshot instant); 10,001 facts are refused with limit_exceeded, 422, exit 2. The threshold is strictly exclusive and matches the documented contract, so **no production change was needed** and the PR is tests-only. - Seeding is a single recursive CTE insert straight into the events table, which the reviewer verified is the same table and shape the production read path queries — so the tests exercise the real pipeline rather than a bypass. The whole pagination test file runs in ~0.3 s. - The refusal exit code is asserted through the shared mapper (client::exit_code_for) against INVALID_INPUT, not a hard-coded literal. - Extra property confirmed: a small page limit is still refused, because the bound protects page-1 materialization rather than the requested page size. - Reviewer also checked every hunk of the shared pagination test file: no existing assertion weakened or moved. - Reported count: 800 passed / 0 failed (798 baseline + 2 new tests). ## Blocked by None. Related: the pagination slice (#45) which introduced the bound.

yuris changed title from ~~/v1/timeline over-bound refusal is untested (new bound introduced by the pagination slice)~~ to /v1/timeline over-bound refusal is untested (new bound introduced by the pagination slice)

2026-09-23 06:25:41 +00:00

yuris referenced this issue from a pull request that will close it,

2026-09-23 06:32:36 +00:00

test(timeline): assert limit_exceeded refusal and boundary at expanded bound #89

yuris closed this issue

2026-09-23 06:39:06 +00:00

yuris referenced this issue from a commit

2026-09-23 06:39:08 +00:00

test(timeline): assert limit_exceeded refusal and boundary at expanded bound (#89)

yuris referenced this issue from a commit

2026-09-23 11:54:43 +00:00

test(timeline): assert limit_exceeded refusal and boundary at expanded bound (#89)