Skip to content

fix(mt#4783): Refuse a memory census that cannot prove it covered the corpus - #3545

Merged
edobry merged 2 commits into
mainfrom
task/mt-4783
Sep 1, 2026
Merged

fix(mt#4783): Refuse a memory census that cannot prove it covered the corpus#3545
edobry merged 2 commits into
mainfrom
task/mt-4783

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

mt#4761 gave MemoryService.list() a 500-record default cap (DEFAULT_LIST_CAP). Its own docblock
says the consequence out loud — "list() deliberately stays capped, so its own array length cannot
answer '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.tsFound 500 memories to scan.
  • normalize-memory-associations.tsScanned 500 memories. / divergent keys: 0

The 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 against
count() and throws on a short read — so the scan is able to FAIL (mem#704: a probe that returns
the 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/, not packages/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 — was list({}).
  • normalize-memory-associations.ts:214 — was list({}).
  • rederive-memory-associations.ts:260 — already correct; now consumes the shared helper.
  • import-claude-code-memory.ts:290 — was a bare list(), and this one is worse than the
    others. That read builds the importer's idempotency hash set, then it calls create(). A
    capped 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 with import.meta.main (SC5). Unguarded, a bare import
initialized config, opened Postgres, began a full scan, and could process.exit out from under its
caller. 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 a
floor 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 backfill alone; SC2 asked for the others to be "fixed the same way or recorded as
intentionally 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

isDeploySurfaceFile returns false for all 7 changed files, run over the actual diff rather
than recalled — hence [no-deploy-impact]. No new external-system integration; no contract change
(count was already on MemoryServiceSurface).

Testing

Execution evidence:

SC3 / AT3 — 6 new tests, scripts/lib/list-every-memory.test.ts:

$ bun test --preload ./tests/setup.ts --timeout=15000 scripts/lib/list-every-memory.test.ts
 6 pass
 0 fail
 9 expect() calls
Ran 6 tests across 1 file. [57.00ms]

AT1 — fixed backfill, dry-run against live prod. Asserted against the live count(), not a
literal; the corpus grows continuously (1,347 six minutes earlier):

$ bun scripts/backfill-memory-associations.ts
Found 1349 memories to scan.
DRY RUN — no changes written. Pass --execute to apply.
EXIT=0

AT5 — fixed normalize, dry-run against live prod. divergent keys: 0 is now a corpus-wide
result rather than one over 37% of it, which incidentally confirms mt#4448's normalization held
across the whole corpus:

$ bun scripts/normalize-memory-associations.ts
Scanned 1349 memories.
Records with divergent keys:      0
Divergent values total:           0

AT4 — entrypoint guard, both directions, as PR #3496 did:

=== bare import of the GUARDED module ===
IMPORT_COMPLETED_INERT

=== same module, guard condition forced true (pre-fix behaviour) ===
IMPORT_COMPLETED
info: Persistence provider created: PostgresVectorPersistenceProvider
info: PersistenceService initialized successfully
Found 1349 memories to scan.

AT2 / SC2 — call-site audit table with a verdict per site, in the spec's ## Outcome. Four
FIXED; four verified intentionally paginated (memory-bundle.ts and memories-stats.ts carry
explicit limits, memories-list.ts and the memory.list command are mt#4761's own paginated
consumers).

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.

guard neutralized
(fail) listEveryMemory > throws rather than returning a partial scan when paging stops short
(fail) listEveryMemory > refuses when the service silently caps every page below the requested limit
 4 pass
 2 fail

Negative control — SC3 missing-count refusal: removing it fails exactly the one no-count test.

refusal neutralized
(fail) listEveryMemory > throws when the service exposes no count(), rather than trusting list()
 5 pass
 1 fail

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 /tmp and
imported it, which failed on module resolution (./lib/list-every-memory does not exist there), and
the 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):

$ bun scripts/run-related-tests.ts scripts/lib/list-every-memory.ts scripts/backfill-memory-associations.ts \
    scripts/normalize-memory-associations.ts scripts/rederive-memory-associations.ts scripts/import-claude-code-memory.ts
 61 pass
 0 fail
run-related-tests.ts: 4 related test file(s) passed

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.json skipped with its documented reason.
Lint — 0 errors, 0 warnings across 4,294 files. Formatformat:check exit 0.

Test-double change

tests/scripts/import-claude-code-memory.test.ts's fake now honours limit/offset and implements
count(). count is OPTIONAL on MemoryServiceSurface and the fake omitted it, so the census
refused — 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.

… 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

minsky-reviewer Bot commented Sep 1, 2026

Copy link
Copy Markdown

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 439K prompt, 7K completion | Duration: 105s
Mode: normal

Commands

  • /review — request a fresh review

@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 1, 2026

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SIZE is hardcoded to 500 at scripts/lib/list-every-memory.ts:28, with a comment that it “matches DEFAULT_LIST_CAP.” This duplicates a source-of-truth that already exists at packages/domain/src/utils/list-pagination.ts:26 (export const DEFAULT_LIST_CAP = 500). If DEFAULT_LIST_CAP changes (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.

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@edobry
edobry merged commit 5abbd62 into main Sep 1, 2026
13 checks passed
@edobry
edobry deleted the task/mt-4783 branch September 1, 2026 03:19

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant