Skip to content

feat(server): page the listing endpoints over cells the caller may read - #11

Closed
glatinone wants to merge 1 commit into
feat/api-key-authfrom
feat/pagination
Closed

glatinone wants to merge 1 commit into
feat/api-key-authfrom
feat/pagination

Conversation

@glatinone

Copy link
Copy Markdown
Owner

feat(server): page the listing endpoints over cells the caller may read

GET /memories (and its /memories/query alias) took a limit and applied it in
the store, before the access filter. Two consequences, both invisible to the
caller: a page could come back short while readable cells sat just past the
store-level limit, and the response could not say whether that was "that is all"
or "you were filtered". readable_by holds patterns rather than ids, so the
access rule cannot be pushed into the query - it has to be applied over a scan,
which is why limit now counts cells the caller may read instead of cells
examined.

  • amp_server.paging owns that rule, for the same reason ranking, retention
    and the record rules live in their own modules.
  • offset is new, and indexes the readable stream, so page 2 is page 2 of what
    the caller can see. Both backends window the same way - query() gained
    offset, and the adapter contract suite proves the window is over the filtered
    set on each of them.
  • limit is bounded by max_page_size (100), advertised at GET /spec, and a
    larger value is refused with 422 rather than silently clamped.
  • The scan is capped, so a filter that matches almost nothing cannot make one
    request walk the whole store.

Two fields changed meaning, which is why this is a feature and not a tweak:

  • total is gone from the listing response. It was len(results) - the size of
    the page just returned - wearing the name of a count.
  • SearchResponse.total is now returned. Its documentation said "total number
    of cells matched (may exceed limit)"; the value was the length of the list
    beside it, so it could never exceed limit. A name that promises a count the
    server does not compute is worse than no count, and one concept with two names
    across two endpoints is how a client ends up trusting the wrong one.

Also fixed: GET /memories/query was unreachable. FastAPI matches routes in
registration order, and /memories/{memory_id} was declared before the static
alias, so the alias was parsed as a memory id named "query" and answered 403.
The auth sweep test in the previous PR could not catch this - the shadowed route
also required a key, so it answered 401 for the right-looking reason. The new
paging test caught it by asserting on the body. It is now declared first, with a
comment saying why the order matters.

Verified against a live server: 38/38 conformance vectors, 215 server tests,
25 SDK tests, Node 10 offline, mkdocs build --strict clean.

`GET /memories` (and its `/memories/query` alias) took a `limit` and applied it in
the store, before the access filter. Two consequences, both invisible to the
caller: a page could come back short while readable cells sat just past the
store-level limit, and the response could not say whether that was "that is all"
or "you were filtered". `readable_by` holds patterns rather than ids, so the
access rule cannot be pushed into the query - it has to be applied over a scan,
which is why `limit` now counts cells the caller may read instead of cells
examined.

- `amp_server.paging` owns that rule, for the same reason `ranking`, `retention`
  and the record rules live in their own modules.
- `offset` is new, and indexes the readable stream, so page 2 is page 2 of what
  the caller can see. Both backends window the same way - `query()` gained
  `offset`, and the adapter contract suite proves the window is over the filtered
  set on each of them.
- `limit` is bounded by `max_page_size` (100), advertised at `GET /spec`, and a
  larger value is refused with `422` rather than silently clamped.
- The scan is capped, so a filter that matches almost nothing cannot make one
  request walk the whole store.

Two fields changed meaning, which is why this is a feature and not a tweak:

- `total` is gone from the listing response. It was `len(results)` - the size of
  the page just returned - wearing the name of a count.
- `SearchResponse.total` is now `returned`. Its documentation said "total number
  of cells matched (may exceed `limit`)"; the value was the length of the list
  beside it, so it could never exceed `limit`. A name that promises a count the
  server does not compute is worse than no count, and one concept with two names
  across two endpoints is how a client ends up trusting the wrong one.

Also fixed: **`GET /memories/query` was unreachable.** FastAPI matches routes in
registration order, and `/memories/{memory_id}` was declared before the static
alias, so the alias was parsed as a memory id named "query" and answered `403`.
The auth sweep test in the previous PR could not catch this - the shadowed route
also required a key, so it answered 401 for the right-looking reason. The new
paging test caught it by asserting on the body. It is now declared first, with a
comment saying why the order matters.

Verified against a live server: 38/38 conformance vectors, 215 server tests,
25 SDK tests, Node 10 offline, `mkdocs build --strict` clean.
@glatinone

Copy link
Copy Markdown
Owner Author

Landed on master in the v0.1.0 chain: the branch was fast-forward merged as part of b940905..92b88ee and released as v0.1.0. Closing so the open list matches reality - the commits are in master, and the tag points at them.

@glatinone glatinone closed this Oct 4, 2026
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.

1 participant