diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8846e99e6..882d38356 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed Hindsight retain/recall/reflect calls staying pinned to the bank that was selected when the session started after the operator edited `hindsight.bankId`, `hindsight.bankIdPrefix`, or `hindsight.scoping` mid-session. The backend now subscribes to those settings via `onHindsightScopeChanged` and rebuilds the active `HindsightSessionState` against the recomputed scope, disposing the old state after flushing its queue so in-flight tool-initiated retains still land in the bank they were enqueued for. Also renamed `ensureBankMission` to `ensureBankExists` so a blank `bankMission` no longer skips bank creation entirely, and called it before mental-model bootstrap so `createMentalModel` is never the first POST against a missing bank. `AgentSession.dispose` now flushes the retain queue before clearing `#hindsightSessionState`, since the queue's identity guard would otherwise drop the spliced batch ([#1902](https://github.com/can1357/oh-my-pi/issues/1902)). + ## [15.9.1] - 2026-06-04 ### Added diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index c735aa770..f5dc33299 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -907,6 +907,9 @@ const SETTING_HOOKS: Partial>> = { for (const cb of appendOnlyModeCallbacks) cb(value); } }, + "hindsight.bankId": () => fireHindsightScopeChanged(), + "hindsight.bankIdPrefix": () => fireHindsightScopeChanged(), + "hindsight.scoping": () => fireHindsightScopeChanged(), }; /** Callbacks invoked when `provider.appendOnlyContext` changes at runtime. */ const appendOnlyModeCallbacks = new Set<(value: string) => void>(); @@ -923,6 +926,41 @@ export function onAppendOnlyModeChanged(cb: (value: string) => void): () => void }; } +/** Callbacks fired when any `hindsight.bankId` / `bankIdPrefix` / `scoping` value changes. */ +const hindsightScopeCallbacks = new Set<() => void>(); + +function fireHindsightScopeChanged(): void { + // Snapshot the callback set before invoking — a callback's body is allowed + // to subscribe a NEW callback (the Hindsight backend re-registers the + // fresh state's listener on every rebuild). Iterating the live Set would + // re-invoke those just-added callbacks within the same fire, which spins + // in place: subscribe → invoke → subscribe → invoke → … + for (const cb of [...hindsightScopeCallbacks]) { + try { + cb(); + } catch (err) { + logger.warn("Settings: hindsight scope hook failed", { error: String(err) }); + } + } +} + +/** + * Subscribe to changes in the Hindsight bank-scoping settings. Lets the + * Hindsight backend rebuild the active `HindsightSessionState` when the + * operator switches `hindsight.bankId`, `hindsight.bankIdPrefix`, or + * `hindsight.scoping` mid-session so subsequent retain/recall calls land in + * the new bank instead of the one selected at session start. + * + * Returns an unsubscribe function. The callback receives no arguments — the + * caller is expected to re-read the relevant settings via `Settings.get`. + */ +export function onHindsightScopeChanged(cb: () => void): () => void { + hindsightScopeCallbacks.add(cb); + return () => { + hindsightScopeCallbacks.delete(cb); + }; +} + // ═══════════════════════════════════════════════════════════════════════════ // Global Singleton // ═══════════════════════════════════════════════════════════════════════════ diff --git a/packages/coding-agent/src/hindsight/backend.ts b/packages/coding-agent/src/hindsight/backend.ts index 2d00dee27..79ff8599f 100644 --- a/packages/coding-agent/src/hindsight/backend.ts +++ b/packages/coding-agent/src/hindsight/backend.ts @@ -9,10 +9,10 @@ import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import { logger } from "@oh-my-pi/pi-utils"; -import type { Settings } from "../config/settings"; +import { onHindsightScopeChanged, type Settings } from "../config/settings"; import type { MemoryBackend, MemoryBackendStartOptions } from "../memory-backend/types"; import type { AgentSession } from "../session/agent-session"; -import { computeBankScope } from "./bank"; +import { type BankScope, computeBankScope } from "./bank"; import { createHindsightClient } from "./client"; import { isHindsightConfigured, loadHindsightConfig } from "./config"; import type { HindsightMessage } from "./content"; @@ -60,12 +60,16 @@ export const hindsightBackend: MemoryBackend = { recallTagsMatch: parent.recallTagsMatch, config: parent.config, session, - missionsSet: parent.missionsSet, + banksSet: parent.banksSet, lastRetainedTurn: 0, hasRecalledForFirstTurn: true, aliasOf: parent, }), ); + // Aliases don't run auto-recall/auto-retain, so any pending retain + // queue belongs to the previous alias and is safe to drop after a + // best-effort flush (`flushRetainQueue` is no-op when empty). + await previous?.flushRetainQueue(); previous?.dispose(); return; } @@ -76,38 +80,7 @@ export const hindsightBackend: MemoryBackend = { return; } - const client = createHindsightClient(config); - const scope = computeBankScope(config, session.sessionManager.getCwd()); - - const state = new HindsightSessionState({ - sessionId, - client, - bankId: scope.bankId, - retainTags: scope.retainTags, - recallTags: scope.recallTags, - recallTagsMatch: scope.recallTagsMatch, - config, - session, - missionsSet: new Set(), - lastRetainedTurn: 0, - hasRecalledForFirstTurn: false, - }); - - // Cleanup any stale state for this session (defensive — prevents leaks - // when a session is reused without going through dispose). - const previous = session.setHindsightSessionState(state); - previous?.dispose(); - state.attachSessionListeners(); - - // Kick off mental-model bootstrap. Resolves asynchronously; the first - // turn races and is covered in `beforeAgentStartPrompt` via - // `mentalModelsLoadPromise`. Subsequent turns see the populated cache - // because `runMentalModelLoad` calls `refreshBaseSystemPrompt`. - if (config.mentalModelsEnabled) { - state.mentalModelsLoadPromise = state.runMentalModelLoad(scope).catch(err => { - logger.debug("Hindsight: mental-model bootstrap failed", { bankId: state.bankId, error: String(err) }); - }); - } + await installPrimaryState(session, settings, new Set()); }, async buildDeveloperInstructions(_agentDir, settings, session): Promise { @@ -173,6 +146,131 @@ export const hindsightBackend: MemoryBackend = { return await state.recallForCompaction(flat); }, }; +/** + * Build (or rebuild) the primary `HindsightSessionState` for `session` from + * the current settings and install it. Disposes any previous primary state + * after flushing its retain queue so in-flight tool-initiated retains land in + * the bank that was selected when they were enqueued, not in the new bank. + * + * The created state takes ownership of the `onHindsightScopeChanged` + * subscription so subsequent `hindsight.bankId` / `bankIdPrefix` / `scoping` + * edits trigger another rebuild from the same wiring. + */ +async function installPrimaryState( + session: AgentSession, + settings: Settings, + banksSet: Set, +): Promise { + const sessionId = session.sessionId; + if (!sessionId) return undefined; + + const config = loadHindsightConfig(settings); + if (!isHindsightConfigured(config)) return undefined; + + const client = createHindsightClient(config); + const scope = computeBankScope(config, session.sessionManager.getCwd()); + + const state = new HindsightSessionState({ + sessionId, + client, + bankId: scope.bankId, + retainTags: scope.retainTags, + recallTags: scope.recallTags, + recallTagsMatch: scope.recallTagsMatch, + config, + session, + banksSet, + lastRetainedTurn: 0, + hasRecalledForFirstTurn: false, + }); + + // Subscribe BEFORE installing: if the operator manages to flip another + // setting between install and subscribe, we'd miss the edge. + state.unsubscribeScope = onHindsightScopeChanged(() => { + void rebuildPrimaryStateOnScopeChange(session); + }); + + // Cleanup any stale state for this session (defensive — prevents leaks + // when a session is reused without going through dispose). Flush the + // previous state's retain queue BEFORE clearing it, otherwise + // `HindsightRetainQueue.#doFlush` sees `session.getHindsightSessionState() + // !== state` and drops the batch. + const previous = session.getHindsightSessionState(); + if (previous && previous !== state) { + await previous.flushRetainQueue(); + } + session.setHindsightSessionState(state); + previous?.dispose(); + state.attachSessionListeners(); + + // Kick off mental-model bootstrap. Resolves asynchronously; the first + // turn races and is covered in `beforeAgentStartPrompt` via + // `mentalModelsLoadPromise`. Subsequent turns see the populated cache + // because `runMentalModelLoad` calls `refreshBaseSystemPrompt`. + if (config.mentalModelsEnabled) { + state.mentalModelsLoadPromise = state.runMentalModelLoad(scope).catch(err => { + logger.debug("Hindsight: mental-model bootstrap failed", { bankId: state.bankId, error: String(err) }); + }); + } + + return state; +} + +/** + * `onHindsightScopeChanged` handler: re-evaluate the bank scope from current + * settings and rebuild the primary state when it has actually drifted. No-op + * when the scope is unchanged or the session is no longer hosting a primary + * state (e.g. it was wiped to `undefined`, or this is a subagent alias). + */ +async function rebuildPrimaryStateOnScopeChange(session: AgentSession): Promise { + const current = session.getHindsightSessionState(); + if (!current || current.aliasOf) return; + + const settings = session.settings; + const config = loadHindsightConfig(settings); + if (!isHindsightConfigured(config)) { + // Hindsight effectively unwired mid-session. Flush before clearing so + // queued retains don't get dropped by `HindsightRetainQueue.#doFlush`. + await current.flushRetainQueue(); + const previous = session.setHindsightSessionState(undefined); + previous?.dispose(); + return; + } + + const next = computeBankScope(config, session.sessionManager.getCwd()); + if (bankScopesEqual(next, current)) return; + + // Preserve the banksSet so we don't re-PUT banks we've already confirmed. + await installPrimaryState(session, settings, current.banksSet); +} + +/** Tag-array equality: order matters because we never reorder on the way in. */ +function stringArraysEqual(a: string[] | undefined, b: string[] | undefined): boolean { + if (a === b) return true; + if (!a || !b) return false; + if (a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + if (a[i] !== b[i]) return false; + } + return true; +} + +/** + * Structural compare of a freshly resolved `BankScope` against a live state's + * bank routing. Used by the scope-change handler to skip rebuilds that don't + * actually move the bank or its tag filters. + */ +function bankScopesEqual( + scope: BankScope, + state: Pick, +): boolean { + return ( + scope.bankId === state.bankId && + stringArraysEqual(scope.retainTags, state.retainTags) && + stringArraysEqual(scope.recallTags, state.recallTags) && + scope.recallTagsMatch === state.recallTagsMatch + ); +} /** Reduce arbitrary AgentMessages into the Hindsight flat-text shape. */ function flattenMessagesForRecall(messages: AgentMessage[]): HindsightMessage[] { diff --git a/packages/coding-agent/src/hindsight/bank.ts b/packages/coding-agent/src/hindsight/bank.ts index a6f9cc149..32bc28ce4 100644 --- a/packages/coding-agent/src/hindsight/bank.ts +++ b/packages/coding-agent/src/hindsight/bank.ts @@ -1,5 +1,5 @@ /** - * Bank ID derivation, project-tag scoping, and first-use mission setup. + * Bank ID derivation, project-tag scoping, and first-use bank setup. * * Three scoping modes (`HindsightConfig.scoping`): * - `global` — single shared bank, no per-project filter. @@ -11,10 +11,13 @@ * The base bank id is `bankIdPrefix-bankId` (default `omp`). Per-project mode * appends `-`; tagged mode leaves the bank untouched and uses tags. * - * Mission setup is idempotent at module level — a missionsSet keeps track of - * banks we've already POSTed to so each session boundary doesn't fire a fresh - * `createBank` call. Failures are swallowed: missions are an optimisation, not - * a precondition for retain/recall. + * Bank existence is idempotent at module level — a banksSet keeps track of + * banks we've already PUT so each session boundary doesn't fire a fresh + * `createBank` call. The PUT is idempotent server-side, so re-firing on a hot + * path would only burn round-trips. Failures are swallowed: missing the + * mission patch is an optimisation, but the bank ITSELF must exist before + * mental-model bootstrap or the first retain, otherwise the very first POST + * lands against a missing bank. */ import * as path from "node:path"; @@ -93,39 +96,46 @@ export function deriveBankId(config: HindsightConfig, directory: string): string } /** - * Ensure a bank's reflect/retain mission is set, exactly once per process. + * Ensure a bank exists, and patch its reflect/retain mission on first use. * - * Tracked via the supplied set; on overflow we drop the oldest half so the set - * cannot grow unboundedly across long-lived processes. + * Idempotent: skips the PUT when the bank id is already in the supplied set. + * The mission body is optional — when `bankMission` is blank we still PUT to + * make sure the bank itself is created, so mental-model bootstrap and the + * first retain don't land against a non-existent bank. + * + * The set is capped; on overflow we drop the oldest half so it cannot grow + * unboundedly across long-lived processes. */ -export async function ensureBankMission( +export async function ensureBankExists( client: HindsightApi, bankId: string, config: HindsightConfig, - missionsSet: Set, + banksSet: Set, ): Promise { + if (banksSet.has(bankId)) return; + const mission = config.bankMission?.trim(); - if (!mission) return; - if (missionsSet.has(bankId)) return; + const retainMission = config.retainMission?.trim(); try { await client.createBank(bankId, { - reflectMission: mission, - retainMission: config.retainMission?.trim() || undefined, + reflectMission: mission || undefined, + retainMission: retainMission || undefined, }); - missionsSet.add(bankId); - if (missionsSet.size > MISSION_SET_CAP) { - const keys = [...missionsSet].sort(); + banksSet.add(bankId); + if (banksSet.size > MISSION_SET_CAP) { + const keys = [...banksSet].sort(); for (const key of keys.slice(0, keys.length >> 1)) { - missionsSet.delete(key); + banksSet.delete(key); } } if (config.debug) { - logger.debug("Hindsight: set mission for bank", { bankId }); + logger.debug("Hindsight: ensured bank", { bankId, mission: Boolean(mission) }); } } catch (err) { - // Mission set is best-effort; the bank may not exist yet, or the API may - // reject the call. Either way, retain/recall still work, so swallow. - logger.debug("Hindsight: ensureBankMission failed", { bankId, error: String(err) }); + // Bank creation is best-effort; the server may already have it, or the + // API may reject the call. Either way, downstream retain/recall calls + // will surface a clearer error if the bank really is missing. + logger.debug("Hindsight: ensureBankExists failed", { bankId, error: String(err) }); } } diff --git a/packages/coding-agent/src/hindsight/mental-models.ts b/packages/coding-agent/src/hindsight/mental-models.ts index 294fb5fab..210137193 100644 --- a/packages/coding-agent/src/hindsight/mental-models.ts +++ b/packages/coding-agent/src/hindsight/mental-models.ts @@ -112,7 +112,7 @@ function dedupe(items: T[]): T[] { * Idempotently create any seed mental models that don't already exist on the * bank. Best-effort: a list/create failure does not throw — mental models are * an optimization, not a precondition for retain/recall, and we mirror the - * swallow-on-failure pattern used by `ensureBankMission`. + * swallow-on-failure pattern used by `ensureBankExists`. * * Existing models are NEVER modified. See module docstring. */ diff --git a/packages/coding-agent/src/hindsight/state.ts b/packages/coding-agent/src/hindsight/state.ts index 883e5f714..26f3e7d58 100644 --- a/packages/coding-agent/src/hindsight/state.ts +++ b/packages/coding-agent/src/hindsight/state.ts @@ -1,6 +1,6 @@ import { logger } from "@oh-my-pi/pi-utils"; import type { AgentSession } from "../session/agent-session"; -import { type BankScope, ensureBankMission } from "./bank"; +import { type BankScope, ensureBankExists } from "./bank"; import type { HindsightApi, MemoryItemInput } from "./client"; import type { HindsightConfig } from "./config"; import { @@ -45,12 +45,12 @@ export interface HindsightSessionStateOptions { recallTagsMatch?: "any" | "all" | "any_strict" | "all_strict"; config: HindsightConfig; session: AgentSession; - missionsSet: Set; + banksSet: Set; lastRetainedTurn?: number; hasRecalledForFirstTurn?: boolean; /** * When set, this entry is a subagent alias that reuses the parent's bank, - * scope, config, client, and missionsSet. Aliases skip auto-recall and + * scope, config, client, and banksSet. Aliases skip auto-recall and * auto-retain — those run on the parent only — but the recall/retain/reflect * tools resolve via the alias so they persist to the same bank as the parent. */ @@ -148,7 +148,7 @@ export class HindsightRetainQueue { } try { - await ensureBankMission(state.client, state.bankId, state.config, state.missionsSet); + await ensureBankExists(state.client, state.bankId, state.config, state.banksSet); const batch: MemoryItemInput[] = items.map(item => ({ content: item.content, context: item.context ?? state.config.retainContext, @@ -198,7 +198,7 @@ export class HindsightSessionState { recallTagsMatch?: "any" | "all" | "any_strict" | "all_strict"; config: HindsightConfig; session: AgentSession; - missionsSet: Set; + banksSet: Set; lastRetainedTurn: number; hasRecalledForFirstTurn: boolean; lastRecallSnippet?: string; @@ -213,6 +213,12 @@ export class HindsightSessionState { */ mentalModelsLoadPromise?: Promise; unsubscribe?: () => void; + /** + * Releases the `onHindsightScopeChanged` subscription that drives live + * rebuilds when `hindsight.bankId` / `bankIdPrefix` / `scoping` change. + * Only set on primary states; aliases inherit the parent's subscription. + */ + unsubscribeScope?: () => void; /** Alias states delegate persistence config to a primary parent state. */ aliasOf?: HindsightSessionState; readonly retainQueue: HindsightRetainQueue; @@ -226,7 +232,7 @@ export class HindsightSessionState { this.recallTagsMatch = options.recallTagsMatch; this.config = options.config; this.session = options.session; - this.missionsSet = options.missionsSet; + this.banksSet = options.banksSet; this.lastRetainedTurn = options.lastRetainedTurn ?? 0; this.hasRecalledForFirstTurn = options.hasRecalledForFirstTurn ?? false; this.aliasOf = options.aliasOf; @@ -291,7 +297,7 @@ export class HindsightSessionState { const { transcript } = prepareRetentionTranscript(target, true); if (!transcript) return; - await ensureBankMission(this.client, this.bankId, this.config, this.missionsSet); + await ensureBankExists(this.client, this.bankId, this.config, this.banksSet); await this.client.retain(this.bankId, transcript, { documentId, context: this.config.retainContext, @@ -398,6 +404,12 @@ export class HindsightSessionState { async runMentalModelLoad(scope: BankScope): Promise { if (!this.config.mentalModelsEnabled) return; + // Create/ensure the bank BEFORE the first mental-model POST so we don't + // land `createMentalModel` against a bank the server has never seen — + // that surfaces as a FK / 404 on Hindsight's side. `ensureBankExists` + // is idempotent (PUT) and skips after the first call via `banksSet`. + await ensureBankExists(this.client, this.bankId, this.config, this.banksSet); + // Seeding is opt-in (`hindsight.mentalModelAutoSeed`). Default behaviour is // read-only: we surface whatever models the operator has curated on the // bank, but we do NOT POST to create new ones unless they explicitly @@ -456,6 +468,8 @@ export class HindsightSessionState { dispose(): void { this.unsubscribe?.(); this.unsubscribe = undefined; + this.unsubscribeScope?.(); + this.unsubscribeScope = undefined; this.retainQueue.dispose(); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index b7760bb5c..d0ea75ac8 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -2990,8 +2990,13 @@ export class AgentSession { this.#releasePowerAssertion(); await this.sessionManager.close(); this.#closeAllProviderSessions("dispose"); - const hindsightState = this.setHindsightSessionState(undefined); + // Flush the retain queue BEFORE clearing the session's pointer so + // `HindsightRetainQueue.#doFlush` still sees `session.getHindsightSessionState() === state`. + // Reversed, the spliced batch survives just long enough to fail the + // identity check and get dropped with a `session vanished` warning. + const hindsightState = this.getHindsightSessionState(); await hindsightState?.flushRetainQueue(); + this.setHindsightSessionState(undefined); hindsightState?.dispose(); const mnemopiState = setMnemopiSessionState(this, undefined); mnemopiState?.dispose(); diff --git a/packages/coding-agent/src/tools/memory-reflect.ts b/packages/coding-agent/src/tools/memory-reflect.ts index 19e5fd2d3..8227a3328 100644 --- a/packages/coding-agent/src/tools/memory-reflect.ts +++ b/packages/coding-agent/src/tools/memory-reflect.ts @@ -1,7 +1,7 @@ import type { AgentTool, AgentToolResult } from "@oh-my-pi/pi-agent-core"; import { logger, untilAborted } from "@oh-my-pi/pi-utils"; import * as z from "zod/v4"; -import { ensureBankMission } from "../hindsight/bank"; +import { ensureBankExists } from "../hindsight/bank"; import reflectDescription from "../prompts/tools/reflect.md" with { type: "text" }; import type { ToolSession } from "."; @@ -67,7 +67,7 @@ export class MemoryReflectTool implements AgentTool } try { - await ensureBankMission(state.client, state.bankId, state.config, state.missionsSet); + await ensureBankExists(state.client, state.bankId, state.config, state.banksSet); const response = await state.client.reflect(state.bankId, params.query, { context: params.context, budget: state.config.recallBudget, diff --git a/packages/coding-agent/test/hindsight-backend.test.ts b/packages/coding-agent/test/hindsight-backend.test.ts index 550306447..fbe17d9fe 100644 --- a/packages/coding-agent/test/hindsight-backend.test.ts +++ b/packages/coding-agent/test/hindsight-backend.test.ts @@ -19,6 +19,7 @@ interface FakeSessionDeps { sessionId: string | null; cwd?: string; entries?: Array<{ role: "user" | "assistant"; text: string }>; + settings?: Settings; } function makeFakeSession(deps: FakeSessionDeps) { @@ -27,7 +28,7 @@ function makeFakeSession(deps: FakeSessionDeps) { let hindsightState: HindsightSessionState | undefined; const session = { sessionId: deps.sessionId, - settings: Settings.isolated(), + settings: deps.settings ?? Settings.isolated(), sessionManager: { getEntries: () => entries.map((e, i) => ({ @@ -207,7 +208,7 @@ describe("hindsightBackend.start", () => { expect(subState?.aliasOf).toBe(parentState); expect(subState?.bankId).toBe(parentState?.bankId); expect(subState?.client).toBe(parentState?.client); - expect(subState?.missionsSet).toBe(parentState?.missionsSet); + expect(subState?.banksSet).toBe(parentState?.banksSet); // Aliases must not subscribe to session events — the parent owns auto-recall/auto-retain. expect(subState?.unsubscribe).toBeUndefined(); // hasRecalledForFirstTurn=true suppresses beforeAgentStartPrompt auto-recall on the sub. @@ -532,3 +533,252 @@ describe("hindsightBackend.clear", () => { expect(deleteSpy).not.toHaveBeenCalled(); }); }); + +describe("hindsightBackend live bank routing", () => { + beforeEach(() => { + resetSettingsForTest(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + // Regression for issue #1902: changing `hindsight.bankId` during a live + // session used to leave the active `HindsightSessionState` pinned to the + // bank that was selected when the session started, so subsequent retains + // kept landing in the stale bank ("omp") instead of the new one + // ("Minigames"). The bank-routing settings must re-resolve on `set`. + it("rebuilds the primary state when hindsight.bankId changes mid-session", async () => { + vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + "hindsight.scoping": "global", + }); + // Seed bankId via `set` (not `isolated` overrides), otherwise the + // follow-up `set` writes to `#global` while `get` keeps returning the + // `#overrides` value — exactly the precedence the live settings UI + // does NOT have, since real config writes land in `#global`. + settings.set("hindsight.bankId", "omp"); + const session = makeFakeSession({ sessionId: "s-rebuild", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + + const initial = session.getHindsightSessionState(); + expect(initial?.bankId).toBe("omp"); + + settings.set("hindsight.bankId", "Minigames"); + // Hook is sync but the rebuild is async; yield once so the handler runs. + await Bun.sleep(0); + + const next = session.getHindsightSessionState(); + expect(next).toBeDefined(); + expect(next?.bankId).toBe("Minigames"); + // Must be a brand-new state — the old one was disposed. + expect(next).not.toBe(initial); + }); + + // Same regression, exercising the `hindsight.scoping` axis: switching + // scope mode also reshapes the bank id / tag filters and must rebuild. + it("rebuilds the primary state when hindsight.scoping changes mid-session", async () => { + vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + }); + settings.set("hindsight.scoping", "global"); + const session = makeFakeSession({ sessionId: "s-scoping", cwd: "/work/proj", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + + const initial = session.getHindsightSessionState(); + expect(initial?.bankId).toBe("omp"); + expect(initial?.retainTags).toBeUndefined(); + + settings.set("hindsight.scoping", "per-project"); + await Bun.sleep(0); + + const next = session.getHindsightSessionState(); + expect(next).toBeDefined(); + expect(next?.bankId).toBe("omp-proj"); + expect(next).not.toBe(initial); + }); + + // Same setting written with the same value MUST NOT rebuild — a rebuild + // would reset `lastRetainedTurn` / `hasRecalledForFirstTurn` and force a + // fresh mental-model bootstrap for no observable reason. + it("does not rebuild when the bank-routing setting is rewritten with the same value", async () => { + vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + }); + settings.set("hindsight.bankId", "omp"); + const session = makeFakeSession({ sessionId: "s-noop", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + + const initial = session.getHindsightSessionState(); + settings.set("hindsight.bankId", "omp"); // unchanged + await Bun.sleep(0); + + expect(session.getHindsightSessionState()).toBe(initial); + }); + + // Regression for issue #1902 fix #2: mental-model auto-seed used to POST + // `createMentalModel` against a bank the server never saw, because the + // old `ensureBankMission` skipped creation entirely when `bankMission` + // was blank. The bank MUST be PUT (created) before any mental-model POST. + it("creates the bank before mental-model bootstrap even when bankMission is blank", async () => { + const callOrder: string[] = []; + const createBankSpy = vi.spyOn(HindsightApi.prototype, "createBank").mockImplementation(async () => { + callOrder.push("createBank"); + return {} as never; + }); + const listMentalSpy = vi.spyOn(HindsightApi.prototype, "listMentalModels").mockImplementation(async () => { + callOrder.push("listMentalModels"); + return { items: [] } as never; + }); + const createMentalModelSpy = vi + .spyOn(HindsightApi.prototype, "createMentalModel") + .mockImplementation(async () => { + callOrder.push("createMentalModel"); + return {} as never; + }); + + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + "hindsight.mentalModelsEnabled": true, + "hindsight.mentalModelAutoSeed": true, + "hindsight.bankMission": "", + }); + const session = makeFakeSession({ sessionId: "s-bank-first", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + await session.getHindsightSessionState()?.mentalModelsLoadPromise; + + expect(createBankSpy).toHaveBeenCalled(); + // First call must be `createBank`. Otherwise the mental-model POST + // lands against a never-created bank and the server FK-fails it. + expect(callOrder[0]).toBe("createBank"); + // Mental-model POSTs are allowed but they MUST come after the bank + // is on the server. + if (createMentalModelSpy.mock.calls.length > 0) { + const bankIdx = callOrder.indexOf("createBank"); + const mmIdx = callOrder.indexOf("createMentalModel"); + expect(bankIdx).toBeLessThan(mmIdx); + } + void listMentalSpy; + }); +}); + +describe("hindsightBackend retain queue flush on session teardown", () => { + beforeEach(() => { + resetSettingsForTest(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + // Regression for issue #1902 fix #3: `AgentSession.dispose` used to clear + // `#hindsightSessionState` BEFORE flushing the retain queue, so + // `HindsightRetainQueue.#doFlush` saw `session.getHindsightSessionState() + // !== state` and dropped the spliced batch. The fix flips the order: + // flush MUST complete while the session pointer still references the same + // state. We defend the contract end-to-end by enqueuing a tool-initiated + // retain, then calling `flushRetainQueue` in the order + // `AgentSession.dispose` uses (flush → clear → state.dispose). + it("flushes the retain queue to the server before the session pointer clears", async () => { + const retainBatchSpy = vi.spyOn(HindsightApi.prototype, "retainBatch").mockResolvedValue({} as never); + vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); + + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + "hindsight.bankId": "omp", + }); + const session = makeFakeSession({ sessionId: "s-dispose-flush", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + const state = session.getHindsightSessionState(); + expect(state).toBeDefined(); + + state!.enqueueRetain("durable fact", "test context"); + + // AgentSession.dispose order: flush first, THEN clear, THEN dispose. + // Reversed (clear → flush), `#doFlush`'s identity check would fail and + // the batch would be dropped with a `session vanished` warning. + await state!.flushRetainQueue(); + session.setHindsightSessionState(undefined); + state!.dispose(); + + expect(retainBatchSpy).toHaveBeenCalledTimes(1); + const [bankId, items] = retainBatchSpy.mock.calls[0]; + expect(bankId).toBe("omp"); + expect(items).toHaveLength(1); + expect(items[0].content).toBe("durable fact"); + }); + + // Companion contract test: documents the failure mode the dispose-order + // fix prevents. If the session pointer is cleared first, the queue's + // identity guard drops the spliced batch (and `retainBatch` is NOT + // called). This is exactly what was happening before the fix. + it("drops the spliced batch when the session pointer is cleared before flush (anti-regression)", async () => { + const retainBatchSpy = vi.spyOn(HindsightApi.prototype, "retainBatch").mockResolvedValue({} as never); + vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); + + const settings = Settings.isolated({ + "memory.backend": "hindsight", + "hindsight.apiUrl": "http://localhost:8888", + }); + const session = makeFakeSession({ sessionId: "s-buggy-order", settings }); + + await hindsightBackend.start({ + session: session as never, + settings, + modelRegistry: {} as never, + agentDir: "/tmp", + taskDepth: 0, + }); + const state = session.getHindsightSessionState(); + state!.enqueueRetain("dropped fact"); + + // Buggy order — clear THEN flush. The queue's identity check fails. + session.setHindsightSessionState(undefined); + await state!.flushRetainQueue(); + + expect(retainBatchSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/coding-agent/test/hindsight-bank.test.ts b/packages/coding-agent/test/hindsight-bank.test.ts index bbda61602..0ce81b792 100644 --- a/packages/coding-agent/test/hindsight-bank.test.ts +++ b/packages/coding-agent/test/hindsight-bank.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from "bun:test"; -import { computeBankScope, deriveBankId, ensureBankMission } from "@oh-my-pi/pi-coding-agent/hindsight/bank"; +import { computeBankScope, deriveBankId, ensureBankExists } from "@oh-my-pi/pi-coding-agent/hindsight/bank"; import { HindsightApi } from "@oh-my-pi/pi-coding-agent/hindsight/client"; import type { HindsightConfig } from "@oh-my-pi/pi-coding-agent/hindsight/config"; @@ -117,7 +117,7 @@ describe("deriveBankId (legacy wrapper)", () => { }); }); -describe("ensureBankMission", () => { +describe("ensureBankExists", () => { let client: HindsightApi; let createSpy: Mock | undefined; @@ -129,14 +129,14 @@ describe("ensureBankMission", () => { createSpy?.mockRestore(); }); - it("calls createBank exactly once per bank id", async () => { + it("calls createBank exactly once per bank id and forwards the mission body", async () => { createSpy = vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); const seen = new Set(); const config = baseConfig({ bankMission: "remember everything", retainMission: "extract facts" }); - await ensureBankMission(client, "bank-a", config, seen); - await ensureBankMission(client, "bank-a", config, seen); - await ensureBankMission(client, "bank-b", config, seen); + await ensureBankExists(client, "bank-a", config, seen); + await ensureBankExists(client, "bank-a", config, seen); + await ensureBankExists(client, "bank-b", config, seen); expect(createSpy).toHaveBeenCalledTimes(2); expect(createSpy).toHaveBeenCalledWith( @@ -148,13 +148,22 @@ describe("ensureBankMission", () => { expect(seen.has("bank-b")).toBe(true); }); - it("is a no-op when no mission is configured", async () => { + // Regression: mental-model auto-seed used to POST `createMentalModel` against + // a never-created bank when `bankMission` was blank, because the old + // `ensureBankMission` skipped creation entirely without a mission. + it("still PUTs the bank when no mission is configured (so the bank gets created)", async () => { createSpy = vi.spyOn(HindsightApi.prototype, "createBank").mockResolvedValue({} as never); const seen = new Set(); - await ensureBankMission(client, "bank", baseConfig({ bankMission: "" }), seen); - await ensureBankMission(client, "bank", baseConfig({ bankMission: " " }), seen); - expect(createSpy).not.toHaveBeenCalled(); - expect(seen.size).toBe(0); + + await ensureBankExists(client, "bank", baseConfig({ bankMission: "" }), seen); + await ensureBankExists(client, "bank", baseConfig({ bankMission: " " }), seen); + + expect(createSpy).toHaveBeenCalledTimes(1); + expect(createSpy).toHaveBeenCalledWith( + "bank", + expect.objectContaining({ reflectMission: undefined, retainMission: undefined }), + ); + expect(seen.has("bank")).toBe(true); }); it("swallows API failures and does not mark the bank as initialised", async () => { @@ -162,7 +171,7 @@ describe("ensureBankMission", () => { const seen = new Set(); const config = baseConfig({ bankMission: "do the thing" }); - await expect(ensureBankMission(client, "bank-x", config, seen)).resolves.toBeUndefined(); + await expect(ensureBankExists(client, "bank-x", config, seen)).resolves.toBeUndefined(); expect(seen.has("bank-x")).toBe(false); }); }); diff --git a/packages/coding-agent/test/memory-tools.test.ts b/packages/coding-agent/test/memory-tools.test.ts index bd9da3a22..9875bf69d 100644 --- a/packages/coding-agent/test/memory-tools.test.ts +++ b/packages/coding-agent/test/memory-tools.test.ts @@ -100,7 +100,7 @@ function registerState(client: HindsightApi, settings?: Settings, opts: Register getHindsightSessionState: () => registeredState, ...opts.sessionOverrides, } as never, - missionsSet: new Set(), + banksSet: new Set(), lastRetainedTurn: 0, hasRecalledForFirstTurn: false, });