diff --git a/scripts/backfill-memory-associations.ts b/scripts/backfill-memory-associations.ts index 9262eb3800..06e0668dd0 100644 --- a/scripts/backfill-memory-associations.ts +++ b/scripts/backfill-memory-associations.ts @@ -38,6 +38,7 @@ import "reflect-metadata"; import type { MemoryServiceSurface, MemoryServiceDb } from "@minsky/domain/memory/memory-service"; import { extractTrackingTaskRefs } from "@minsky/domain/memory/staleness"; +import { listEveryMemory } from "./lib/list-every-memory"; async function buildMemoryService(): Promise { const { initializeConfiguration, CustomConfigFactory } = await import( @@ -84,7 +85,9 @@ async function main() { const execute = process.argv.includes("--execute"); const memoryService = await buildMemoryService(); - const allMemories = await memoryService.list({}); + // Census, not a page. `list({})` here silently capped at 500 over a 1,347-record corpus and + // printed a plausible count (mt#4783); this throws on a short scan instead. + const allMemories = await listEveryMemory(memoryService); console.log(`Found ${allMemories.length} memories to scan.\n`); @@ -244,7 +247,14 @@ function countTaskRefs(content: string): number { return refs.size; } -main().catch((err) => { - console.error("Fatal error:", err); - process.exit(1); -}); +// Guarded so a bare IMPORT of this module is inert (mt#4783 SC5). Unguarded, `main()` ran at +// module scope: any importer — a test, a future reuse of `buildMemoryService()` — would +// initialize config, open a Postgres connection, begin a full corpus scan, and be able to +// `process.exit` out from under its caller. The reviewer raised exactly this as BLOCKING against +// the sibling script in PR #3496, where a negative control confirmed each step. +if (import.meta.main) { + main().catch((err) => { + console.error("Fatal error:", err); + process.exit(1); + }); +} diff --git a/scripts/import-claude-code-memory.ts b/scripts/import-claude-code-memory.ts index 1a7567dd40..71b1329da5 100644 --- a/scripts/import-claude-code-memory.ts +++ b/scripts/import-claude-code-memory.ts @@ -38,6 +38,7 @@ import type { MemoryRecord, } from "@minsky/domain/memory/types"; import { MEMORY_TYPES } from "@minsky/domain/memory/types"; +import { listEveryMemory } from "./lib/list-every-memory"; // ─── Types ─────────────────────────────────────────────────────────────────── @@ -287,7 +288,12 @@ export async function runImport( // Build existing hash set for idempotency let existingHashes: Set; try { - const existing = await service.list(); + // Census, not a page (mt#4783). This set is the IDEMPOTENCY guard, so a capped read does + // not merely under-report — every memory outside the window is invisible to the dedup + // check and gets re-imported as a duplicate by `service.create()` below. Throws on a short + // scan; the catch below turns that into a reported error and aborts the import, which is + // the right outcome — better no import than a silently duplicating one. + const existing = await listEveryMemory(service); existingHashes = buildExistingHashSet(existing); } catch (err) { report.errors.push({ file: "(list)", error: `Cannot list existing memories: ${String(err)}` }); diff --git a/scripts/lib/list-every-memory.test.ts b/scripts/lib/list-every-memory.test.ts new file mode 100644 index 0000000000..cb2054b3d4 --- /dev/null +++ b/scripts/lib/list-every-memory.test.ts @@ -0,0 +1,129 @@ +/** + * mt#4783 SC3 / AT3 — the census read refuses a partial scan instead of reporting a number. + * + * The defect this guards is silent by construction: `list({})` capped at 500 over a 1,347-record + * corpus returns a plausible array, and every figure derived from it is internally consistent. + * So the assertions below are about the scan's ability to FAIL — a probe that returns the same + * result when the system is broken is not verification (mem#704). + * + * The stubs model ROW COUNTS, which is the whole subject here; record contents are irrelevant, + * hence the minimal cast in `rows()`. + */ + +import { describe, expect, test } from "bun:test"; +import type { MemoryRecord } from "@minsky/domain/memory/types"; +import { + listEveryMemory, + MEMORY_CENSUS_PAGE_SIZE, + type MemoryCensusSource, +} from "./list-every-memory"; + +/** `n` distinct placeholder records. Only the count matters to `listEveryMemory`. */ +function rows(n: number, tag = "m"): MemoryRecord[] { + return Array.from({ length: n }, (_, i) => ({ id: `${tag}-${i}` }) as MemoryRecord); +} + +/** + * A service whose corpus is `total` records, paging correctly, reporting `count()` as + * `reportedCount` (defaults to the true total). Records every offset it was asked for, so a test + * can assert the walk actually advanced rather than inferring it from the result length. + */ +function fakeService(opts: { + total: number; + reportedCount?: number; + /** Cap every page at this many rows regardless of the requested limit — the mt#4761 defect. */ + hardCap?: number; +}): MemoryCensusSource & { offsets: number[] } { + const { total, reportedCount = total, hardCap } = opts; + const all = rows(total); + const offsets: number[] = []; + + return { + offsets, + async list(filter) { + const offset = filter?.offset ?? 0; + const limit = filter?.limit ?? MEMORY_CENSUS_PAGE_SIZE; + offsets.push(offset); + const page = all.slice(offset, offset + limit); + return hardCap === undefined ? page : page.slice(0, hardCap); + }, + async count() { + return reportedCount; + }, + }; +} + +describe("listEveryMemory", () => { + test("throws rather than returning a partial scan when paging stops short", async () => { + // Pagination that yields only the first page — the shape a silently-capped read produces. + const service: MemoryCensusSource = { + async list(filter) { + return (filter?.offset ?? 0) === 0 ? rows(MEMORY_CENSUS_PAGE_SIZE) : []; + }, + async count() { + return 1347; + }, + }; + + // The assertion is on the REFUSAL, not on a returned number: one full page out of 1347 is + // exactly what the un-fixed callers printed before exiting 0. Derived from the page size + // rather than restating it — the constant tracks DEFAULT_LIST_CAP. + await expect(listEveryMemory(service)).rejects.toThrow( + new RegExp(`Scan covered ${MEMORY_CENSUS_PAGE_SIZE} of at least 1347 memories`) + ); + }); + + test("throws when the service exposes no count(), rather than trusting list()", async () => { + // `count` is OPTIONAL on MemoryServiceSurface, and consumers that omit it are documented as + // treating list()'s result as the full set. A census caller must not inherit that. + const countless = { + async list() { + return rows(3); + }, + } as MemoryCensusSource; + + await expect(listEveryMemory(countless)).rejects.toThrow(/exposes no count\(\)/); + }); + + test("returns every record across multiple pages, walking offsets in order", async () => { + const service = fakeService({ total: 1347 }); + + const all = await listEveryMemory(service); + + expect(all).toHaveLength(1347); + expect(service.offsets).toEqual([0, MEMORY_CENSUS_PAGE_SIZE, MEMORY_CENSUS_PAGE_SIZE * 2]); + // Distinct records, not one page repeated — a walk that ignored `offset` would still return + // 1347 rows here and pass a length-only assertion. + expect(new Set(all.map((m) => m.id)).size).toBe(1347); + }); + + test("terminates on an empty final page when the total is an exact multiple of the page size", async () => { + const service = fakeService({ total: 1000 }); + + const all = await listEveryMemory(service); + + expect(all).toHaveLength(1000); + // The third call returns an empty page and is what ends the loop; without it this walk + // would not terminate. + expect(service.offsets).toEqual([0, MEMORY_CENSUS_PAGE_SIZE, MEMORY_CENSUS_PAGE_SIZE * 2]); + }); + + test("tolerates a record inserted mid-scan (scan larger than the floor)", async () => { + // Floor taken before the insert; the scan sees one more. Comparing with `<` absorbs this. + const service = fakeService({ total: 1001, reportedCount: 1000 }); + + const all = await listEveryMemory(service); + + expect(all).toHaveLength(1001); + }); + + test("refuses when the service silently caps every page below the requested limit", async () => { + // The mt#4761 defect in its native form: the service honours `offset` but returns fewer rows + // than asked for, so the walk ends after one short page. + const service = fakeService({ total: 1347, hardCap: 100 }); + + await expect(listEveryMemory(service)).rejects.toThrow( + /Scan covered 100 of at least 1347 memories/ + ); + }); +}); diff --git a/scripts/lib/list-every-memory.ts b/scripts/lib/list-every-memory.ts new file mode 100644 index 0000000000..4ecf8da777 --- /dev/null +++ b/scripts/lib/list-every-memory.ts @@ -0,0 +1,105 @@ +/** + * Census read over the whole memory corpus, for scripts that scan every record to decide what + * to WRITE. + * + * `MemoryService.list()` silently caps at `DEFAULT_LIST_CAP` (500, mt#4761) — its own docblock + * says its array length "cannot answer how many rows actually match." A census caller asks it + * exactly that question, so a capped read hands back a confident, wrong number over a fraction + * of the corpus with nothing in the output to show it. That is not hypothetical: on 2026-09-01, + * with the corpus at 1,347, BOTH remaining census callers printed a clean result over 500 rows + * and exited 0 — + * + * `backfill-memory-associations.ts` -> "Found 500 memories to scan." + * `normalize-memory-associations.ts` -> "Scanned 500 memories." / "divergent keys: 0" + * + * — the second one reporting a clean bill of health for a corpus it never looked at 62% of. + * + * So the count is not decoration: it is what makes the scan able to FAIL. A short read throws + * rather than returning a plausible number (mem#704 — a probe that returns the same result when + * the system is broken is not verification). + * + * This lives here, shared, rather than in `packages/domain`, because it deliberately does the + * thing mt#4761 removed from the domain surface: materialize the entire corpus. That is correct + * for an offline script deciding a write set and wrong for a request path, and the module + * boundary is what keeps the distinction legible. Promoted out of + * `scripts/rederive-memory-associations.ts` by mt#4783 — three scripts had begun hand-rolling + * the same contract, which is the divergent-copy shape a centralised pass exists to prevent. + * + * @see mt#4783 (this module) · mt#4761 (the cap) · mt#4765 (the original implementation) + */ + +import type { MemoryServiceSurface } from "@minsky/domain/memory/memory-service"; +import type { MemoryRecord } from "@minsky/domain/memory/types"; +import { DEFAULT_LIST_CAP } from "@minsky/domain/utils/list-pagination"; + +/** + * Page size for the census walk — DERIVED from the cap, never restated. + * + * It has to equal `DEFAULT_LIST_CAP` rather than merely resemble it. Requesting MORE than the cap + * returns the cap, which the walk reads as a short page and stops on: a silent early exit at + * exactly the wrong moment, or (since mt#4783) a thrown coverage failure that breaks all four + * callers. A hardcoded copy is fine until someone LOWERS the cap, and then it is not. + */ +export const MEMORY_CENSUS_PAGE_SIZE = DEFAULT_LIST_CAP; + +/** + * The narrow slice of `MemoryServiceSurface` a census needs. A real `MemoryService` satisfies + * it structurally; a test stub needs only these two methods. + * + * `count` is optional on the surface itself (a fake may omit it), which is why + * {@link listEveryMemory} refuses rather than assuming when it is absent — see below. + */ +export type MemoryCensusSource = Pick; + +/** + * Page through the WHOLE corpus, then assert the scan actually covered it. + * + * Throws — never returns a partial result — when coverage cannot be proven. Both failure modes + * are deliberate: + * + * - **No `count()`.** The surface marks it optional, and consumers that omit it are documented + * as treating `list()`'s result as the full matching set. A census caller must not inherit + * that assumption: without a floor there is nothing to check the scan against, so proceeding + * would produce precisely the unfalsifiable number this function exists to prevent. + * - **Short read.** Paging stopped early and every figure computed downstream would cover a + * fraction of the corpus. + */ +export async function listEveryMemory(service: MemoryCensusSource): Promise { + if (typeof service.count !== "function") { + throw new Error( + "MemoryService exposes no count(); cannot prove the scan covered the corpus. Refusing." + ); + } + + // Count FIRST, and compare with `<`, not `!==`. Both choices absorb the same race — a record + // INSERTED while the scan is in flight — from the two ends it can arrive at. Counting first + // keeps the insert out of the floor; `<` tolerates it showing up in the scan. Either alone + // would turn an ordinary concurrent write into a spurious failure. + // + // A concurrent DELETE is NOT absorbed, and that is the accepted trade rather than an + // oversight: it lowers the scan below a floor already taken, so it throws. Fail-closed is the + // right side to err on here — these are offline scripts about to decide a write set, deletes + // during a run are rare, and offset paging can genuinely SKIP records when rows shift beneath + // it, so a short read during deletion may be a real coverage gap rather than a phantom. Re-run + // the script; do not relax the comparison to make it pass. + // + // What this catches is the case worth catching: a SHORT read, where paging silently stopped + // early and every figure computed downstream covers a fraction of the corpus. + const floor = await service.count(); + + const out: MemoryRecord[] = []; + for (let offset = 0; ; offset += MEMORY_CENSUS_PAGE_SIZE) { + const page = await service.list({ limit: MEMORY_CENSUS_PAGE_SIZE, offset }); + out.push(...page); + if (page.length < MEMORY_CENSUS_PAGE_SIZE) break; + } + + if (out.length < floor) { + throw new Error( + `Scan covered ${out.length} of at least ${floor} memories — refusing to report a rate ` + + "over a partial corpus. Investigate the pagination before trusting any number below." + ); + } + + return out; +} diff --git a/scripts/normalize-memory-associations.ts b/scripts/normalize-memory-associations.ts index 0f430511b7..f14bc791f2 100755 --- a/scripts/normalize-memory-associations.ts +++ b/scripts/normalize-memory-associations.ts @@ -47,6 +47,7 @@ import { randomUUID } from "node:crypto"; import type { MemoryServiceSurface, MemoryServiceDb } from "@minsky/domain/memory/memory-service"; import { isKnownAssociationType } from "@minsky/domain/memory/associations"; +import { listEveryMemory } from "./lib/list-every-memory"; const TASK_ID_RE = /^(?:mt|md|gh)#\d+$/; @@ -211,7 +212,9 @@ async function main() { const execute = process.argv.includes("--execute"); const memoryService = await buildMemoryService(); - const all = await memoryService.list({}); + // Census, not a page: this script decides a WRITE set from what it sees, so a capped read + // would report a clean result over a fraction of the corpus (mt#4783). Throws on a short scan. + const all = await listEveryMemory(memoryService); console.log(`Scanned ${all.length} memories.\n`); const plans: RecordPlan[] = []; diff --git a/scripts/rederive-memory-associations.ts b/scripts/rederive-memory-associations.ts index d05012cf46..239cceb3fe 100644 --- a/scripts/rederive-memory-associations.ts +++ b/scripts/rederive-memory-associations.ts @@ -30,6 +30,7 @@ import { createHash } from "node:crypto"; import type { MemoryServiceSurface, MemoryServiceDb } from "@minsky/domain/memory/memory-service"; import { extractTrackingTaskRefs } from "@minsky/domain/memory/staleness"; import { TRACKS_TASK_ASSOCIATION } from "@minsky/domain/memory/associations"; +import { listEveryMemory } from "./lib/list-every-memory"; // ── Pure core ─────────────────────────────────────────────────────────────────────────────── @@ -225,53 +226,6 @@ async function buildMemoryService(): Promise { }); } -/** - * Page through the WHOLE corpus, then assert the scan actually covered it. - * - * `MemoryService.list()` silently caps at `DEFAULT_LIST_CAP` (500) — its own docblock says its - * array length "cannot answer how many rows actually match" (mt#4761). A capped scan here would - * report a confident, wrong rate over 37% of the corpus with nothing in the output to show it: - * the exact derived-view failure this task exists to repair, reproduced in the tool built to - * repair it. Observed in this script's first live run — 500 scanned against a true 1,342. - * - * So the count is not decoration: it makes the scan able to FAIL. A short read throws rather - * than returning a plausible number (mem#704 — a probe that cannot fail is not verification). - */ -async function listEveryMemory( - service: MemoryServiceSurface -): Promise>> { - if (typeof service.count !== "function") { - throw new Error( - "MemoryService exposes no count(); cannot prove the scan covered the corpus. Refusing." - ); - } - - // Count FIRST, and compare with `<`, not `!==`. Both choices are about the same race: a record - // created while the scan is in flight. Counting first means such a record can only make the - // scan LARGER than the floor, never smaller, so it cannot trip the check; comparing with `<` - // means a concurrent DELETE (which lowers the true total below the floor) does not either. - // What remains catchable is the case worth catching — a SHORT read, where paging silently - // stopped early and every rate below would be computed over a fraction of the corpus. - const floor = await service.count(); - - const PAGE = 500; - const out: Awaited> = []; - for (let offset = 0; ; offset += PAGE) { - const page = await service.list({ limit: PAGE, offset }); - out.push(...page); - if (page.length < PAGE) break; - } - - if (out.length < floor) { - throw new Error( - `Scan covered ${out.length} of at least ${floor} memories — refusing to report a rate ` + - "over a partial corpus. Investigate the pagination before trusting any number below." - ); - } - - return out; -} - async function main(): Promise { const argv = process.argv.slice(2); const execute = argv.includes("--execute"); diff --git a/tests/scripts/import-claude-code-memory.test.ts b/tests/scripts/import-claude-code-memory.test.ts index 7df0220d92..19b348d602 100644 --- a/tests/scripts/import-claude-code-memory.test.ts +++ b/tests/scripts/import-claude-code-memory.test.ts @@ -60,7 +60,19 @@ function makeFakeService(existing: MemoryRecord[] = []): MemoryServiceSurface & return { created, - list: async (_filter?: MemoryListFilter) => existing, + // Honours limit/offset, and reports a true count (mt#4783). Both matter now that the + // importer builds its idempotency set with `listEveryMemory`, which refuses a scan it + // cannot prove covered the corpus. Returning `existing` for every page regardless of the + // filter would also spin forever once a fixture reached the page size, since a full page + // never terminates the walk. + list: async (filter?: MemoryListFilter) => { + const offset = filter?.offset ?? 0; + return filter?.limit === undefined + ? existing.slice(offset) + : existing.slice(offset, offset + filter.limit); + }, + + count: async (_filter?: MemoryListFilter) => existing.length, create: async (input: MemoryCreateInput) => { created.push(input);