diff --git a/packages/opencode/src/plugin/observer.ts b/packages/opencode/src/plugin/observer.ts index 2dbbd805b9bd..9b8a45d6d120 100644 --- a/packages/opencode/src/plugin/observer.ts +++ b/packages/opencode/src/plugin/observer.ts @@ -292,6 +292,7 @@ class SessionObserverManager { let stored = entry await this.mutate(async () => { for (const file of files) { + const scope: "global" | "project" = file === paths.project ? "project" : "global" try { await fs.mkdir(path.dirname(file), { recursive: true }) let existing: SessionLearning[] = [] @@ -302,13 +303,18 @@ class SessionObserverManager { const found = existing.find((e) => e.lesson.trim().toLowerCase() === normalizedText) if (found) { // Re-recording refreshes recency AND moves the rule to the front, so it - // re-enters the injection window; it never duplicates. + // re-enters the injection window; it never duplicates. It also clears any + // tombstone, which is how a forgotten rule is restored. found.timestamp = new Date().toISOString() + found.scope = scope + found.deletedAt = undefined + found.deletedBy = undefined stored = found - await writeJsonAtomic(file, [found, ...existing.filter((e) => e !== found)].slice(0, 100)) + await writeJsonAtomic(file, trimRules([found, ...existing.filter((e) => e !== found)])) continue } - await writeJsonAtomic(file, [entry, ...existing].slice(0, 100)) + await writeJsonAtomic(file, trimRules([{ ...entry, scope }, ...existing])) + stored = { ...entry, scope } } catch {} } }) @@ -337,7 +343,7 @@ class SessionObserverManager { // global note (AIMETH-008 - shared repo facts win). One entry per rule text. const seen = new Set() const merged: SessionLearning[] = [] - for (const rule of [...(await read(paths.project)), ...(await read(paths.global))]) { + for (const rule of [...(await read(paths.project)), ...(await read(paths.global))].filter(isLive)) { const key = learningKey(rule) if (seen.has(key)) continue seen.add(key) @@ -379,8 +385,8 @@ class SessionObserverManager { } } const paths = learningStorePaths(workspaceDir) - const global = await read(paths.global) - const project = await read(paths.project) + const global = (await read(paths.global)).filter(isLive) + const project = (await read(paths.project)).filter(isLive) const globalKeys = new Set(global.map(learningKey)) const merged = [ ...global.map((l) => ({ ...l, origin: "global" as const })), @@ -393,9 +399,9 @@ class SessionObserverManager { } /** - * Remove rules whose text matches (case-insensitive substring) from memory and - * from both persisted stores. Returns the number of distinct rules removed; a - * rule present in both stores is counted once. + * Forget rules whose text matches (case-insensitive substring): mark them deleted + * (tombstone) in both persisted stores. Returns the number of distinct rules newly + * forgotten; a rule present in both stores is counted once. */ public async forgetLearning(lessonText: string, workspaceDir?: string): Promise { const needle = lessonText.trim().toLowerCase() @@ -403,39 +409,56 @@ class SessionObserverManager { const matches = (l: SessionLearning) => String(l.lesson ?? "").toLowerCase().includes(needle) const paths = learningStorePaths(workspaceDir) const files = [paths.global, paths.project].filter((file): file is string => Boolean(file)) - const removed = new Set() - for (const file of files) { - for (const key of await this.dropRules(file, matches)) removed.add(key) - } - return removed.size + // One mutation for both stores, so a record cannot interleave between them. + return this.mutate(async () => { + const results: Array<{ keys: Set; ok: boolean }> = [] + for (const file of files) results.push(await this.applyTombstones(file, matches)) + // If a store could not be read or written, we cannot claim its copy is gone, so + // report nothing rather than a count that may be wrong. + if (results.some((r) => !r.ok)) return 0 + const forgotten = new Set() + for (const result of results) for (const key of result.keys) forgotten.add(key) + return forgotten.size + }) } /** - * Remove matching rules from one store atomically and count them. A removal is - * only reported when the write landed; a failed write returns nothing and the - * file is left untouched. + * Mark matching live rules as deleted in one store. The rows are kept as + * tombstones so a later merge or sync cannot resurrect them. Non-queuing: the + * caller must already hold the mutation lock. Returns the keys acted on and + * whether the read and write both succeeded; an absent store is not a failure. */ - private dropRules(file: string, matches: (l: SessionLearning) => boolean): Promise> { - return this.mutate(async () => { - const found = new Set() - try { - const parsed = JSON.parse(await fs.readFile(file, "utf8")) - if (!Array.isArray(parsed)) return found - const kept: SessionLearning[] = [] - for (const row of parsed as SessionLearning[]) { - if (matches(row)) { - found.add(String(row.lesson ?? "").trim().toLowerCase()) - continue - } - kept.push(row) - } - if (found.size === 0) return found - await writeJsonAtomic(file, kept.slice(0, 100)) - } catch { - return new Set() + private async applyTombstones( + file: string, + matches: (l: SessionLearning) => boolean, + ): Promise<{ keys: Set; ok: boolean }> { + const keys = new Set() + let raw: string + try { + raw = await fs.readFile(file, "utf8") + } catch (error) { + // Only a missing store is a successful absence; any other read error means we + // cannot know what the store holds. + const code = error && typeof error === "object" && "code" in error ? (error as { code?: unknown }).code : undefined + return { keys, ok: code === "ENOENT" } + } + try { + const parsed = JSON.parse(raw) + if (!Array.isArray(parsed)) return { keys, ok: false } + const rows = parsed as SessionLearning[] + let changed = false + for (const row of rows) { + if (!isLive(row) || !matches(row)) continue + row.deletedAt = new Date().toISOString() + row.deletedBy = process.env.USERNAME || process.env.USER || "local" + keys.add(learningKey(row)) + changed = true } - return found - }) + if (changed) await writeJsonAtomic(file, trimRules(rows)) + return { keys, ok: true } + } catch { + return { keys, ok: false } + } } public getLatestRetrospectives(): SessionRetrospective[] { @@ -461,6 +484,23 @@ export function learningStorePaths(workspaceDir?: string): { global?: string; pr const learningKey = (l: SessionLearning) => String(l.lesson ?? "").trim().toLowerCase() +/** A rule is live unless it carries a tombstone. */ +const isLive = (l: SessionLearning) => !l.deletedAt + +const MAX_LIVE_RULES = 100 +const MAX_TOMBSTONES = 100 + +/** + * Bound file size without evicting tombstones: a dropped tombstone would let a + * later merge resurrect the rule it records as deleted. + */ +function trimRules(rows: SessionLearning[]): SessionLearning[] { + return [ + ...rows.filter(isLive).slice(0, MAX_LIVE_RULES), + ...rows.filter((r) => !isLive(r)).slice(0, MAX_TOMBSTONES), + ] +} + /** Write JSON via a temp file + rename so a reader never sees a half-written file. */ async function writeJsonAtomic(file: string, rows: SessionLearning[]): Promise { const tmp = `${file}.${process.pid}.${Math.random().toString(36).slice(2, 7)}.tmp` @@ -474,6 +514,11 @@ export interface SessionLearning { lesson: string context?: string source: "user_feedback" | "buddy_review" | "auto_correction" | "session_retro" + /** Which store this row belongs to. */ + scope?: "global" | "project" + /** Tombstone: set when the rule is forgotten, so a later merge cannot resurrect it. */ + deletedAt?: string + deletedBy?: string } export const sessionObserver = new SessionObserverManager() diff --git a/packages/opencode/test/plugin/observer.test.ts b/packages/opencode/test/plugin/observer.test.ts index 77007c884733..c67c82f0a07a 100644 --- a/packages/opencode/test/plugin/observer.test.ts +++ b/packages/opencode/test/plugin/observer.test.ts @@ -168,4 +168,144 @@ describe("rule visibility", () => { restoreUserProfile(prev) } }) + + test("forgetting keeps the row as a tombstone instead of deleting it", async () => { + await using home = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + const file = path.join(home.path, ".local", "share", "opencode", "learnings.json") + await sessionObserver.recordLearning({ lesson: "tombstone me", source: "user_feedback" }) + expect(await sessionObserver.forgetLearning("tombstone me")).toBe(1) + + expect((await sessionObserver.listLearnings()).find((r) => r.lesson === "tombstone me")).toBeUndefined() + + const rows = JSON.parse(await fs.readFile(file, "utf8")) + const row = rows.find((r: { lesson: string }) => r.lesson === "tombstone me") + expect(row).toBeDefined() + expect(typeof row.deletedAt).toBe("string") + expect(row.scope).toBe("global") + } finally { + restoreUserProfile(prev) + } + }) + + test("re-recording a forgotten rule revives it without duplicating", async () => { + await using home = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + const file = path.join(home.path, ".local", "share", "opencode", "learnings.json") + await sessionObserver.recordLearning({ lesson: "revive me", source: "user_feedback" }) + expect(await sessionObserver.forgetLearning("revive me")).toBe(1) + expect((await sessionObserver.listLearnings()).find((r) => r.lesson === "revive me")).toBeUndefined() + + await sessionObserver.recordLearning({ lesson: "revive me", source: "user_feedback" }) + + expect((await sessionObserver.listLearnings()).find((r) => r.lesson === "revive me")!.injected).toBe(true) + const rows = JSON.parse(await fs.readFile(file, "utf8")) + const rowsForLesson = rows.filter((r: { lesson: string }) => r.lesson === "revive me") + expect(rowsForLesson.length).toBe(1) + expect(rowsForLesson[0].deletedAt).toBeUndefined() + } finally { + restoreUserProfile(prev) + } + }) + + test("records the scope of each rule", async () => { + await using home = await tmpdir() + await using ws = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + await sessionObserver.recordLearning({ lesson: "global rule", source: "user_feedback" }) + await sessionObserver.recordLearning({ lesson: "project rule", source: "user_feedback" }, ws.path) + + const global = JSON.parse( + await fs.readFile(path.join(home.path, ".local", "share", "opencode", "learnings.json"), "utf8"), + ) + const project = JSON.parse( + await fs.readFile(path.join(ws.path, ".system_generated", "logs", "learnings.json"), "utf8"), + ) + expect(global.find((r: { lesson: string }) => r.lesson === "global rule").scope).toBe("global") + expect(project.find((r: { lesson: string }) => r.lesson === "project rule").scope).toBe("project") + } finally { + restoreUserProfile(prev) + } + }) + + test("a tombstone survives the record cap", async () => { + await using home = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + const file = path.join(home.path, ".local", "share", "opencode", "learnings.json") + const rows: Array<{ + id: string + timestamp: string + lesson: string + source: string + deletedAt?: string + deletedBy?: string + }> = Array.from({ length: 100 }, (_, i) => ({ + id: "r" + i, + timestamp: "2026-09-01T00:00:00.000Z", + lesson: "rule " + i, + source: "user_feedback", + })) + rows.push({ + id: "dead", + timestamp: "2026-09-01T00:00:00.000Z", + lesson: "tombstoned rule", + source: "user_feedback", + deletedAt: "2026-09-02T00:00:00.000Z", + deletedBy: "test", + }) + await fs.mkdir(path.dirname(file), { recursive: true }) + await fs.writeFile(file, JSON.stringify(rows), "utf8") + + await sessionObserver.recordLearning({ lesson: "one more", source: "user_feedback" }) + + const after = JSON.parse(await fs.readFile(file, "utf8")) + const tomb = after.find((r: { lesson: string }) => r.lesson === "tombstoned rule") + expect(tomb).toBeDefined() + expect(tomb.deletedAt).toBe("2026-09-02T00:00:00.000Z") + } finally { + restoreUserProfile(prev) + } + }) + + test("forgetting still counts when the other store does not exist", async () => { + await using home = await tmpdir() + await using ws = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + await sessionObserver.recordLearning({ lesson: "only global", source: "user_feedback" }, ws.path) + // Remove the project store so only the global copy remains. + await fs.rm(path.join(ws.path, ".system_generated", "logs", "learnings.json"), { force: true }) + + expect(await sessionObserver.forgetLearning("only global", ws.path)).toBe(1) + } finally { + restoreUserProfile(prev) + } + }) + + test("forgetting reports nothing when a store is unreadable data", async () => { + await using home = await tmpdir() + await using ws = await tmpdir() + const prev = process.env.USERPROFILE + process.env.USERPROFILE = home.path + try { + await sessionObserver.recordLearning({ lesson: "fragile", source: "user_feedback" }) + // The project store exists but holds valid JSON that is not the expected array. + const wsFile = path.join(ws.path, ".system_generated", "logs", "learnings.json") + await fs.mkdir(path.dirname(wsFile), { recursive: true }) + await fs.writeFile(wsFile, JSON.stringify("not a rules array"), "utf8") + + expect(await sessionObserver.forgetLearning("fragile", ws.path)).toBe(0) + } finally { + restoreUserProfile(prev) + } + }) })