feat(memory): add bounded Memory entry pagination - #1767
wutongyuonce wants to merge 12 commits into
Conversation
Preserve pagination alongside upstream capacity and MCP additions. Keep revision-valid directory updates in the same commit transaction and verify pinned traversal across compaction. Regenerate the checked-in DSH bundle to satisfy the package freshness check.
|
@Teingi CI update: the previous 23 checks are now passing, including Python 3.11–3.14 and OceanBase acceptance. The only remaining failure is Acceptance (sqlite). Its uploaded exception evidence shows that the acceptance, LoCoMo, and fail-fast batches all stopped during agent dependency installation, before scenario execution: The download failed after three retries. This remaining failure is an external dependency-download error, not a pagination assertion failure; it does not establish a SQLite acceptance pass yet. Could a maintainer please rerun the failed SQLite acceptance job? My attempt to rerun it was rejected with HTTP 403 due to repository permissions. |
Preserve both the Memory directory cursor secret and the runtime model usage recorder in scoped service wiring.
|
Minimize additional tables and justify their responsibilities. |
|
Align schema changes and historical backfills with PR #1771.
|
Teingi
left a comment
There was a problem hiding this comment.
Reviewed 6eb0eb3. Three reproducible issues are detailed inline: unbounded authorization work per page, oversized cursors for valid tag filters, and directory reads ignoring an explicitly bound transaction. The third issue is limited to explicit backend composition in the reproduction.
| _COLLECTION_CONTENT_OPERATIONS = frozenset({ | ||
| "search_memory", | ||
| "list_memory_entries", | ||
| "query_memory_entries", |
There was a problem hiding this comment.
[P2] Keep owner-readiness checks bounded for directory pages
Adding this operation to the collection preflight makes every page materialize all retained identities in the Scope and perform a sequential owner lookup for each. With enforced authorization and real SQLite HTTP requests, limit=1 read 200/1,000 identities and issued 200/1,000 owner SELECTs respectively; continuation pages repeated the same work, taking approximately 0.57/2.52 seconds to return one item. The bounded directory query therefore still has whole-collection work ahead of it. Please preserve owner-readiness enforcement without a whole-Scope application scan and per-entry database round trips, and include the enforced-auth HTTP path in the scale checks.
| "scope_id": scope_id, | ||
| "memory_artifact_id": artifact_id, | ||
| "include_inactive": query.include_inactive, | ||
| "tags": [] if query.tag_filter is None else list(query.tag_filter.keys), |
There was a problem hiding this comment.
[P2] Keep cursors within the API limit for every legal tag filter
Embedding the complete normalized tags can exceed the 4096-character limit on both cursor and next_cursor. I tagged two entries with 16 distinct, valid 64-character Chinese labels and queried with that filter and limit=1: the runtime generated a 4592-character cursor, and response mapping returned HTTP 422 with an error on next_cursor. The same filter with limit=100 returned both entries successfully because no cursor was needed; submitting the generated cursor also returned 422. Please bind the filter with a bounded representation, or reconcile the cursor limits so every accepted filter can complete pagination.
| expected_tag_generation: int | None, | ||
| after_entry_id: str, | ||
| ) -> tuple[int, int | None, list[Mapping[Any, Any]]] | None: | ||
| async with self._database.transaction() as connection: |
There was a problem hiding this comment.
[P2] Reuse the bound connection for directory reads
When a RelationalMemoryBackend is constructed with connection=connection inside an open file-SQLite transaction, this method starts a separate transaction and cannot see the caller's pending writes. I reproduced remember() followed by entries() and head() on the same bound service returning one entry at revision 1, while query_directory() returned memory_ref=None and no items; it became correct after the outer commit. Please use the supplied connection consistently with the other backend reads. This reproduction covers explicit backend transaction composition; the default HTTP query path currently uses an unbound service.
|
@wutongyuonce Thanks for the implementation, tests, and scale validation in this PR. We are reviewing unified database migrations (#1771) and the Memory redesign (#1809, dependent on #1803). These designs would affect the directory projection, revision cursors, and migration approach used here. We therefore plan to close this PR for now and keep the pagination requirement in #1656 open until those designs are settled. We'd welcome your continued involvement, and the work you've done provides a useful reference for the next implementation. |
Which issue or RFC does this PR close?
Closes #1656.
Detailed design: RFC 1656: bounded Memory entry pagination.
Rationale for this change
The existing Memory entry list resolves the complete manifest and loads all selected entry bodies. Slicing its HTTP response would not bound persistence or response-assembly work. Clients need a stable, authorized directory page before choosing exact entries to inspect.
What changes are included in this PR?
POST /v1/memory/entries/queryoperation, generated SDK support, Server mapping, and MCP exposure. The legacy full-result operation is unchanged.entry_id ASCorder, with a default limit of 50, maximum of 100, signed continuation, and a 4 MiB encoded item budget.plan/ revision-batchedapply/verifymigration. Until verification succeeds, only the new query returns503 memory_query_index_unavailable; it never falls back to the unbounded path.Are there any user-facing changes?
Yes, but the API change is additive. Existing list, exact-detail, search, and write behavior is unchanged.
Existing databases must complete the documented offline Memory query-index migration before using the new operation. The response is intentionally a compact directory and omits entry text/source/artifact references; callers use the existing exact-detail operation for those fields.
How was this change tested?
make checkmake docs-test— built and verified 859 public pagesmake build— source distribution and wheel builtmake contract-test— 50 passedmake js-test— 266 DSH unit tests and 9 e2e tests passed; the committed bundle matches a fresh builduv run python scripts/measure_memory_directory.py— SQLite 3.50.4 at 200/1,000/5,000 entries; each page returned 100 and materialized 101 compact rows, used the directory and entry-version indexes, and a one-entry revision added/closed one directory row with no head-row count changeThe full local Python suite is not claimed green: the native-code parser worker failed before test logic with
code_parser_failedon this macOS/Python 3.13 environment in the pre-merge run, and the full suite was not rerun locally after integrating master. Fresh hosted CI validates the updated head separately. The live OceanBase migration/capacity tests are present but skipped locally becausePOWERCONTEXT_TEST_OCEANBASE_URLis not configured.AI usage statement
OpenAI Codex GPT-6 Sol was used for repository analysis, implementation, tests, documentation, and validation. The contributor reviewed the design boundaries and generated changes.