fix(coding-agent/hindsight): reacted to live bank scope changes and flushed before dispose
Mid-session edits to hindsight.bankId / bankIdPrefix / scoping kept the active HindsightSessionState pinned to the bank selected at session start, so retain/recall/reflect calls landed in the stale bank. Settings hooks now fire onHindsightScopeChanged; the backend rebuilds the primary state against the recomputed scope, disposing the previous one after flushing its queue so queued tool-initiated retains still land in the bank they were enqueued for. Also: - Renamed ensureBankMission to ensureBankExists. The old version skipped creation entirely when bankMission was blank, so the first mental-model POST (auto-seed) could land against a never-PUT bank. Bank creation is now idempotent and unconditional, and runs before mental-model bootstrap. - Fixed AgentSession.dispose to flush the retain queue BEFORE clearing the session state pointer. Reversed, HindsightRetainQueue.#doFlush's identity guard would see the cleared pointer and drop the spliced batch with a 'session vanished' warning. - Snapshotted hindsightScopeCallbacks before iterating because each rebuild subscribes a fresh callback inside the same fire; iterating the live Set would spin. Fixes #1902
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -907,6 +907,9 @@ const SETTING_HOOKS: Partial<Record<SettingPath, SettingHook<any>>> = {
|
||||
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
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
@@ -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<string | undefined> {
|
||||
@@ -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<string>,
|
||||
): Promise<HindsightSessionState | undefined> {
|
||||
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<void> {
|
||||
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<HindsightSessionState, "bankId" | "retainTags" | "recallTags" | "recallTagsMatch">,
|
||||
): 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[] {
|
||||
|
||||
@@ -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 `-<project>`; 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<string>,
|
||||
banksSet: Set<string>,
|
||||
): Promise<void> {
|
||||
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) });
|
||||
}
|
||||
}
|
||||
|
||||
@@ -112,7 +112,7 @@ function dedupe<T>(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.
|
||||
*/
|
||||
|
||||
@@ -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<string>;
|
||||
banksSet: Set<string>;
|
||||
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<string>;
|
||||
banksSet: Set<string>;
|
||||
lastRetainedTurn: number;
|
||||
hasRecalledForFirstTurn: boolean;
|
||||
lastRecallSnippet?: string;
|
||||
@@ -213,6 +213,12 @@ export class HindsightSessionState {
|
||||
*/
|
||||
mentalModelsLoadPromise?: Promise<void>;
|
||||
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<void> {
|
||||
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();
|
||||
}
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<typeof memoryReflectSchema>
|
||||
}
|
||||
|
||||
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,
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<HindsightApi["createBank"]> | 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<string>();
|
||||
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<string>();
|
||||
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<string>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user