diff --git a/src/services/cache.ts b/src/services/cache.ts index 1b84179..f2f7966 100644 --- a/src/services/cache.ts +++ b/src/services/cache.ts @@ -180,29 +180,37 @@ export class CacheService { * Set cached result */ async setResult(key: string, data: T, ttlMs: number): Promise { - if (!this.enabled) return; - try { - const now = Date.now(); - const entry: CacheEntry = { - key, - created_at: now, - expires_at: now + ttlMs, - data, - }; - - const filename = this.keyToFilename(key); - const filepath = join(this.resultsDir, filename); - const content = JSON.stringify(entry, null, 2); - - writeFileSync(filepath, content, { mode: 0o600 }); - console.error(`[cache] SET: ${key} (TTL: ${ttlMs}ms)`); + await this.writeResult(key, data, ttlMs); } catch (error) { console.warn(`[cache] Error writing ${key}:`, error); // Don't disable cache on write errors - might be transient } } + /** + * Like setResult, but rejects on a write error. Used under the effect + * tracker so cache write failures are counted (GIT-104). + */ + private async writeResult(key: string, data: T, ttlMs: number): Promise { + if (!this.enabled) return; + + const now = Date.now(); + const entry: CacheEntry = { + key, + created_at: now, + expires_at: now + ttlMs, + data, + }; + + const filename = this.keyToFilename(key); + const filepath = join(this.resultsDir, filename); + const content = JSON.stringify(entry, null, 2); + + writeFileSync(filepath, content, { mode: 0o600 }); + console.error(`[cache] SET: ${key} (TTL: ${ttlMs}ms)`); + } + /** * Convenience method: get or fetch scar search results */ @@ -224,7 +232,7 @@ export class CacheService { // Cache the result (tracked, fire-and-forget) getEffectTracker().track("cache_set", "scar_search", () => - this.setResult(key, data, TTL.SCAR_SEARCH) + this.writeResult(key, data, TTL.SCAR_SEARCH) ); return { data, cache_hit: false }; @@ -250,7 +258,7 @@ export class CacheService { // Cache the result (tracked, fire-and-forget) getEffectTracker().track("cache_set", "decisions", () => - this.setResult(key, data, TTL.DECISIONS) + this.writeResult(key, data, TTL.DECISIONS) ); return { data, cache_hit: false }; @@ -276,7 +284,7 @@ export class CacheService { // Cache the result (tracked, fire-and-forget) getEffectTracker().track("cache_set", "wins", () => - this.setResult(key, data, TTL.WINS) + this.writeResult(key, data, TTL.WINS) ); return { data, cache_hit: false }; @@ -314,7 +322,7 @@ export class CacheService { const data = await fetcher(); getEffectTracker().track("cache_set", "sessions", () => - this.setResult(key, data, TTL.SESSIONS) + this.writeResult(key, data, TTL.SESSIONS) ); return { data, cache_hit: false }; } @@ -337,7 +345,7 @@ export class CacheService { const data = await fetcher(); getEffectTracker().track("cache_set", "scar_usage", () => - this.setResult(key, data, TTL.SCAR_USAGE) + this.writeResult(key, data, TTL.SCAR_USAGE) ); return { data, cache_hit: false }; } diff --git a/src/services/effect-tracker.ts b/src/services/effect-tracker.ts index 0df8005..9d3a1ca 100644 --- a/src/services/effect-tracker.ts +++ b/src/services/effect-tracker.ts @@ -91,6 +91,27 @@ class RingBuffer { } } +// --------------------------------------------------------------------------- +// Result inspection +// --------------------------------------------------------------------------- + +/** + * If a resolved value reports its own failure (`{ success: false }`), return a + * message describing it; otherwise null. Prefers the value's error, then + * message, then display text. + */ +function reportedFailure(value: unknown): string | null { + if (typeof value !== "object" || value === null) return null; + const v = value as Record; + if (v.success !== false) return null; + for (const key of ["error", "message", "display"] as const) { + const field = v[key]; + if (field instanceof Error) return field.message; + if (typeof field === "string" && field.length > 0) return field; + } + return "resolved with success: false"; +} + // --------------------------------------------------------------------------- // EffectTracker // --------------------------------------------------------------------------- @@ -106,7 +127,10 @@ export class EffectTracker { * Wrap an async fire-and-forget operation for tracking. * * - On success: records success + duration, returns the result. - * - On failure: records failure + error message, returns undefined. + * - On a result of the form `{ success: false, ... }`: records a failure + * (GIT-104 — many tools catch their own errors and resolve with that + * shape), and still returns the result to the caller. + * - On rejection: records failure + error message, returns undefined. * The error is **not** rethrown — this is intentionally fire-and-forget. */ track( @@ -118,29 +142,36 @@ export class EffectTracker { const stats = this.getOrCreatePath(path); stats.attempted++; + const recordFailure = (errorMsg: string) => { + const duration = Date.now() - start; + stats.failed++; + stats.totalDurationMs += duration; + stats.lastFailure = { + error: errorMsg, + timestamp: new Date().toISOString(), + }; + this.failures.push({ + path, + target, + error: errorMsg, + timestamp: new Date().toISOString(), + durationMs: duration, + }); + }; + return fn().then( (result) => { - const duration = Date.now() - start; + const reported = reportedFailure(result); + if (reported !== null) { + recordFailure(reported); + return result; + } stats.succeeded++; - stats.totalDurationMs += duration; + stats.totalDurationMs += Date.now() - start; return result; }, (error) => { - const duration = Date.now() - start; - const errorMsg = error instanceof Error ? error.message : String(error); - stats.failed++; - stats.totalDurationMs += duration; - stats.lastFailure = { - error: errorMsg, - timestamp: new Date().toISOString(), - }; - this.failures.push({ - path, - target, - error: errorMsg, - timestamp: new Date().toISOString(), - durationMs: duration, - }); + recordFailure(error instanceof Error ? error.message : String(error)); return undefined; }, ); diff --git a/src/services/triple-writer.ts b/src/services/triple-writer.ts index 97c14f8..81d8410 100644 --- a/src/services/triple-writer.ts +++ b/src/services/triple-writer.ts @@ -276,7 +276,11 @@ export function extractDecisionTriples(params: DecisionTripleParams): TripleCand /** * Write triples to knowledge_triples table. - * Fire-and-forget: errors logged but never thrown to caller. + * + * Every candidate is attempted even if an earlier one fails. If any failed, + * rejects afterwards with a summary so the caller's effect tracker records the + * failure (GIT-104) — callers run this under getEffectTracker().track(), which + * never rethrows. */ export async function writeTriples(candidates: TripleCandidate[]): Promise { if (candidates.length === 0 || !hasSupabase()) { @@ -284,6 +288,7 @@ export async function writeTriples(candidates: TripleCandidate[]): Promise 0) { + throw new Error(`${errors.length}/${candidates.length} triples failed: ${errors[0]}`); + } + return written; } @@ -325,7 +336,7 @@ export async function writeTriples(candidates: TripleCandidate[]): Promise {}). + * Fire-and-forget — run under getEffectTracker().track(); rejects if any triple failed. */ export function writeTriplesForLearning(params: LearningTripleParams): Promise { const triples = extractLearningTriples(params); @@ -334,7 +345,7 @@ export function writeTriplesForLearning(params: LearningTripleParams): Promise {}). + * Fire-and-forget — run under getEffectTracker().track(); rejects if any triple failed. */ export function writeTriplesForDecision(params: DecisionTripleParams): Promise { const triples = extractDecisionTriples(params); @@ -430,7 +441,7 @@ export function extractThreadResolutionTriples(params: ThreadResolutionTriplePar /** * Generate and write triples for a newly created thread. - * Fire-and-forget — call with .catch(() => {}). + * Fire-and-forget — run under getEffectTracker().track(); rejects if any triple failed. */ export function writeTriplesForThreadCreation(params: ThreadCreationTripleParams): Promise { const triples = extractThreadCreationTriples(params); @@ -439,7 +450,7 @@ export function writeTriplesForThreadCreation(params: ThreadCreationTripleParams /** * Generate and write triples for a resolved thread. - * Fire-and-forget — call with .catch(() => {}). + * Fire-and-forget — run under getEffectTracker().track(); rejects if any triple failed. */ export function writeTriplesForThreadResolution(params: ThreadResolutionTripleParams): Promise { const triples = extractThreadResolutionTriples(params); diff --git a/src/services/variant-generation.ts b/src/services/variant-generation.ts index ef997e3..9bfa80b 100644 --- a/src/services/variant-generation.ts +++ b/src/services/variant-generation.ts @@ -335,4 +335,12 @@ export async function generateVariantsForScar(scar: ScarData): Promise { console.error( `[variant-generation] ${generationSource}: created ${created}/2 variants for scar ${scar.id} (pipeline ${PIPELINE_VERSION})` ); + + // Surface partial failure to the effect tracker (GIT-104) instead of + // resolving as if both variants were written. + const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === "rejected"); + if (firstFailure) { + const reason = firstFailure.reason instanceof Error ? firstFailure.reason.message : String(firstFailure.reason); + throw new Error(`created ${created}/${variants.length} variants: ${reason}`); + } } diff --git a/src/tools/record-scar-usage-batch.ts b/src/tools/record-scar-usage-batch.ts index b50c5ab..2a914aa 100644 --- a/src/tools/record-scar-usage-batch.ts +++ b/src/tools/record-scar-usage-batch.ts @@ -206,6 +206,7 @@ export async function recordScarUsageBatch( return { success: false, + error: error instanceof Error ? error.message : String(error), usage_ids: usageIds, resolved_count: resolvedCount, failed_count: params.scars.length - resolvedCount, diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index 42fc99e..59633ec 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -1384,6 +1384,8 @@ export async function sessionClose( }) .catch((err) => console.error("[session_close] Transcript processing failed:", err instanceof Error ? err.message : err)); } + // Return the result so a { success: false } save is counted as a failure (GIT-104). + return saveResult; }); } } diff --git a/src/types/index.ts b/src/types/index.ts index 6bd49ec..61654e7 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -480,6 +480,8 @@ export interface RecordScarUsageBatchResult { resolved_count: number; failed_count: number; failed_identifiers?: string[]; + /** Why the batch failed (set when success is false) */ + error?: string; display?: string; performance: PerformanceData; } diff --git a/tests/unit/services/effect-tracker.test.ts b/tests/unit/services/effect-tracker.test.ts new file mode 100644 index 0000000..16a67dd --- /dev/null +++ b/tests/unit/services/effect-tracker.test.ts @@ -0,0 +1,95 @@ +/** + * Unit tests for EffectTracker.track() outcome accounting (GIT-104). + * + * Many tracked operations catch their own errors and resolve with + * { success: false }. Those must count as failures, not successes. + */ + +import { describe, it, expect } from "vitest"; +import { EffectTracker } from "../../../src/services/effect-tracker.js"; + +function statsFor(tracker: EffectTracker, path: string) { + return tracker.getHealthReport().byPath[path]; +} + +describe("EffectTracker.track outcome accounting", () => { + it("counts a resolved { success: false } as a failure and still returns the value", async () => { + const tracker = new EffectTracker(); + const value = { success: false, error: "Supabase upsert error: 404 - table not found" }; + + const returned = await tracker.track("scar_usage", "batch", async () => value); + + expect(returned).toBe(value); + expect(statsFor(tracker, "scar_usage")).toMatchObject({ attempted: 1, succeeded: 0, failed: 1 }); + const report = tracker.getHealthReport(); + expect(report.overall.paths_with_failures).toEqual(["scar_usage"]); + expect(report.recentFailures[0]).toMatchObject({ + path: "scar_usage", + target: "batch", + error: "Supabase upsert error: 404 - table not found", + }); + }); + + it("uses message, then display, when a { success: false } result has no error", async () => { + const tracker = new EffectTracker(); + await tracker.track("p", "message", async () => ({ success: false, message: "from message" })); + await tracker.track("p", "display", async () => ({ success: false, display: "from display" })); + await tracker.track("p", "error-object", async () => ({ success: false, error: new Error("from Error") })); + await tracker.track("p", "bare", async () => ({ success: false })); + + const errors = tracker.getHealthReport().recentFailures.map((f) => [f.target, f.error]); + expect(errors).toEqual([ + ["bare", "resolved with success: false"], + ["error-object", "from Error"], + ["display", "from display"], + ["message", "from message"], + ]); + expect(statsFor(tracker, "p")).toMatchObject({ attempted: 4, succeeded: 0, failed: 4 }); + }); + + it("counts a resolved { success: true } as a success", async () => { + const tracker = new EffectTracker(); + const value = { success: true, usage_ids: ["a"] }; + + const returned = await tracker.track("scar_usage", "batch", async () => value); + + expect(returned).toBe(value); + expect(statsFor(tracker, "scar_usage")).toMatchObject({ attempted: 1, succeeded: 1, failed: 0 }); + }); + + it("counts plain values (including undefined, null and numbers) as successes", async () => { + const tracker = new EffectTracker(); + await tracker.track("p", "undefined", async () => undefined); + await tracker.track("p", "null", async () => null); + await tracker.track("p", "number", async () => 0); + await tracker.track("p", "object-without-success", async () => ({ id: "x" })); + await tracker.track("p", "success-not-boolean-false", async () => ({ success: 0 })); + + expect(statsFor(tracker, "p")).toMatchObject({ attempted: 5, succeeded: 5, failed: 0 }); + expect(tracker.getHealthReport().recentFailures).toEqual([]); + }); + + it("counts a rejection as a failure and resolves undefined without rethrowing", async () => { + const tracker = new EffectTracker(); + + const returned = await tracker.track("triple_write", "thread_creation", async () => { + throw new Error("Supabase upsert error: 400 - invalid input syntax for type uuid"); + }); + + expect(returned).toBeUndefined(); + expect(statsFor(tracker, "triple_write")).toMatchObject({ attempted: 1, succeeded: 0, failed: 1 }); + expect(tracker.getHealthReport().recentFailures[0].error).toBe( + "Supabase upsert error: 400 - invalid input syntax for type uuid" + ); + }); + + it("formatSummary reports a { success: false } result as failed", async () => { + const tracker = new EffectTracker(); + await tracker.track("scar_usage", "session_close_batch", async () => ({ success: false, error: "404" })); + + const summary = tracker.formatSummary(); + expect(summary).toContain("scar_usage"); + expect(summary).toContain("0/1 succeeded (1 failed)"); + expect(summary).toContain("[scar_usage/session_close_batch] 404"); + }); +}); diff --git a/tests/unit/services/tracked-writers-surface-failures.test.ts b/tests/unit/services/tracked-writers-surface-failures.test.ts new file mode 100644 index 0000000..e3f5dd6 --- /dev/null +++ b/tests/unit/services/tracked-writers-surface-failures.test.ts @@ -0,0 +1,84 @@ +/** + * GIT-104: writers that run under getEffectTracker().track() must let the + * tracker see their failures. writeTriples and generateVariantsForScar used to + * catch every write error and resolve normally, so `health` counted them as + * successes. + */ + +import { describe, it, expect, vi } from "vitest"; + +// Implementations are reset between tests by the config (clearMocks/restoreMocks). +const directUpsert = vi.fn(); + +vi.mock("../../../src/services/supabase-client.js", () => ({ + directUpsert: (...args: unknown[]) => directUpsert(...args), + isConfigured: () => true, +})); +vi.mock("../../../src/services/tier.js", () => ({ + hasSupabase: () => true, + getTableName: (base: string) => `gitmem_${base}`, +})); +vi.mock("../../../src/services/license.js", () => ({ + getProConfig: () => ({}), +})); + +const { writeTriples } = await import("../../../src/services/triple-writer.js"); +const { generateVariantsForScar } = await import("../../../src/services/variant-generation.js"); +const { EffectTracker } = await import("../../../src/services/effect-tracker.js"); + +const candidate = (object: string) => ({ + subject: "Session: s1", + predicate: "created_thread", + object, + half_life_days: 30, + source_type: "thread", + source_id: "t-abc12345", + project: "test-project", + created_by: "cli", +}); + +describe("writeTriples", () => { + it("attempts every triple, then rejects if any failed", async () => { + directUpsert + .mockRejectedValueOnce(new Error("Supabase upsert error: 400 - invalid input syntax for type uuid")) + .mockResolvedValueOnce({ id: "ok" }); + + await expect(writeTriples([candidate("a"), candidate("b")])).rejects.toThrow( + "1/2 triples failed: Supabase upsert error: 400" + ); + expect(directUpsert).toHaveBeenCalledTimes(2); + }); + + it("resolves with the count when every triple is written", async () => { + directUpsert.mockResolvedValue({ id: "ok" }); + await expect(writeTriples([candidate("a"), candidate("b")])).resolves.toBe(2); + }); + + it("is recorded as a failure by the effect tracker", async () => { + directUpsert.mockImplementation(async () => { throw new Error("400"); }); + const tracker = new EffectTracker(); + + await tracker.track("triple_write", "thread_creation", () => writeTriples([candidate("a")])); + + expect(tracker.getHealthReport().byPath.triple_write).toMatchObject({ succeeded: 0, failed: 1 }); + }); +}); + +describe("generateVariantsForScar", () => { + const scar = { id: "scar-1", title: "t", description: "d" }; + + it("rejects when a variant insert fails", async () => { + directUpsert + .mockResolvedValueOnce({ id: "v1" }) + .mockRejectedValueOnce(new Error("column scar_enforcement_variants.active does not exist")); + + await expect(generateVariantsForScar(scar)).rejects.toThrow( + "created 1/2 variants: column scar_enforcement_variants.active does not exist" + ); + }); + + it("resolves when both variants are written", async () => { + directUpsert.mockResolvedValue({ id: "v" }); + await expect(generateVariantsForScar(scar)).resolves.toBeUndefined(); + }); +});