From 3f773624df0721d448b3a25ea81df9e80d407877 Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Thu, 3 Sep 2026 23:03:44 +0300 Subject: [PATCH 1/5] fix(memory): reflection must not reject after a store closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ReflectionRunner.reflect()` is documented fire-safe and `AgentLoop` calls it as a bare `void`. Both decorators that wrap it hydrate candidate ids out of SQLite-backed stores *after* awaiting the inner runner, and that hydration sat outside every `try`. Runtime `shutdown()` calls `reflectionRunner.abortPending()` and then closes every store. The abort settles the inner reflection, so the decorator continuation resumes and reads stores the shutdown has since closed — better-sqlite3 answers a statement on a closed handle with a real `TypeError: The database connection is not open`. That escaped `reflect()` and surfaced as an unhandled rejection. Separately, `profileFactsProvider` is a raw `profileStore.list()` evaluated synchronously at two points in `runTurn`. The one inside the step loop throws into the step's own catch, where a `TypeError` is classified `tool` and fails the turn the user is waiting on — for prompt decoration the renderer would have dropped anyway. - vote-aware / link-aware decorators: hydration is guarded, so a failed read skips the sub-call instead of rejecting. - `profileFactsProvider` is guarded at both call sites; the step-loop one logs a warning and renders without profile facts. - the `void reflect(...)` call site carries a `.catch` as the outermost guarantee. Tests: `src/memory/reflection-decorator-fire-safety.test.ts` drives the real stores and closes them mid-flight (4 of its 7 cases fail without the src change); `src/agent/agent-loop-reflection-fire-safety.test.ts` watches for an unhandled rejection and pins the turn outcome (2 of 3 fail without it). `npm run lint` clean; `src/memory` + `src/agent` 64 files / 849 tests green, `src/runtime` 11 / 121 green. Sentry: CLI-B6 (16 events / 2 users, live on 0.5.4), CLI-6G (16 / 2), CLI-6H (14 / 3) all carry the `hydrateCandidates` -> store `.get` / `.getById` TypeError signature; CLI-34 (9 / 1) is the `profileFactsProvider` -> `list` variant with `category=tool`. --- .../agent-loop-reflection-fire-safety.test.ts | 231 +++++++++++++ src/agent/agent-loop.ts | 37 ++- src/memory/links/link-aware-reflection.ts | 17 +- .../reflection-decorator-fire-safety.test.ts | 305 ++++++++++++++++++ src/memory/voting/vote-aware-reflection.ts | 15 +- 5 files changed, 596 insertions(+), 9 deletions(-) create mode 100644 src/agent/agent-loop-reflection-fire-safety.test.ts create mode 100644 src/memory/reflection-decorator-fire-safety.test.ts diff --git a/src/agent/agent-loop-reflection-fire-safety.test.ts b/src/agent/agent-loop-reflection-fire-safety.test.ts new file mode 100644 index 00000000..c362a2e6 --- /dev/null +++ b/src/agent/agent-loop-reflection-fire-safety.test.ts @@ -0,0 +1,231 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { AgentLoop } from "./agent-loop.js"; +import type { MemoryContextProvider } from "./agent-loop.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import type { CompletionResult } from "../llm/llama-server-client.js"; +import type { + CapabilitiesSummary, + SkillCatalogEntry, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; +import type { ProfileFact } from "../memory/profile-store.js"; +import type { + ReflectionInput, + ReflectionRunner, +} from "../memory/reflection/reflection-runner.js"; + +/** + * `runTurn` fires reflection as a bare `void` — it is background + * bookkeeping the user is not waiting on. Two ways that used to hurt + * the turn: + * + * - a decorator that reads a store closed by shutdown rejects, and + * with nothing attached to the promise the process reports an + * unhandled rejection (`error-reporting/error-reporter.ts` + * forwards those to the crash reporter); + * - `profileFactsProvider` is a raw `profileStore.list()` evaluated + * synchronously to build the reflection allowlist, so a store + * failure there failed the *turn*. + * + * Neither is recoverable by the loop and neither should be visible to + * the user, so both are pinned here. + */ + +function makeCompletion(content: string): CompletionResult { + return { + content, + reasoningContent: "", + stop: true, + truncated: false, + timing: { + promptMs: 1, + predictedMs: 1, + promptTokens: 10, + predictedTokens: 5, + }, + cacheHitTokens: 0, + slotId: 0, + modelId: "mock", + }; +} + +const TOOLS: ToolDescriptor[] = [ + { + name: "finish", + summary: "Finish the session with a summary.", + argsSchema: '{"summary": string}', + }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +const SKILLS: SkillCatalogEntry[] = []; + +const NOOP_PROVIDER: MemoryContextProvider = { + buildMemoryContext: () => ({ recalled: [], index: [] }), +}; + +function makeFact(id: number): ProfileFact { + return { + id, + key: "editor", + value: "vim", + validFrom: 1, + updatedAt: 1, + pinned: true, + keywords: [], + supersedes: null, + supersededBy: null, + voteScore: 0, + }; +} + +function makeLoop(deps: { + reflectionRunner: ReflectionRunner; + profileFactsProvider?: () => readonly ProfileFact[]; +}): AgentLoop { + return new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => + makeCompletion(JSON.stringify({ tool: "reply", args: { text: "ok" } })), + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + memoryContextProvider: NOOP_PROVIDER, + reflectionRunner: deps.reflectionRunner, + ...(deps.profileFactsProvider + ? { profileFactsProvider: deps.profileFactsProvider } + : {}), + }); +} + +/** Collect unhandled rejections raised while `body` runs. */ +async function withUnhandledRejectionWatch( + body: () => Promise, +): Promise { + const seen: unknown[] = []; + const onRejection = (reason: unknown) => seen.push(reason); + // Vitest installs its own handler; prepend so ours observes first + // and keep the runner's in place. + process.prependListener("unhandledRejection", onRejection); + try { + await body(); + // An unhandled rejection is reported after the microtask queue + // drains — give the loop's `void` promise two macrotask ticks. + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + } finally { + process.removeListener("unhandledRejection", onRejection); + } + return seen; +} + +describe("AgentLoop reflection is background work, never a turn hazard", () => { + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-reflect-loop-")); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + it("a rejecting reflectionRunner raises no unhandled rejection", async () => { + let called = false; + const loop = makeLoop({ + reflectionRunner: { + async reflect(_input: ReflectionInput) { + called = true; + throw new TypeError("The database connection is not open"); + }, + abortPending() { + /* no-op */ + }, + }, + }); + const session = createEmptySessionState({ id: "s1", workingDir }); + + const seen = await withUnhandledRejectionWatch(async () => { + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + expect(result.session.id).toBe("s1"); + }); + + expect(called).toBe(true); + expect(seen).toEqual([]); + }); + + it("a throwing profileFactsProvider does not fail the turn", async () => { + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => { + throw new TypeError("The database connection is not open"); + }, + }); + const session = createEmptySessionState({ id: "s2", workingDir }); + + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + expect(result.session.id).toBe("s2"); + // Reflection still fires — just without profile candidates. + expect(inputs).toHaveLength(1); + expect(inputs[0]!.recalledProfileFactIds).toBeUndefined(); + }); + + it("a healthy profileFactsProvider still supplies the allowlist", async () => { + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => [makeFact(7)], + }); + const session = createEmptySessionState({ id: "s3", workingDir }); + + await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + expect(inputs).toHaveLength(1); + expect(inputs[0]!.recalledProfileFactIds).toEqual([7]); + }); +}); diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index ae8f91ec..92fd7b0d 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -651,7 +651,22 @@ export class AgentLoop { "This is the final allowed step. Do not call any non-terminal tool; " + "summarize the completed work with reply, or end the session with finish."; try { - const profileFacts = this.deps.profileFactsProvider?.(); + // `profileFactsProvider` is a raw `profileStore.list()`. The + // facts are prompt decoration — the renderer already drops + // them when the contextual gate does not match — so a store + // failure must render the step without them rather than throw + // into the catch below, where a `TypeError` from a closed + // SQLite handle would be classified `tool` and fail the turn. + let profileFacts: readonly ProfileFact[] | undefined; + try { + profileFacts = this.deps.profileFactsProvider?.(); + } catch (err) { + this.deps.logger?.warn("profile facts unavailable for this step", { + sessionId: state.id, + stepIndex: i, + error: err instanceof Error ? err.message : String(err), + }); + } const activeProfile = this.deps.profileManager?.getProfile() ?? this.deps.profile ?? @@ -1147,8 +1162,22 @@ export class AgentLoop { // renderer already surfaces them whenever they pass the // contextual-keyword gate. Sourcing them here keeps the // decorator's hydration cheap. - const profileFacts = - this.deps.profileFactsProvider?.() ?? []; + // `profileFactsProvider` is a raw `profileStore.list()`. + // It is only ever an input to the fire-and-forget reflection + // below, so a store failure here must not fail the turn the + // user is waiting on — an empty allowlist just means the + // vote-runner sees no profile candidates this turn. + let profileFacts: readonly ProfileFact[] = []; + try { + profileFacts = this.deps.profileFactsProvider?.() ?? []; + } catch { + // best-effort: reflection candidates only + } + // `reflect()` is documented fire-safe, but it is composed at + // runtime from decorators that read SQLite stores. A bare + // `void` turns any escape into an unhandled rejection the + // loop can neither see nor recover from, so the trailing + // `.catch` pins the contract at the call site too. void this.deps.reflectionRunner.reflect({ sessionId: state.id, userMessage, @@ -1184,7 +1213,7 @@ export class AgentLoop { ...(segmentationActive && transcript.length > 0 ? { transcript } : {}), - }); + }).catch(() => {}); } } } diff --git a/src/memory/links/link-aware-reflection.ts b/src/memory/links/link-aware-reflection.ts index 409730a7..6a0be0b2 100644 --- a/src/memory/links/link-aware-reflection.ts +++ b/src/memory/links/link-aware-reflection.ts @@ -51,11 +51,20 @@ export function createLinkAwareReflectionRunner(args: { } const ids = input.recalledMemoryIds ?? []; if (ids.length < minCandidates) return; + // Same shutdown race as the vote-aware decorator: `notesStore` + // is a SQLite handle that runtime shutdown may close while this + // fire-and-forget continuation is pending, and a closed + // better-sqlite3 statement throws `TypeError`. `reflect()` must + // stay fire-safe for the agent loop's bare `void` call. const candidates: { id: number; body: string }[] = []; - for (const id of ids) { - const entry = args.notesStore.get(id); - if (!entry) continue; - candidates.push({ id: entry.id, body: entry.content }); + try { + for (const id of ids) { + const entry = args.notesStore.get(id); + if (!entry) continue; + candidates.push({ id: entry.id, body: entry.content }); + } + } catch { + return; } if (candidates.length < minCandidates) return; try { diff --git a/src/memory/reflection-decorator-fire-safety.test.ts b/src/memory/reflection-decorator-fire-safety.test.ts new file mode 100644 index 00000000..aa11e09c --- /dev/null +++ b/src/memory/reflection-decorator-fire-safety.test.ts @@ -0,0 +1,305 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { MemoryStore } from "./memory-store.js"; +import { ProfileStore } from "./profile-store.js"; +import { LessonStore } from "./lessons/lesson-store.js"; +import { ProcedureStore } from "./procedures/procedure-store.js"; +import { createVoteAwareReflectionRunner } from "./voting/vote-aware-reflection.js"; +import { createLinkAwareReflectionRunner } from "./links/link-aware-reflection.js"; +import type { ReflectionInput, ReflectionRunner } from "./reflection/reflection-runner.js"; +import type { VoteRunner, VoteRunnerInput } from "./voting/vote-runner.js"; +import type { + LinkGeneratorInput, + LinkGeneratorRunner, +} from "./links/link-generator-runner.js"; + +/** + * Regression pin for the reflection decorators' fire-safety contract. + * + * `ReflectionRunner.reflect()` is documented fire-safe — see the + * invariants block in `reflection/reflection-runner.ts` — and + * `AgentLoop.runTurn` relies on that by calling it as a bare `void`. + * Both decorators hydrate candidate ids out of SQLite-backed stores + * *after* awaiting the inner runner, and that hydration used to sit + * outside every `try`. + * + * The live failure that motivated this file: runtime `shutdown()` + * calls `reflectionRunner.abortPending()` and then closes every store + * (`bootstrap.ts`). The abort settles the *inner* reflection, so the + * decorator's continuation resumes and reads stores the shutdown has + * since closed. better-sqlite3 answers a statement on a closed handle + * with a real `TypeError: The database connection is not open`, which + * escaped `reflect()` and landed as an unhandled rejection. + */ + +interface Fixture { + dir: string; + memoryStore: MemoryStore; + profileStore: ProfileStore; + lessonStore: LessonStore; + procedureStore: ProcedureStore; + ids: { + memory: number; + memory2: number; + profile: number; + lesson: number; + procedure: number; + }; + closeAll: () => void; +} + +const fixtures: Fixture[] = []; + +function makeFixture(): Fixture { + const dir = mkdtempSync(join(tmpdir(), "atomic-reflect-firesafe-")); + const dbFile = join(dir, "memory.sqlite"); + const memoryStore = new MemoryStore({ + dbFile, + maxEntries: 100, + eviction: { utilityWeighted: true, maxAgeMs: 1_000_000 }, + }); + const profileStore = new ProfileStore({ dbFile }); + const lessonStore = new LessonStore({ dbFile }); + const procedureStore = new ProcedureStore({ dbFile }); + + const memory = memoryStore.store({ content: "note one" }).id; + const memory2 = memoryStore.store({ content: "note two" }).id; + const profile = profileStore.set("editor", "vim").id; + const lesson = lessonStore.create({ + activation: "when the build fails", + principle: "read the first error", + parentIds: [memory], + }).id; + const procedure = procedureStore.create({ + activation: "when releasing", + steps: [ + { description: "run the tests" }, + { description: "tag the commit" }, + ], + parentLessonIds: [lesson], + parentMemoryIds: [memory], + }).id; + + const fx: Fixture = { + dir, + memoryStore, + profileStore, + lessonStore, + procedureStore, + ids: { memory, memory2, profile, lesson, procedure }, + closeAll() { + // Mirrors bootstrap `shutdown()` ordering. + try { + profileStore.close(); + } catch { + /* already closed */ + } + try { + lessonStore.close(); + } catch { + /* already closed */ + } + try { + procedureStore.close(); + } catch { + /* already closed */ + } + try { + memoryStore.close(); + } catch { + /* already closed */ + } + }, + }; + fixtures.push(fx); + return fx; +} + +afterEach(() => { + for (const fx of fixtures.splice(0)) { + fx.closeAll(); + rmSync(fx.dir, { recursive: true, force: true }); + } +}); + +/** Inner runner that resolves normally, optionally with a side effect. */ +function innerRunner(onReflect?: () => void): ReflectionRunner { + return { + async reflect() { + onReflect?.(); + }, + abortPending() { + /* no-op */ + }, + }; +} + +function recordingVoteRunner(calls: VoteRunnerInput[]): VoteRunner { + return { + async run(input) { + calls.push(input); + return { outcome: "applied", applied: 1, rejected: 0 }; + }, + abortPending() { + /* no-op */ + }, + }; +} + +function recordingLinkGenerator(calls: LinkGeneratorInput[]): LinkGeneratorRunner { + return { + async generate(input) { + calls.push(input); + return 1; + }, + abortPending() { + /* no-op */ + }, + }; +} + +function inputFor(fx: Fixture): ReflectionInput { + return { + sessionId: "s1", + userMessage: "hello", + assistantReply: "hi", + recalledMemoryIds: [fx.ids.memory, fx.ids.memory2], + recalledLessonIds: [fx.ids.lesson], + recalledProfileFactIds: [fx.ids.profile], + recalledProcedureIds: [fx.ids.procedure], + }; +} + +describe("reflection decorators are fire-safe across a store close", () => { + it("premise: a better-sqlite3 read after close throws a TypeError", () => { + const fx = makeFixture(); + expect(fx.memoryStore.get(fx.ids.memory)).not.toBeNull(); + fx.closeAll(); + let thrown: unknown; + try { + fx.memoryStore.get(fx.ids.memory); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(TypeError); + expect((thrown as Error).message).toContain("database connection is not open"); + }); + + it("vote-aware: hydration reaches the vote runner while the stores are open", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await runner.reflect(inputFor(fx)); + + expect(calls).toHaveLength(1); + // All four kinds hydrated — this is what the close-race test kills. + expect(calls[0]!.candidates.map((c) => c.kind).sort()).toEqual([ + "lesson", + "memory", + "memory", + "procedure", + "profile", + ]); + }); + + it("vote-aware: a store closed mid-flight does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + // The shutdown race: `abortPending()` settles the inner + // reflection, then the closes land before this continuation + // gets to hydrate. + reflection: innerRunner(() => fx.closeAll()), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + // Hydration failed wholesale, so the vote runner is never called + // with a partial allowlist. + expect(calls).toHaveLength(0); + }); + + it("vote-aware: a closed profile store alone does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(() => fx.profileStore.close()), + voteRunner: recordingVoteRunner(calls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("link-aware: hydration reaches the link generator while the store is open", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(calls), + notesStore: fx.memoryStore, + }); + + await runner.reflect(inputFor(fx)); + + expect(calls).toHaveLength(1); + expect(calls[0]!.candidates.map((c) => c.body)).toEqual([ + "note one", + "note two", + ]); + }); + + it("link-aware: a store closed mid-flight does not reject reflect()", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + linkGenerator: recordingLinkGenerator(calls), + notesStore: fx.memoryStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("both decorators composed: the close race still cannot reject", async () => { + const fx = makeFixture(); + const voteCalls: VoteRunnerInput[] = []; + const linkCalls: LinkGeneratorInput[] = []; + const runner = createVoteAwareReflectionRunner({ + reflection: createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: fx.memoryStore, + }), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(linkCalls).toHaveLength(0); + expect(voteCalls).toHaveLength(0); + }); +}); diff --git a/src/memory/voting/vote-aware-reflection.ts b/src/memory/voting/vote-aware-reflection.ts index 2c5dac65..e346cfc2 100644 --- a/src/memory/voting/vote-aware-reflection.ts +++ b/src/memory/voting/vote-aware-reflection.ts @@ -58,7 +58,20 @@ export function createVoteAwareReflectionRunner(args: { } catch { // ReflectionRunner is already fire-safe — defence in depth. } - const candidates = hydrateCandidates(input, args, previewChars); + // Hydration reads four SQLite-backed stores. Those reads can + // throw — most often `TypeError: The database connection is not + // open`, because runtime shutdown settles the inner reflection + // via `abortPending()` and then closes every store while this + // fire-and-forget continuation is still pending. `reflect()` is + // contractually fire-safe (see `reflection-runner.ts`) and the + // agent loop calls it as a bare `void`, so a throw escaping here + // becomes an unhandled rejection rather than a swallowed miss. + let candidates: VoteCandidate[]; + try { + candidates = hydrateCandidates(input, args, previewChars); + } catch { + return; + } if (candidates.length === 0) return; try { await args.voteRunner.run({ From 7c2a4cac1eac568a7f0f4cc1c4db70b6fd52a9ef Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Thu, 3 Sep 2026 23:31:24 +0300 Subject: [PATCH 2/5] fix(memory): report a swallowed hydration failure instead of hiding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit: the guards traded a visible crash for total silence. Both decorator factories took no logger, so a persistent hydration failure would disable the vote-runner and link-generator for the life of the process with no signal anywhere — the opposite of what the PR argued for, and out of step with the sibling paths (`memory context provider failed` is logged). - `createVoteAwareReflectionRunner` / `createLinkAwareReflectionRunner` take an optional `StructuredLogger` and warn on a caught hydration failure; `bootstrap.ts` passes the `logger` already in scope at both construction sites. - the trailing `.catch` on the `void reflect(...)` call warns instead of discarding. - the stale claim at `agent-loop.ts:507` that shutdown "drains" every in-flight reflection is corrected: `abortPending()` only signals, which is precisely why these guards exist. Tests, closing the coverage the review measured: - partial-hydration cases for both decorators — a store that answers the first id and then fails yields NO partial allowlist. This pins the wholesale-vs-per-id decision the PR body argues for; the link-aware "continue with a partial set" mutation survived the whole suite before this. - a non-`TypeError` store failure is contained just the same, so the guards are not silently narrowed to the closed-handle case. - the warn itself is asserted (message, sessionId, error text) for both decorators. - the turn-outcome assertions now check `reason` / `status`, not just the session id, which is identical on the failing path. `npm run lint` clean; `src/memory src/agent src/runtime` 75 files / 974 tests green. Not changed: `catch { return; }` → `catch { candidates = []; }` in the vote-aware guard survives the suite, but it is an equivalent mutant — the very next line is `if (candidates.length === 0) return;`. --- .../agent-loop-reflection-fire-safety.test.ts | 6 + src/agent/agent-loop.ts | 14 +- src/memory/links/link-aware-reflection.ts | 9 +- .../reflection-decorator-fire-safety.test.ts | 159 ++++++++++++++++++ src/memory/voting/vote-aware-reflection.ts | 12 +- src/runtime/bootstrap.ts | 2 + 6 files changed, 197 insertions(+), 5 deletions(-) diff --git a/src/agent/agent-loop-reflection-fire-safety.test.ts b/src/agent/agent-loop-reflection-fire-safety.test.ts index c362a2e6..a610026c 100644 --- a/src/agent/agent-loop-reflection-fire-safety.test.ts +++ b/src/agent/agent-loop-reflection-fire-safety.test.ts @@ -169,6 +169,7 @@ describe("AgentLoop reflection is background work, never a turn hazard", () => { signal: new AbortController().signal, }); expect(result.session.id).toBe("s1"); + expect(result.reason).toBe("reply"); }); expect(called).toBe(true); @@ -198,6 +199,11 @@ describe("AgentLoop reflection is background work, never a turn hazard", () => { signal: new AbortController().signal, }); + // The load-bearing assertion: on main this returns + // `reason: "failed"` / `status: "failed"` — the session id alone + // is the same either way, so it proves nothing on its own. + expect(result.reason).toBe("reply"); + expect(result.session.status).not.toBe("failed"); expect(result.session.id).toBe("s2"); // Reflection still fires — just without profile candidates. expect(inputs).toHaveLength(1); diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 92fd7b0d..039a4454 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -505,8 +505,11 @@ export class AgentLoop { // strictly larger memory set. // // Shutdown path still calls `abortPending()` with no sessionId - // to drain every in-flight reflection before the runtime tears - // down SQLite handles. + // before the runtime tears down SQLite handles. Note that it + // *signals* — nothing is awaited, so a reflection can still be + // resuming when the stores close. That is why the decorators and + // this call site guard their store reads rather than relying on + // the abort to have finished. if (options.userMessage !== undefined) { const text = options.userMessage; @@ -1213,7 +1216,12 @@ export class AgentLoop { ...(segmentationActive && transcript.length > 0 ? { transcript } : {}), - }).catch(() => {}); + }).catch((err: unknown) => { + this.deps.logger?.warn("reflection failed after dispatch", { + sessionId: state.id, + error: err instanceof Error ? err.message : String(err), + }); + }); } } } diff --git a/src/memory/links/link-aware-reflection.ts b/src/memory/links/link-aware-reflection.ts index 6a0be0b2..c9ef08d7 100644 --- a/src/memory/links/link-aware-reflection.ts +++ b/src/memory/links/link-aware-reflection.ts @@ -1,3 +1,4 @@ +import type { StructuredLogger } from "../../tracing/structured-logger.js"; import type { MemoryStore } from "../memory-store.js"; import type { ReflectionInput, @@ -40,6 +41,8 @@ export function createLinkAwareReflectionRunner(args: { notesStore: MemoryStore; /** Mirrors `LinkGeneratorRunnerDeps.minCandidates`. Defaults to 2. */ minCandidates?: number; + /** Reports a hydration failure — see the guard in `reflect`. */ + logger?: StructuredLogger; }): ReflectionRunner { const minCandidates = args.minCandidates ?? 2; return { @@ -63,7 +66,11 @@ export function createLinkAwareReflectionRunner(args: { if (!entry) continue; candidates.push({ id: entry.id, body: entry.content }); } - } catch { + } catch (err) { + args.logger?.warn("link candidate hydration failed", { + sessionId: input.sessionId, + error: err instanceof Error ? err.message : String(err), + }); return; } if (candidates.length < minCandidates) return; diff --git a/src/memory/reflection-decorator-fire-safety.test.ts b/src/memory/reflection-decorator-fire-safety.test.ts index aa11e09c..9568aafc 100644 --- a/src/memory/reflection-decorator-fire-safety.test.ts +++ b/src/memory/reflection-decorator-fire-safety.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { MemoryStore } from "./memory-store.js"; +import type { StructuredLogger } from "../tracing/structured-logger.js"; import { ProfileStore } from "./profile-store.js"; import { LessonStore } from "./lessons/lesson-store.js"; import { ProcedureStore } from "./procedures/procedure-store.js"; @@ -281,6 +282,164 @@ describe("reflection decorators are fire-safe across a store close", () => { expect(calls).toHaveLength(0); }); + it("vote-aware: a store that fails part-way yields no partial allowlist", async () => { + const fx = makeFixture(); + const calls: VoteRunnerInput[] = []; + // `memoryStore` answers the first id, then the handle goes away — + // the interleaving where hydration is already under way when + // shutdown lands. Guarding wholesale (not per-id) is a deliberate + // choice: the vote-runner scores a *set*, and a silently truncated + // allowlist would let it deprecate the entries that happened to be + // hydrated before the failure. + let reads = 0; + const flaky = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return (id: number) => { + reads += 1; + if (reads > 1) { + throw new TypeError("The database connection is not open"); + } + return target.get(id); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const runner = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(calls), + memoryStore: flaky, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(reads).toBe(2); + expect(calls).toEqual([]); + }); + + it("link-aware: a store that fails part-way yields no partial allowlist", async () => { + const fx = makeFixture(); + const calls: LinkGeneratorInput[] = []; + let reads = 0; + const flaky = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return (id: number) => { + reads += 1; + if (reads > 1) { + throw new TypeError("The database connection is not open"); + } + return target.get(id); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const runner = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(calls), + notesStore: flaky, + }); + + await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(reads).toBe(2); + expect(calls).toEqual([]); + }); + + it("the guards are not narrowed to TypeError — any store error is contained", async () => { + const fx = makeFixture(); + const voteCalls: VoteRunnerInput[] = []; + const linkCalls: LinkGeneratorInput[] = []; + // `MemoryStore.get` validates its id first and answers a bad one + // with a plain `MemoryValidationError`, not a `TypeError`. The + // closed-handle case is merely the one Sentry saw most. + const boom = new Proxy(fx.memoryStore, { + get(target, prop, receiver) { + if (prop === "get") { + return () => { + throw new RangeError("Too few parameter values were provided"); + }; + } + return Reflect.get(target, prop, receiver) as unknown; + }, + }) as MemoryStore; + + const vote = createVoteAwareReflectionRunner({ + reflection: innerRunner(), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: boom, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + }); + const link = createLinkAwareReflectionRunner({ + reflection: innerRunner(), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: boom, + }); + + await expect(vote.reflect(inputFor(fx))).resolves.toBeUndefined(); + await expect(link.reflect(inputFor(fx))).resolves.toBeUndefined(); + expect(voteCalls).toEqual([]); + expect(linkCalls).toEqual([]); + }); + + it("a hydration failure is logged, not silently swallowed", async () => { + const fx = makeFixture(); + const warnings: { message: string; fields?: Record }[] = []; + const logger = { + debug() { + /* unused */ + }, + info() { + /* unused */ + }, + warn(message: string, fields?: Record) { + warnings.push({ message, ...(fields ? { fields } : {}) }); + }, + error() { + /* unused */ + }, + } as unknown as StructuredLogger; + + const voteCalls: VoteRunnerInput[] = []; + const vote = createVoteAwareReflectionRunner({ + reflection: innerRunner(() => fx.closeAll()), + voteRunner: recordingVoteRunner(voteCalls), + memoryStore: fx.memoryStore, + lessonStore: fx.lessonStore, + profileStore: fx.profileStore, + procedureStore: fx.procedureStore, + logger, + }); + await vote.reflect(inputFor(fx)); + + expect(warnings).toHaveLength(1); + expect(warnings[0]!.message).toBe("vote candidate hydration failed"); + expect(warnings[0]!.fields).toMatchObject({ sessionId: "s1" }); + expect(String(warnings[0]!.fields?.error)).toContain( + "database connection is not open", + ); + + const fx2 = makeFixture(); + const linkCalls: LinkGeneratorInput[] = []; + const link = createLinkAwareReflectionRunner({ + reflection: innerRunner(() => fx2.closeAll()), + linkGenerator: recordingLinkGenerator(linkCalls), + notesStore: fx2.memoryStore, + logger, + }); + await link.reflect(inputFor(fx2)); + + expect(warnings).toHaveLength(2); + expect(warnings[1]!.message).toBe("link candidate hydration failed"); + }); + it("both decorators composed: the close race still cannot reject", async () => { const fx = makeFixture(); const voteCalls: VoteRunnerInput[] = []; diff --git a/src/memory/voting/vote-aware-reflection.ts b/src/memory/voting/vote-aware-reflection.ts index e346cfc2..fed748cc 100644 --- a/src/memory/voting/vote-aware-reflection.ts +++ b/src/memory/voting/vote-aware-reflection.ts @@ -1,3 +1,5 @@ +import type { StructuredLogger } from "../../tracing/structured-logger.js"; + import type { LessonStore } from "../lessons/lesson-store.js"; import type { MemoryStore } from "../memory-store.js"; import type { ProcedureStore } from "../procedures/procedure-store.js"; @@ -49,6 +51,8 @@ export function createVoteAwareReflectionRunner(args: { procedureStore?: ProcedureStore | null; /** Per-preview character cap. Defaults to 80. */ previewChars?: number; + /** Reports a hydration failure — see the guard in `reflect`. */ + logger?: StructuredLogger; }): ReflectionRunner { const previewChars = args.previewChars ?? 80; return { @@ -69,7 +73,13 @@ export function createVoteAwareReflectionRunner(args: { let candidates: VoteCandidate[]; try { candidates = hydrateCandidates(input, args, previewChars); - } catch { + } catch (err) { + // Swallowing without a word would trade a visible crash for + // silent curation loss, so the failure still gets a line. + args.logger?.warn("vote candidate hydration failed", { + sessionId: input.sessionId, + error: err instanceof Error ? err.message : String(err), + }); return; } if (candidates.length === 0) return; diff --git a/src/runtime/bootstrap.ts b/src/runtime/bootstrap.ts index 742a4bc4..5c6c5e7f 100644 --- a/src/runtime/bootstrap.ts +++ b/src/runtime/bootstrap.ts @@ -1863,6 +1863,7 @@ export async function createAgentRuntime( linkGenerator, notesStore, minCandidates: config.memory.links.minCandidates, + logger, }); } @@ -1949,6 +1950,7 @@ export async function createAgentRuntime( lessonStore, profileStore, procedureStore: config.memory.procedures.enabled ? procedureStore : null, + logger, }); } From edd1b847814d992400a895a548f1e8d8b798691c Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Thu, 3 Sep 2026 23:34:38 +0300 Subject: [PATCH 3/5] test(memory): make the link-aware partial-allowlist pin load-bearing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first version used two ids and failed on the second. That proves nothing: `minCandidates` defaults to 2, so a truncated list of one is dropped by the length gate whether the guard returns or falls through — the "continue with a partial set" mutation survived it. Three ids, failing on the third, so a fall-through guard would hand the link-generator a 2-entry set that passes the gate. Mutation confirmed killed. Battery re-run on this branch, all 8 functional mutations killed: per-id vote hydration (2 tests), link-aware partial fall-through, either warn dropped, guards narrowed to TypeError, the trailing `.catch` deleted, and each `profileFactsProvider` guard deleted — the step-loop one now dies on `expected 'failed' to be 'reply'`, i.e. on the turn outcome rather than incidentally. The one survivor is an equivalent mutant: `catch { return; }` → `catch { candidates = []; }` in the vote-aware guard, whose very next line is `if (candidates.length === 0) return;`. --- .../reflection-decorator-fire-safety.test.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/memory/reflection-decorator-fire-safety.test.ts b/src/memory/reflection-decorator-fire-safety.test.ts index 9568aafc..c8373c1b 100644 --- a/src/memory/reflection-decorator-fire-safety.test.ts +++ b/src/memory/reflection-decorator-fire-safety.test.ts @@ -324,13 +324,20 @@ describe("reflection decorators are fire-safe across a store close", () => { it("link-aware: a store that fails part-way yields no partial allowlist", async () => { const fx = makeFixture(); const calls: LinkGeneratorInput[] = []; + // Three ids, failing on the third. Two is not enough to prove + // anything: `minCandidates` defaults to 2, so a truncated list of + // one is dropped by the length gate whether the guard returns or + // falls through. With three, a "continue with what we have" guard + // would hand the link-generator a 2-entry set that passes the + // gate — which is exactly the behaviour being ruled out. + const third = fx.memoryStore.store({ content: "note three" }).id; let reads = 0; const flaky = new Proxy(fx.memoryStore, { get(target, prop, receiver) { if (prop === "get") { return (id: number) => { reads += 1; - if (reads > 1) { + if (reads > 2) { throw new TypeError("The database connection is not open"); } return target.get(id); @@ -346,8 +353,14 @@ describe("reflection decorators are fire-safe across a store close", () => { notesStore: flaky, }); - await expect(runner.reflect(inputFor(fx))).resolves.toBeUndefined(); - expect(reads).toBe(2); + const input = inputFor(fx); + await expect( + runner.reflect({ + ...input, + recalledMemoryIds: [...(input.recalledMemoryIds ?? []), third], + }), + ).resolves.toBeUndefined(); + expect(reads).toBe(3); expect(calls).toEqual([]); }); From 793c69b6d39ad1f82f574230777c54161592c744 Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Thu, 3 Sep 2026 23:35:03 +0300 Subject: [PATCH 4/5] docs(agent): correct the profile-facts guard rationale Review caught the comment (and the PR body) claiming the renderer already drops these facts when the contextual gate does not match. `profile-renderer.ts:63` returns true for every pinned fact before the gate is consulted, so the guard omits the whole `### profile` section for the rest of the turn. Still the right trade against failing the turn, but say so accurately. --- src/agent/agent-loop.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 039a4454..7c301929 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -654,12 +654,13 @@ export class AgentLoop { "This is the final allowed step. Do not call any non-terminal tool; " + "summarize the completed work with reply, or end the session with finish."; try { - // `profileFactsProvider` is a raw `profileStore.list()`. The - // facts are prompt decoration — the renderer already drops - // them when the contextual gate does not match — so a store - // failure must render the step without them rather than throw - // into the catch below, where a `TypeError` from a closed - // SQLite handle would be classified `tool` and fail the turn. + // `profileFactsProvider` is a raw `profileStore.list()`. + // Dropping the facts is a real loss — `profile-renderer` emits + // pinned facts regardless of the contextual gate, so this + // omits the whole `### profile` section for the rest of the + // turn — but it is the lesser one: a throw here lands in the + // catch below, where a `TypeError` from a closed SQLite handle + // classifies `tool` and fails the turn outright. let profileFacts: readonly ProfileFact[] | undefined; try { profileFacts = this.deps.profileFactsProvider?.(); From c372b003c9a55c0d4fe7da946179ad7c3baacd25 Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Fri, 4 Sep 2026 00:02:18 +0300 Subject: [PATCH 5/5] fix(agent): the reflection-input profile guard reports too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round found the one remaining silent swallow — the `profileFactsProvider` guard feeding the reflection allowlist — which made the PR's own "nothing is swallowed silently" claim false. Usually the step guard has already warned for that turn (same provider, same store), but the store can close between the last step and this block. Also corrected two comments the review measured as imprecise: - the step guard drops the `### profile` section for that *step*, not the rest of the turn; - the surviving comment at the reflection block still said the renderer surfaces profile facts "whenever they pass the contextual-keyword gate", the same imprecision fixed 500 lines above — pinned facts bypass the gate. New test asserts BOTH guards report, with sessionId and error text; reverting the new warn kills it. `npm run lint` clean; `src/memory src/agent src/runtime` 75 files / 975 tests green. --- .../agent-loop-reflection-fire-safety.test.ts | 65 +++++++++++++++++++ src/agent/agent-loop.ts | 20 ++++-- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/agent/agent-loop-reflection-fire-safety.test.ts b/src/agent/agent-loop-reflection-fire-safety.test.ts index a610026c..9a30e9da 100644 --- a/src/agent/agent-loop-reflection-fire-safety.test.ts +++ b/src/agent/agent-loop-reflection-fire-safety.test.ts @@ -15,6 +15,7 @@ import type { ToolDescriptor, } from "../prompt/stable-prefix.js"; import type { ProfileFact } from "../memory/profile-store.js"; +import type { StructuredLogger } from "../tracing/structured-logger.js"; import type { ReflectionInput, ReflectionRunner, @@ -97,6 +98,7 @@ function makeFact(id: number): ProfileFact { function makeLoop(deps: { reflectionRunner: ReflectionRunner; profileFactsProvider?: () => readonly ProfileFact[]; + logger?: StructuredLogger; }): AgentLoop { return new AgentLoop({ registry: buildDefaultToolRegistry(), @@ -112,9 +114,32 @@ function makeLoop(deps: { ...(deps.profileFactsProvider ? { profileFactsProvider: deps.profileFactsProvider } : {}), + ...(deps.logger ? { logger: deps.logger } : {}), }); } +interface Warning { + message: string; + fields?: Record; +} + +function capturingLogger(into: Warning[]): StructuredLogger { + return { + debug() { + /* unused */ + }, + info() { + /* unused */ + }, + warn(message: string, fields?: Record) { + into.push({ message, ...(fields ? { fields } : {}) }); + }, + error() { + /* unused */ + }, + } as unknown as StructuredLogger; +} + /** Collect unhandled rejections raised while `body` runs. */ async function withUnhandledRejectionWatch( body: () => Promise, @@ -210,6 +235,46 @@ describe("AgentLoop reflection is background work, never a turn hazard", () => { expect(inputs[0]!.recalledProfileFactIds).toBeUndefined(); }); + it("both profile-facts guards report rather than swallow", async () => { + const warnings: Warning[] = []; + const inputs: ReflectionInput[] = []; + const loop = makeLoop({ + reflectionRunner: { + async reflect(input: ReflectionInput) { + inputs.push(input); + }, + abortPending() { + /* no-op */ + }, + }, + profileFactsProvider: () => { + throw new TypeError("The database connection is not open"); + }, + logger: capturingLogger(warnings), + }); + const session = createEmptySessionState({ id: "s4", workingDir }); + + const result = await loop.runTurn(session, { + userMessage: "hello", + maxSteps: 2, + signal: new AbortController().signal, + }); + + expect(result.reason).toBe("reply"); + expect(inputs).toHaveLength(1); + // The step guard fires per step; the reflection guard fires once + // at the end of the turn. Neither may be silent. + const messages = warnings.map((w) => w.message); + expect(messages).toContain("profile facts unavailable for this step"); + expect(messages).toContain("profile facts unavailable for reflection"); + for (const w of warnings) { + expect(w.fields).toMatchObject({ sessionId: "s4" }); + expect(String(w.fields?.error)).toContain( + "database connection is not open", + ); + } + }); + it("a healthy profileFactsProvider still supplies the allowlist", async () => { const inputs: ReflectionInput[] = []; const loop = makeLoop({ diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 7c301929..00a80e76 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -656,9 +656,9 @@ export class AgentLoop { try { // `profileFactsProvider` is a raw `profileStore.list()`. // Dropping the facts is a real loss — `profile-renderer` emits - // pinned facts regardless of the contextual gate, so this - // omits the whole `### profile` section for the rest of the - // turn — but it is the lesser one: a throw here lands in the + // pinned facts regardless of the contextual gate, so this step + // renders with no `### profile` section at all — but it is the + // lesser one: a throw here lands in the // catch below, where a `TypeError` from a closed SQLite handle // classifies `tool` and fails the turn outright. let profileFacts: readonly ProfileFact[] | undefined; @@ -1163,8 +1163,8 @@ export class AgentLoop { // recalled across all steps of this turn) ∪ (profile // facts currently active). Profile facts are not gated // by recall — they're always candidates because the - // renderer already surfaces them whenever they pass the - // contextual-keyword gate. Sourcing them here keeps the + // renderer surfaces them whenever they are pinned or pass + // the contextual-keyword gate. Sourcing them here keeps the // decorator's hydration cheap. // `profileFactsProvider` is a raw `profileStore.list()`. // It is only ever an input to the fire-and-forget reflection @@ -1174,8 +1174,14 @@ export class AgentLoop { let profileFacts: readonly ProfileFact[] = []; try { profileFacts = this.deps.profileFactsProvider?.() ?? []; - } catch { - // best-effort: reflection candidates only + } catch (err) { + // Usually the step guard above has already warned for this + // turn — same provider, same store. Not always: the store + // can close between the last step and this block. + this.deps.logger?.warn("profile facts unavailable for reflection", { + sessionId: state.id, + error: err instanceof Error ? err.message : String(err), + }); } // `reflect()` is documented fire-safe, but it is composed at // runtime from decorators that read SQLite stores. A bare