Skip to content

Core code treats one query() page as the complete result set#89

Merged
cuibonobo merged 1 commit into
mainfrom
claude/issue-50-design-review-t7kx2m
Jul 13, 2026
Merged

Core code treats one query() page as the complete result set#89
cuibonobo merged 1 commit into
mainfrom
claude/issue-50-design-review-t7kx2m

Conversation

@cuibonobo

Copy link
Copy Markdown
Member

Summary

query() always paginates — an absent limit returns one adapter-default page, never every match — but several internal call sites in packages/core/src/stack.ts assumed a single call was exhaustive:

  • ScopedStack.query()'s grant prefetch and hasGrant()'s non-prefetched path: only the first page of _grant@1 records was ever considered, so a grant past page one was silently invisible — legitimate access denied with no error.
  • Stack.deleteAttachment()'s fallback metadata scan: on adapters without contentFieldQuery, only the first page of _attachment@1 records was scanned, so metadata beyond page one survived bytes deletion — an orphan.
  • ScopedStack.getAttachment()'s uploader check: hardcoded limit: 1 even on the in-memory-matching path, so it could only ever see one of the requester's uploads. This one didn't need 50+ records to misfire — it false-denies with as few as two uploads.

Changes

  • Added queryAllPages() — a cursor-walking helper (throws StackQueryError rather than silently truncating past a generous safety cap) — and applied it at the three sites above. Left migrateAll()'s existing cursor walk as-is: it interleaves per-page mutation with fetching (already-migrated records naturally drop out of the filter as it goes), so "collect everything first" doesn't fit its access pattern.
  • Kept the contentFieldQuery fast path everywhere it applies (a narrowed query already terminates in one page) — the full scan only runs on adapters without that capability.
  • Brought MemoryAdapter (core's test double) up to real filter parity — content, tags, hasAttachment, relatedTo, appId, date ranges, and payload-aware dissociate() — mirroring sqlite-shared's buildWhereClause, so permission-logic tests exercise real predicates instead of an adapter that quietly ignored most filters.
  • Named the "query() always paginates" rule in docs/spec.md §Queries — it was previously unstated, which is how the ambiguity crept in.

Test plan

  • Added regression tests: a grant beyond the first page (>50 _grant records) is still honored; deleteAttachment() leaves no orphaned metadata when the match is beyond the first page (>50 records); a new ScopedStack.getAttachment test suite (previously uncovered) including the n=2 uploader-access regression.
  • Verified each new test fails against the pre-fix code (temporarily reverted the stack.ts fix and confirmed all three failed with the expected symptoms) and passes with it restored.
  • pnpm -r build
  • pnpm -r typecheck
  • pnpm -r lint
  • pnpm -r test (all packages green)
  • pnpm format:check

Generated by Claude Code

Several internal consumers treated a single query() page as the
complete result set, even though query() always paginates: the grant
prefetch behind ScopedStack.query()/hasGrant() could silently deny an
existing grant past the default page, deleteAttachment()'s fallback
could leave orphaned _attachment@1 metadata past page one, and the
getAttachment() uploader check hardcoded limit: 1 on the in-memory
matching path, false-denying access to any upload but the one it
happened to fetch.

Adds a queryAllPages() helper that cursor-walks to exhaustion (and
throws rather than silently truncating past a safety cap), applies it
at each site while keeping the contentFieldQuery fast path where a
narrowed query already terminates immediately, and brings
MemoryAdapter's filter support up to parity with the real adapters so
permission-logic tests exercise real predicates instead of an adapter
that quietly ignores most filters.

Fixes #50
@cuibonobo cuibonobo merged commit 556bb8e into main Jul 13, 2026
5 checks passed
@cuibonobo cuibonobo deleted the claude/issue-50-design-review-t7kx2m branch July 13, 2026 13:55
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.

2 participants