From 0841c4af712ee42f97958738f9b1316a43299b4b Mon Sep 17 00:00:00 2001 From: raw34 Date: Tue, 28 Apr 2026 10:27:34 +0000 Subject: [PATCH 1/4] fix(auto-recall): Tier 1 memory counter fix (ms-based suppression + lazy-heal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite the Tier 1 auto-recall lifecycle to fix counter bugs that blocked decay-engine, access-tracker reinforcement, and future tier-manager promotion. See Issue #633 / RFC #445. Schema: - Add optional `suppressed_until_ms?: number` field to SmartMemoryMetadata. parseSmartMetadata preserves the `undefined` sentinel for "never touched by Tier 1" vs "touched, no active suppression". Includes guard comment + NaN test to prevent accidental clampCount refactors. Config: - Add `autoRecallBadRecallDecayMs` (default 24h, Option C decay window) and `autoRecallSuppressionDurationMs` (default 30min). Both intentionally omit a `maximum` bound (documented via $comment). Auto-recall inline patch: - Increment access_count and last_accessed_at on every injection — the core fix unblocking downstream lifecycle components. - Lazy-heal legacy pollution: memories with no suppressed_until_ms AND non-zero bad_recall_count / suppressed_until_turn get reset on first Tier 1 touch. - Option C decay: if gap since last_injected_at exceeds badRecallDecayMs, reset bad_recall_count to 0 before stale-injection increment. Negative gaps from clock skew fall through as "no decay" (conservative). - Write ms-based suppressed_until_ms (replacing turn-number suppression); gateway restart no longer creates false "just expired" windows. - Always zero legacy suppressed_until_turn to prevent stale-number leaks. Governance filter: - Replace `suppressed_until_turn` vs `currentTurn` check with absolute-time comparison against `suppressed_until_ms`. Suppression anchor is now wall-clock, not session-local. Out of scope (intentionally unchanged): - staleInjected judgment (Proposal A / PR #597 territory) - Manual memory_recall path - Tier-manager activation Tests: - New test/tier1-counters.test.mjs asserts access_count increments 0->1 via the shared parseSmartMetadata plumbing. - Registered in npm test chain, ci-test-manifest, and verify-ci-test-manifest baseline. Co-Authored-By: Claude Opus 4.7 (1M context) --- index.ts | 71 +++++- openclaw.plugin.json | 14 ++ package.json | 2 +- scripts/ci-test-manifest.mjs | 2 + scripts/verify-ci-test-manifest.mjs | 2 + src/smart-metadata.ts | 22 ++ test/smart-memory-lifecycle.mjs | 43 ++++ test/tier1-counters.test.mjs | 346 ++++++++++++++++++++++++++++ 8 files changed, 495 insertions(+), 7 deletions(-) create mode 100644 test/tier1-counters.test.mjs diff --git a/index.ts b/index.ts index 5a9b5fe9..5765389f 100644 --- a/index.ts +++ b/index.ts @@ -106,6 +106,12 @@ interface PluginConfig { autoRecall?: boolean; autoRecallMinLength?: number; autoRecallMinRepeated?: number; + /** If a memory's last auto-recall injection was more than this many ms ago, + * its bad_recall_count is reset to 0 on the next injection. 0 disables decay. Default: 86400000 (24h). */ + autoRecallBadRecallDecayMs?: number; + /** When bad_recall_count reaches the suppression threshold, the memory is + * suppressed from auto-recall for this many ms from now. Default: 1800000 (30min). */ + autoRecallSuppressionDurationMs?: number; autoRecallTimeoutMs?: number; autoRecallMaxItems?: number; autoRecallMaxChars?: number; @@ -2635,7 +2641,9 @@ const memoryLanceDBProPlugin = { api.logger.debug(`memory-lancedb-pro: governance: filtered id=${r.entry.id} reason=layer(${meta.memory_layer}) score=${r.score?.toFixed(3)} text=${r.entry.text.slice(0, 50)}`); return false; } - if (meta.suppressed_until_turn > 0 && currentTurn <= meta.suppressed_until_turn) { + const nowMs = Date.now(); + const suppressUntil = meta.suppressed_until_ms ?? 0; + if (suppressUntil > 0 && nowMs < suppressUntil) { suppressedFilteredCount++; return false; } @@ -2759,9 +2767,45 @@ const memoryLanceDBProPlugin = { } const injectedAt = Date.now(); + const badRecallDecayMs = + config.autoRecallBadRecallDecayMs ?? 86_400_000; + const suppressionDurationMs = + config.autoRecallSuppressionDurationMs ?? 1_800_000; await Promise.allSettled( selected.map(async (item) => { const meta = item.meta; + + // Tier 1 lazy heal: a memory has never been touched by Tier 1 + // code if `suppressed_until_ms` is undefined. If such a memory + // still carries legacy pollution, reset the counters before new + // logic runs so it starts fresh. + let baseBadRecall = meta.bad_recall_count; + if ( + meta.suppressed_until_ms === undefined && + (meta.bad_recall_count > 0 || meta.suppressed_until_turn > 0) + ) { + baseBadRecall = 0; + } + + // Tier 1 Option C: decay by gap. If the last injection was + // longer than the decay window ago, reset bad_recall_count + // before the stale-injection increment — "this memory is being + // needed again". + const gapSinceLastInjection = + typeof meta.last_injected_at === "number" + ? injectedAt - meta.last_injected_at + : Infinity; + // Negative gap (clock skew, e.g. NTP resync) falls through the + // `gap > badRecallDecayMs` check as "no decay" — conservative: + // never falsely reset due to apparent time travel. + const decayedBadRecall = + badRecallDecayMs > 0 && gapSinceLastInjection > badRecallDecayMs + ? 0 + : baseBadRecall; + + // Existing staleInjected judgment preserved verbatim — Tier 1 + // does NOT change how "was this injection confirmed" is + // determined (PR #597 / Proposal A owns that). const staleInjected = typeof meta.last_injected_at === "number" && meta.last_injected_at > 0 && @@ -2770,18 +2814,33 @@ const memoryLanceDBProPlugin = { meta.last_confirmed_use_at < meta.last_injected_at ); const nextBadRecallCount = staleInjected - ? meta.bad_recall_count + 1 - : meta.bad_recall_count; + ? decayedBadRecall + 1 + : decayedBadRecall; const shouldSuppress = nextBadRecallCount >= 3 && minRepeated > 0; + await store.patchMetadata( item.id, { + // Tier 1 additions + access_count: meta.access_count + 1, + last_accessed_at: injectedAt, + + // Existing fields (semantics unchanged) injected_count: meta.injected_count + 1, last_injected_at: injectedAt, bad_recall_count: nextBadRecallCount, - suppressed_until_turn: shouldSuppress - ? Math.max(meta.suppressed_until_turn, currentTurn + minRepeated) - : meta.suppressed_until_turn, + + // Tier 1 ms-based suppression + suppressed_until_ms: shouldSuppress + ? Math.max( + meta.suppressed_until_ms ?? 0, + injectedAt + suppressionDurationMs, + ) + : (meta.suppressed_until_ms ?? 0), + + // Legacy cleanup — always zero out the turn field on any + // Tier-1-era patch so stale numbers cannot leak through. + suppressed_until_turn: 0, }, accessibleScopes, ); diff --git a/openclaw.plugin.json b/openclaw.plugin.json index 3daf120a..d3c04727 100644 --- a/openclaw.plugin.json +++ b/openclaw.plugin.json @@ -125,6 +125,20 @@ "default": 8, "description": "Minimum number of turns before the same memory can be recalled again in the same session. Set to 0 to disable deduplication." }, + "autoRecallBadRecallDecayMs": { + "type": "integer", + "minimum": 0, + "default": 86400000, + "$comment": "No maximum: 0 disables decay; very large values also effectively disable. Unlike sibling autoRecall* integers, this is a time window with no natural upper bound.", + "description": "If a memory's last auto-recall injection was more than this many ms ago, its bad_recall_count is reset to 0 on the next injection. 0 disables decay. Default: 86400000 (24 hours)." + }, + "autoRecallSuppressionDurationMs": { + "type": "integer", + "minimum": 0, + "default": 1800000, + "$comment": "No maximum: 0 effectively disables suppression; large values allow long quarantine windows.", + "description": "When bad_recall_count reaches the suppression threshold, the memory is suppressed from auto-recall for this many ms from now. Default: 1800000 (30 minutes)." + }, "autoRecallTimeoutMs": { "type": "integer", "minimum": 500, diff --git a/package.json b/package.json index fbcb9d98..5fb2ea7f 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "author": "win4r", "license": "MIT", "scripts": { - "test": "node test/embedder-error-hints.test.mjs && node test/cjk-recursion-regression.test.mjs && node test/migrate-legacy-schema.test.mjs && node --test test/config-session-strategy-migration.test.mjs && node --test test/scope-access-undefined.test.mjs && node --test test/reflection-bypass-hook.test.mjs && node --test test/smart-extractor-scope-filter.test.mjs && node --test test/store-empty-scope-filter.test.mjs && node --test test/recall-text-cleanup.test.mjs && node test/update-consistency-lancedb.test.mjs && node --test test/strip-envelope-metadata.test.mjs && node test/cli-smoke.mjs && node test/functional-e2e.mjs && node --test test/per-agent-auto-recall.test.mjs && node test/retriever-rerank-regression.mjs && node test/smart-memory-lifecycle.mjs && node test/smart-extractor-branches.mjs && node test/plugin-manifest-regression.mjs && node --test test/session-summary-before-reset.test.mjs && node --test test/sync-plugin-version.test.mjs && node test/smart-metadata-v2.mjs && node test/vector-search-cosine.test.mjs && node test/context-support-e2e.mjs && node test/temporal-facts.test.mjs && node test/memory-update-supersede.test.mjs && node test/memory-upgrader-diagnostics.test.mjs && node --test test/llm-api-key-client.test.mjs && node --test test/llm-oauth-client.test.mjs && node --test test/cli-oauth-login.test.mjs && node --test test/workflow-fork-guards.test.mjs && node --test test/clawteam-scope.test.mjs && node --test test/cross-process-lock.test.mjs && node --test test/preference-slots.test.mjs && node test/is-latest-auto-supersede.test.mjs && node --test test/temporal-awareness.test.mjs && node --test test/command-reflection-guard.test.mjs", + "test": "node test/embedder-error-hints.test.mjs && node test/cjk-recursion-regression.test.mjs && node test/migrate-legacy-schema.test.mjs && node --test test/config-session-strategy-migration.test.mjs && node --test test/scope-access-undefined.test.mjs && node --test test/reflection-bypass-hook.test.mjs && node --test test/smart-extractor-scope-filter.test.mjs && node --test test/store-empty-scope-filter.test.mjs && node --test test/recall-text-cleanup.test.mjs && node test/update-consistency-lancedb.test.mjs && node --test test/strip-envelope-metadata.test.mjs && node test/cli-smoke.mjs && node test/functional-e2e.mjs && node --test test/per-agent-auto-recall.test.mjs && node test/retriever-rerank-regression.mjs && node test/smart-memory-lifecycle.mjs && node test/smart-extractor-branches.mjs && node test/plugin-manifest-regression.mjs && node --test test/session-summary-before-reset.test.mjs && node --test test/sync-plugin-version.test.mjs && node test/smart-metadata-v2.mjs && node test/vector-search-cosine.test.mjs && node test/context-support-e2e.mjs && node test/temporal-facts.test.mjs && node test/memory-update-supersede.test.mjs && node test/memory-upgrader-diagnostics.test.mjs && node --test test/llm-api-key-client.test.mjs && node --test test/llm-oauth-client.test.mjs && node --test test/cli-oauth-login.test.mjs && node --test test/workflow-fork-guards.test.mjs && node --test test/clawteam-scope.test.mjs && node --test test/cross-process-lock.test.mjs && node --test test/preference-slots.test.mjs && node test/is-latest-auto-supersede.test.mjs && node --test test/temporal-awareness.test.mjs && node --test test/command-reflection-guard.test.mjs && node --test test/tier1-counters.test.mjs", "test:cli-smoke": "node scripts/run-ci-tests.mjs --group cli-smoke", "test:core-regression": "node scripts/run-ci-tests.mjs --group core-regression", "test:storage-and-schema": "node scripts/run-ci-tests.mjs --group storage-and-schema", diff --git a/scripts/ci-test-manifest.mjs b/scripts/ci-test-manifest.mjs index fc6435dc..0ac6fcd6 100644 --- a/scripts/ci-test-manifest.mjs +++ b/scripts/ci-test-manifest.mjs @@ -62,6 +62,8 @@ export const CI_TEST_MANIFEST = [ // Issue #492 agentId validation tests { group: "core-regression", runner: "node", file: "test/agentid-validation.test.mjs", args: ["--test"] }, { group: "core-regression", runner: "node", file: "test/command-reflection-guard.test.mjs", args: ["--test"] }, + // Tier 1 memory counter fix + { group: "core-regression", runner: "node", file: "test/tier1-counters.test.mjs", args: ["--test"] }, ]; export function getEntriesForGroup(group) { diff --git a/scripts/verify-ci-test-manifest.mjs b/scripts/verify-ci-test-manifest.mjs index fee475c3..0d2df2d5 100644 --- a/scripts/verify-ci-test-manifest.mjs +++ b/scripts/verify-ci-test-manifest.mjs @@ -63,6 +63,8 @@ const EXPECTED_BASELINE = [ // Issue #492 agentId validation tests { group: "core-regression", runner: "node", file: "test/agentid-validation.test.mjs", args: ["--test"] }, { group: "core-regression", runner: "node", file: "test/command-reflection-guard.test.mjs", args: ["--test"] }, + // Tier 1 memory counter fix + { group: "core-regression", runner: "node", file: "test/tier1-counters.test.mjs", args: ["--test"] }, ]; function fail(message) { diff --git a/src/smart-metadata.ts b/src/smart-metadata.ts index da7e79cd..add3b50b 100644 --- a/src/smart-metadata.ts +++ b/src/smart-metadata.ts @@ -61,6 +61,13 @@ export interface SmartMemoryMetadata { last_confirmed_use_at?: number; bad_recall_count: number; suppressed_until_turn: number; + /** + * Unix ms timestamp until which auto-recall should suppress this memory. + * OPTIONAL: `undefined` means this memory has never been written by Tier 1 + * code (sentinel used for lazy heal of legacy data). `0` means Tier 1 has + * touched the memory but there is no active suppression. + */ + suppressed_until_ms?: number; canonical_id?: string; [key: string]: unknown; } @@ -330,6 +337,15 @@ export function parseSmartMetadata( last_confirmed_use_at: normalizeOptionalTimestamp(parsed.last_confirmed_use_at), bad_recall_count: clampCount(parsed.bad_recall_count, 0), suppressed_until_turn: clampCount(parsed.suppressed_until_turn, 0), + // DO NOT replace with `clampCount(parsed.suppressed_until_ms, 0)` directly — + // preserving `undefined` is load-bearing for the Tier 1 lazy-heal sentinel + // (see JSDoc on SmartMemoryMetadata.suppressed_until_ms). The `undefined` + // signal distinguishes "never touched by Tier 1 code" from "Tier 1 touched + // but no active suppression (0)". + suppressed_until_ms: + parsed.suppressed_until_ms !== undefined + ? clampCount(parsed.suppressed_until_ms, 0) + : undefined, canonical_id: normalizeOptionalString(parsed.canonical_id), }; @@ -419,6 +435,12 @@ export function buildSmartMetadata( patch.suppressed_until_turn, base.suppressed_until_turn, ), + suppressed_until_ms: + patch.suppressed_until_ms === undefined + ? base.suppressed_until_ms + : (typeof patch.suppressed_until_ms === "number" && patch.suppressed_until_ms >= 0 + ? Math.floor(patch.suppressed_until_ms) + : 0), canonical_id: patch.canonical_id === undefined ? base.canonical_id diff --git a/test/smart-memory-lifecycle.mjs b/test/smart-memory-lifecycle.mjs index 3f4793fb..5aeb4884 100644 --- a/test/smart-memory-lifecycle.mjs +++ b/test/smart-memory-lifecycle.mjs @@ -217,3 +217,46 @@ assert.equal( ); console.log("OK: smart memory lifecycle test passed"); + +// ============================================================================ +// Tier 1 integration assertion +// ============================================================================ +// Verify the auto-recall injection path's computed patch correctly increments +// access_count. This test uses the same pure helper as test/tier1-counters.test.mjs +// but runs it in the lifecycle context to guard against regression in the shared +// parse/build plumbing. If parseSmartMetadata or the injection patch ever breaks +// the access_count contract, this integration assertion fails alongside the +// unit-level tests in tier1-counters.test.mjs. +{ + function computeTier1Patch(meta, injectedAt) { + let baseBadRecall = meta.bad_recall_count ?? 0; + if (meta.suppressed_until_ms === undefined && + ((meta.bad_recall_count ?? 0) > 0 || (meta.suppressed_until_turn ?? 0) > 0)) { + baseBadRecall = 0; + } + const gap = typeof meta.last_injected_at === "number" + ? injectedAt - meta.last_injected_at : Infinity; + const decayed = (gap > 86_400_000) ? 0 : baseBadRecall; + const stale = typeof meta.last_injected_at === "number" && meta.last_injected_at > 0 + && (typeof meta.last_confirmed_use_at !== "number" + || meta.last_confirmed_use_at < meta.last_injected_at); + const nextBadRecall = stale ? decayed + 1 : decayed; + return { + access_count: (meta.access_count ?? 0) + 1, + last_accessed_at: injectedAt, + injected_count: (meta.injected_count ?? 0) + 1, + last_injected_at: injectedAt, + bad_recall_count: nextBadRecall, + suppressed_until_ms: (nextBadRecall >= 3) ? injectedAt + 1_800_000 : (meta.suppressed_until_ms ?? 0), + suppressed_until_turn: 0, + }; + } + + const freshMeta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "lifecycle-tier1", access_count: 0 }), + { text: "lifecycle-tier1", category: "fact" }, + ); + const patched = computeTier1Patch(freshMeta, Date.now()); + assert.equal(patched.access_count, 1, "Tier 1: access_count must increment from 0 to 1 on auto-recall injection"); + console.log("✓ Tier 1 access_count integration assertion passed"); +} diff --git a/test/tier1-counters.test.mjs b/test/tier1-counters.test.mjs new file mode 100644 index 00000000..ba8600ff --- /dev/null +++ b/test/tier1-counters.test.mjs @@ -0,0 +1,346 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import path from "node:path"; +import fs from "node:fs"; +import jitiFactory from "jiti"; + +const jiti = jitiFactory(import.meta.url, { interopDefault: true }); +const { + parseSmartMetadata, + buildSmartMetadata, +} = jiti("../src/smart-metadata.ts"); + +describe("Tier 1: suppressed_until_ms field presence semantics", () => { + it("returns undefined when raw JSON does not include the key (sentinel for 'never touched by Tier 1')", () => { + const meta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "legacy", bad_recall_count: 5 }), + { text: "legacy", category: "fact" }, + ); + assert.equal(meta.suppressed_until_ms, undefined); + }); + + it("clamps to non-negative integer when raw JSON includes a numeric value", () => { + const meta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "tier1-touched", suppressed_until_ms: 1713700000000 }), + { text: "tier1-touched", category: "fact" }, + ); + assert.equal(meta.suppressed_until_ms, 1713700000000); + }); + + it("coerces negative or NaN to 0", () => { + const negMeta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "x", suppressed_until_ms: -100 }), + { text: "x", category: "fact" }, + ); + assert.equal(negMeta.suppressed_until_ms, 0); + + const nanMeta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "x", suppressed_until_ms: "not-a-number" }), + { text: "x", category: "fact" }, + ); + assert.equal(nanMeta.suppressed_until_ms, 0); + }); + + it("preserves 0 explicitly (not coerced to undefined)", () => { + const meta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "explicit-zero", suppressed_until_ms: 0 }), + { text: "explicit-zero", category: "fact" }, + ); + assert.equal(meta.suppressed_until_ms, 0); + }); +}); + +describe("Tier 1: plugin config schema", () => { + it("openclaw.plugin.json declares autoRecallBadRecallDecayMs and autoRecallSuppressionDurationMs", () => { + const pluginJsonPath = path.resolve( + path.dirname(new URL(import.meta.url).pathname), + "../openclaw.plugin.json", + ); + const raw = fs.readFileSync(pluginJsonPath, "utf8"); + const schema = JSON.parse(raw); + // Tolerate either top-level "properties" or nested config object — search recursively. + function findProperty(obj, key) { + if (!obj || typeof obj !== "object") return null; + if (obj.properties && Object.prototype.hasOwnProperty.call(obj.properties, key)) { + return obj.properties[key]; + } + for (const v of Object.values(obj)) { + const found = findProperty(v, key); + if (found) return found; + } + return null; + } + const decay = findProperty(schema, "autoRecallBadRecallDecayMs"); + const suppress = findProperty(schema, "autoRecallSuppressionDurationMs"); + assert.ok(decay, "autoRecallBadRecallDecayMs missing from schema"); + assert.ok(suppress, "autoRecallSuppressionDurationMs missing from schema"); + assert.equal(decay.default, 86400000); + assert.equal(suppress.default, 1800000); + assert.equal(decay.minimum, 0); + assert.equal(suppress.minimum, 0); + }); +}); + +describe("Tier 1: governance filter reads suppressed_until_ms", () => { + // Pure-logic test of the filter predicate. We define a local helper that + // mirrors the production code, then test it directly. This avoids booting + // the plugin runtime for a 2-line condition. End-to-end wiring is verified + // by visual inspection of index.ts after the rewrite. + function isSuppressed(meta, nowMs) { + const suppressUntil = meta.suppressed_until_ms ?? 0; + return suppressUntil > 0 && nowMs < suppressUntil; + } + + it("suppresses when nowMs < suppressed_until_ms", () => { + const future = Date.now() + 60_000; + assert.equal(isSuppressed({ suppressed_until_ms: future }, Date.now()), true); + }); + + it("does not suppress when nowMs >= suppressed_until_ms", () => { + const past = Date.now() - 60_000; + assert.equal(isSuppressed({ suppressed_until_ms: past }, Date.now()), false); + }); + + it("does not suppress when suppressed_until_ms is undefined (legacy memory)", () => { + assert.equal(isSuppressed({ suppressed_until_ms: undefined }, Date.now()), false); + }); + + it("does not suppress when suppressed_until_ms is 0 (Tier 1 touched, no active suppression)", () => { + assert.equal(isSuppressed({ suppressed_until_ms: 0 }, Date.now()), false); + }); + + it("ignores legacy suppressed_until_turn field entirely", () => { + // A memory with only legacy turn-based suppression: Tier 1 filter does not + // look at it. The legacy turn field is retired in the Tier 1 read path. + const meta = { suppressed_until_turn: 9999, suppressed_until_ms: undefined }; + assert.equal(isSuppressed(meta, Date.now()), false); + }); +}); + +describe("Tier 1: bad_recall_count decay and patch shape (Option C)", () => { + // Pure-logic helper that mirrors the production computation. + function computeTier1Patch(meta, opts) { + const { + injectedAt, + badRecallDecayMs = 86_400_000, + suppressionDurationMs = 1_800_000, + minRepeated = 8, + } = opts; + + // Lazy heal + let baseBadRecall = meta.bad_recall_count ?? 0; + if (meta.suppressed_until_ms === undefined && + ((meta.bad_recall_count ?? 0) > 0 || (meta.suppressed_until_turn ?? 0) > 0)) { + baseBadRecall = 0; + } + + // Option C: decay by gap + const gapSinceLastInjection = typeof meta.last_injected_at === "number" + ? injectedAt - meta.last_injected_at + : Infinity; + const decayedBadRecall = (badRecallDecayMs > 0 && gapSinceLastInjection > badRecallDecayMs) + ? 0 + : baseBadRecall; + + // staleInjected (existing judgment preserved verbatim) + const staleInjected = + typeof meta.last_injected_at === "number" && + meta.last_injected_at > 0 && + ( + typeof meta.last_confirmed_use_at !== "number" || + meta.last_confirmed_use_at < meta.last_injected_at + ); + const nextBadRecallCount = staleInjected + ? decayedBadRecall + 1 + : decayedBadRecall; + + const shouldSuppress = nextBadRecallCount >= 3 && minRepeated > 0; + + return { + access_count: (meta.access_count ?? 0) + 1, + last_accessed_at: injectedAt, + injected_count: (meta.injected_count ?? 0) + 1, + last_injected_at: injectedAt, + bad_recall_count: nextBadRecallCount, + suppressed_until_ms: shouldSuppress + ? Math.max(meta.suppressed_until_ms ?? 0, injectedAt + suppressionDurationMs) + : (meta.suppressed_until_ms ?? 0), + suppressed_until_turn: 0, + }; + } + + it("T1 access_count: accumulates 0 → 1 on first injection", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { access_count: 0, bad_recall_count: 0, injected_count: 0 }, + { injectedAt: now }, + ); + assert.equal(patch.access_count, 1); + assert.equal(patch.last_accessed_at, now); + }); + + it("T1 access_count: 1 → 2 on repeated injection", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { access_count: 1, bad_recall_count: 0, injected_count: 1, last_injected_at: now - 60_000 }, + { injectedAt: now }, + ); + assert.equal(patch.access_count, 2); + }); + + it("T2 decay: gap > decay window resets bad_recall before staleInjected increment", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 5, + bad_recall_count: 2, + injected_count: 3, + last_injected_at: now - 25 * 3600 * 1000, // 25h ago + suppressed_until_ms: 0, + }, + { injectedAt: now, badRecallDecayMs: 86_400_000 }, + ); + // gap=25h > 24h → decayedBadRecall=0, staleInjected=true → next=1 + assert.equal(patch.bad_recall_count, 1); + }); + + it("T2 no-decay: gap < decay window keeps bad_recall accumulating", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 5, + bad_recall_count: 2, + injected_count: 3, + last_injected_at: now - 3600 * 1000, // 1h ago + suppressed_until_ms: 0, + }, + { injectedAt: now, badRecallDecayMs: 86_400_000 }, + ); + // gap=1h < 24h → no decay, staleInjected=true → next=3 + assert.equal(patch.bad_recall_count, 3); + }); + + it("T2 first-ever injection: gap=Infinity, staleInjected=false", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { access_count: 0, bad_recall_count: 0, injected_count: 0 }, + { injectedAt: now }, + ); + // last_injected_at undefined → staleInjected=false → next=0 + assert.equal(patch.bad_recall_count, 0); + }); + + it("T2 badRecallDecayMs=0: decay disabled", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 5, + bad_recall_count: 2, + injected_count: 3, + last_injected_at: now - 100 * 24 * 3600 * 1000, // 100 days ago + suppressed_until_ms: 0, // Tier-1 touched already; no lazy heal + }, + { injectedAt: now, badRecallDecayMs: 0 }, + ); + // decay disabled → baseBadRecall=2, staleInjected=true → next=3 + assert.equal(patch.bad_recall_count, 3); + }); + + it("T3 shouldSuppress=true writes suppressed_until_ms ≈ injectedAt + duration", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 5, + bad_recall_count: 2, + injected_count: 3, + last_injected_at: now - 3600 * 1000, + suppressed_until_ms: 0, // Tier-1 touched already; no lazy heal + }, + { injectedAt: now, suppressionDurationMs: 1_800_000, minRepeated: 8 }, + ); + // next=3, suppress → until = now + 30min + assert.equal(patch.bad_recall_count, 3); + assert.equal(patch.suppressed_until_ms, now + 1_800_000); + assert.equal(patch.suppressed_until_turn, 0); + }); + + it("T3 shouldSuppress extends existing suppression (Math.max)", () => { + const now = Date.now(); + const farFuture = now + 7_200_000; // 2h from now + const patch = computeTier1Patch( + { + access_count: 5, + bad_recall_count: 2, + injected_count: 3, + last_injected_at: now - 3600 * 1000, + suppressed_until_ms: farFuture, + }, + { injectedAt: now, suppressionDurationMs: 1_800_000, minRepeated: 8 }, + ); + // next=3, suppress: Math.max(farFuture, now + 30min) = farFuture + assert.equal(patch.suppressed_until_ms, farFuture); + }); + + it("T3 always zeroes suppressed_until_turn even when not suppressing", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 1, + bad_recall_count: 0, + injected_count: 1, + last_injected_at: now - 60_000, + suppressed_until_turn: 999, + }, + { injectedAt: now }, + ); + assert.equal(patch.suppressed_until_turn, 0); + }); + + it("T4 lazy heal: memory with legacy bad_recall_count > 0 and no suppressed_until_ms", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 0, + bad_recall_count: 5, + injected_count: 0, + // no last_injected_at, no suppressed_until_ms → legacy-shaped record + }, + { injectedAt: now }, + ); + // Lazy heal: baseBadRecall=0 (was 5). No last_injected → staleInjected=false → next=0 + assert.equal(patch.bad_recall_count, 0); + }); + + it("T4 lazy heal: memory with legacy suppressed_until_turn > 0", () => { + const now = Date.now(); + const patch = computeTier1Patch( + { + access_count: 0, + bad_recall_count: 0, + injected_count: 1, + suppressed_until_turn: 9999, + // suppressed_until_ms missing + last_injected_at: now - 60_000, + }, + { injectedAt: now }, + ); + // Lazy heal triggers because suppressed_until_turn > 0. staleInjected=true → next=1 + assert.equal(patch.bad_recall_count, 1); + assert.equal(patch.suppressed_until_turn, 0); + }); + + it("T4 heal fires once: after first Tier 1 touch, future patches do not re-trigger heal", () => { + const now = Date.now(); + const tierOneTouched = { + access_count: 1, + bad_recall_count: 2, + injected_count: 1, + last_injected_at: now - 3600 * 1000, + suppressed_until_ms: 0, // present, means Tier 1 touched it before + }; + const patch = computeTier1Patch(tierOneTouched, { injectedAt: now }); + // No heal (suppressed_until_ms !== undefined); Option C: gap=1h < 24h → no decay + // staleInjected=true → next=3 (not reset) + assert.equal(patch.bad_recall_count, 3); + }); +}); From 97cffbe0ecbde58a24040d286ed780ad56e3b039 Mon Sep 17 00:00:00 2001 From: raw34 Date: Tue, 5 May 2026 14:14:53 +0800 Subject: [PATCH 2/4] fix(auto-recall): address PR #712 review (F2 + F3 + F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per rwmjhb's CHANGES_REQUESTED review (2026-05-05): F2: manual `memory_recall` now clears `suppressed_until_ms` alongside `suppressed_until_turn` in src/tools.ts:594-614. Without this, after the governance check moved to ms-based suppression, a memory the user just explicitly searched for would remain suppressed — a regression vs pre-Tier-1 semantics where zeroing the turn field cleared the only suppression mechanism. Same fix applied to `memory_promote` at line ~1939, which carries identical positive-signal semantics. F3: `memory_explain_rank` debug output now reports `suppressedUntilMs` instead of the retired `suppressedUntilTurn` (which is always 0 after Tier 1 touches a memory). Cosmetic but eliminates a misleading metric. F4: extracted Tier 1 patch logic to `src/auto-recall-tier1.ts` as pure functions (`isSuppressed`, `computeTier1Patch`, plus default constants). - index.ts auto-recall path now imports and calls these helpers, collapsing ~85 lines of inline logic into a single computeTier1Patch call site. - test/tier1-counters.test.mjs and test/smart-memory-lifecycle.mjs now import the same helpers via jiti, replacing the local mirrored copies the reviewer flagged. Tests now drive the production code path directly, so any future drift surfaces immediately. EF1 (test suite failure) is the pre-existing `smart-extractor-branches.mjs` flake on master, verified against upstream/master CI runs. Out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) --- index.ts | 98 +++++------------------ src/auto-recall-tier1.ts | 137 ++++++++++++++++++++++++++++++++ src/tools.ts | 12 ++- test/smart-memory-lifecycle.mjs | 35 ++------ test/tier1-counters.test.mjs | 68 +++------------- 5 files changed, 183 insertions(+), 167 deletions(-) create mode 100644 src/auto-recall-tier1.ts diff --git a/index.ts b/index.ts index 5765389f..c9cb7d1c 100644 --- a/index.ts +++ b/index.ts @@ -70,6 +70,12 @@ import { stringifySmartMetadata, toLifecycleMemory, } from "./src/smart-metadata.js"; +import { + computeTier1Patch, + isSuppressed as isTier1Suppressed, + TIER1_DEFAULT_BAD_RECALL_DECAY_MS, + TIER1_DEFAULT_SUPPRESSION_DURATION_MS, +} from "./src/auto-recall-tier1.js"; import { filterUserMdExclusiveRecallResults, isUserMdExclusiveMemory, @@ -2641,9 +2647,7 @@ const memoryLanceDBProPlugin = { api.logger.debug(`memory-lancedb-pro: governance: filtered id=${r.entry.id} reason=layer(${meta.memory_layer}) score=${r.score?.toFixed(3)} text=${r.entry.text.slice(0, 50)}`); return false; } - const nowMs = Date.now(); - const suppressUntil = meta.suppressed_until_ms ?? 0; - if (suppressUntil > 0 && nowMs < suppressUntil) { + if (isTier1Suppressed(meta, Date.now())) { suppressedFilteredCount++; return false; } @@ -2767,84 +2771,22 @@ const memoryLanceDBProPlugin = { } const injectedAt = Date.now(); - const badRecallDecayMs = - config.autoRecallBadRecallDecayMs ?? 86_400_000; - const suppressionDurationMs = - config.autoRecallSuppressionDurationMs ?? 1_800_000; + const tier1PatchOpts = { + injectedAt, + badRecallDecayMs: + config.autoRecallBadRecallDecayMs ?? TIER1_DEFAULT_BAD_RECALL_DECAY_MS, + suppressionDurationMs: + config.autoRecallSuppressionDurationMs ?? TIER1_DEFAULT_SUPPRESSION_DURATION_MS, + minRepeated, + }; await Promise.allSettled( - selected.map(async (item) => { - const meta = item.meta; - - // Tier 1 lazy heal: a memory has never been touched by Tier 1 - // code if `suppressed_until_ms` is undefined. If such a memory - // still carries legacy pollution, reset the counters before new - // logic runs so it starts fresh. - let baseBadRecall = meta.bad_recall_count; - if ( - meta.suppressed_until_ms === undefined && - (meta.bad_recall_count > 0 || meta.suppressed_until_turn > 0) - ) { - baseBadRecall = 0; - } - - // Tier 1 Option C: decay by gap. If the last injection was - // longer than the decay window ago, reset bad_recall_count - // before the stale-injection increment — "this memory is being - // needed again". - const gapSinceLastInjection = - typeof meta.last_injected_at === "number" - ? injectedAt - meta.last_injected_at - : Infinity; - // Negative gap (clock skew, e.g. NTP resync) falls through the - // `gap > badRecallDecayMs` check as "no decay" — conservative: - // never falsely reset due to apparent time travel. - const decayedBadRecall = - badRecallDecayMs > 0 && gapSinceLastInjection > badRecallDecayMs - ? 0 - : baseBadRecall; - - // Existing staleInjected judgment preserved verbatim — Tier 1 - // does NOT change how "was this injection confirmed" is - // determined (PR #597 / Proposal A owns that). - const staleInjected = - typeof meta.last_injected_at === "number" && - meta.last_injected_at > 0 && - ( - typeof meta.last_confirmed_use_at !== "number" || - meta.last_confirmed_use_at < meta.last_injected_at - ); - const nextBadRecallCount = staleInjected - ? decayedBadRecall + 1 - : decayedBadRecall; - const shouldSuppress = nextBadRecallCount >= 3 && minRepeated > 0; - - await store.patchMetadata( + selected.map(async (item) => + store.patchMetadata( item.id, - { - // Tier 1 additions - access_count: meta.access_count + 1, - last_accessed_at: injectedAt, - - // Existing fields (semantics unchanged) - injected_count: meta.injected_count + 1, - last_injected_at: injectedAt, - bad_recall_count: nextBadRecallCount, - - // Tier 1 ms-based suppression - suppressed_until_ms: shouldSuppress - ? Math.max( - meta.suppressed_until_ms ?? 0, - injectedAt + suppressionDurationMs, - ) - : (meta.suppressed_until_ms ?? 0), - - // Legacy cleanup — always zero out the turn field on any - // Tier-1-era patch so stale numbers cannot leak through. - suppressed_until_turn: 0, - }, + computeTier1Patch(item.meta, tier1PatchOpts), accessibleScopes, - ); - }), + ), + ), ); const memoryContext = selected.map((item) => item.line).join("\n"); diff --git a/src/auto-recall-tier1.ts b/src/auto-recall-tier1.ts new file mode 100644 index 00000000..2a84b27f --- /dev/null +++ b/src/auto-recall-tier1.ts @@ -0,0 +1,137 @@ +import type { SmartMemoryMetadata } from "./smart-metadata.ts"; + +// Suppression fires when bad_recall_count reaches this value (and the recall +// dedup window is active). Hardcoded by Tier 1 design; if this becomes +// configurable, expose via opts on computeTier1Patch. +export const TIER1_BAD_RECALL_SUPPRESSION_THRESHOLD = 3; + +// Default values for the two plugin config fields. Kept here so the +// production code path and tests share a single source of truth. +export const TIER1_DEFAULT_BAD_RECALL_DECAY_MS = 86_400_000; // 24h +export const TIER1_DEFAULT_SUPPRESSION_DURATION_MS = 1_800_000; // 30min + +// Subset of SmartMemoryMetadata that Tier 1 actually reads. Using a structural +// type lets unit tests pass partial objects without losing type safety in +// production (where the full SmartMemoryMetadata is supplied). +export interface Tier1MetaInput { + access_count?: number; + injected_count?: number; + bad_recall_count?: number; + last_injected_at?: number; + last_confirmed_use_at?: number; + suppressed_until_turn?: number; + // Presence semantics: `undefined` = never touched by Tier 1 (lazy-heal + // sentinel); `0` = touched, no active suppression; `> 0` = suppressed. + suppressed_until_ms?: number; +} + +export interface ComputeTier1PatchOpts { + injectedAt: number; + badRecallDecayMs?: number; + suppressionDurationMs?: number; + // Recall-dedup window. When 0, suppression cannot fire even if the threshold + // is reached — there is no per-session repeat-injection mechanism in play. + minRepeated?: number; +} + +// Patch shape produced by Tier 1 for an auto-recall injection. The keys are +// a subset of SmartMemoryMetadata so the result can be passed directly to +// store.patchMetadata(). +export interface Tier1Patch { + access_count: number; + last_accessed_at: number; + injected_count: number; + last_injected_at: number; + bad_recall_count: number; + suppressed_until_ms: number; + suppressed_until_turn: 0; +} + +// Tier 1 governance predicate: is this memory currently suppressed from +// auto-recall? Reads only the ms-based field; the legacy turn field is +// retired in the read path. +export function isSuppressed(meta: Tier1MetaInput, nowMs: number): boolean { + const until = meta.suppressed_until_ms ?? 0; + return until > 0 && nowMs < until; +} + +// Tier 1 staleInjected judgment — whether the previous injection of this +// memory ever got confirmed by user behavior. Preserved verbatim from the +// pre-Tier-1 path: PR #597 / Proposal A owns any future change to this rule. +function isStaleInjection(meta: Tier1MetaInput): boolean { + return ( + typeof meta.last_injected_at === "number" && + meta.last_injected_at > 0 && + (typeof meta.last_confirmed_use_at !== "number" || + meta.last_confirmed_use_at < meta.last_injected_at) + ); +} + +// Compute the metadata patch to apply to a memory after Tier 1 auto-recall +// injects it. Pure function — caller persists the patch. +export function computeTier1Patch( + meta: Tier1MetaInput, + opts: ComputeTier1PatchOpts, +): Tier1Patch { + const { + injectedAt, + badRecallDecayMs = TIER1_DEFAULT_BAD_RECALL_DECAY_MS, + suppressionDurationMs = TIER1_DEFAULT_SUPPRESSION_DURATION_MS, + minRepeated = 0, + } = opts; + + const accessCount = meta.access_count ?? 0; + const injectedCount = meta.injected_count ?? 0; + const rawBadRecall = meta.bad_recall_count ?? 0; + const turnLegacy = meta.suppressed_until_turn ?? 0; + + // Lazy heal: a memory has never been touched by Tier 1 if + // `suppressed_until_ms` is undefined. If it still carries legacy pollution, + // reset before any new logic runs. + let baseBadRecall = rawBadRecall; + if ( + meta.suppressed_until_ms === undefined && + (rawBadRecall > 0 || turnLegacy > 0) + ) { + baseBadRecall = 0; + } + + // Option C decay: if the gap since the last injection exceeds the decay + // window, reset bad_recall_count — "this memory is being needed again". + // Negative gap (clock skew, e.g. NTP resync) falls through as "no decay": + // never falsely reset due to apparent time travel. + const gapSinceLastInjection = + typeof meta.last_injected_at === "number" + ? injectedAt - meta.last_injected_at + : Infinity; + const decayedBadRecall = + badRecallDecayMs > 0 && gapSinceLastInjection > badRecallDecayMs + ? 0 + : baseBadRecall; + + const staleInjected = isStaleInjection(meta); + const nextBadRecallCount = staleInjected + ? decayedBadRecall + 1 + : decayedBadRecall; + const shouldSuppress = + nextBadRecallCount >= TIER1_BAD_RECALL_SUPPRESSION_THRESHOLD && + minRepeated > 0; + + return { + access_count: accessCount + 1, + last_accessed_at: injectedAt, + injected_count: injectedCount + 1, + last_injected_at: injectedAt, + bad_recall_count: nextBadRecallCount, + suppressed_until_ms: shouldSuppress + ? Math.max(meta.suppressed_until_ms ?? 0, injectedAt + suppressionDurationMs) + : (meta.suppressed_until_ms ?? 0), + // Always zero the legacy turn field on any Tier-1-era patch so stale + // numbers cannot leak through. + suppressed_until_turn: 0, + }; +} + +// Re-export the SmartMemoryMetadata type alias used here so callers don't +// need a second import for the patch input. +export type { SmartMemoryMetadata }; diff --git a/src/tools.ts b/src/tools.ts index 648eee6d..20b570af 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -602,6 +602,12 @@ export function registerMemoryRecallTool( last_confirmed_use_at: now, bad_recall_count: 0, suppressed_until_turn: 0, + // Manual recall is a strong positive signal — clear active + // ms-based suppression too, matching pre-Tier1 semantics + // where zeroing the turn field cleared the only suppression + // mechanism. Without this, governance keeps suppressing a + // memory the user just explicitly searched for. + suppressed_until_ms: 0, }, scopeFilter, ); @@ -1931,6 +1937,10 @@ export function registerMemoryPromoteTool( last_confirmed_use_at: state === "confirmed" ? now : undefined, bad_recall_count: 0, suppressed_until_turn: 0, + // Promotion is a manual confirmation — clear active ms-based + // suppression alongside the legacy turn field (parallel to + // memory_recall above). + suppressed_until_ms: 0, }, scopeFilter, ); @@ -2198,7 +2208,7 @@ export function registerMemoryExplainRankTool( return [ `${idx + 1}. [${r.entry.id}] score=${r.score.toFixed(3)} ${sourceBreakdown.join(" ")}`.trim(), ` state=${meta.state} layer=${meta.memory_layer} source=${meta.source} tier=${meta.tier}`, - ` access=${meta.access_count} injected=${meta.injected_count} badRecall=${meta.bad_recall_count} suppressedUntilTurn=${meta.suppressed_until_turn}`, + ` access=${meta.access_count} injected=${meta.injected_count} badRecall=${meta.bad_recall_count} suppressedUntilMs=${meta.suppressed_until_ms ?? "—"}`, ` text=${truncateText(normalizeInlineText(meta.l0_abstract || r.entry.text), 180)}`, ].join("\n"); }); diff --git a/test/smart-memory-lifecycle.mjs b/test/smart-memory-lifecycle.mjs index 5aeb4884..9dc1607f 100644 --- a/test/smart-memory-lifecycle.mjs +++ b/test/smart-memory-lifecycle.mjs @@ -8,6 +8,7 @@ const { buildSmartMetadata, parseSmartMetadata, toLifecycleMemory } = jiti("../s const { createDecayEngine, DEFAULT_DECAY_CONFIG } = jiti("../src/decay-engine.ts"); const { createTierManager, DEFAULT_TIER_CONFIG } = jiti("../src/tier-manager.ts"); const { createRetriever, DEFAULT_RETRIEVAL_CONFIG } = jiti("../src/retriever.ts"); +const { computeTier1Patch } = jiti("../src/auto-recall-tier1.ts"); const now = Date.now(); @@ -222,41 +223,15 @@ console.log("OK: smart memory lifecycle test passed"); // Tier 1 integration assertion // ============================================================================ // Verify the auto-recall injection path's computed patch correctly increments -// access_count. This test uses the same pure helper as test/tier1-counters.test.mjs -// but runs it in the lifecycle context to guard against regression in the shared -// parse/build plumbing. If parseSmartMetadata or the injection patch ever breaks -// the access_count contract, this integration assertion fails alongside the -// unit-level tests in tier1-counters.test.mjs. +// access_count. Imports the same computeTier1Patch consumed by index.ts so any +// regression in the production helper or in parseSmartMetadata's plumbing +// surfaces here. { - function computeTier1Patch(meta, injectedAt) { - let baseBadRecall = meta.bad_recall_count ?? 0; - if (meta.suppressed_until_ms === undefined && - ((meta.bad_recall_count ?? 0) > 0 || (meta.suppressed_until_turn ?? 0) > 0)) { - baseBadRecall = 0; - } - const gap = typeof meta.last_injected_at === "number" - ? injectedAt - meta.last_injected_at : Infinity; - const decayed = (gap > 86_400_000) ? 0 : baseBadRecall; - const stale = typeof meta.last_injected_at === "number" && meta.last_injected_at > 0 - && (typeof meta.last_confirmed_use_at !== "number" - || meta.last_confirmed_use_at < meta.last_injected_at); - const nextBadRecall = stale ? decayed + 1 : decayed; - return { - access_count: (meta.access_count ?? 0) + 1, - last_accessed_at: injectedAt, - injected_count: (meta.injected_count ?? 0) + 1, - last_injected_at: injectedAt, - bad_recall_count: nextBadRecall, - suppressed_until_ms: (nextBadRecall >= 3) ? injectedAt + 1_800_000 : (meta.suppressed_until_ms ?? 0), - suppressed_until_turn: 0, - }; - } - const freshMeta = parseSmartMetadata( JSON.stringify({ l0_abstract: "lifecycle-tier1", access_count: 0 }), { text: "lifecycle-tier1", category: "fact" }, ); - const patched = computeTier1Patch(freshMeta, Date.now()); + const patched = computeTier1Patch(freshMeta, { injectedAt: Date.now() }); assert.equal(patched.access_count, 1, "Tier 1: access_count must increment from 0 to 1 on auto-recall injection"); console.log("✓ Tier 1 access_count integration assertion passed"); } diff --git a/test/tier1-counters.test.mjs b/test/tier1-counters.test.mjs index ba8600ff..6fccd2d7 100644 --- a/test/tier1-counters.test.mjs +++ b/test/tier1-counters.test.mjs @@ -9,6 +9,10 @@ const { parseSmartMetadata, buildSmartMetadata, } = jiti("../src/smart-metadata.ts"); +const { + computeTier1Patch, + isSuppressed, +} = jiti("../src/auto-recall-tier1.ts"); describe("Tier 1: suppressed_until_ms field presence semantics", () => { it("returns undefined when raw JSON does not include the key (sentinel for 'never touched by Tier 1')", () => { @@ -82,14 +86,9 @@ describe("Tier 1: plugin config schema", () => { }); describe("Tier 1: governance filter reads suppressed_until_ms", () => { - // Pure-logic test of the filter predicate. We define a local helper that - // mirrors the production code, then test it directly. This avoids booting - // the plugin runtime for a 2-line condition. End-to-end wiring is verified - // by visual inspection of index.ts after the rewrite. - function isSuppressed(meta, nowMs) { - const suppressUntil = meta.suppressed_until_ms ?? 0; - return suppressUntil > 0 && nowMs < suppressUntil; - } + // Drives the production isSuppressed predicate (imported above). The same + // function is consumed by the governance filter in index.ts, so a regression + // in either place surfaces here. it("suppresses when nowMs < suppressed_until_ms", () => { const future = Date.now() + 60_000; @@ -118,56 +117,9 @@ describe("Tier 1: governance filter reads suppressed_until_ms", () => { }); describe("Tier 1: bad_recall_count decay and patch shape (Option C)", () => { - // Pure-logic helper that mirrors the production computation. - function computeTier1Patch(meta, opts) { - const { - injectedAt, - badRecallDecayMs = 86_400_000, - suppressionDurationMs = 1_800_000, - minRepeated = 8, - } = opts; - - // Lazy heal - let baseBadRecall = meta.bad_recall_count ?? 0; - if (meta.suppressed_until_ms === undefined && - ((meta.bad_recall_count ?? 0) > 0 || (meta.suppressed_until_turn ?? 0) > 0)) { - baseBadRecall = 0; - } - - // Option C: decay by gap - const gapSinceLastInjection = typeof meta.last_injected_at === "number" - ? injectedAt - meta.last_injected_at - : Infinity; - const decayedBadRecall = (badRecallDecayMs > 0 && gapSinceLastInjection > badRecallDecayMs) - ? 0 - : baseBadRecall; - - // staleInjected (existing judgment preserved verbatim) - const staleInjected = - typeof meta.last_injected_at === "number" && - meta.last_injected_at > 0 && - ( - typeof meta.last_confirmed_use_at !== "number" || - meta.last_confirmed_use_at < meta.last_injected_at - ); - const nextBadRecallCount = staleInjected - ? decayedBadRecall + 1 - : decayedBadRecall; - - const shouldSuppress = nextBadRecallCount >= 3 && minRepeated > 0; - - return { - access_count: (meta.access_count ?? 0) + 1, - last_accessed_at: injectedAt, - injected_count: (meta.injected_count ?? 0) + 1, - last_injected_at: injectedAt, - bad_recall_count: nextBadRecallCount, - suppressed_until_ms: shouldSuppress - ? Math.max(meta.suppressed_until_ms ?? 0, injectedAt + suppressionDurationMs) - : (meta.suppressed_until_ms ?? 0), - suppressed_until_turn: 0, - }; - } + // Drives the production computeTier1Patch (imported above). Tests that + // assert on suppression firing pass minRepeated explicitly; tests that only + // care about counter shape rely on minRepeated=0 default (no suppression). it("T1 access_count: accumulates 0 → 1 on first injection", () => { const now = Date.now(); From 49693bcd189fb5421fd98d2a8551274043ab26d8 Mon Sep 17 00:00:00 2001 From: raw34 Date: Tue, 5 May 2026 19:41:40 +0800 Subject: [PATCH 3/4] fix(auto-recall): address PR #712 second-round review (F1 + MR2 + MR3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (must-fix): parsePluginConfig was dropping the two new Tier 1 config fields. The interface declared them and index.ts read them, but the return-object construction never copied them through, so user config was always silently discarded and the runtime always saw undefined. - Add a parseNonNegativeInt helper (parsePositiveInt rejects 0, but 0 is a meaningful sentinel for both fields: disable decay / collapse suppression to a no-op). - Wire autoRecallBadRecallDecayMs and autoRecallSuppressionDurationMs through parsePluginConfig. - Add 5 regression tests in tier1-counters.test.mjs covering: field propagation, the 0 sentinel, undefined fallback, and negative-input rejection. MR2 (nice-to-have): parseSmartMetadata used `!== undefined` to detect the lazy-heal sentinel. If any persistence layer ever round-trips undefined → null on the metadata JSON, the sentinel was lost (null fell through to clampCount → 0, marking the memory as "Tier 1 touched" incorrectly). - Switch to `!= null` (covers both undefined and null) in both parseSmartMetadata and the patch path in buildSmartMetadata. - Add a regression test for the null round-trip case. MR3 (nice-to-have): document why TIER1_BAD_RECALL_SUPPRESSION_THRESHOLD is a constant while companion knobs are public config — "3 strikes" is a behavioral design choice; decay/duration are ops tuning. Note the existing `minRepeated` opt seam is available if tuning is ever needed. EF1: pre-existing master flake, documented in earlier comment. MR1 (legacy turn-based suppression at deploy): deferred — old turn numbers were never deploy-stable in the pre-Tier-1 design (turn counter resets per session), so "currently-suppressed memories" do not have well-defined cross-deploy semantics. Lazy-heal is the intended migration. Will document in the follow-up PR comment. Co-Authored-By: Claude Opus 4.7 (1M context) --- index.ts | 20 ++++++++++ src/auto-recall-tier1.ts | 8 +++- src/smart-metadata.ts | 11 ++++-- test/tier1-counters.test.mjs | 71 ++++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 5 deletions(-) diff --git a/index.ts b/index.ts index c9cb7d1c..833ae492 100644 --- a/index.ts +++ b/index.ts @@ -338,6 +338,22 @@ function parsePositiveInt(value: unknown): number | undefined { return undefined; } +// Like parsePositiveInt but allows 0. Used for fields where 0 is a meaningful +// "disabled" sentinel (e.g. autoRecallBadRecallDecayMs=0 disables decay). +function parseNonNegativeInt(value: unknown): number | undefined { + if (typeof value === "number" && Number.isFinite(value) && value >= 0) { + return Math.floor(value); + } + if (typeof value === "string") { + const s = value.trim(); + if (!s) return undefined; + const resolved = resolveEnvVars(s); + const n = Number(resolved); + if (Number.isFinite(n) && n >= 0) return Math.floor(n); + } + return undefined; +} + function clampInt(value: number, min: number, max: number): number { if (!Number.isFinite(value)) return min; return Math.min(max, Math.max(min, Math.floor(value))); @@ -4272,6 +4288,10 @@ export function parsePluginConfig(value: unknown): PluginConfig { autoRecall: cfg.autoRecall === true, autoRecallMinLength: parsePositiveInt(cfg.autoRecallMinLength), autoRecallMinRepeated: parsePositiveInt(cfg.autoRecallMinRepeated) ?? 8, + // 0 is a meaningful sentinel for both Tier 1 knobs (disable decay / + // collapse suppression to a no-op), so use the non-negative parser. + autoRecallBadRecallDecayMs: parseNonNegativeInt(cfg.autoRecallBadRecallDecayMs), + autoRecallSuppressionDurationMs: parseNonNegativeInt(cfg.autoRecallSuppressionDurationMs), autoRecallMaxItems: parsePositiveInt(cfg.autoRecallMaxItems) ?? 3, autoRecallMaxChars: parsePositiveInt(cfg.autoRecallMaxChars) ?? 600, autoRecallPerItemMaxChars: parsePositiveInt(cfg.autoRecallPerItemMaxChars) ?? 180, diff --git a/src/auto-recall-tier1.ts b/src/auto-recall-tier1.ts index 2a84b27f..d963a6f5 100644 --- a/src/auto-recall-tier1.ts +++ b/src/auto-recall-tier1.ts @@ -1,8 +1,12 @@ import type { SmartMemoryMetadata } from "./smart-metadata.ts"; // Suppression fires when bad_recall_count reaches this value (and the recall -// dedup window is active). Hardcoded by Tier 1 design; if this becomes -// configurable, expose via opts on computeTier1Patch. +// dedup window is active). Intentionally a constant rather than public config: +// the "3 strikes" rule is a behavioral design choice that should hold across +// deployments, while the companion knobs (decay window, suppression duration) +// are operational tuning parameters that ops may legitimately tune. If a real +// use case for tuning the threshold appears, add it as an opt on +// computeTier1Patch (it already accepts `minRepeated`, so the seam exists). export const TIER1_BAD_RECALL_SUPPRESSION_THRESHOLD = 3; // Default values for the two plugin config fields. Kept here so the diff --git a/src/smart-metadata.ts b/src/smart-metadata.ts index add3b50b..0bcb3841 100644 --- a/src/smart-metadata.ts +++ b/src/smart-metadata.ts @@ -341,9 +341,11 @@ export function parseSmartMetadata( // preserving `undefined` is load-bearing for the Tier 1 lazy-heal sentinel // (see JSDoc on SmartMemoryMetadata.suppressed_until_ms). The `undefined` // signal distinguishes "never touched by Tier 1 code" from "Tier 1 touched - // but no active suppression (0)". + // but no active suppression (0)". `null` is treated as missing too — + // some persistence layers serialize undefined → null on round-trip, and + // we want the sentinel to survive that. suppressed_until_ms: - parsed.suppressed_until_ms !== undefined + parsed.suppressed_until_ms != null ? clampCount(parsed.suppressed_until_ms, 0) : undefined, canonical_id: normalizeOptionalString(parsed.canonical_id), @@ -435,8 +437,11 @@ export function buildSmartMetadata( patch.suppressed_until_turn, base.suppressed_until_turn, ), + // Treat null patches the same as undefined (leave base value alone), + // mirroring parseSmartMetadata. A patch caller that wants to clear + // suppression must pass 0 explicitly. suppressed_until_ms: - patch.suppressed_until_ms === undefined + patch.suppressed_until_ms == null ? base.suppressed_until_ms : (typeof patch.suppressed_until_ms === "number" && patch.suppressed_until_ms >= 0 ? Math.floor(patch.suppressed_until_ms) diff --git a/test/tier1-counters.test.mjs b/test/tier1-counters.test.mjs index 6fccd2d7..201f514f 100644 --- a/test/tier1-counters.test.mjs +++ b/test/tier1-counters.test.mjs @@ -2,9 +2,17 @@ import assert from "node:assert/strict"; import { describe, it } from "node:test"; import path from "node:path"; import fs from "node:fs"; +import { fileURLToPath } from "node:url"; import jitiFactory from "jiti"; +const testDir = path.dirname(fileURLToPath(import.meta.url)); +const pluginSdkStubPath = path.resolve(testDir, "helpers", "openclaw-plugin-sdk-stub.mjs"); + const jiti = jitiFactory(import.meta.url, { interopDefault: true }); +const jitiWithSdkStub = jitiFactory(import.meta.url, { + interopDefault: true, + alias: { "openclaw/plugin-sdk": pluginSdkStubPath }, +}); const { parseSmartMetadata, buildSmartMetadata, @@ -13,6 +21,7 @@ const { computeTier1Patch, isSuppressed, } = jiti("../src/auto-recall-tier1.ts"); +const { parsePluginConfig } = jitiWithSdkStub("../index.ts"); describe("Tier 1: suppressed_until_ms field presence semantics", () => { it("returns undefined when raw JSON does not include the key (sentinel for 'never touched by Tier 1')", () => { @@ -52,6 +61,16 @@ describe("Tier 1: suppressed_until_ms field presence semantics", () => { ); assert.equal(meta.suppressed_until_ms, 0); }); + + it("MR2: treats null as missing (sentinel survives undefined→null round-trip in any persistence layer)", () => { + const meta = parseSmartMetadata( + JSON.stringify({ l0_abstract: "null-roundtrip", suppressed_until_ms: null }), + { text: "null-roundtrip", category: "fact" }, + ); + // Without the null-tolerant fix, this would be 0 (treating the memory as + // "Tier 1 touched, no suppression") and lazy-heal would never fire. + assert.equal(meta.suppressed_until_ms, undefined); + }); }); describe("Tier 1: plugin config schema", () => { @@ -296,3 +315,55 @@ describe("Tier 1: bad_recall_count decay and patch shape (Option C)", () => { assert.equal(patch.bad_recall_count, 3); }); }); + +describe("Tier 1: parsePluginConfig propagates new config fields", () => { + // F1 regression: the openclaw.plugin.json schema declares + // autoRecallBadRecallDecayMs and autoRecallSuppressionDurationMs, but they + // were dropped during config parsing — the runtime always saw undefined and + // fell back to defaults regardless of user config. + function baseConfig() { + return { embedding: { apiKey: "test-api-key" } }; + } + + it("propagates autoRecallBadRecallDecayMs when set", () => { + const parsed = parsePluginConfig({ + ...baseConfig(), + autoRecallBadRecallDecayMs: 3_600_000, // 1h + }); + assert.equal(parsed.autoRecallBadRecallDecayMs, 3_600_000); + }); + + it("propagates autoRecallSuppressionDurationMs when set", () => { + const parsed = parsePluginConfig({ + ...baseConfig(), + autoRecallSuppressionDurationMs: 600_000, // 10min + }); + assert.equal(parsed.autoRecallSuppressionDurationMs, 600_000); + }); + + it("preserves 0 as a meaningful value (disable decay / collapse suppression)", () => { + const parsed = parsePluginConfig({ + ...baseConfig(), + autoRecallBadRecallDecayMs: 0, + autoRecallSuppressionDurationMs: 0, + }); + assert.equal(parsed.autoRecallBadRecallDecayMs, 0); + assert.equal(parsed.autoRecallSuppressionDurationMs, 0); + }); + + it("returns undefined when not set (caller falls back to TIER1_DEFAULT_*)", () => { + const parsed = parsePluginConfig(baseConfig()); + assert.equal(parsed.autoRecallBadRecallDecayMs, undefined); + assert.equal(parsed.autoRecallSuppressionDurationMs, undefined); + }); + + it("rejects negative values (returns undefined → falls back to default)", () => { + const parsed = parsePluginConfig({ + ...baseConfig(), + autoRecallBadRecallDecayMs: -1, + autoRecallSuppressionDurationMs: -1, + }); + assert.equal(parsed.autoRecallBadRecallDecayMs, undefined); + assert.equal(parsed.autoRecallSuppressionDurationMs, undefined); + }); +}); From a45f71bcd0b644ac680dc25d4e4f610db939804d Mon Sep 17 00:00:00 2001 From: raw34 Date: Tue, 5 May 2026 20:04:29 +0800 Subject: [PATCH 4/4] test: print 'OK' only after Tier 1 assertion runs (lifecycle test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix the smart-memory-lifecycle test printed "OK: passed" before the appended Tier 1 integration assertion ran, so a Tier 1 failure would surface as "OK" followed by an AssertionError — confusing under log skimming. Move the print to the end and reword it to reflect both the legacy lifecycle and Tier 1 integration coverage. Caught by self-audit before the next reviewer round; reviewer flagged the same as a Scope Drift signal in the previous review. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/smart-memory-lifecycle.mjs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/smart-memory-lifecycle.mjs b/test/smart-memory-lifecycle.mjs index 9dc1607f..ccfd0f8d 100644 --- a/test/smart-memory-lifecycle.mjs +++ b/test/smart-memory-lifecycle.mjs @@ -217,8 +217,6 @@ assert.equal( "fresh working-tier memories should survive decay + hardMinScore filtering", ); -console.log("OK: smart memory lifecycle test passed"); - // ============================================================================ // Tier 1 integration assertion // ============================================================================ @@ -233,5 +231,9 @@ console.log("OK: smart memory lifecycle test passed"); ); const patched = computeTier1Patch(freshMeta, { injectedAt: Date.now() }); assert.equal(patched.access_count, 1, "Tier 1: access_count must increment from 0 to 1 on auto-recall injection"); - console.log("✓ Tier 1 access_count integration assertion passed"); } + +// Final success message printed only after every assertion (legacy lifecycle + +// Tier 1 integration) has run. Earlier this print was above the Tier 1 block, +// which made a failing Tier 1 assertion look like "OK passed" + an error. +console.log("OK: smart memory lifecycle test passed (incl. Tier 1 access_count integration)");