From a9100a33c24b292bffedbbc967e1f4f7bbfb577d Mon Sep 17 00:00:00 2001 From: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> Date: Fri, 10 Apr 2026 18:57:06 -0500 Subject: [PATCH] fix teams feedback learning filename collisions --- .../msteams/src/feedback-reflection-store.ts | 69 ++++++++++++------- .../msteams/src/feedback-reflection.test.ts | 56 ++++++++++++++- 2 files changed, 97 insertions(+), 28 deletions(-) diff --git a/extensions/msteams/src/feedback-reflection-store.ts b/extensions/msteams/src/feedback-reflection-store.ts index 83125372aba..fd104f882bb 100644 --- a/extensions/msteams/src/feedback-reflection-store.ts +++ b/extensions/msteams/src/feedback-reflection-store.ts @@ -7,10 +7,36 @@ const lastReflectionBySession = new Map(); /** Maximum cooldown entries before pruning expired ones. */ const MAX_COOLDOWN_ENTRIES = 500; -function sanitizeSessionKey(sessionKey: string): string { +function legacySanitizeSessionKey(sessionKey: string): string { return sessionKey.replace(/[^a-zA-Z0-9_-]/g, "_"); } +function encodeSessionKey(sessionKey: string): string { + return Buffer.from(sessionKey, "utf8").toString("base64url"); +} + +function resolveLearningsFilePath(storePath: string, sessionKey: string): string { + return `${storePath}/${encodeSessionKey(sessionKey)}.learnings.json`; +} + +function resolveLegacyLearningsFilePath(storePath: string, sessionKey: string): string { + return `${storePath}/${legacySanitizeSessionKey(sessionKey)}.learnings.json`; +} + +async function readLearningsFile( + filePath: string, +): Promise<{ exists: boolean; learnings: string[] }> { + const fs = await import("node:fs/promises"); + + try { + const content = await fs.readFile(filePath, "utf-8"); + const parsed = JSON.parse(content); + return { exists: true, learnings: Array.isArray(parsed) ? parsed : [] }; + } catch { + return { exists: false, learnings: [] }; + } +} + /** Prune expired cooldown entries to prevent unbounded memory growth. */ function pruneExpiredCooldowns(cooldownMs: number): void { if (lastReflectionBySession.size <= MAX_COOLDOWN_ENTRIES) { @@ -54,21 +80,15 @@ export async function storeSessionLearning(params: { const fs = await import("node:fs/promises"); const path = await import("node:path"); - const learningsFile = path.join( - params.storePath, - `${sanitizeSessionKey(params.sessionKey)}.learnings.json`, - ); + const learningsFile = resolveLearningsFilePath(params.storePath, params.sessionKey); + const legacyLearningsFile = resolveLegacyLearningsFilePath(params.storePath, params.sessionKey); + const { exists, learnings: existingLearnings } = await readLearningsFile(learningsFile); + const { learnings: legacyLearnings } = + exists || legacyLearningsFile === learningsFile + ? { learnings: [] as string[] } + : await readLearningsFile(legacyLearningsFile); - let learnings: string[] = []; - try { - const existing = await fs.readFile(learningsFile, "utf-8"); - const parsed = JSON.parse(existing); - if (Array.isArray(parsed)) { - learnings = parsed; - } - } catch { - // File doesn't exist yet — start fresh. - } + let learnings = exists ? existingLearnings : legacyLearnings; learnings.push(params.learning); if (learnings.length > 10) { @@ -77,6 +97,9 @@ export async function storeSessionLearning(params: { await fs.mkdir(path.dirname(learningsFile), { recursive: true }); await fs.writeFile(learningsFile, JSON.stringify(learnings, null, 2), "utf-8"); + if (!exists && legacyLearningsFile !== learningsFile) { + await fs.rm(legacyLearningsFile, { force: true }).catch(() => undefined); + } } /** Load session learnings for injection into extraSystemPrompt. */ @@ -84,16 +107,10 @@ export async function loadSessionLearnings( storePath: string, sessionKey: string, ): Promise { - const fs = await import("node:fs/promises"); - const path = await import("node:path"); - - const learningsFile = path.join(storePath, `${sanitizeSessionKey(sessionKey)}.learnings.json`); - - try { - const content = await fs.readFile(learningsFile, "utf-8"); - const parsed = JSON.parse(content); - return Array.isArray(parsed) ? parsed : []; - } catch { - return []; + const learningsFile = resolveLearningsFilePath(storePath, sessionKey); + const { exists, learnings } = await readLearningsFile(learningsFile); + if (exists) { + return learnings; } + return (await readLearningsFile(resolveLegacyLearningsFilePath(storePath, sessionKey))).learnings; } diff --git a/extensions/msteams/src/feedback-reflection.test.ts b/extensions/msteams/src/feedback-reflection.test.ts index 7c07101edf1..7993e3bbb39 100644 --- a/extensions/msteams/src/feedback-reflection.test.ts +++ b/extensions/msteams/src/feedback-reflection.test.ts @@ -2,6 +2,7 @@ import { mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { storeSessionLearning } from "./feedback-reflection-store.js"; import { buildFeedbackEvent, buildReflectionPrompt, @@ -174,12 +175,63 @@ describe("loadSessionLearnings", () => { it("reads existing learnings", async () => { tmpDir = await mkdtemp(path.join(os.tmpdir(), "learnings-test-")); - // Colons are sanitized to underscores in filenames (Windows compat) - const safeKey = "msteams_user1"; + const safeKey = Buffer.from("msteams:user1", "utf8").toString("base64url"); const filePath = path.join(tmpDir, `${safeKey}.learnings.json`); await writeFile(filePath, JSON.stringify(["Be concise", "Use examples"]), "utf-8"); const learnings = await loadSessionLearnings(tmpDir, "msteams:user1"); expect(learnings).toEqual(["Be concise", "Use examples"]); }); + + it("keeps distinct session keys isolated across the filename persistence boundary", async () => { + tmpDir = await mkdtemp(path.join(os.tmpdir(), "learnings-test-")); + + await storeSessionLearning({ + storePath: tmpDir, + sessionKey: "msteams:user1", + learning: "Use bullets", + }); + await storeSessionLearning({ + storePath: tmpDir, + sessionKey: "msteams/user1", + learning: "Avoid bullets", + }); + + await expect(loadSessionLearnings(tmpDir, "msteams:user1")).resolves.toEqual(["Use bullets"]); + await expect(loadSessionLearnings(tmpDir, "msteams/user1")).resolves.toEqual(["Avoid bullets"]); + }); + + it("reads and migrates legacy sanitized session learning files", async () => { + tmpDir = await mkdtemp(path.join(os.tmpdir(), "learnings-test-")); + const legacyFile = path.join(tmpDir, "msteams_user1.learnings.json"); + await writeFile(legacyFile, JSON.stringify(["Legacy learning"]), "utf-8"); + + await expect(loadSessionLearnings(tmpDir, "msteams:user1")).resolves.toEqual([ + "Legacy learning", + ]); + + await storeSessionLearning({ + storePath: tmpDir, + sessionKey: "msteams:user1", + learning: "New learning", + }); + + const migratedFile = path.join( + tmpDir, + `${Buffer.from("msteams:user1", "utf8").toString("base64url")}.learnings.json`, + ); + await expect(loadSessionLearnings(tmpDir, "msteams:user1")).resolves.toEqual([ + "Legacy learning", + "New learning", + ]); + await expect(rm(legacyFile, { force: false })).rejects.toMatchObject({ code: "ENOENT" }); + await expect(loadSessionLearnings(tmpDir, "msteams:user1")).resolves.toEqual([ + "Legacy learning", + "New learning", + ]); + await expect(loadSessionLearnings(tmpDir, "msteams/user1")).resolves.toEqual([]); + await expect( + import("node:fs/promises").then((fs) => fs.readFile(migratedFile, "utf-8")), + ).resolves.toContain("Legacy learning"); + }); });