diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f102de..fc543b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `session_close` no longer reports every open thread as failed when a session is closed a second + time (or closed after the server restarted). Threads stored on the session row come back as JSON + strings, and they were handed to the Supabase thread sync without being parsed, so each one had no + id and the sync threw `Cannot read properties of undefined (reading 'includes')`. The close + returned `PARTIAL` with `undefined (...)` entries for all of them. The stored threads are now + parsed first, and a thread with no id fails with a clear message instead of an opaque error. + No data was lost: the unsynced threads stayed in `threads.json` and were retried by the next close. + ## [1.11.1] - 2026-09-22 **Just upgrade the package. Nothing to change on your database.** Every fix below works on a project diff --git a/src/services/thread-supabase.ts b/src/services/thread-supabase.ts index 3eb723c..b11be05 100644 --- a/src/services/thread-supabase.ts +++ b/src/services/thread-supabase.ts @@ -671,6 +671,16 @@ export async function syncThreadsToSupabase( } for (const thread of threads) { + // A thread without an id cannot be looked up or written, and an undefined id used to reach + // directQuery as the filter { thread_id: undefined }, which threw an unhelpful "Cannot read + // properties of undefined (reading 'includes')" for every such thread. Name the real problem. + if (!thread || typeof thread.id !== "string" || thread.id.length === 0) { + const got = typeof thread === "string" ? "a raw string" : thread ? "an object with no id" : String(thread); + const message = `thread has no id (expected a ThreadObject, got ${got})`; + console.error(`[thread-supabase] Skipping thread: ${message}`); + failed.push({ id: "(no id)", error: message }); + continue; + } try { // Check if thread exists in Supabase by ID const existing = await supabase.directQuery(getTableName("threads"), { diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index ffe352a..a740096 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -1421,7 +1421,15 @@ export async function sessionClose( // there is no sync to await — `skipped: true`, and the prune proceeds on // local persistence alone. Awaiting a sync that does not exist would // fail-close every free user's session close. - const closeThreads = (sessionData.open_threads || []) as ThreadObject[]; + // open_threads can still be raw here: orchestra_sessions.open_threads is a text[] whose + // elements are JSON-encoded threads, and on a re-close with no new threads and no in-memory + // thread state the stored strings flow through buildSessionRecord untouched. Casting them to + // ThreadObject left thread.id undefined for every thread, and the sync threw on the undefined + // filter value. normalizeThreads parses JSON strings and migrates plain-text ones. + const closeThreads = normalizeThreads( + (sessionData.open_threads || []) as (string | ThreadObject)[], + sessionId, + ); let threadSync: ThreadSyncResult = { attempted: 0, synced: [], failed: [], skipped: true, all_synced: true, dedup_coverage: "complete", dedup_candidates: 0, diff --git a/tests/unit/services/thread-sync-result.test.ts b/tests/unit/services/thread-sync-result.test.ts index 21055c4..ee9e02e 100644 --- a/tests/unit/services/thread-sync-result.test.ts +++ b/tests/unit/services/thread-sync-result.test.ts @@ -117,6 +117,29 @@ describe("outcome reporting", () => { expect(r.failed[0].error).toContain("boom"); }); + it("fails a thread that has no id with a clear message, without querying Supabase", async () => { + // Regression: stringified threads reached the sync as raw strings, so thread.id was undefined + // and directQuery threw "Cannot read properties of undefined (reading 'includes')". + mockDirectQuery.mockResolvedValue([]); + const noId = { text: "orphan", status: "open", created_at: "2026-08-06T00:00:00.000Z" }; + + const r = await syncThreadsToSupabase( + [noId as never, '{"id":"t-raw"}' as never, thread("t-ok")], + "gitmem", + "s-1" + ); + + expect(r.attempted).toBe(3); + expect(r.synced).toEqual(["t-ok"]); // the valid thread still syncs + expect(r.failed).toHaveLength(2); + expect(r.failed[0].error).toContain("thread has no id"); + expect(r.failed[1].error).toContain("a raw string"); + expect(r.failed.map((f) => f.id)).toEqual(["(no id)", "(no id)"]); + expect(r.failed.some((f) => f.error.includes("includes"))).toBe(false); + // only the valid thread was looked up by id + expect(mockDirectQuery).toHaveBeenCalledTimes(1); + }); + it("continues past a failure and reports the partial split", async () => { // A partial sync is still worth completing — but the miss must be named, // because the local file is the only remaining record of it. diff --git a/tests/unit/tools/session-close-restored-threads.test.ts b/tests/unit/tools/session-close-restored-threads.test.ts new file mode 100644 index 0000000..b533517 --- /dev/null +++ b/tests/unit/tools/session-close-restored-threads.test.ts @@ -0,0 +1,237 @@ +/** + * Regression: session_close must hand ThreadObjects (with ids) to the Supabase thread sync + * even when the session row's open_threads come back from Supabase as JSON strings. + * + * orchestra_sessions.open_threads is a text[] whose elements are JSON-encoded threads. + * On a re-close with no new threads passed and no in-memory thread state (a second close, + * or a close after a server restart), buildSessionRecord left those raw strings in + * sessionData.open_threads and session_close cast them `as ThreadObject[]` without + * normalizing. thread.id was undefined for every thread, so syncThreadsToSupabase built + * the filter { thread_id: undefined } and threw "Cannot read properties of undefined + * (reading 'includes')" for all of them (28 of 28 in the field). + * + * NOTE: vitest.config.ts has `restoreMocks: true`; re-establish spy implementations in beforeEach. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// --- Mock all dependencies before importing session-close --- +// Use plain functions (not vi.fn()) for mocks we don't need to spy on. +// restoreMocks: true in vitest config clears vi.fn() implementations between tests. + +vi.mock("../../../src/services/agent-detection.js", () => ({ + detectAgent: () => ({ agent: "CLI", entrypoint: "cli", docker: true, hostname: "test" }), +})); + +// These tests model nTEG's store (orchestra_ prefix), which has the +// production-only columns and tables. Customer-store behaviour (columns absent) +// is covered in tests/unit/services/store-columns.test.ts. +vi.mock("../../../src/services/store-columns.js", () => ({ + supportedColumns: async (_table: string, candidates: string[]) => new Set(candidates), + storeHasTable: async () => true, + resetStoreColumnCache: () => {}, +})); + +vi.mock("../../../src/services/supabase-client.js", () => ({ + listRecords: vi.fn(), + getRecord: vi.fn(), + directUpsert: vi.fn(), + directPatch: vi.fn(), +})); + +vi.mock("../../../src/services/embedding.js", () => ({ + embed: () => Promise.resolve(null), + isEmbeddingAvailable: () => false, +})); + +vi.mock("../../../src/services/tier.js", () => ({ + hasSupabase: vi.fn(), + hasBatchOperations: () => true, + hasTranscripts: () => false, + hasCacheManagement: () => true, + hasVariants: () => true, + hasEmbeddings: () => true, + hasMetrics: () => true, + getTier: () => "pro", + resetTier: () => {}, + hasProInsights: () => false, + getTablePrefix: () => "orchestra_", + getTableName: (base: string) => `orchestra_${base}`, +})); + +vi.mock("../../../src/services/analytics.js", () => ({ + queryScarUsageByDateRange: vi.fn().mockResolvedValue([]), + enrichScarUsageTitles: vi.fn().mockResolvedValue([]), + formatBlindspotSnippet: vi.fn().mockReturnValue(null), + querySessionsByDateRange: vi.fn().mockResolvedValue([]), + computeLightweightSummary: vi.fn().mockReturnValue(null), +})); + +vi.mock("../../../src/services/storage.js", () => ({ + getStorage: () => ({ + get: () => Promise.resolve(null), + upsert: () => Promise.resolve(undefined), + }), +})); + +vi.mock("../../../src/services/session-state.js", () => ({ + clearCurrentSession: () => {}, + getSurfacedScars: () => [], + getConfirmations: () => [], + getObservations: () => [], + getChildren: () => [], + getThreads: () => [], + getSessionActivity: () => null, + isRecallCalled: () => true, +})); + +vi.mock("../../../src/services/thread-manager.js", async (importOriginal) => ({ + // Real normalizeThreads / migrateStringThread / mergeThreadStates: the bug lives in what + // session_close hands to the sync, so stubbing normalization would hide it. + ...(await importOriginal()), + saveThreadsFile: () => {}, + loadThreadsFile: () => [], +})); + +vi.mock("../../../src/services/thread-dedup.js", () => ({ + deduplicateThreadList: (threads: unknown[]) => threads, +})); + +vi.mock("../../../src/services/thread-supabase.js", () => ({ + syncThreadsToSupabase: vi.fn(), + loadOpenThreadEmbeddings: () => Promise.resolve([]), +})); + +vi.mock("../../../src/services/compliance-validator.js", () => ({ + validateSessionClose: () => ({ valid: true, errors: [], warnings: [] }), + buildCloseCompliance: (_params: unknown, _agent: string, _learningsCount: number) => ({ + close_type: "quick", + agent: "CLI", + checklist_displayed: true, + questions_answered_by_agent: false, + human_asked_for_corrections: false, + learnings_stored: 0, + scars_applied: 0, + }), +})); + +vi.mock("../../../src/services/metrics.js", () => ({ + Timer: class { stop() { return 100; } }, + recordMetrics: () => Promise.resolve(undefined), + buildPerformanceData: (_name: string, latency: number, count: number) => ({ + latency_ms: latency, + target_ms: 3000, + meets_target: latency < 3000, + result_count: count, + }), + updateRelevanceData: () => Promise.resolve(undefined), +})); + +vi.mock("../../../src/tools/record-scar-usage-batch.js", () => ({ + recordScarUsageBatch: () => Promise.resolve({ success: true }), +})); + +vi.mock("../../../src/services/effect-tracker.js", () => ({ + getEffectTracker: () => ({ + track: () => {}, + formatSummary: () => "No tracked effects this session.", + getHealthReport: () => ({ overall: { attempted: 0, succeeded: 0, failed: 0, successRate: "N/A", paths_with_failures: [] }, byPath: {}, recentFailures: [] }), + }), +})); + +vi.mock("../../../src/tools/save-transcript.js", () => ({ + saveTranscript: () => Promise.resolve({ success: false }), +})); + +vi.mock("../../../src/services/transcript-chunker.js", () => ({ + processTranscript: () => Promise.resolve({ success: false }), +})); + +vi.mock("../../../src/services/gitmem-dir.js", () => ({ + getGitmemPath: (filename: string) => `/tmp/.gitmem/${filename}`, + getGitmemDir: () => "/tmp/.gitmem", + getSessionPath: (sid: string, filename: string) => `/tmp/.gitmem/sessions/${sid}/${filename}`, + getSessionDir: (sid: string) => `/tmp/.gitmem/sessions/${sid}`, +})); + +vi.mock("../../../src/services/active-sessions.js", () => ({ + unregisterSession: () => {}, + findSessionByHostPid: () => null, + findSessionById: () => null, // GIT-86: session-close's registry fallback looks up by id +})); + +vi.mock("../../../src/services/thread-suggestions.js", () => ({ + loadSuggestions: () => [], + saveSuggestions: () => {}, + detectSuggestedThreads: () => [], + loadRecentSessionEmbeddings: () => Promise.resolve(null), +})); + +// --- Import after mocks --- +import { sessionClose } from "../../../src/tools/session-close.js"; +import * as supabase from "../../../src/services/supabase-client.js"; +import { hasSupabase } from "../../../src/services/tier.js"; +import { syncThreadsToSupabase } from "../../../src/services/thread-supabase.js"; + +const SESSION_ID = "393adb34-a80c-4c3a-b71a-bc0053b7a7ea"; + +function thread(id: string, text: string) { + return { id, text, status: "open", created_at: "2026-09-01T00:00:00.000Z", source_session: SESSION_ID }; +} + +function sessionRow(openThreads: unknown[]) { + const now = new Date().toISOString(); + return { + id: SESSION_ID, agent: "CLI", project: "test-project", session_title: "Interactive Session", + session_date: now.split("T")[0], created_at: now, close_compliance: null, + open_threads: openThreads, embedding: null, + }; +} + +const okSync = (attempted: number) => ({ + attempted, synced: [], failed: [], skipped: false, all_synced: true, + dedup_coverage: "complete" as const, dedup_candidates: 0, +}); + +describe("session_close re-close with stored (stringified) threads", () => { + beforeEach(() => { + vi.mocked(hasSupabase).mockReturnValue(true); + vi.mocked(supabase.listRecords).mockResolvedValue([]); + vi.mocked(supabase.directUpsert).mockResolvedValue(undefined); + vi.mocked(supabase.directPatch).mockResolvedValue(undefined); + vi.mocked(syncThreadsToSupabase).mockImplementation(async (threads) => okSync(threads.length)); + }); + + it("passes ThreadObjects with ids to the sync when open_threads are stored as JSON strings", async () => { + const stored = [JSON.stringify(thread("t-aaa111", "first thread")), JSON.stringify(thread("t-bbb222", "second thread"))]; + vi.mocked(supabase.getRecord).mockResolvedValue(sessionRow(stored)); + + const result = await sessionClose({ session_id: SESSION_ID, close_type: "quick" }); + + expect(result.success).toBe(true); + expect(syncThreadsToSupabase).toHaveBeenCalledTimes(1); + const sent = vi.mocked(syncThreadsToSupabase).mock.calls[0][0]; + expect(sent.map((t) => t.id)).toEqual(["t-aaa111", "t-bbb222"]); + expect(sent.every((t) => typeof t === "object" && typeof t.text === "string")).toBe(true); + }); + + it("gives plain-text legacy strings a generated id instead of undefined", async () => { + vi.mocked(supabase.getRecord).mockResolvedValue(sessionRow(["legacy plain text thread"])); + + await sessionClose({ session_id: SESSION_ID, close_type: "quick" }); + + const sent = vi.mocked(syncThreadsToSupabase).mock.calls[0][0]; + expect(sent).toHaveLength(1); + expect(sent[0].id).toMatch(/^t-/); + expect(sent[0].text).toBe("legacy plain text thread"); + }); + + it("still passes already-object threads through unchanged", async () => { + vi.mocked(supabase.getRecord).mockResolvedValue(sessionRow([thread("t-ccc333", "object thread")])); + + await sessionClose({ session_id: SESSION_ID, close_type: "quick" }); + + const sent = vi.mocked(syncThreadsToSupabase).mock.calls[0][0]; + expect(sent.map((t) => t.id)).toEqual(["t-ccc333"]); + }); +});