From 843b09fb21ee769b72d021bd29868d64cb9f0692 Mon Sep 17 00:00:00 2001 From: Willi Budzinski Date: Fri, 19 Jun 2026 19:35:15 +0200 Subject: [PATCH 1/2] fix: refuse unsafe graph snapshot rebuild stubs --- .../plan.md | 409 ++++++++++++++++++ .../todo.md | 249 +++++++++++ src/functions/graph.ts | 20 +- test/graph.test.ts | 156 +++++++ 4 files changed, 829 insertions(+), 5 deletions(-) create mode 100644 docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md create mode 100644 docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md diff --git a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md new file mode 100644 index 00000000..8ff14d58 --- /dev/null +++ b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md @@ -0,0 +1,409 @@ +# Issue 330 Legacy Graph Rebuild Guard Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make unforced graph snapshot rebuild refuse empty legacy snapshot stubs before graph-scope enumeration. + +**Architecture:** Keep the fix inside `mem::graph-snapshot-rebuild` as a preflight guard before `kv.list(KV.graphNodes)` / `kv.list(KV.graphEdges)`. Reuse the existing no-snapshot refusal contract and avoid route, schema, persistence, MCP, or iii-engine changes. The resetAt arena selected `resetAt` as the existing live-boundary marker, so unforced rebuild refuses both empty snapshots and reset-scoped snapshots. Tests use the existing `test/graph.test.ts` mock SDK/KV harness and prove `kv.list` is not reached. + +**Tech Stack:** TypeScript ESM, iii-sdk function registration, Vitest, existing in-memory `mockKV()` and `mockSdk()` helpers. + +--- + +## Files + +- Modify: `test/graph.test.ts` +- Modify: `src/functions/graph.ts` +- Modify: `docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md` + +## Task 1: Add Red Empty-Stub Rebuild Test + +**Files:** +- Modify: `test/graph.test.ts` + +- [x] **Step 1: Write the failing test** + +Add this test inside `describe("budget + tooLarge guards (#814 v2)", () => { ... })`, near the existing no-snapshot rebuild refusal test: + +```ts + it("graph-snapshot-rebuild refuses empty snapshot stubs without kv.list", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:snapshot", "current", { + version: 1, + topNodes: [], + topEdges: [], + topDegrees: {}, + stats: { + totalNodes: 0, + totalEdges: 0, + nodesByType: {}, + edgesByType: {}, + }, + updatedAt: "2026-01-01T00:00:00.000Z", + dirty: false, + }); + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/graph\/reset|force|empty snapshot/i); + expect(listCalls).toBe(0); + }); +``` + +- [x] **Step 2: Run the focused red test** + +Run: + +```bash +corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses empty snapshot stubs without kv.list" --exclude test/integration.test.ts +``` + +Expected before implementation: FAIL because `mem::graph-snapshot-rebuild` reaches `localKv.list`, catches `"unsafe list"`, returns that error, and increments `listCalls`. + +## Task 2: Add Minimal Empty-Stub Guard + +**Files:** +- Modify: `src/functions/graph.ts` + +- [x] **Step 1: Implement the guard** + +Change only the existing preflight block inside `mem::graph-snapshot-rebuild`. The intended shape is: + +```ts + const existing = await readSnapshot(kv); + const snapshotTotalNodes = existing?.stats?.totalNodes ?? 0; + const hasResetBoundary = + typeof existing?.resetAt === "string" && existing.resetAt.length > 0; + if ( + !forceRebuild && + (!existing || hasResetBoundary || snapshotTotalNodes === 0) + ) { + logger.warn("Graph snapshot rebuild refused: unsafe snapshot source", { + hint: hasResetBoundary + ? "reset snapshot boundary with possible orphaned graph rows" + : existing + ? "empty snapshot stub" + : "legacy corpus or empty store", + }); + return { + success: false, + legacyCorpus: true, + error: + "No safe graph snapshot rebuild source found. Rebuild would call kv.list " + + "on KV.graphNodes/Edges, which heartbeat-crashes the worker " + + "on large legacy or post-reset orphaned corpora. " + + "Either (a) call POST /agentmemory/graph/reset to drop into " + + "incremental-only mode and rebuild from new extracts, or " + + "(b) re-send with `force: true` if you're certain the " + + "corpus is small.", + }; + } +``` + +Keep the strict `data?.force === true` semantics unchanged. + +- [x] **Step 2: Run the focused green test** + +Run: + +```bash +corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses empty snapshot stubs without kv.list" --exclude test/integration.test.ts +``` + +Expected after implementation: PASS. + +## Task 3: Add Reset and Populated-Snapshot Regression Tests + +**Files:** +- Modify: `test/graph.test.ts` + +- [x] **Step 1: Add post-reset no-enumeration regression** + +Add this test in the same `budget + tooLarge guards (#814 v2)` block: + +```ts + it("graph-snapshot-rebuild does not enumerate after graph-reset", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + await localSdk.trigger("mem::graph-reset", {}); + + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/graph\/reset|force|snapshot/i); + expect(listCalls).toBe(0); + }); +``` + +- [x] **Step 2: Add populated snapshot still rebuilds regression** + +Add this test in the same block: + +```ts + it("graph-snapshot-rebuild still rebuilds populated snapshots under the ceiling", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:snapshot", "current", { + version: 1, + topNodes: [], + topEdges: [], + topDegrees: {}, + stats: { + totalNodes: 1, + totalEdges: 0, + nodesByType: { concept: 1 }, + edgesByType: {}, + }, + updatedAt: "2026-01-01T00:00:00.000Z", + dirty: false, + }); + await localKv.set("mem:graph:nodes", "small_n", { + id: "small_n", + type: "concept", + name: "small", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; totalNodes?: number }; + + expect(result.success).toBe(true); + expect(result.totalNodes).toBe(1); + }); +``` + +- [x] **Step 3: Add populated reset snapshot refusal regression** + +Add this test in the same block: + +```ts + it("graph-snapshot-rebuild refuses populated reset snapshots without kv.list", async () => { + const localKv = mockKV(); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + + await localSdk.trigger("mem::graph-reset", {}); + await localSdk.trigger("mem::graph-extract", { observations: [testObs] }); + + const snap = await localKv.get<{ + resetAt?: string; + stats: { totalNodes: number }; + }>("mem:graph:snapshot", "current"); + expect(snap?.resetAt).toEqual(expect.any(String)); + expect(snap?.stats.totalNodes).toBeGreaterThan(0); + + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/reset|snapshot|force/i); + expect(listCalls).toBe(0); + }); +``` + +- [x] **Step 4: Run rebuild regression tests** + +Run: + +```bash +corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild" --exclude test/integration.test.ts +``` + +Expected before the resetAt guard extension: the populated reset snapshot test +fails because `kv.list` is reached. Expected after the guard extension: PASS for +the new guard tests and existing rebuild tests. + +## Task 4: Focused Verification and Simplification + +**Files:** +- Modify: `src/functions/graph.ts` +- Modify: `test/graph.test.ts` +- Modify: `docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md` + +- [x] **Step 1: Run the focused graph test file** + +Run: + +```bash +corepack pnpm exec vitest run test/graph.test.ts --exclude test/integration.test.ts +``` + +Expected: PASS. + +- [x] **Step 2: Run lint** + +Run: + +```bash +corepack pnpm run lint +``` + +Expected: PASS. + +- [x] **Step 3: Run diff whitespace check** + +Run: + +```bash +git diff --check +``` + +Expected: no output and exit 0. + +- [x] **Step 4: Run focused Semgrep** + +Run: + +```bash +semgrep scan --config p/default --error --metrics=off src/functions/graph.ts test/graph.test.ts docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset +``` + +Expected: 0 findings and exit 0. + +- [x] **Step 5: Do a focused simplification pass** + +Reread the active diff and keep only changes needed for: +- the empty-stub preflight guard, +- the reset no-enumeration regression, +- the populated-snapshot regression, +- task-state evidence. + +Do not change routes, REST payload parsing, schema, persistence scopes, MCP tools, +or iii-engine integration. + +## Task 5: Commit and PR Preparation + +**Files:** +- Modify: task-owned files only. + +- [x] **Step 1: Inspect final status and diff** + +Run: + +```bash +git status -sb --untracked-files=all +git diff -- src/functions/graph.ts test/graph.test.ts docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset +``` + +Expected: only task-owned files changed. + +- [x] **Step 2: Stage task-owned files** + +Run: + +```bash +git add src/functions/graph.ts test/graph.test.ts docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md +``` + +Expected: only those paths staged. + +- [x] **Step 3: Run staged secret scan** + +Run: + +```bash +gitleaks protect --staged --redact +``` + +Expected: no leaks. + +- [ ] **Step 4: Commit** + +Run: + +```bash +git commit -m "fix: refuse unsafe graph snapshot rebuild stubs" +``` + +Expected: commit created on `issue/330-legacy-graph-rebuild-reset`. + +- [ ] **Step 5: Remote PR actions** + +Before `git push`, GitHub issue comment, PR creation, or PR merge, confirm current-turn approval for the exact remote action if it has not already been granted after implementation. + +## Self-Review + +Spec coverage: +- Empty snapshot stub crash path: covered by Task 1 and Task 2. +- Reset remains safe: covered by Task 3. +- Populated snapshot compatibility: covered by Task 3. +- Verification/security gates: covered by Task 4 and Task 5. +- Pre-implementation review finding resolved: post-reset empty snapshots are + intentionally covered by the same unforced empty-snapshot refusal contract, + with the separate reset regression proving no enumeration. +- ResetAt arena finding resolved in plan: populated snapshots with a reset + boundary are intentionally refused before enumeration; populated snapshots + without `resetAt` remain eligible for rebuild under the existing ceiling. + +Placeholder scan: +- No `TBD`, unresolved placeholders, or unowned file scopes remain. + +Type consistency: +- Tests use existing `mockKV`, `mockSdk`, `registerGraphFunction`, and string KV scope constants already used in `test/graph.test.ts`. diff --git a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md new file mode 100644 index 00000000..103ce257 --- /dev/null +++ b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md @@ -0,0 +1,249 @@ +# Issue 330 Legacy Graph Rebuild Reset + +Task id: `2026-06-19-issue-330-legacy-graph-rebuild-reset` + +## Scope + +Handle GitHub issue #330 on branch +`issue/330-legacy-graph-rebuild-reset` in the isolated worktree +`/Users/A1538552/.codex/worktrees/ab59/agentmemory`. + +Target remote is only `origin`: +`https://github.com/wbugitlab1/agentmemory.git`. Do not target +`upstream` / `https://github.com/rohitg00/agentmemory/`. + +## Sprint Contract + +Goal: prevent unforced `POST /agentmemory/graph/snapshot-rebuild` from +heartbeat-crashing large legacy graph stores that have an empty version-1 +snapshot stub, while preserving the existing safe `graph-reset` behavior. + +Scope: +- Validate issue #330 with arena before implementation. +- Modify only graph rebuild/reset safety behavior in `src/functions/graph.ts` + and focused tests in `test/graph.test.ts`, unless the human checkpoint + explicitly expands scope. +- Record task state under this directory. + +Non-goals: +- Do not add iii-engine pagination, key-only list, or clear-scope primitives. +- Do not add a REST `force` option. +- Do not clean or vacuum legacy graph rows. +- Do not change MCP graph resources unless explicitly approved as scope + expansion. +- Do not target or push to the upstream remote. + +Acceptance criteria: +- Arena validity synthesis is recorded before implementation. +- `graph/reset` remains enumeration-free. +- `graph-snapshot-rebuild` refuses an unforced empty snapshot stub before any + `kv.list(KV.graphNodes)` or `kv.list(KV.graphEdges)` call. +- `graph-snapshot-rebuild` refuses an unforced snapshot carrying `resetAt` + before any graph-scope `kv.list`, even after later extracts make the snapshot + populated again. +- A populated snapshot under the safe ceiling still rebuilds without `force`. +- `force: true` remains a strict internal operator escape hatch for known-small + corpora. +- Focused tests prove the red/green behavior. +- Required verification and security gates are run or blockers are recorded. + +Intended verification: +- `corepack pnpm exec vitest run test/graph.test.ts --exclude test/integration.test.ts` +- `corepack pnpm test` +- `corepack pnpm run lint` +- `git diff --check` +- `semgrep scan --config p/default --error --metrics=off .` +- Staged `gitleaks protect --staged --redact` before any commit. +- OSV is not planned unless dependency, lockfile, vendored, package-manager, + container, or third-party package surfaces change. + +Known boundaries: +- This changes observable public REST behavior for + `POST /agentmemory/graph/snapshot-rebuild` on empty snapshot stubs: it will + refuse safely instead of attempting unsafe enumeration. +- No schema, migration, dependency, iii-engine, MCP tool, auth, or persistence + model change is planned. +- `node_modules` is not present in this worktree at kickoff. If tests cannot + run, use `corepack pnpm install --frozen-lockfile --ignore-scripts` as the + deterministic verification setup, without changing manifests or lockfiles. + +Stop conditions: +- Human checkpoint is not approved for the REST behavior guard. +- Tests require dependency setup that is blocked by credentials, registry, or + pnpm hardening. +- The smallest fix requires schema, migration, iii-engine, MCP, or broader + persistence behavior changes. +- Required security gates are missing, fail, or report unresolved findings. + +## Arena Synthesis + +Arena reports: +- Candidate A: `/private/tmp/arena-330/candidate-a/report.md` +- Candidate B: `/private/tmp/arena-330/candidate-b/report.md` +- Candidate C: `/private/tmp/arena-330/candidate-c/report.md` +- Cross-judge: `/private/tmp/arena-330/judge/report.md` +- ResetAt candidate A: `/private/tmp/arena-330-resetat/candidate-a/report.md` +- ResetAt candidate B: `/private/tmp/arena-330-resetat/candidate-b/report.md` +- ResetAt candidate C: `/private/tmp/arena-330-resetat/candidate-c/report.md` +- ResetAt cross-judge: `/private/tmp/arena-330-resetat/judge/report.md` + +Decision: issue #330 is partially valid on local `origin/main` +`67bb438b4158d74771ed285e06c9ac078985d603`. + +Findings: +- `graph/reset` is already fixed for the reported `state::list` crash class: + `mem::graph-reset` writes an empty reset snapshot, clears bounded name + shards, marks indexes ready, and does not enumerate graph node or edge + scopes. +- `snapshot-rebuild` is still unsafe for the latest reported case: + `readSnapshot()` accepts a valid version-1 empty snapshot stub whose + `stats.totalNodes` is `0`; `mem::graph-snapshot-rebuild` then reaches + `kv.list(KV.graphNodes)` and `kv.list(KV.graphEdges)` before the too-large + guard can run. +- Arena base: Candidate B. +- Graft from Candidate A: record adjacent residual risk that MCP resource + `agentmemory://graph/stats` directly enumerates graph node and edge scopes. +- Graft from Candidate C: include a green regression for populated snapshots + under the safe ceiling. +- Rejected: engine-level pagination/keys/clear-scope and graph row vacuum as + out of scope for this issue slice. +- ResetAt arena decision: all candidates and the cross-judge selected option 2. + Unforced rebuild must also refuse snapshots with a non-empty `resetAt`. + Rationale: `resetAt` is the existing live-boundary marker; `graph-reset` + intentionally leaves legacy rows as orphaned physical state; later + `graph-extract` preserves `resetAt` while incrementing snapshot counts; and a + rebuild from raw arrays can both enumerate old orphan rows and erase the reset + boundary. + +## Human Checkpoint + +Status: done. + +Decision needed: approve a narrow public REST behavior guard for +`POST /agentmemory/graph/snapshot-rebuild`. + +Approved option: treat any empty snapshot reached by unforced +`snapshot-rebuild` as unsafe to enumerate, including both legacy empty stubs +without `resetAt` and intentional post-reset empty snapshots with `resetAt`. +The follow-up resetAt arena extends this to every unforced snapshot carrying a +real `resetAt`, even when later extracts have repopulated the snapshot. Return a +safe refusal before any graph-scope `kv.list`. Use the existing refusal style: +`success: false`, `legacyCorpus: true`, and an operator message pointing to +`POST /agentmemory/graph/reset` or literal `force: true` only for known-small +lower-level callers. + +Alternative option: treat any `stats.totalNodes === 0` snapshot, including +post-reset snapshots, as a safe no-op response. This is less precise and could +hide a legacy corpus with orphan rows, so it is not recommended. + +Scope-expansion option: also fix the adjacent MCP `agentmemory://graph/stats` +enumeration risk. This is explicitly outside the narrow #330 reset/rebuild +scope and should be a separate approved change or issue. + +## Feature / Verification Matrix + +| Change | Verification method | Status | Evidence | +| --- | --- | --- | --- | +| Context confirmed | Local git/repo commands | Done | `pwd`, `git status -sb --untracked-files=all`, `git remote -v`, `git worktree list --porcelain`, `git rev-parse HEAD`, and `git rev-parse origin/main` inspected. | +| Branch created | Git branch command | Done | `git switch -c issue/330-legacy-graph-rebuild-reset origin/main` succeeded from `67bb438b4158d74771ed285e06c9ac078985d603`. | +| Issue evidence inspected | GitHub issue read and repo source inspection | Done | `gh issue view 330 --repo wbugitlab1/agentmemory ...`; graph source and tests inspected. | +| Arena validation | Three candidates plus cross-judge | Done | Candidate and judge reports under `/private/tmp/arena-330/`; synthesis recorded above. | +| Human checkpoint | User decision | Done | User approved the narrow REST behavior guard. | +| Empty snapshot guard | TDD red/green | Done | Red observed: focused Vitest failed because the empty-stub rebuild returned the `unsafe list` error path instead of `legacyCorpus: true`; green after preflight guard: 1 focused test passed. | +| Reset remains enumeration-free | Focused Vitest | Done | `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild" --exclude test/integration.test.ts` passed 5 rebuild-focused tests, including empty post-reset no-enumeration. | +| Populated reset snapshot refuses rebuild | TDD red/green | Done | Red observed: focused Vitest failed because populated reset snapshot rebuild returned the `unsafe list` error path instead of `legacyCorpus: true`; green after resetAt guard. | +| Populated snapshot still rebuilds | Focused Vitest | Done | Same rebuild-focused run passed the populated-snapshot regression. | +| Full test suite | Repo-native Vitest suite | Done | `corepack pnpm test` passed 211 files / 2906 tests. | +| Security/secret gates | Semgrep, Codex Security diff scan, and staged Gitleaks | Done | Full `semgrep scan --config p/default --error --metrics=off .` passed with 0 findings. Codex Security diff scan completed with no findings; reports at `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.md` and `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.html`. `gitleaks protect --staged --redact` passed with no leaks. | + +## Subagent Ledger + +| Workstream | Scope | Edits allowed | Expected output | Result | Residual risk | +| --- | --- | --- | --- | --- | --- | +| Arena candidate A | Read-only issue validity and fix-boundary report | No | Report with evidence, tests, risks | Completed | Flagged adjacent MCP stats enumeration risk. | +| Arena candidate B | Read-only issue validity and fix-boundary report | No | Report with evidence, tests, risks | Completed | Recommended as base by judge. | +| Arena candidate C | Read-only issue validity and fix-boundary report | No | Report with evidence, tests, risks | Completed | Added populated-snapshot regression emphasis. | +| Arena cross-judge | Read-only scoring and synthesis | No | Scores, base pick, grafts, risks | Completed | Recommended Candidate B base and narrow guard. | +| Final security/persistence review | Read-only current diff review | No | Findings, inspected files, commands, residual risks | Completed | No findings. Confirmed REST wrapper does not expose `force`; noted unchanged MCP `graph/stats` enumeration as out of scope. | +| Final behavioral review | Read-only current diff review | No | Findings, inspected files, commands, residual risks | Completed | No findings. Confirmed empty-stub and resetAt guard blocks unsafe `kv.list` path while preserving normal populated snapshot rebuild. | +| Final test coverage review | Read-only current diff review | No | Findings, inspected files, commands, residual risks | Completed | No findings. Confirmed tests cover empty stubs, post-reset empty snapshots, populated reset snapshots, populated safe snapshots, and reset enumeration-free behavior. | + +## Progress Notes + +- 2026-06-19: Read `AGENTS.md`, project-local + `triage-next-github-issues` skill, `arena`, `github-feature-loop`, + `writing-plans`, `test-driven-development`, `simple-code`, and + `verification-before-completion`. +- 2026-06-19: Confirmed worktree is clean, detached at the required start ref, + and `origin/main` resolves to `67bb438b4158d74771ed285e06c9ac078985d603`. +- 2026-06-19: Created branch + `issue/330-legacy-graph-rebuild-reset` from `origin/main`. +- 2026-06-19: Read issue #330 via `gh issue view`; latest comment narrows the + live bug to `snapshot-rebuild` on empty snapshot stubs while `graph/reset` + is reportedly fixed. +- 2026-06-19: Arena completed. All candidates and cross-judge agree the issue + is valid for `snapshot-rebuild`, already fixed for `graph/reset`, and should + be handled with a pre-enumeration empty-stub guard after human approval. +- 2026-06-19: Human checkpoint approved. The implementation contract is now: + unforced `snapshot-rebuild` refuses any empty snapshot before enumeration; + populated snapshots under the safe ceiling still rebuild; `force: true` + remains the strict lower-level escape hatch. +- 2026-06-19: Dependency setup for verification was required because + `node_modules` was absent and `corepack pnpm exec` hit + `ERR_PNPM_IGNORED_BUILDS`. Ran + `corepack pnpm install --frozen-lockfile --ignore-scripts`; it completed + without manifest or lockfile changes and warned only about the unbuilt + `dist/cli.mjs` bin target. +- 2026-06-19: Red test + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses empty snapshot stubs without kv.list" --exclude test/integration.test.ts` + failed before implementation because `legacyCorpus` was `undefined`, showing + the function still reached the unsafe `kv.list` error path. +- 2026-06-19: Green focused test passed after the guard: + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses empty snapshot stubs without kv.list" --exclude test/integration.test.ts`. +- 2026-06-19: Rebuild-focused regression passed: + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild" --exclude test/integration.test.ts` + passed 5 tests. +- 2026-06-19: Green resetAt regression passed: + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses populated reset snapshots without kv.list" --exclude test/integration.test.ts`. +- 2026-06-19: Rebuild-focused regression passed after resetAt guard: + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild" --exclude test/integration.test.ts` + passed 6 tests. +- 2026-06-19: Full graph test passed after resetAt guard: + `corepack pnpm exec vitest run test/graph.test.ts --exclude test/integration.test.ts` + passed 34 tests. +- 2026-06-19: Lint passed: + `corepack pnpm run lint`. +- 2026-06-19: Full repo test suite passed: + `corepack pnpm test` passed 211 files / 2906 tests. +- 2026-06-19: Full Semgrep passed: + `semgrep scan --config p/default --error --metrics=off .` scanned 949 + tracked files with 0 findings. +- 2026-06-19: Codex Security diff scan completed for the local patch: + 1/1 runtime diff row covered, no candidates, no findings. Reports: + `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.md` + and + `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.html`. +- 2026-06-19: Final read-only reviewers reported no findings across + security/persistence/API boundaries, behavior, and test coverage. Residuals: + unchanged MCP `agentmemory://graph/stats` enumeration remains out of scope; + `force: true` remains an intentionally internal lower-level escape hatch. +- 2026-06-19: GitHub PR base refreshed with `git fetch origin main`. + Captured base SHA: + `51f926fe918a100228d6ce92dadb19933e46ccbf`. Merge-base before local commit: + `67bb438b4158d74771ed285e06c9ac078985d603`. +- 2026-06-19: Staged only task-owned files: + `src/functions/graph.ts`, `test/graph.test.ts`, and this task-state + directory. Staged Gitleaks passed with no leaks. +- 2026-06-19: Pre-implementation reviewer finding about empty `resetAt` + semantics resolved: approved contract intentionally refuses all unforced empty + snapshots before enumeration. Dependency setup side effect in + `pnpm-workspace.yaml` was inspected and removed. +- 2026-06-19: Final maintainability review found an important gap for + populated post-reset snapshots. Ran a focused resetAt arena; all candidates + and the cross-judge converged on refusing every unforced snapshot carrying + `resetAt`, preserving populated non-reset rebuild and rejecting new markers or + public API expansion. +- 2026-06-19: Red resetAt regression + `corepack pnpm exec vitest run test/graph.test.ts -t "graph-snapshot-rebuild refuses populated reset snapshots without kv.list" --exclude test/integration.test.ts` + failed before the resetAt guard because `legacyCorpus` was `undefined`, + showing the function still reached the unsafe `kv.list` error path. diff --git a/src/functions/graph.ts b/src/functions/graph.ts index c19452e1..1e2a0c5f 100644 --- a/src/functions/graph.ts +++ b/src/functions/graph.ts @@ -1023,17 +1023,27 @@ export function registerGraphFunction( const forceRebuild = data?.force === true; try { const existing = await readSnapshot(kv); - if (!existing && !forceRebuild) { - logger.warn("Graph snapshot rebuild refused: no prior snapshot", { - hint: "legacy corpus or empty store", + const snapshotTotalNodes = existing?.stats?.totalNodes ?? 0; + const hasResetBoundary = + typeof existing?.resetAt === "string" && existing.resetAt.length > 0; + if ( + !forceRebuild && + (!existing || hasResetBoundary || snapshotTotalNodes === 0) + ) { + logger.warn("Graph snapshot rebuild refused: unsafe snapshot source", { + hint: hasResetBoundary + ? "reset snapshot boundary with possible orphaned graph rows" + : existing + ? "empty snapshot stub" + : "legacy corpus or empty store", }); return { success: false, legacyCorpus: true, error: - "No prior snapshot found. Rebuild would call kv.list on " + + "No safe graph snapshot rebuild source found. Rebuild would call kv.list on " + "KV.graphNodes/Edges, which heartbeat-crashes the worker " + - "on corpora past the iii state response budget (~25K nodes). " + + "on large legacy or post-reset orphaned corpora. " + "Either (a) call POST /agentmemory/graph/reset to drop into " + "incremental-only mode and rebuild from new extracts, or " + "(b) re-send with `force: true` if you're certain the " + diff --git a/test/graph.test.ts b/test/graph.test.ts index 963e80df..08dad698 100644 --- a/test/graph.test.ts +++ b/test/graph.test.ts @@ -837,6 +837,162 @@ describe("Graph Functions", () => { expect(result.error).toMatch(/graph\/reset|force/); }); + it("graph-snapshot-rebuild refuses empty snapshot stubs without kv.list", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:snapshot", "current", { + version: 1, + topNodes: [], + topEdges: [], + topDegrees: {}, + stats: { + totalNodes: 0, + totalEdges: 0, + nodesByType: {}, + edgesByType: {}, + }, + updatedAt: "2026-01-01T00:00:00.000Z", + dirty: false, + }); + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/graph\/reset|force|empty snapshot/i); + expect(listCalls).toBe(0); + }); + + it("graph-snapshot-rebuild does not enumerate after graph-reset", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + await localSdk.trigger("mem::graph-reset", {}); + + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/graph\/reset|force|snapshot/i); + expect(listCalls).toBe(0); + }); + + it("graph-snapshot-rebuild refuses populated reset snapshots without kv.list", async () => { + const localKv = mockKV(); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + await localKv.set("mem:graph:nodes", "legacy_n", { + id: "legacy_n", + type: "concept", + name: "legacy", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + + await localSdk.trigger("mem::graph-reset", {}); + await localSdk.trigger("mem::graph-extract", { observations: [testObs] }); + + const snap = await localKv.get<{ + resetAt?: string; + stats: { totalNodes: number }; + }>("mem:graph:snapshot", "current"); + expect(snap?.resetAt).toEqual(expect.any(String)); + expect(snap?.stats.totalNodes).toBeGreaterThan(0); + + let listCalls = 0; + localKv.list = async (): Promise => { + listCalls += 1; + throw new Error("unsafe list"); + }; + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; legacyCorpus?: boolean; error?: string }; + + expect(result.success).toBe(false); + expect(result.legacyCorpus).toBe(true); + expect(result.error).toMatch(/reset|snapshot|force/i); + expect(listCalls).toBe(0); + }); + + it("graph-snapshot-rebuild still rebuilds populated snapshots under the ceiling", async () => { + const localKv = mockKV(); + await localKv.set("mem:graph:snapshot", "current", { + version: 1, + topNodes: [], + topEdges: [], + topDegrees: {}, + stats: { + totalNodes: 1, + totalEdges: 0, + nodesByType: { concept: 1 }, + edgesByType: {}, + }, + updatedAt: "2026-01-01T00:00:00.000Z", + dirty: false, + }); + await localKv.set("mem:graph:nodes", "small_n", { + id: "small_n", + type: "concept", + name: "small", + properties: {}, + sourceObservationIds: [], + createdAt: "2026-01-01T00:00:00Z", + stale: false, + }); + const localSdk = mockSdk(); + registerGraphFunction(localSdk as never, localKv as never, mockProvider as never); + + const result = (await localSdk.trigger( + "mem::graph-snapshot-rebuild", + {}, + )) as { success: boolean; totalNodes?: number }; + + expect(result.success).toBe(true); + expect(result.totalNodes).toBe(1); + }); + it("graph-reset is enumeration-free (does not call kv.list)", async () => { // Wrap the mock kv.list with a counter; assert it stays at 0 // across a full reset cycle. From 7e6aa3902b7f7c8e2ac9cda65d4bf619783e32ae Mon Sep 17 00:00:00 2001 From: Willi Budzinski Date: Fri, 19 Jun 2026 19:40:55 +0200 Subject: [PATCH 2/2] docs: record issue 330 pr prep verification --- .../plan.md | 12 +++++++++++- .../todo.md | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md index 8ff14d58..c52053d6 100644 --- a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md +++ b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/plan.md @@ -374,7 +374,7 @@ gitleaks protect --staged --redact Expected: no leaks. -- [ ] **Step 4: Commit** +- [x] **Step 4: Commit** Run: @@ -388,6 +388,16 @@ Expected: commit created on `issue/330-legacy-graph-rebuild-reset`. Before `git push`, GitHub issue comment, PR creation, or PR merge, confirm current-turn approval for the exact remote action if it has not already been granted after implementation. +Local PR prep result: +- Implementation commit: + `843b09fb21ee769b72d021bd29868d64cb9f0692`. +- Captured PR base: + `51f926fe918a100228d6ce92dadb19933e46ccbf`. +- Base merge commit: + `6a9cff176bfe938c81d19c5b564f2ee22a9735d6`. +- Push, PR creation, and issue comment were not run because they are remote + writes and still need explicit current-turn confirmation. + ## Self-Review Spec coverage: diff --git a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md index 103ce257..b5f68fe9 100644 --- a/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md +++ b/docs/todos/2026-06-19-issue-330-legacy-graph-rebuild-reset/todo.md @@ -155,6 +155,8 @@ scope and should be a separate approved change or issue. | Populated snapshot still rebuilds | Focused Vitest | Done | Same rebuild-focused run passed the populated-snapshot regression. | | Full test suite | Repo-native Vitest suite | Done | `corepack pnpm test` passed 211 files / 2906 tests. | | Security/secret gates | Semgrep, Codex Security diff scan, and staged Gitleaks | Done | Full `semgrep scan --config p/default --error --metrics=off .` passed with 0 findings. Codex Security diff scan completed with no findings; reports at `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.md` and `/tmp/codex-security-scans/agentmemory/67bb438b_20260619T173113Z/report.html`. `gitleaks protect --staged --redact` passed with no leaks. | +| PR base integration | `github-push-prepare` local branch prep | Done | Fetched `origin/main` at `51f926fe918a100228d6ce92dadb19933e46ccbf` and merged that captured commit into the issue branch with no conflicts. Branch diff against the PR base contains only the four task-owned files. | +| Post-merge verification | Repo-native checks and security gates | Done | After base merge: `corepack pnpm exec vitest run test/graph.test.ts --exclude test/integration.test.ts` passed 34 tests; `corepack pnpm run lint` passed; `corepack pnpm test` passed 211 files / 2923 tests; `semgrep scan --config p/default --error --metrics=off .` scanned 955 tracked files with 0 findings; `gitleaks detect --source . --redact --log-opts 51f926fe918a100228d6ce92dadb19933e46ccbf..HEAD` passed with no leaks. | ## Subagent Ledger @@ -234,6 +236,21 @@ scope and should be a separate approved change or issue. - 2026-06-19: Staged only task-owned files: `src/functions/graph.ts`, `test/graph.test.ts`, and this task-state directory. Staged Gitleaks passed with no leaks. +- 2026-06-19: Created implementation commit + `843b09fb21ee769b72d021bd29868d64cb9f0692` + (`fix: refuse unsafe graph snapshot rebuild stubs`). +- 2026-06-19: Merged captured PR base + `51f926fe918a100228d6ce92dadb19933e46ccbf` into the branch with merge commit + `6a9cff176bfe938c81d19c5b564f2ee22a9735d6`; no conflicts. Branch diff + against the PR base remains limited to the four task-owned files. +- 2026-06-19: Post-merge checks passed: + graph test file 34 tests, lint, full `corepack pnpm test` 211 files / 2923 + tests, full Semgrep 0 findings over 955 tracked files, and branch-range + Gitleaks no leaks for `51f926fe918a100228d6ce92dadb19933e46ccbf..HEAD`. + A broad full-history Gitleaks run found 14 redacted historical `.pnpm-store` + hits in an old `Codex worktree snapshot` commit; that commit is not in the + current PR base-to-HEAD branch range, so the PR-relevant Gitleaks gate is + clean. - 2026-06-19: Pre-implementation reviewer finding about empty `resetAt` semantics resolved: approved contract intentionally refuses all unforced empty snapshots before enumeration. Dependency setup side effect in