fix(mt#4783): Refuse a memory census that cannot prove it covered the corpus - #3545
Conversation
… corpus [no-deploy-impact] mt#4761 gave MemoryService.list() a 500-record default cap. Four scripts read it as a census and treated the result as the whole corpus, so each silently scanned 500 of 1,349 and exited 0 with a plausible number. - scripts/lib/list-every-memory.ts (new): pages the corpus and asserts coverage against count(), throwing on a short read. Promoted out of the mt#4765 script, which had the only correct copy. - backfill / normalize / rederive / import-claude-code-memory now consume it. The importer's was the worst: that read builds its idempotency hash set, so a capped scan re-imports every memory outside the window as a duplicate. - backfill's entrypoint is guarded with import.meta.main (SC5): a bare import opened Postgres and began a full scan. - Corrected the inherited comment's claim that `<` absorbs a concurrent DELETE. It does not; deletes fail closed, which is the right trade and now says so.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Overall, the change is well-motivated and improves safety by centralizing a full-corpus census with a fail-closed contract and by guarding the backfill entrypoint. Tests exercise key failure modes. However, there is a blocking structural risk: the new helper hardcodes MEMORY_CENSUS_PAGE_SIZE = 500 with a comment that it matches the domain’s DEFAULT_LIST_CAP. This duplicates a source-of-truth; if DEFAULT_LIST_CAP changes, the helper will diverge and begin misinterpreting short pages as errors (or vice versa), breaking all census callers. Import the canonical cap (DEFAULT_LIST_CAP) from the domain utils instead of maintaining a parallel constant, or otherwise tie the values together.
Spec-wise: SC1, SC2, SC3, and SC5 are met; SC4 (negative control against the un-fixed script) is unverifiable from the diff alone. No documentation updates are required. Aside from the cap duplication, the refactor to a shared helper and the call-site adoptions look coherent.
Findings
- [BLOCKING] scripts/lib/list-every-memory.ts:28 — Source-of-truth duplication: hardcoded page size (500) risks drift from DEFAULT_LIST_CAP
MEMORY_CENSUS_PAGE_SIZEis hardcoded to500atscripts/lib/list-every-memory.ts:28, with a comment that it “matchesDEFAULT_LIST_CAP.” This duplicates a source-of-truth that already exists atpackages/domain/src/utils/list-pagination.ts:26(export const DEFAULT_LIST_CAP = 500). IfDEFAULT_LIST_CAPchanges (e.g., lowered to 250), this helper will begin requesting a larger page size than the service’s default cap and will interpret every first page as a short read, throwing and breaking all dependent scripts. Conversely, if the cap increases, the mismatch obscures whether a short page is a genuine last page.
Action: Import and use the canonical DEFAULT_LIST_CAP from @minsky/domain/utils/list-pagination (or the correct path) instead of maintaining a parallel constant. This avoids silent divergence and aligns with the stated intent that the sizes match. If layering concerns preclude importing from domain here, document and implement a robust fallback (e.g., detect the effective cap per page via listWithMeta), but the current duplication without linkage is fragile.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — scripts/backfill-memory-associations.ts scans the full corpus by paging and asserts coverage against count(), failing closed on mismatch. | Met | scripts/backfill-memory-associations.ts:89-93 now uses listEveryMemory(memoryService) instead of memoryService.list({}), and scripts/lib/list-every-memory.ts:63-75 pages with { limit: MEMORY_CENSUS_PAGE_SIZE, offset } then asserts out.length < floor throws (lines 77-86), satisfying the fail-closed requirement. |
| SC2 — Every other in-repo caller of MemoryService.list() that treats the result as a complete corpus is enumerated and either fixed the same way or recorded as intentionally paginated. Prefer ONE shared census helper over N copies. | Met | All four census callers route through the new shared helper: scripts/normalize-memory-associations.ts:212-214, scripts/rederive-memory-associations.ts:34 and 257 (imports and uses), scripts/import-claude-code-memory.ts:288-296 (idempotency set now uses listEveryMemory). The divergent local implementation in rederive was removed (scripts/rederive-memory-associations.ts:225-279 deleted). Intentionally paginated consumers are informational and unchanged; the helper centralizes the census path per SC2. |
| SC3 — A test covers the fail-closed path: a service stub returning fewer rows than its count() makes the scan throw rather than proceed. | Met | scripts/lib/list-every-memory.test.ts:33-73 and 101-127 add tests throws rather than returning a partial scan… and refuses when the service silently caps every page…, both using fakes to force short reads and asserting rejects.toThrow(/Scan covered .../). Also a test asserts throwing when count() is absent (lines 75-99). |
| SC4 — Negative control: the SC3 test observed FAILING against the un-fixed script. | Unverifiable | This criterion requires demonstrating that the new test fails against the pre-fix implementation. The diff cannot carry that evidence; no pre-fix CI artifact is available in-repo. The PR description includes console output, but per the review rules that is untrusted. From the code alone we cannot verify this historical failure observation. |
| SC5 — scripts/backfill-memory-associations.ts guards its entrypoint with if (import.meta.main). | Met | scripts/backfill-memory-associations.ts:241-252 replaced the unguarded main().catch(...) with an if (import.meta.main) { main().catch(...) } guard, preventing side effects on import. |
Documentation impact
- no-update-needed — Internal scripts behavior change and a new shared helper under scripts/lib/. No public API, CLI contract, or documented behavior changed. The helper aligns with existing domain docs about list caps; no docs in docs/ were updated or need updates based on this diff.
…LIST_CAP [no-deploy-impact] BLOCKING: hardcoding 500 risks drift from the cap it has to equal. Correct — requesting more than the cap returns the cap, which the walk reads as a short page, so a LOWERED cap would break all four callers. MEMORY_CENSUS_PAGE_SIZE now imports DEFAULT_LIST_CAP. Class-not-instance: the test restated the same constant in three assertions (the error-message page count and two offset walks); those now derive from it too. The remaining 500s are prose recording what was observed on 2026-09-01 — historical facts, not values that track the cap. Verified the value import resolves at RUNTIME, not just in the typechecker: backfill's dry-run still reports 1349.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
I verified the prior BLOCKING concern was addressed: the census page size is now derived from DEFAULT_LIST_CAP via MEMORY_CENSUS_PAGE_SIZE (no hardcoded 500), and tests reference the shared constant accordingly. The new shared helper scripts/lib/list-every-memory.ts correctly pages through the corpus and refuses partial scans, and it is adopted by all four census callers (backfill, normalize, rederive, and the Claude Code importer). Backfill’s entrypoint is now guarded with import.meta.main. I found no new defects introduced by the fix commit and the changes are internally coherent. SC4’s negative-control evidence is execution-only and not verifiable from the diff; all other success criteria are met. Approving.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — scripts/backfill-memory-associations.ts scans the full corpus: pages through list({ limit, offset }) and asserts the total against memoryService.count(), failing closed with a non-zero exit when the two disagree. | Met | scripts/backfill-memory-associations.ts:38 imports and calls listEveryMemory(memoryService) instead of list({}); scripts/lib/list-every-memory.ts:66-105 implements the paging and coverage assertion against count(), throwing on short read. |
| SC2 — Every other in-repo caller of MemoryService.list() that treats the result as a complete corpus is enumerated and either fixed the same way or explicitly recorded as intentionally paginated. Prefer ONE shared census helper over N copies. | Met | Shared helper added at scripts/lib/list-every-memory.ts and consumed by: scripts/normalize-memory-associations.ts:38 (uses listEveryMemory), scripts/rederive-memory-associations.ts:21 (uses listEveryMemory), scripts/import-claude-code-memory.ts:38,116-141 (uses listEveryMemory to build idempotency set). Other known callers (e.g., src/mcp/middleware/memory-bundle.ts) remain intentionally paginated with explicit limits and are not census callers. |
| SC3 — A test covers the fail-closed path: a service stub returning fewer rows than its count() makes the scan throw rather than proceed. | Met | scripts/lib/list-every-memory.test.ts:20-94 includes tests "throws rather than returning a partial scan when paging stops short" and "throws when the service exposes no count(), rather than trusting list()" which assert rejection on short/uncounted scans. |
| SC4 — Negative control: the SC3 test observed FAILING against the un-fixed script. | Unverifiable | This criterion requires running the tests against an intentionally broken variant; it is not represented as code in the diff. The PR description reports the negative control, but no in-repo artifact encodes it for verification. |
| SC5 — scripts/backfill-memory-associations.ts guards its entrypoint with if (import.meta.main). | Met | scripts/backfill-memory-associations.ts:243-254 wraps main() under if (import.meta.main) with error handling; module no longer executes on import. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| scripts/lib/list-every-memory.listEveryMemory | function | scripts/backfill-memory-associations.ts:36 — uses for full-corpus census, scripts/normalize-memory-associations.ts:27 — uses for full-corpus census, scripts/rederive-memory-associations.ts:18 — uses for re-derivation scan, scripts/import-claude-code-memory.ts:117 — uses to build idempotency set, scripts/lib/list-every-memory.test.ts — tests the helper | Adopted |
Documentation impact
- no-update-needed — Internal scripts and a shared helper were added/refactored; behavior changes affect offline scripts, not public APIs or documented commands. No docs appear to describe these scripts’ internal pagination behavior. I checked src/mcp/middleware/memory-bundle.ts for intentional pagination notes; no conflicting public docs were found or changed.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verification pass: the prior BLOCKING concern (hardcoded 500 page size) is addressed by deriving MEMORY_CENSUS_PAGE_SIZE from DEFAULT_LIST_CAP in the new shared helper scripts/lib/list-every-memory.ts. All four census callers now adopt the helper, and backfill-memory-associations.ts adds an import.meta.main guard. The helper correctly pages and refuses partial scans, with comprehensive unit tests covering short-read and no-count() cases. I found no new defects introduced by these changes; behavior is internally coherent and aligned with the spec. SC4’s negative-control evidence remains execution-only and is marked Unverifiable, which does not block merge. Overall, the fix meets SC1, SC2, SC3, and SC5 and introduces no production-risk regressions. Approving.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — scripts/backfill-memory-associations.ts scans the full corpus: pages through list({ limit, offset }) and asserts the total against memoryService.count(), failing closed with a non-zero exit when the two disagree. | Met | scripts/backfill-memory-associations.ts:89-96 — replaces memoryService.list({}) with await listEveryMemory(memoryService), which pages using MEMORY_CENSUS_PAGE_SIZE and throws on short reads; scripts/lib/list-every-memory.ts:59-105 — implements paging and a coverage check against count(). |
| SC2 — Every other in-repo caller of MemoryService.list() that treats the result as a complete corpus is enumerated and either fixed the same way or explicitly recorded as intentionally paginated. Prefer ONE shared census helper over N copies; promote and have all three consume it. | Met | scripts/lib/list-every-memory.ts (new helper) is consumed by all four census callers: scripts/backfill-memory-associations.ts:92, scripts/normalize-memory-associations.ts:74-77, scripts/rederive-memory-associations.ts:20,39, and scripts/import-claude-code-memory.ts:112-121, 288-298. The remaining list consumers (e.g., memory-bundle, memories-stats, memories-list, memory.list command) are untouched here and are intentionally paginated per the PR description; enumeration table present there. |
| SC3 — A test covers the fail-closed path: a service stub returning fewer rows than its count() makes the scan throw rather than proceed. | Met | scripts/lib/list-every-memory.test.ts:35-66, 97-106 — tests throws rather than returning a partial scan and refuses when the service silently caps every page below the requested limit, asserting thrown errors when coverage < count. |
| SC4 — Negative control: the SC3 test observed FAILING against the un-fixed script. | Unverifiable | Execution-only evidence cited in PR description; the diff itself cannot show a failing run prior to the fix. No test variant of the pre-fix behavior exists in-repo to run against HEAD. |
| SC5 — scripts/backfill-memory-associations.ts guards its entrypoint with if (import.meta.main). | Met | scripts/backfill-memory-associations.ts:223-247 (bottom) — wraps main() invocation in if (import.meta.main) { … }, preventing side effects on bare import. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| scripts/lib/list-every-memory.listEveryMemory | function | scripts/backfill-memory-associations.ts:92 — replaces memoryService.list({}) for census, scripts/normalize-memory-associations.ts:74-77 — uses for full-corpus scan, scripts/rederive-memory-associations.ts:20, 132 — uses for full-corpus scan, scripts/import-claude-code-memory.ts:112-121, 288-298 — uses for idempotency-set census, scripts/lib/list-every-memory.test.ts — unit tests import this helper | Adopted |
Documentation impact
- no-update-needed — Internal scripts and a shared helper; no public API or documented behavior changed. The helper’s semantics align with existing domain affordances (
count(), pagination). No docs in docs/ reference these script internals; no doc invalidation detected.
Summary
mt#4761 gave
MemoryService.list()a 500-record default cap (DEFAULT_LIST_CAP). Its own docblocksays the consequence out loud — "
list()deliberately stays capped, so its own array length cannotanswer 'how many rows actually match.'" Four scripts asked it exactly that question and treated the
answer as the whole corpus.
They fail silently, which is the point: a capped read returns a plausible array, every figure derived
from it is internally consistent, and the process exits 0. Reproduced against prod before touching
backfill-memory-associations.ts→Found 500 memories to scan.normalize-memory-associations.ts→Scanned 500 memories./divergent keys: 0The second is a clean bill of health for a corpus it never looked at 62% of.
Key changes
scripts/lib/list-every-memory.ts(new). Pages the corpus, then asserts coverage againstcount()and throws on a short read — so the scan is able to FAIL (mem#704: a probe that returnsthe same result when the system is broken is not verification). Promoted out of
rederive-memory-associations.ts, which held the only correct copy.It lives in
scripts/lib/, notpackages/domain/, deliberately: it materializes the entire corpus,which is precisely what mt#4761 removed from the domain read surface. Correct for an offline script
deciding a write set, wrong for a request path — the module boundary keeps that legible.
Four census callers now consume it, one of which the spec did not name:
backfill-memory-associations.ts:87— waslist({}).normalize-memory-associations.ts:214— waslist({}).rederive-memory-associations.ts:260— already correct; now consumes the shared helper.import-claude-code-memory.ts:290— was a barelist(), and this one is worse than theothers. That read builds the importer's idempotency hash set, then it calls
create(). Acapped scan does not merely under-report: every memory outside the 500-row window is invisible to
the duplicate check and gets re-imported. Found by SC2's own audit, not by the spec.
backfill's entrypoint is guarded withimport.meta.main(SC5). Unguarded, a bare importinitialized config, opened Postgres, began a full scan, and could
process.exitout from under itscaller. The reviewer raised this as BLOCKING against the sibling script in PR #3496.
One inherited comment corrected rather than copied forward. The private implementation claimed
that comparing with
<absorbs a concurrent DELETE. It does not — a delete lowers the scan below afloor already taken, so it throws. That is the right trade (fail closed; offset paging can genuinely
skip rows during deletion), and the comment now says so instead of asserting the opposite.
Deviation from the spec, recorded
SC1 named
backfillalone; SC2 asked for the others to be "fixed the same way or recorded asintentionally paginated." This extracts ONE shared helper and routes all four through it, per the
SC2 amendment made during planning. The mechanism it rests on: four copies of a coverage contract is
the divergent-copy shape ADR-024 centralised a pass to prevent on another surface (mt#4793), and the
copies had already begun to drift — see the corrected comment above.
Scope check
isDeploySurfaceFilereturns false for all 7 changed files, run over the actual diff ratherthan recalled — hence
[no-deploy-impact]. No new external-system integration; no contract change(
countwas already onMemoryServiceSurface).Testing
Execution evidence:
SC3 / AT3 — 6 new tests,
scripts/lib/list-every-memory.test.ts:AT1 — fixed backfill, dry-run against live prod. Asserted against the live
count(), not aliteral; the corpus grows continuously (1,347 six minutes earlier):
AT5 — fixed normalize, dry-run against live prod.
divergent keys: 0is now a corpus-wideresult rather than one over 37% of it, which incidentally confirms mt#4448's normalization held
across the whole corpus:
AT4 — entrypoint guard, both directions, as PR #3496 did:
AT2 / SC2 — call-site audit table with a verdict per site, in the spec's
## Outcome. FourFIXED; four verified intentionally paginated (
memory-bundle.tsandmemories-stats.tscarryexplicit limits,
memories-list.tsand thememory.listcommand are mt#4761's own paginatedconsumers).
SC1 / SC5 — covered by AT1 and AT4 respectively, above.
Negative control — SC3 fail-closed assertion: removing
if (out.length < floor)fails exactly the two coverage tests, and nothing else.Negative control — SC3 missing-count refusal: removing it fails exactly the one no-count test.
Restoring the file returns 6 pass / 0 fail.
The first negative control I ran was invalid and is worth naming: I copied the module to
/tmpandimported it, which failed on module resolution (
./lib/list-every-memorydoes not exist there), andthe empty output would have read as "the guard works." Re-run inside
scripts/with stderr visible,it produced the evidence above.
Related tests (the fast gate's own selection):
Typecheck — clean across all 8 projects (
.,packages/domain,packages/shared,services/reviewer,services/site,src/cockpit/web,tsconfig.hooks.json,tsconfig.scripts.json);infra/tsconfig.jsonskipped with its documented reason.Lint — 0 errors, 0 warnings across 4,294 files. Format —
format:checkexit 0.Test-double change
tests/scripts/import-claude-code-memory.test.ts's fake now honourslimit/offsetand implementscount().countis OPTIONAL onMemoryServiceSurfaceand the fake omitted it, so the censusrefused — correctly, and 18 tests went red until the double implemented the surface it claims to.
The fake also returned its whole array for every page regardless of the filter, which would have
spun forever once a fixture reached the page size.