diff --git a/AGENTS.md b/AGENTS.md index f424528..5403ce8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,7 +28,7 @@ Top-level directories, by purpose (one line each — not a file enumeration): - `agent-skills/` — canonical Agent Skills (SKILL.md, one per skill dir) installed to `~/.claude/skills` and `~/.omp/agent/skills`, discovered by Pi through the root package manifest, and generated into native host plugin payloads — the single `recall-*` command surface (the former `/Recall:*` slash commands, #228) - `plugins/` — native host plugin bundles, one per host: Codex in `plugins/recall/`, Claude Code in `plugins/recall-claude/`, each packaging MCP plus its own skill payload - `docs/` — user-facing published docs + ADRs (`docs/adr/`) + agent skill docs (`docs/agents/`) -- `lib/` — shared bash for the install / update / uninstall lifecycle scripts +- `lib/` — shared bash for the install / update / uninstall lifecycle scripts, plus the dependency-free `jsonc-mcp.ts` runtime helper they shell out to for JSONC config edits - `opencode/` — OpenCode host integration (plugins / hooks / guide) - `pi/` — Pi package extensions for native lifecycle capture and memory injection - `scripts/` — dev / CI helper scripts (version check, e2e) @@ -177,7 +177,7 @@ Child AGENTS.md files own domain-specific local rules. Read the applicable one b - [`agent-skills/AGENTS.md`](agent-skills/AGENTS.md) — `recall-*` Agent Skill definitions - [`plugins/AGENTS.md`](plugins/AGENTS.md) — per-host native plugin manifests, MCP registration, and generated skill payloads -Owned at root (no child doc): lifecycle scripts (`install.sh`, `update.sh`, `uninstall.sh`) + their shared `lib/install-lib.sh`; platform guides (`FOR_CLAUDE.md`, `FOR_OPENCODE.md`, `FOR_PI.md`); `CONTEXT.md`; and `assets/` (banner + VHS demo tapes/gifs). +Owned at root (no child doc): lifecycle scripts (`install.sh`, `update.sh`, `uninstall.sh`) + their shared `lib/install-lib.sh` and `lib/jsonc-mcp.ts`; platform guides (`FOR_CLAUDE.md`, `FOR_OPENCODE.md`, `FOR_PI.md`); `CONTEXT.md`; and `assets/` (banner + VHS demo tapes/gifs). ## Maintaining this file diff --git a/bun.lock b/bun.lock index 9b0e68f..df4314a 100644 --- a/bun.lock +++ b/bun.lock @@ -7,7 +7,6 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.25.3", "commander": "^12.1.0", - "jsonc-parser": "^3.3.1", "sqlite-vec": "0.1.9", "zod": "^3.24.0", }, @@ -296,8 +295,6 @@ "json-schema-typed": ["json-schema-typed@8.0.2", "", {}, "sha512-fQhoXdcvc3V28x7C7BMs4P5+kNlgUURe2jmUT1T//oBRMDrqy1QPelJimwZGo7Hg9VPV3EQV5Bnq4hbFy2vetA=="], - "jsonc-parser": ["jsonc-parser@3.3.1", "", {}, "sha512-HUgH65KyejrUFPvHFPbqOY0rsFip3Bo5wb4ngvdi1EpCYWUQDC5V+Y7mZws+DLkr4M//zQJoanu1SP+87Dv1oQ=="], - "lilconfig": ["lilconfig@3.1.3", "", {}, "sha512-/vlFKAoH5Cgt3Ie+JLhRbwOsCQePABiU3tJ1egGvyQ+33R/vcwM2Zl2QR/LzjsBeItPt3oSVXapn+m4nQDvpzw=="], "lines-and-columns": ["lines-and-columns@1.2.4", "", {}, "sha512-7ylylesZQ/PV29jhEDl3Ufjo6ZX7gCqJr5F7PKrqc93v7fzSymt1BpwEU8nAUXs8qzzvqhbjhK5QZg6Mt/HkBg=="], diff --git a/docs/OPENCODE_INTEGRATION.md b/docs/OPENCODE_INTEGRATION.md index 0c1d250..f19bb69 100644 --- a/docs/OPENCODE_INTEGRATION.md +++ b/docs/OPENCODE_INTEGRATION.md @@ -235,7 +235,7 @@ register_opencode_mcp() { local resolved_db_path resolved_db_path="$(eval echo "$RECALL_DB_PATH_DEFAULT")" # Resolve ~ to absolute - # Use bun -e with jsonc-parser for a surgical JSONC-safe merge. + # Use the bundled dependency-free JSONC helper for a surgical merge. local mem_mcp_path mem_mcp_path="$(which recall-mcp 2>/dev/null || echo "$HOME/.bun/bin/recall-mcp")" @@ -264,9 +264,11 @@ install_opencode_guide() { ``` The live installer delegates to `lib/install-lib.sh`'s shared -`_recall_jsonc_merge_mcp_entry` helper. It preserves comments, trailing commas, -sibling MCP entries, and custom Recall entry fields; malformed or unwritable -configs fail before writing. Uninstall uses the matching surgical removal helper. +`_recall_jsonc_merge_mcp_entry` helper and the bundled dependency-free +`lib/jsonc-mcp.ts` parser. It preserves comments, trailing commas, sibling MCP +entries, and custom Recall entry fields; malformed or unwritable configs fail +before writing. Uninstall uses the matching surgical removal helper, leaves a +malformed OpenCode config unchanged, and continues the remaining integrations. ## Database Schema Change @@ -364,13 +366,14 @@ OpenCode prefixes MCP tools with the server name + underscore: | Tilde in RECALL_DB_PATH | MEDIUM | Absolute path resolved at install | | `recall import --source` doesn't exist | LOW-MEDIUM | Eliminated — uses drop dir + RecallBatchExtract | | Bun not guaranteed | LOW-MEDIUM | Documented as requirement (same as Claude Code) | -| JSONC parsing | LOW | `jsonc-parser` surgical merge/removal preserves comments and refuses malformed writes | +| JSONC parsing | LOW | Bundled dependency-free parser performs surgical merge/removal, preserves comments, and refuses malformed writes | ## Phase 4 Evidence 1. OpenCode 1.18.4 emits `session.idle` through the `event` hook with - `properties.sessionID`; the e2e invokes that exact payload and verifies the - resulting markdown drop. + `properties.sessionID`; the e2e verifies the plugin factory exposes the real + `event` boundary, rejects the obsolete `session.idle` key, invokes that exact + payload, and verifies the resulting markdown drop. 2. `opencode export ` is the supported JSON export command. Recall owns JSON-to-markdown normalization because the CLI does not expose the old `-f markdown -o` session-export interface. diff --git a/docs/architecture.md b/docs/architecture.md index 84032af..21f16de 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -212,7 +212,8 @@ The current Claude lifecycle adapter tries Claude Haiku first and falls back to - **File permissions** set to 0600 (owner read/write only) - **Chunked extraction** for sessions >120K characters with meta-extraction merging - **Quality gate** rejects extractions missing required sections -- **Retry window** of 24 hours for failed extractions +- **Persistence check** marks a quality-passing extraction failed rather than extracted when its SQLite dual-write fails (for example an unwritable or locked database), so a session is never recorded as complete when some of its records did not land +- **Retry window** of 24 hours for failed extractions (quality-gate, extraction, and persistence failures alike) - **Parameterized queries** — no SQL injection vectors - **PRAGMA user_version** migration system for schema upgrades diff --git a/docs/installation.md b/docs/installation.md index 97a3dbf..003c257 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -276,7 +276,7 @@ cd /path/to/Recall - `~/.claude/hooks/lib/{extraction-*,pid-utils}.ts` — only Recall-owned files, never the whole `hooks/lib/` directory - The `## MEMORY` section in `~/.claude/CLAUDE.md` only if Recall generated it (current ownership marker or a normalized exact match of the complete legacy-generated body); unmarked customized/externally owned sections and the rest of `CLAUDE.md` are preserved; a marked section remains Recall-owned even if its body was edited - `~/.claude/MEMORY/extract_prompt.md` — only if unmodified from source; user-edited versions are preserved -- OpenCode MCP entry + plugins + agent (unless `--skip-opencode`) +- OpenCode MCP entry + plugins + agent + guide (unless `--skip-opencode`). An `opencode.json` that Recall cannot parse is reported and left untouched; the plugins, agent, and guide are still removed and the rest of the uninstall continues - Recall's native Pi package registration, owned Pi MCP entry, guide link, and Recall-generated `AGENTS.md` MEMORY section (current marker or normalized exact legacy Pi body); legacy Recall extension/skill links are removed, while unrelated Pi packages and `pi-mcp-adapter` remain (unless `--skip-pi`) - `bun unlink` (removes `recall` and `recall-mcp` from your PATH) diff --git a/hooks/AGENTS.md b/hooks/AGENTS.md index 7dad467..a0aae3c 100644 --- a/hooks/AGENTS.md +++ b/hooks/AGENTS.md @@ -25,6 +25,7 @@ Installed as per-file symlinks into `~/.claude/hooks/` from `~/.agents/Recall/sh - Generic hook helpers depend on `lib/events.ts`, `lib/extraction-provider.ts`, and the native-provider registry in `lib/hosts/`; native payloads, path encoding, commands, auth, and recursion guards stay in a host adapter. - Documented DRY exception: small utilities (e.g. bun-path resolution) are intentionally duplicated inside `RecallExtract.ts` / `RecallBatchExtract.ts` so they never reach into `src/`. Do not "DRY this up." - DB-path resolution is centralized in `lib/db-path.ts` — the CLI and every hook agree through it. +- Extraction dual-write is REPLAYED (archive crash or partial SQLite failure both leave the conversation retryable), so `dualWriteToSqlite` passes `skipDuplicates` to the plain-INSERT writers in `lib/sqlite-writers.ts`: a row already present for the same session, keyed on (session_id, content), is skipped. Content-scoped, never session-scoped: in-session windows write many different rows under one session_id. The flag is opt-in: the correction writer must keep recording repeated identical corrections. - Shebang `#!/usr/bin/env bun`. No build step — editing the canonical file updates the live symlink. - Hook registration (`Stop`, `SessionStart`, `PreCompact`, `PostToolUse`, `UserPromptSubmit`) lives in `~/.claude/settings.json`; the installer wires it. diff --git a/hooks/RecallBatchExtract.ts b/hooks/RecallBatchExtract.ts index ba83150..24b9aaf 100644 --- a/hooks/RecallBatchExtract.ts +++ b/hooks/RecallBatchExtract.ts @@ -74,6 +74,10 @@ export interface TrackerEntry { export type Tracker = Record; +export function isExtractionFailureOutput(output: string): boolean { + return /quality gate failed|all extraction methods failed|\[FabricExtract\] (?:markdown )?extraction failed|sqlite write failed|persistence failed/i.test(output); +} + // Parse args (top-level so main() and re-entrant tests share) const args = process.argv.slice(2); const dryRun = args.includes('--dry-run'); @@ -327,7 +331,7 @@ function extractFile(convPath: string, cwd: string): boolean { throw result.error ?? new Error(`RecallExtract exited with status ${result.status}`); } // Check if quality gate failed - if (output.includes('QUALITY GATE FAILED') || output.includes('All extraction methods failed') || output.includes('Extraction failed')) { + if (isExtractionFailureOutput(output)) { log(` QUALITY GATE FAILED or extraction failed for ${convPath}`); return false; } @@ -336,7 +340,7 @@ function extractFile(convPath: string, cwd: string): boolean { // execSync throws on non-zero exit, but RecallExtract exits 0 even on failure // Check stderr/stdout for quality gate failure const output = err.stdout || err.stderr || err.message || ''; - if (output.includes('QUALITY GATE FAILED') || output.includes('All extraction methods failed')) { + if (isExtractionFailureOutput(output)) { log(` QUALITY GATE FAILED for ${convPath}`); return false; } @@ -392,12 +396,17 @@ async function main() { const success = extractFile(candidate.path, cwd); if (success) { - extracted++; // RecallExtract's own markAsExtracted already updates the tracker. // Belt-and-suspenders: stamp it again here so dedup stays consistent // even if the child process exited before reaching its own writer. - recordSuccess(dbPath, candidate.path, candidate.size); - log(` SUCCESS: Extracted and tracked`); + if (recordSuccess(dbPath, candidate.path, candidate.size)) { + extracted++; + log(` SUCCESS: Extracted and tracked`); + } else { + failed++; + recordFailure(dbPath, candidate.path, candidate.size, 'RecallBatchExtract: tracker write failure'); + log(` FAILED: Extraction completed but tracking failed; will retry after 24h cooldown`); + } } else { failed++; recordFailure(dbPath, candidate.path, candidate.size, 'RecallBatchExtract: quality gate or runtime failure'); diff --git a/hooks/RecallExtract.ts b/hooks/RecallExtract.ts index b41bcd1..4cbafef 100644 --- a/hooks/RecallExtract.ts +++ b/hooks/RecallExtract.ts @@ -614,7 +614,14 @@ async function extractAndAppend(conversationPath: string, cwd: string): Promise< markAsFailed(convHash, `quality gate failed: ${core.quality?.reason}`); return; } - logExtract("QUALITY GATE PASSED: extraction contains required sections"); + const persistenceFailed = core.outcome === 'persistence_failed'; + if (persistenceFailed) { + const failures = JSON.stringify(core.dualWrite?.failures ?? {}); + console.error(`[FabricExtract] SQLite write failed: ${failures}`); + logExtract(`FAILURE: SQLite write failed: ${failures}`); + } else { + logExtract("QUALITY GATE PASSED: extraction contains required sections"); + } // Scrub the extracted text BEFORE it reaches any on-disk archive writer // (#132): the SQLite path already scrubs inside runExtractCore, but the @@ -677,6 +684,11 @@ async function extractAndAppend(conversationPath: string, cwd: string): Promise< appendErrors(extracted, sessionLabel, timestamp); // 6. Mark as extracted (dedup) + if (persistenceFailed) { + const failures = JSON.stringify(core.dualWrite?.failures ?? {}); + markAsFailed(convHash, `SQLite write failed: ${failures}`); + return; + } markAsExtracted(convHash); logExtract(`SUCCESS: All memory files updated for session=${sessionLabel}`); @@ -768,7 +780,14 @@ async function extractAndAppendMarkdown(mdPath: string, cwd: string): Promise r.text), }); + if (Object.keys(dualWrite.failures).length > 0) { + return { + outcome: 'persistence_failed', + quality, + extracted, + topics, + summary, + redactions, + threats, + dualWrite, + }; + } + return { outcome: 'extracted', quality, diff --git a/hooks/lib/extraction-parsers.ts b/hooks/lib/extraction-parsers.ts index 6ec8f8e..61c52be 100644 --- a/hooks/lib/extraction-parsers.ts +++ b/hooks/lib/extraction-parsers.ts @@ -15,8 +15,12 @@ import { writeExtractionSession, writeLearningsBatch, writeLoaEntryFromExtraction, + type WriteOptions, } from './sqlite-writers'; +/** This seam is replayed on retry: every plain-INSERT writer skips rows it already wrote. */ +const RETRY_SAFE: WriteOptions = { skipDuplicates: true }; + export interface DualWriteContext { sessionId: string; sessionLabel: string; @@ -139,6 +143,11 @@ export function parseErrorPatternItems(extracted: string): ExtractionErrorInput[ * Dual-write extracted content into SQLite. Every section is wrapped in its * own try/catch — a failure on one writer must not block the others, and the * whole function must not throw. + * + * The plain-INSERT writers run with `skipDuplicates` because this seam is + * replayed: the Stop hook marks a conversation extracted only after its markdown + * archive writes, and a partial SQLite failure marks it failed + retryable. See + * the RETRY IDEMPOTENCY note in sqlite-writers.ts. */ export function dualWriteToSqlite( dbPath: string, @@ -155,7 +164,7 @@ export function dualWriteToSqlite( }; if (!ensureDbWritable(dbPath)) { - result.failures._db = 'not writable'; + result.failures._db = 'not writable or locked'; return result; } @@ -174,13 +183,21 @@ export function dualWriteToSqlite( } try { - result.decisions = writeDecisionsBatch(dbPath, parseDecisionItems(ctx.extracted, ctx)); + result.decisions = writeDecisionsBatch( + dbPath, + parseDecisionItems(ctx.extracted, ctx), + RETRY_SAFE + ); } catch (e: any) { result.failures.decisions = e?.message || String(e); } try { - result.learnings = writeLearningsBatch(dbPath, parseLearningItems(ctx.extracted, ctx)); + result.learnings = writeLearningsBatch( + dbPath, + parseLearningItems(ctx.extracted, ctx), + RETRY_SAFE + ); } catch (e: any) { result.failures.learnings = e?.message || String(e); } @@ -188,7 +205,8 @@ export function dualWriteToSqlite( try { result.breadcrumbs = writeBreadcrumbsBatch( dbPath, - parseBreadcrumbItems(ctx.extracted, ctx) + parseBreadcrumbItems(ctx.extracted, ctx), + RETRY_SAFE ); } catch (e: any) { result.failures.breadcrumbs = e?.message || String(e); @@ -201,15 +219,19 @@ export function dualWriteToSqlite( } try { - const loaId = writeLoaEntryFromExtraction(dbPath, { - title: `${ctx.sessionLabel} — ${ctx.timestamp}`, - description: ctx.summary, - fabricExtract: ctx.extracted, - sessionId: ctx.sessionId, - project: ctx.project, - tags: ctx.topics.join(','), - messageCount: ctx.messageCount ?? null, - }); + const loaId = writeLoaEntryFromExtraction( + dbPath, + { + title: `${ctx.sessionLabel} — ${ctx.timestamp}`, + description: ctx.summary, + fabricExtract: ctx.extracted, + sessionId: ctx.sessionId, + project: ctx.project, + tags: ctx.topics.join(','), + messageCount: ctx.messageCount ?? null, + }, + RETRY_SAFE + ); result.loa = loaId ? 1 : 0; } catch (e: any) { result.failures.loa = e?.message || String(e); diff --git a/hooks/lib/insession.ts b/hooks/lib/insession.ts index 34c11ae..c7667d6 100644 --- a/hooks/lib/insession.ts +++ b/hooks/lib/insession.ts @@ -380,7 +380,8 @@ export type ExtractionOutcome = | 'no_transcript' | 'empty_slice' | 'extraction_failed' - | 'quality_failed'; + | 'quality_failed' + | 'persistence_failed'; export interface ExtractionResult { ran: boolean; @@ -463,10 +464,10 @@ export async function runInSessionExtraction( }; } - // extraction_failed | quality_failed — consume the window but keep the cursor + // extraction_failed | quality_failed | persistence_failed — consume the window but keep the cursor // so the same slice (plus new content) is retried next time. No data loss. resetWindow(dbPath, params.sessionId); - return { ran: false, outcome: core.outcome }; + return { ran: false, outcome: core.outcome, dualWrite: core.dualWrite }; } finally { childReleaseLock(dbPath, params.convPath, deps.pid); } diff --git a/hooks/lib/sqlite-writers.ts b/hooks/lib/sqlite-writers.ts index 0ada0c9..d1d4dc0 100644 --- a/hooks/lib/sqlite-writers.ts +++ b/hooks/lib/sqlite-writers.ts @@ -6,6 +6,25 @@ // All functions take dbPath as the first parameter for testability. // Every write is wrapped at the call site in try/catch — failures must not // block the legacy file path or cause the extraction to be marked failed. +// +// RETRY IDEMPOTENCY (`skipDuplicates`): the Stop hook only marks a conversation +// extracted AFTER its markdown archive writes, and a partial SQLite failure now +// marks it failed + retryable, so the same extraction can reach these writers +// more than once. The four plain-INSERT writers below therefore accept an opt-in +// `skipDuplicates` that drops a row already present for the same session, keyed +// on (session_id, content). Content-scoped, NOT session-scoped: in-session +// extraction (insession.ts) writes many DIFFERENT rows under one session_id +// across successive transcript slices, so anything that replaced or deleted by +// session_id would destroy earlier windows. A partially-failed dual-write still +// repairs on retry: the writer that landed skips, the writer that threw inserts. +// +// Opt-in, not default, because only the extraction dual-write is replayed: +// insession.ts's correction writer legitimately records the same correction text +// twice in one session (tests/hooks/corrections.test.ts) and must not dedup. +// +// TRADEOFF: the decision dedup key is the decision text alone, so two slices +// yielding the same decision at different confidence collapse to the first one +// written. Accepted: not duplicating beats upgrading confidence. import { Database } from 'bun:sqlite'; import { existsSync } from 'fs'; @@ -15,6 +34,11 @@ import { resolveDbPath } from './db-path'; // RecallBatchExtract, RecallClearExtract) don't need to change their imports. export const getDbPath = resolveDbPath; +export interface WriteOptions { + /** Skip a row already persisted for this session (extraction-retry replay). */ + skipDuplicates?: boolean; +} + function openDb(dbPath: string): Database { const db = new Database(dbPath); db.exec('PRAGMA journal_mode = WAL'); @@ -112,7 +136,11 @@ export interface DecisionInput { importance?: number; } -export function writeDecisionsBatch(dbPath: string, items: DecisionInput[]): number { +export function writeDecisionsBatch( + dbPath: string, + items: DecisionInput[], + opts: WriteOptions = {} +): number { if (items.length === 0) return 0; const db = openDb(dbPath); try { @@ -125,9 +153,13 @@ export function writeDecisionsBatch(dbPath: string, items: DecisionInput[]): num : `INSERT INTO decisions (session_id, category, project, decision, status, importance${provenance.col}) VALUES (?, ?, ?, ?, 'active', ?${provenance.val})`; const stmt = db.prepare(sql); + const alreadyWritten = opts.skipDuplicates + ? db.prepare('SELECT 1 FROM decisions WHERE session_id IS ? AND decision = ? LIMIT 1') + : null; const insertMany = db.transaction((batch: DecisionInput[]) => { let n = 0; for (const it of batch) { + if (alreadyWritten?.get(it.sessionId ?? null, it.decision)) continue; const importance = clampImportance(it.importance, 5); if (hasConfidence) { stmt.run( @@ -173,7 +205,11 @@ export interface LearningInput { confidence?: 'high' | 'medium' | 'low'; } -export function writeLearningsBatch(dbPath: string, items: LearningInput[]): number { +export function writeLearningsBatch( + dbPath: string, + items: LearningInput[], + opts: WriteOptions = {} +): number { if (items.length === 0) return 0; const db = openDb(dbPath); try { @@ -186,9 +222,15 @@ export function writeLearningsBatch(dbPath: string, items: LearningInput[]): num : `INSERT INTO learnings (session_id, category, project, problem, solution, prevention, tags, importance${provenance.col}) VALUES (?, ?, ?, ?, ?, ?, ?, ?${provenance.val})`; const stmt = db.prepare(sql); + const alreadyWritten = opts.skipDuplicates + ? db.prepare( + 'SELECT 1 FROM learnings WHERE session_id IS ? AND problem = ? AND solution IS ? LIMIT 1' + ) + : null; const insertMany = db.transaction((batch: LearningInput[]) => { let n = 0; for (const it of batch) { + if (alreadyWritten?.get(it.sessionId ?? null, it.problem, it.solution ?? null)) continue; const importance = clampImportance(it.importance, 5); if (hasConfidence) { stmt.run( @@ -237,7 +279,11 @@ export interface BreadcrumbInput { expiresAt?: string | null; } -export function writeBreadcrumbsBatch(dbPath: string, items: BreadcrumbInput[]): number { +export function writeBreadcrumbsBatch( + dbPath: string, + items: BreadcrumbInput[], + opts: WriteOptions = {} +): number { if (items.length === 0) return 0; const db = openDb(dbPath); try { @@ -247,9 +293,13 @@ export function writeBreadcrumbsBatch(dbPath: string, items: BreadcrumbInput[]): `INSERT INTO breadcrumbs (session_id, content, category, project, importance, expires_at${provenance.col}) VALUES (?, ?, ?, ?, ?, ?${provenance.val})` ); + const alreadyWritten = opts.skipDuplicates + ? db.prepare('SELECT 1 FROM breadcrumbs WHERE session_id IS ? AND content = ? LIMIT 1') + : null; const insertMany = db.transaction((batch: BreadcrumbInput[]) => { let n = 0; for (const it of batch) { + if (alreadyWritten?.get(it.sessionId ?? null, it.content)) continue; stmt.run( it.sessionId ?? null, it.content, @@ -283,10 +333,22 @@ export interface LoaInput { importance?: number; } -export function writeLoaEntryFromExtraction(dbPath: string, entry: LoaInput): number { +export function writeLoaEntryFromExtraction( + dbPath: string, + entry: LoaInput, + opts: WriteOptions = {} +): number { const db = openDb(dbPath); try { if (!tableExists(db, 'loa_entries')) return 0; + // Retry of an already-persisted extraction: return the existing id rather + // than inserting a second copy of the same fabric extract. + if (opts.skipDuplicates) { + const existing = db + .prepare('SELECT id FROM loa_entries WHERE session_id IS ? AND fabric_extract = ? LIMIT 1') + .get(entry.sessionId ?? null, entry.fabricExtract) as { id: number } | undefined; + if (existing) return existing.id; + } // LoA importance is floored at 5 (curated tier guardrail). const importance = Math.max(5, clampImportance(entry.importance, 8)); const provenance = provenanceFragment(db, 'loa_entries'); diff --git a/lib/install-lib.sh b/lib/install-lib.sh index f2a44eb..b44867e 100644 --- a/lib/install-lib.sh +++ b/lib/install-lib.sh @@ -2229,104 +2229,9 @@ Read and follow the canonical Recall guide at \`$CLAUDE_DIR/Recall_GUIDE.md\`. U # names); every other key on the entry and every other env var survive. # V-4 PARENT_KEY present but not a plain object → refuse, file unchanged. _recall_jsonc_merge_mcp_entry() { - CONFIG_PATH="$1" PARENT_KEY="$2" ENTRY_JSON="$3" PRESERVE_KEYS="${4:-}" REPO_DIR="$RECALL_REPO_DIR" \ - bun -e ' - const fs = require("fs"); - const path = require("path"); - const { parse, modify, applyEdits } = require(process.env.REPO_DIR + "/node_modules/jsonc-parser"); - const file = process.env.CONFIG_PATH; - const parentKey = process.env.PARENT_KEY; - const preserveKeys = new Set((process.env.PRESERVE_KEYS || "").split(",").filter(Boolean)); - const name = path.basename(file); - const entry = JSON.parse(process.env.ENTRY_JSON); - - // Read once. Empty file is treated as {} so first-time installs work. - // We keep the original text for modify/applyEdits so surrounding bytes - // (// comments, /* */ blocks, JSON5 trailing commas, sibling MCP - // entries) survive byte-for-byte on the write path. - let text = fs.existsSync(file) ? fs.readFileSync(file, "utf-8") : "{}"; - if (text.trim() === "") text = "{}"; - - // V-1: refuse on unparseable input instead of throwing-then-reporting - // success. We exit non-zero BEFORE any write, so the file is untouched. - // Uses jsonc-parser so // line comments, /* */ block comments, and - // JSON5 trailing commas are tolerated (allowTrailingCommas). The earlier - // regex stripper corrupted `//` inside string values (e.g. https:// URLs - // in sibling MCP entries) — A7 — and is replaced by a real tokenizer. - const errors = []; - const existing = parse(text, errors, { allowTrailingComma: true }); - if (errors.length > 0 || existing === undefined || existing === null || typeof existing !== "object" || Array.isArray(existing)) { - console.error("recall: refusing to modify " + name + " — existing file is not valid JSON/JSONC"); - process.exit(1); - } - - // V-4: the container exists but is not a plain object (e.g. - // "mcp": "disabled"). Refuse rather than silently rewrite. - if (Object.prototype.hasOwnProperty.call(existing, parentKey)) { - const container = existing[parentKey]; - if (container === null || typeof container !== "object" || Array.isArray(container)) { - console.error("recall: refusing to modify " + name + " — \"" + parentKey + "\" exists but is not an object"); - process.exit(1); - } - } + bun run "$RECALL_REPO_DIR/lib/jsonc-mcp.ts" merge "$1" "$2" "$3" "${4:-}" + return $? - // V-3: deep-merge. We own only the keys named in ENTRY_JSON and the env - // vars it lists; every other key the user set on the entry — and every - // other environment var — survives a refresh. - const container = existing[parentKey] || {}; - const prevRaw = container["recall-memory"]; - const prev = (prevRaw && typeof prevRaw === "object" && !Array.isArray(prevRaw)) ? prevRaw : {}; - const mergedEntry = { - ...prev, - ...entry - }; - for (const key of preserveKeys) { - if (Object.prototype.hasOwnProperty.call(prev, key)) mergedEntry[key] = prev[key]; - } - // Claude/OpenCode use `environment`; pi-mcp-adapter uses `env`. - // Deep-merge whichever owned field the caller supplied so custom sibling - // variables survive without inventing one normalized cross-host schema. - for (const envKey of ["environment", "env"]) { - if (!Object.prototype.hasOwnProperty.call(entry, envKey)) continue; - const prevEnv = (prev[envKey] && typeof prev[envKey] === "object" && !Array.isArray(prev[envKey])) ? prev[envKey] : {}; - const newEnv = (entry[envKey] && typeof entry[envKey] === "object" && !Array.isArray(entry[envKey])) ? entry[envKey] : {}; - mergedEntry[envKey] = { ...prevEnv, ...newEnv }; - } - - // In-place edit via jsonc-parser modify/applyEdits: only the - // recall-memory entry'"'"'s value slot is rewritten. Surrounding bytes - // (comments, JSON5 trailing commas, sibling MCP entry formatting, - // top-of-file leading comments) survive byte-for-byte. The Cycle 1 - // preservation tests in tests/{opencode,pi}-integration.test.ts pin - // this contract. - const hasContainer = Object.prototype.hasOwnProperty.call(existing, parentKey); - const editPath = hasContainer ? [parentKey, "recall-memory"] : [parentKey]; - const editValue = hasContainer ? mergedEntry : { "recall-memory": mergedEntry }; - const edits = modify(text, editPath, editValue, { - formattingOptions: { tabSize: 2, insertSpaces: true } - }); - const newText = applyEdits(text, edits); - - // jsonc-parser cannot materialize a missing intermediate object when - // asked to edit [parentKey, child]. Create the parent explicitly above, - // then reject any no-op so a fresh install can never print false success. - if (!hasContainer && (edits.length === 0 || newText === text)) { - console.error("recall: failed to construct " + name + " — merge produced no edit"); - process.exit(1); - } - - // V-8: a write failure (read-only file/dir, ENOSPC) must surface as a - // non-zero exit. `bun -e` swallows an uncaught synchronous writeFileSync - // EACCES (exits 0, prints nothing), so without this guard the caller - // would print "✓ Registered" while the file was never modified — a silent - // failure. Catch explicitly and exit non-zero, like the V-1 parse guard. - try { - fs.writeFileSync(file, newText); - } catch (e) { - console.error("recall: failed to write " + name + " — " + e.message); - process.exit(1); - } - ' } # _recall_jsonc_remove_mcp_entry FILE PARENT_KEY @@ -2336,42 +2241,9 @@ _recall_jsonc_merge_mcp_entry() { # input is a hard failure before any write so uninstall cannot claim success # after damaging or ignoring a user config. _recall_jsonc_remove_mcp_entry() { - CONFIG_PATH="$1" PARENT_KEY="$2" REPO_DIR="$RECALL_REPO_DIR" \ - bun -e ' - const fs = require("fs"); - const { parse, modify, applyEdits } = require(process.env.REPO_DIR + "/node_modules/jsonc-parser"); - const file = process.env.CONFIG_PATH; - const parentKey = process.env.PARENT_KEY; - const text = fs.readFileSync(file, "utf-8"); - const errors = []; - const existing = parse(text, errors, { allowTrailingComma: true }); - - if (errors.length > 0 || existing === undefined || existing === null || typeof existing !== "object" || Array.isArray(existing)) { - console.error("recall: refusing to modify " + file + " — existing file is not valid JSON/JSONC"); - process.exit(1); - } + bun run "$RECALL_REPO_DIR/lib/jsonc-mcp.ts" remove "$1" "$2" + return $? - const container = existing[parentKey]; - if (container === undefined) process.exit(0); - if (container === null || typeof container !== "object" || Array.isArray(container)) { - console.error(`recall: refusing to modify ${file} — "${parentKey}" exists but is not an object`); - process.exit(1); - } - if (!Object.prototype.hasOwnProperty.call(container, "recall-memory")) process.exit(0); - - const edits = modify(text, [parentKey, "recall-memory"], undefined, { - formattingOptions: { tabSize: 2, insertSpaces: true } - }); - const newText = applyEdits(text, edits); - if (newText === text) process.exit(0); - - try { - fs.writeFileSync(file, newText); - } catch (e) { - console.error("recall: failed to write " + file + " — " + e.message); - process.exit(1); - } - ' } # ── OpenCode ───────────────────────────────────────────────────────────────── diff --git a/lib/jsonc-mcp.ts b/lib/jsonc-mcp.ts new file mode 100644 index 0000000..f9a74b7 --- /dev/null +++ b/lib/jsonc-mcp.ts @@ -0,0 +1,236 @@ +#!/usr/bin/env bun + +// Small, dependency-free JSONC editor for the lifecycle scripts. The published +// Recall package ships lib/ but not node_modules/, so installer/uninstaller +// config repair cannot require the repository's jsonc-parser installation. + +import { existsSync, readFileSync, writeFileSync } from 'fs'; + +type JsonObject = Record; +type Property = { key: string; keyStart: number; value: Node }; +type Node = { + start: number; + end: number; + value: unknown; + properties?: Property[]; + contentEnd?: number; + trailingComma?: boolean; +}; + +class JsoncParser { + private index = 0; + + constructor(private readonly text: string) {} + + parse(): Node { + this.skipSpaceAndComments(); + const root = this.value(); + this.skipSpaceAndComments(); + if (this.index !== this.text.length) throw new Error('trailing content'); + return root; + } + + private value(): Node { + this.skipSpaceAndComments(); + const start = this.index; + const char = this.text[this.index]; + if (char === '{') return this.object(); + if (char === '[') return this.array(); + if (char === '"') return this.string(); + + const match = this.text.slice(this.index).match(/^(true|false|null|-?(?:0|[1-9]\d*)(?:\.\d+)?(?:[eE][+-]?\d+)?)/); + if (!match) throw new Error(`expected value at ${this.index}`); + this.index += match[0].length; + return { start, end: this.index, value: JSON.parse(match[0]) }; + } + + private string(): Node { + const start = this.index; + this.index++; + let escaped = false; + while (this.index < this.text.length) { + const char = this.text[this.index++]; + if (escaped) { + escaped = false; + } else if (char === '\\') { + escaped = true; + } else if (char === '"') { + const raw = this.text.slice(start, this.index); + return { start, end: this.index, value: JSON.parse(raw) }; + } + } + throw new Error('unterminated string'); + } + + private object(): Node { + const start = this.index++; + const properties: Property[] = []; + const value: JsonObject = {}; + let contentEnd = this.index; + let trailingComma = false; + this.skipSpaceAndComments(); + while (this.text[this.index] !== '}') { + const keyNode = this.string(); + this.skipSpaceAndComments(); + if (this.text[this.index++] !== ':') throw new Error(`expected colon at ${this.index}`); + const child = this.value(); + properties.push({ key: keyNode.value as string, keyStart: keyNode.start, value: child }); + value[keyNode.value as string] = child.value; + contentEnd = child.end; + trailingComma = false; + this.skipSpaceAndComments(); + if (this.text[this.index] === ',') { + this.index++; + contentEnd = this.index; + trailingComma = true; + this.skipSpaceAndComments(); + continue; + } + if (this.text[this.index] !== '}') throw new Error(`expected comma at ${this.index}`); + } + this.index++; + return { start, end: this.index, value, properties, contentEnd, trailingComma }; + } + + private array(): Node { + const start = this.index++; + const value: unknown[] = []; + this.skipSpaceAndComments(); + while (this.text[this.index] !== ']') { + value.push(this.value().value); + this.skipSpaceAndComments(); + if (this.text[this.index] === ',') { + this.index++; + this.skipSpaceAndComments(); + continue; + } + if (this.text[this.index] !== ']') throw new Error(`expected comma at ${this.index}`); + } + this.index++; + return { start, end: this.index, value }; + } + + private skipSpaceAndComments(): void { + while (this.index < this.text.length) { + if (/\s/.test(this.text[this.index])) { + this.index++; + continue; + } + if (this.text.startsWith('//', this.index)) { + const end = this.text.indexOf('\n', this.index + 2); + this.index = end < 0 ? this.text.length : end + 1; + continue; + } + if (this.text.startsWith('/*', this.index)) { + const end = this.text.indexOf('*/', this.index + 2); + if (end < 0) throw new Error('unterminated comment'); + this.index = end + 2; + continue; + } + return; + } + } +} + +function parse(text: string): Node { + return new JsoncParser(text).parse(); +} + +function isObject(value: unknown): value is JsonObject { + return value !== null && typeof value === 'object' && !Array.isArray(value); +} + +function apply(text: string, start: number, end: number, replacement: string): string { + return text.slice(0, start) + replacement + text.slice(end); +} + +function lineIndent(text: string, position: number): string { + const lineStart = text.lastIndexOf('\n', position - 1) + 1; + return text.slice(lineStart, position).match(/^[ \t]*/)?.[0] ?? ''; +} + +function formatted(value: unknown, indent: string): string { + return JSON.stringify(value, null, 2).replace(/\n/g, `\n${indent}`); +} + +function insertProperty(text: string, object: Node, key: string, value: unknown): string { + const close = object.end - 1; + const anchor = object.contentEnd ?? close; + const keyIndent = object.properties?.[0] + ? lineIndent(text, object.properties[0].keyStart) + : lineIndent(text, object.start) + ' '; + const objectIndent = lineIndent(text, object.start); + const separator = object.properties?.length && !object.trailingComma ? ',' : ''; + const closing = text.slice(anchor, close).includes('\n') ? '' : `\n${objectIndent}`; + const insertion = `${separator}\n${keyIndent}"${key}": ${formatted(value, keyIndent)}${closing}`; + return apply(text, anchor, anchor, insertion); +} + +function merge(file: string, parentKey: string, entry: JsonObject, preserveKeys: string[]): void { + let text = existsSync(file) ? readFileSync(file, 'utf8') : '{}'; + if (text.trim() === '') text = '{}'; + const root = parse(text); + if (!isObject(root.value)) throw new Error('root is not an object'); + + const parent = root.properties?.find(property => property.key === parentKey); + if (!parent) { + writeFileSync(file, insertProperty(text, root, parentKey, { 'recall-memory': entry })); + return; + } + if (!isObject(parent.value.value)) throw new Error(`"${parentKey}" exists but is not an object`); + + const container = parent.value; + const current = container.properties?.find(property => property.key === 'recall-memory'); + const previous = current && isObject(current.value.value) ? current.value.value : {}; + const merged: JsonObject = { ...previous, ...entry }; + for (const key of ['environment', 'env']) { + if (isObject(entry[key])) merged[key] = { ...(isObject(previous[key]) ? previous[key] : {}), ...entry[key] }; + } + for (const key of preserveKeys) { + if (Object.prototype.hasOwnProperty.call(previous, key)) merged[key] = previous[key]; + } + + if (current) { + const indent = lineIndent(text, current.value.start); + writeFileSync(file, apply(text, current.value.start, current.value.end, formatted(merged, indent))); + } else { + writeFileSync(file, insertProperty(text, container, 'recall-memory', merged)); + } +} + +function remove(file: string, parentKey: string): void { + const text = readFileSync(file, 'utf8'); + const root = parse(text); + if (!isObject(root.value)) throw new Error('root is not an object'); + const parent = root.properties?.find(property => property.key === parentKey); + if (!parent) return; + if (!isObject(parent.value.value)) throw new Error(`"${parentKey}" exists but is not an object`); + const properties = parent.value.properties ?? []; + const currentIndex = properties.findIndex(property => property.key === 'recall-memory'); + if (currentIndex < 0) return; + const current = properties[currentIndex]; + if (properties.length === 1) { + writeFileSync(file, apply(text, current.keyStart, current.value.end, '')); + return; + } + if (currentIndex < properties.length - 1) { + writeFileSync(file, apply(text, current.keyStart, properties[currentIndex + 1].keyStart, '')); + } else { + writeFileSync(file, apply(text, properties[currentIndex - 1].value.end, current.value.end, '')); + } +} + +try { + const [, , action, file, parentKey, entryJson, preserveCsv] = process.argv; + if (action === 'merge') { + const entry = JSON.parse(entryJson) as JsonObject; + merge(file, parentKey, entry, (preserveCsv ?? '').split(',').filter(Boolean)); + } else if (action === 'remove') { + remove(file, parentKey); + } else { + throw new Error('usage: jsonc-mcp.ts merge|remove ...'); + } +} catch (error) { + console.error(`recall: JSONC operation failed — ${error instanceof Error ? error.message : String(error)}`); + process.exit(1); +} diff --git a/package.json b/package.json index 5f43f03..6901456 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,6 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.25.3", "commander": "^12.1.0", - "jsonc-parser": "^3.3.1", "sqlite-vec": "0.1.9", "zod": "^3.24.0" }, diff --git a/scripts/e2e-opencode.ts b/scripts/e2e-opencode.ts index 604f346..0cbd7af 100644 --- a/scripts/e2e-opencode.ts +++ b/scripts/e2e-opencode.ts @@ -261,6 +261,8 @@ async function main(): Promise { const plugin = await pluginModule.RecallExtract({ $: async (_strings: TemplateStringsArray, ...values: unknown[]) => runOpenCode(['export', String(values[0])], env), } as never); + assert(typeof plugin.event === 'function', 'Recall OpenCode plugin did not expose the real event hook boundary'); + assert(!('session.idle' in plugin), 'Recall OpenCode plugin exposed the obsolete session.idle hook instead of event'); await plugin.event?.({ event: { type: 'session.idle', properties: { sessionID: sessionId } } }); const dropDir = join(testRecallHome, 'MEMORY', 'opencode-sessions'); diff --git a/tests/hooks/RecallBatchExtract.test.ts b/tests/hooks/RecallBatchExtract.test.ts index 59f21d2..52c024a 100644 --- a/tests/hooks/RecallBatchExtract.test.ts +++ b/tests/hooks/RecallBatchExtract.test.ts @@ -10,7 +10,8 @@ import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; import { Database } from 'bun:sqlite'; -import { mkdtempSync, rmSync, readFileSync, existsSync } from 'fs'; +import { mkdirSync, mkdtempSync, rmSync, readFileSync, existsSync, writeFileSync } from 'fs'; +import { spawnSync } from 'child_process'; import { tmpdir } from 'os'; import { join } from 'path'; import { @@ -19,6 +20,7 @@ import { hasExtractionTracker, recordSuccess, recordFailure, + isExtractionFailureOutput, type Tracker, } from '../../hooks/RecallBatchExtract'; import { @@ -156,6 +158,74 @@ describe('findCandidates — SQLite tracker integration', () => { }); }); +describe('batch extraction result classification', () => { + test('recognizes lowercase markdown extraction failures as retryable', () => { + expect(isExtractionFailureOutput('[FabricExtract] Markdown extraction failed: provider unavailable')).toBe(true); + expect(isExtractionFailureOutput('[FabricExtract] QUALITY GATE PASSED (markdown)')).toBe(false); + }); + + test('does not classify SQLite persistence failure as extraction success', () => { + expect(isExtractionFailureOutput('[FabricExtract] SQLite write failed: {"sessions":"database is locked"}')).toBe(true); + }); + + test('does not classify a non-fatal provider cascade fallback as a failure', () => { + expect(isExtractionFailureOutput('[FabricExtract] Claude CLI extraction failed: spawn claude ENOENT')).toBe(false); + expect(isExtractionFailureOutput('[FabricExtract] Ollama extraction failed: fetch failed')).toBe(false); + expect(isExtractionFailureOutput('[FabricExtract] All extraction methods failed, no extraction')).toBe(true); + }); +}); + +// The run loop only counts a drop file as extracted when recordSuccess actually +// persisted the tracker row. Driven end-to-end through the script (the counters +// live in main()), with a stub RecallExtract standing in for a clean extraction. +describe('batch extraction success accounting', () => { + function stageBatchRun(): { home: string; recallHome: string; drop: string } { + const home = join(tmp, 'home'); + const recallHome = join(tmp, 'recall-home'); + mkdirSync(join(home, '.claude', 'projects'), { recursive: true }); + mkdirSync(join(recallHome, 'shared', 'hooks'), { recursive: true }); + mkdirSync(join(recallHome, 'MEMORY', 'opencode-sessions'), { recursive: true }); + writeFileSync(join(recallHome, 'shared', 'hooks', 'RecallExtract.ts'), 'process.exit(0);\n'); + const drop = join(recallHome, 'MEMORY', 'opencode-sessions', 'ses_accounting.md'); + writeFileSync(drop, `# OpenCode session\n\n${'Batch accounting fixture. '.repeat(200)}`); + return { home, recallHome, drop }; + } + + function runBatchScript(home: string, recallHome: string): string { + const result = spawnSync( + 'bun', + ['run', join(import.meta.dir, '..', '..', 'hooks', 'RecallBatchExtract.ts'), '--all'], + { + encoding: 'utf-8', + env: { ...process.env, HOME: home, RECALL_HOME: recallHome, RECALL_DB_PATH: dbPath }, + }, + ); + return `${result.stdout ?? ''}${result.stderr ?? ''}`; + } + + test('counts a clean extraction whose tracker row persisted as extracted', () => { + initTrackerDb(); + const { home, recallHome, drop } = stageBatchRun(); + + const output = runBatchScript(home, recallHome); + + expect(output).toContain('SUCCESS: Extracted and tracked'); + expect(output).toContain('1 extracted, 0 failed'); + expect(getExtractionRecord(dbPath, drop)!.extracted_at).not.toBeNull(); + }); + + test('does not count an extraction as extracted when the tracker write fails', () => { + initEmptyDb(); // no extraction_tracker table — every tracker write throws + const { home, recallHome } = stageBatchRun(); + + const output = runBatchScript(home, recallHome); + + expect(output).toContain('FAILED: Extraction completed but tracking failed'); + expect(output).not.toContain('SUCCESS: Extracted and tracked'); + expect(output).toContain('0 extracted, 1 failed'); + }); +}); + describe('Graceful degrade when extraction_tracker is missing', () => { test('(c) loadTracker on a DB without the table returns {} and logs', () => { initEmptyDb(); diff --git a/tests/hooks/extract-core.test.ts b/tests/hooks/extract-core.test.ts index 8f442e5..540e2eb 100644 --- a/tests/hooks/extract-core.test.ts +++ b/tests/hooks/extract-core.test.ts @@ -9,7 +9,8 @@ import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; import { Database } from 'bun:sqlite'; -import { readFileSync } from 'fs'; +import { mkdtempSync, readFileSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; import { join } from 'path'; import { setupTestDb, teardownTestDb } from '../helpers/setup'; import { runExtractCore, type ExtractCoreContext } from '../../hooks/lib/extract-core'; @@ -140,6 +141,35 @@ describe('runExtractCore — dual-write parity', () => { expect(result.summary).toBe('We migrated the Stop hook to write to SQLite.'); expect(result.topics).toEqual(['migration', 'sqlite']); }); + + test('surfaces a failed SQLite persistence as non-success', async () => { + const result = await runExtractCore('/path/that/does/not/exist.db', 'raw', BASE_CTX, { + extract: async () => CLEAN_FIXTURE, + deriveMeta, + }); + + expect(result.outcome).toBe('persistence_failed'); + expect(result.dualWrite?.failures._db).toBe('not writable or locked'); + }); + + test('surfaces an exclusively locked database instead of reporting an empty success', async () => { + const busyDir = mkdtempSync(join(tmpdir(), 'recall-busy-')); + const busyPath = join(busyDir, 'busy.db'); + const blocker = new Database(busyPath); + blocker.exec('PRAGMA journal_mode = DELETE; CREATE TABLE lock_probe (id INTEGER); BEGIN EXCLUSIVE'); + try { + const result = await runExtractCore(busyPath, 'raw', BASE_CTX, { + extract: async () => CLEAN_FIXTURE, + deriveMeta, + }); + expect(result.outcome).toBe('persistence_failed'); + expect(result.dualWrite?.failures._db).toContain('locked'); + } finally { + blocker.exec('ROLLBACK'); + blocker.close(); + rmSync(busyDir, { recursive: true, force: true }); + } + }); }); describe('runExtractCore — scrub is active on every persisted field', () => { diff --git a/tests/hooks/extraction-parsers.test.ts b/tests/hooks/extraction-parsers.test.ts index 5331ed9..42b53b0 100644 --- a/tests/hooks/extraction-parsers.test.ts +++ b/tests/hooks/extraction-parsers.test.ts @@ -32,6 +32,24 @@ We migrated the Stop hook to write to SQLite. ## SESSION CONTEXT A surgical migration of the Stop hook to use SQLite-native helpers.`; +// A later in-session window of the SAME session, with different content, so +// every row must still persist alongside the first slice's rows. +const SECOND_SLICE_FIXTURE = `## ONE SENTENCE SUMMARY +A later window covering the installer half of the session. + +## MAIN IDEAS +- Lifecycle scripts share lib/install-lib.sh and nothing else +- JSONC edits run through a dependency-free helper + +## DECISIONS MADE +- Drop the jsonc-parser runtime dependency (confidence: HIGH) + +## ERRORS FIXED +- Trailing comma on insert: rebuilt the JSONC edit offsets + +## SESSION CONTEXT +The installer half of the same session.`; + let dbPath: string; beforeEach(() => { @@ -132,7 +150,89 @@ describe('dualWriteToSqlite', () => { conversationPath: 'x', topics: [], summary: 'x', extracted: FABRIC_FIXTURE, }); - expect(result.failures._db).toBe('not writable'); + expect(result.failures._db).toBe('not writable or locked'); expect(result.sessions).toBe(0); }); }); + +// The Stop hook marks a conversation extracted only AFTER its markdown archive +// writes, and a partial SQLite failure marks it failed + retryable, so the same +// extraction can reach dualWriteToSqlite more than once. These lock the +// insert-if-absent guard in sqlite-writers.ts. +describe('dualWriteToSqlite retry idempotency', () => { + const ctx = { + sessionId: 'sess-retry', + sessionLabel: 'demo-session', + project: 'atlas-recall', + timestamp: '2026-05-17', + conversationPath: '/tmp/conv.jsonl', + topics: ['migration', 'sqlite'], + summary: 'one sentence summary', + extracted: FABRIC_FIXTURE, + }; + + function rowCounts() { + const db = openRead(); + const count = (t: string) => + (db.prepare(`SELECT COUNT(*) c FROM ${t}`).get() as any).c as number; + const counts = { + decisions: count('decisions'), + learnings: count('learnings'), + breadcrumbs: count('breadcrumbs'), + loa: count('loa_entries'), + }; + db.close(); + return counts; + } + + test('re-running an already-persisted extraction does not duplicate rows', () => { + dualWriteToSqlite(dbPath, ctx); + const afterFirst = rowCounts(); + + // Archive write crashed before markAsExtracted → the whole pipeline reruns. + const second = dualWriteToSqlite(dbPath, ctx); + + expect(Object.keys(second.failures)).toEqual([]); + expect(second.decisions).toBe(0); + expect(second.learnings).toBe(0); + expect(second.breadcrumbs).toBe(0); + expect(rowCounts()).toEqual(afterFirst); + expect(afterFirst).toEqual({ decisions: 3, learnings: 2, breadcrumbs: 3, loa: 1 }); + }); + + test('retry after a partial failure repairs the failed table without duplicating the rest', () => { + const blocker = openRead(); + blocker.exec( + `CREATE TRIGGER block_decisions BEFORE INSERT ON decisions + BEGIN SELECT RAISE(ABORT, 'decisions writer down'); END` + ); + blocker.close(); + + const first = dualWriteToSqlite(dbPath, ctx); + expect(first.failures.decisions).toBeDefined(); + expect(first.loa).toBe(1); + expect(rowCounts()).toEqual({ decisions: 0, learnings: 2, breadcrumbs: 3, loa: 1 }); + + const unblocker = openRead(); + unblocker.exec('DROP TRIGGER block_decisions'); + unblocker.close(); + + const second = dualWriteToSqlite(dbPath, ctx); + + expect(Object.keys(second.failures)).toEqual([]); + expect(second.decisions).toBe(3); + expect(rowCounts()).toEqual({ decisions: 3, learnings: 2, breadcrumbs: 3, loa: 1 }); + }); + + test('a different slice of the same session still persists (in-session windows)', () => { + dualWriteToSqlite(dbPath, ctx); + const second = dualWriteToSqlite(dbPath, { ...ctx, extracted: SECOND_SLICE_FIXTURE }); + + expect(Object.keys(second.failures)).toEqual([]); + expect(second.decisions).toBe(1); + expect(second.breadcrumbs).toBe(2); + expect(second.learnings).toBe(1); + expect(second.loa).toBe(1); + expect(rowCounts()).toEqual({ decisions: 4, learnings: 3, breadcrumbs: 5, loa: 2 }); + }); +}); diff --git a/tests/install/uninstall.test.ts b/tests/install/uninstall.test.ts index 92614d5..80e3b85 100644 --- a/tests/install/uninstall.test.ts +++ b/tests/install/uninstall.test.ts @@ -114,6 +114,39 @@ function runUninstallIncludingOpenCode( }; } +function runUninstallAll( + claudeDir: string, + backupBase: string, + opencodeConfigDir: string, + piConfigDir: string, + unlinkMarker: string, + fakeBin: string, +): RunResult { + const result = spawnSync( + 'bash', + [UNINSTALL, '--no-confirm', '--skip-omp'], + { + encoding: 'utf-8', + cwd: REPO, + env: { + ...process.env, + CLAUDE_DIR: claudeDir, + BACKUP_BASE: backupBase, + OPENCODE_CONFIG_DIR: opencodeConfigDir, + PI_CONFIG_DIR: piConfigDir, + HOME: claudeDir, + PATH: `${fakeBin}:${process.env.PATH ?? ''}`, + RECALL_TEST_UNLINK: unlinkMarker, + }, + }, + ); + return { + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + status: result.status ?? 1, + }; +} + describe('uninstall.sh', () => { let tempRoot: string; let claudeDir: string; @@ -515,6 +548,7 @@ Preserve this. expect(after).toContain('other-server'); expect(after).toContain('https://example.test/mcp'); expect(after).not.toContain('recall-memory'); + expect(`${result.stdout}${result.stderr}`).not.toContain('left unchanged'); }); test('OpenCode uninstall rejects malformed config without writing', () => { @@ -526,7 +560,36 @@ Preserve this. const result = runUninstallIncludingOpenCode(claudeDir, backupBase, opencodeConfigDir); - expect(result.status).not.toBe(0); + expect(result.status).toBe(0); expect(readFileSync(opencodeConfig, 'utf-8')).toBe(original); }); + + test('malformed OpenCode JSONC does not abort the remaining uninstall', () => { + const opencodeConfigDir = join(tempRoot, 'opencode-malformed-continuation'); + const piConfigDir = join(tempRoot, 'pi'); + const fakeBin = join(tempRoot, 'bin'); + const unlinkMarker = join(tempRoot, 'bun-unlink-called'); + mkdirSync(opencodeConfigDir, { recursive: true }); + mkdirSync(piConfigDir, { recursive: true }); + mkdirSync(fakeBin, { recursive: true }); + + const opencodeConfig = join(opencodeConfigDir, 'opencode.json'); + const malformed = '{ "mcp": { "recall-memory": { } }'; + writeFileSync(opencodeConfig, malformed); + const piConfig = join(piConfigDir, 'mcp.json'); + writeFileSync(piConfig, JSON.stringify({ mcpServers: { 'recall-memory': { command: 'recall-mcp' } } })); + writeFileSync( + join(fakeBin, 'bun'), + `#!/bin/sh\nif [ "$1" = unlink ]; then touch "$RECALL_TEST_UNLINK"; exit 0; fi\nexec ${process.execPath} "$@"\n`, + { mode: 0o755 }, + ); + + const result = runUninstallAll(claudeDir, backupBase, opencodeConfigDir, piConfigDir, unlinkMarker, fakeBin); + + expect(result.status).toBe(0); + expect(readFileSync(opencodeConfig, 'utf-8')).toBe(malformed); + expect(JSON.parse(readFileSync(piConfig, 'utf-8')).mcpServers).toBeUndefined(); + expect(existsSync(unlinkMarker)).toBe(true); + expect(result.stdout).toContain('Uninstall Complete'); + }); }); diff --git a/tests/opencode-integration.test.ts b/tests/opencode-integration.test.ts index 50caabe..6dc1631 100644 --- a/tests/opencode-integration.test.ts +++ b/tests/opencode-integration.test.ts @@ -371,14 +371,11 @@ describe('installer', () => { const installPath = join(__dirname, '..', 'install.sh'); const libPath = join(__dirname, '..', 'lib', 'install-lib.sh'); const content = readFileSync(installPath, 'utf-8') + '\n' + readFileSync(libPath, 'utf-8'); - // Was: hand-rolled comment-stripping regex pair. That approach silently - // destroyed user `//` comments and crashed on JSON5 trailing commas - // (red team 2026-05-28). Replaced with jsonc-parser's modify() + applyEdits() - // which splices only the recall-memory entry and preserves surrounding - // bytes byte-for-byte. - expect(content).toContain('jsonc-parser'); - expect(content).toContain('modify'); - expect(content).toContain('applyEdits'); + // The dependency-free bundled helper parses comments/trailing commas and + // splices only the recall-memory entry. Packaged installs have no repo + // node_modules tree, so the shell library must not require jsonc-parser. + expect(content).toContain('jsonc-mcp.ts'); + expect(content).not.toContain('/node_modules/jsonc-parser'); }); // update.sh's refresh path propagating the OpenCode guide + agent prompt @@ -596,8 +593,8 @@ describe('Installer: Pi Detection and MCP Config', () => { // recall_link's collision-backup rule. // // Now satisfied by `_recall_jsonc_merge_mcp_entry` (lib/install-lib.sh): reads -// via jsonc-parser's tokenizer (so `//` inside string values like https:// URLs -// is safe — A7), writes via modify+applyEdits so the only bytes touched are +// via the bundled JSONC parser (so `//` inside string values like https:// URLs +// is safe — A7), writes only the targeted entry so surrounding bytes are kept // the recall-memory entry value slot. describe('recall_configure_opencode_mcp preserves user customizations', () => { let sandboxDir: string; @@ -655,14 +652,14 @@ describe('recall_configure_opencode_mcp preserves user customizations', () => { runConfigure(); const after = readFileSync(opencodeJsonPath, 'utf-8'); - // 1. Inline // comments must survive (jsonc-parser tokenizer + modify+applyEdits preserve them). + // 1. Inline // comments must survive the bundled JSONC edit. expect(after).toContain('// User-managed MCP configuration — DO NOT REWRITE'); expect(after).toContain('// GitHub MCP — critical for repo work, hand-tuned'); // 2. Non-Recall MCP entry must survive byte-for-byte (modify only edits the recall-memory slot). expect(after).toMatch(/"github":\s*\{[\s\S]*?"command":\s*\[\s*"gh-mcp"\s*\][\s\S]*?"GITHUB_TOKEN":\s*"ghp_xxx"/); - // 3. JSON5 trailing comma must survive (allowTrailingCommas on read, modify preserves on write). + // 3. JSON5 trailing comma must survive the bundled JSONC edit. expect(after).toMatch(/"ghp_xxx"\s*\}\s*,/); // 4. recall-memory entry must reflect the NEW path (sandbox fake-recall.db), @@ -676,7 +673,7 @@ describe('recall_configure_opencode_mcp preserves user customizations', () => { // regex to strip `//` line comments before JSON.parse; that regex was not // string-aware and corrupted https:// URLs, making the helper falsely refuse // perfectly valid opencode.json. The merged-tip implementation uses - // jsonc-parser's tokenizer (string-aware), and this test locks that in. + // The bundled parser is string-aware, and this test locks that in. test('A7: preserves https:// URLs and other `//` substrings inside string values', () => { const userConfig = [ '{', @@ -710,6 +707,38 @@ describe('recall_configure_opencode_mcp preserves user customizations', () => { expect(after).toContain('"recall-memory"'); expect(after).toContain('fake-recall.db'); }); + + test('adds a missing "mcp" key without a dangling comma or blank line', () => { + const userConfig = [ + '{', + ' "$schema": "https://opencode.ai/config.json",', + ' "theme": "dark"', + '}', + '', + ].join('\n'); + writeFileSync(opencodeJsonPath, userConfig); + + runConfigure(); + const after = readFileSync(opencodeJsonPath, 'utf-8'); + + expect(after).toContain('"theme": "dark",\n "mcp": {'); + expect(after).not.toMatch(/^\s*,\s*$/m); + expect(after).not.toMatch(/\n[ \t]*\n/); + expect(after).toContain('fake-recall.db'); + }); + + test('adds "mcp" to a config whose last property already has a trailing comma', () => { + const userConfig = ['{', ' "theme": "dark",', '}', ''].join('\n'); + writeFileSync(opencodeJsonPath, userConfig); + + runConfigure(); + const after = readFileSync(opencodeJsonPath, 'utf-8'); + + expect(after).toContain('"theme": "dark",\n "mcp": {'); + expect(after).not.toMatch(/,\s*,/); + expect(after).not.toMatch(/\n[ \t]*\n/); + expect(after).toContain('fake-recall.db'); + }); }); // ─── MCP config hardening RED — V-1, V-3, V-4 (recall_configure_opencode_mcp) ─── diff --git a/tests/pi-integration.test.ts b/tests/pi-integration.test.ts index 0216cee..9cfc5a6 100644 --- a/tests/pi-integration.test.ts +++ b/tests/pi-integration.test.ts @@ -387,8 +387,8 @@ describe('Pi extraction tracker (dedup)', () => { // comments, JSON5 trailing commas, and reformatting sibling MCP entries. // // Now satisfied by `_recall_jsonc_merge_mcp_entry` (lib/install-lib.sh): reads -// via jsonc-parser's tokenizer (so `//` inside string values is safe — A7), -// writes via modify+applyEdits so the only bytes touched are the recall-memory +// via the bundled JSONC parser (so `//` inside string values is safe — A7), +// writes only the targeted recall-memory entry so surrounding bytes are kept // entry value slot. describe('recall_configure_pi_mcp preserves user customizations', () => { let sandboxDir: string; @@ -460,14 +460,14 @@ describe('recall_configure_pi_mcp preserves user customizations', () => { runConfigure(); const after = readFileSync(mcpJsonPath, 'utf-8'); - // 1. Inline // comments must survive (jsonc-parser tokenizer + modify+applyEdits preserve them). + // 1. Inline // comments must survive the bundled JSONC edit. expect(after).toContain('// User-managed Pi MCP configuration — DO NOT REWRITE'); expect(after).toContain('// GitHub MCP — critical for Pi workflows, hand-tuned'); // 2. Non-Recall MCP entry must survive (modify only edits the recall-memory slot). expect(after).toMatch(/"github":\s*\{[\s\S]*?"command":\s*"gh-mcp"[\s\S]*?"--scope=repo"[\s\S]*?"GITHUB_TOKEN":\s*"ghp_xxx"/); - // 3. JSON5 trailing comma must survive (allowTrailingCommas on read, modify preserves on write). + // 3. JSON5 trailing comma must survive the bundled JSONC edit. expect(after).toMatch(/"ghp_xxx"\s*\}\s*,/); // 4. recall-memory entry must reflect the NEW path. Proves the function ran. @@ -480,7 +480,7 @@ describe('recall_configure_pi_mcp preserves user customizations', () => { // regex to strip `//` line comments before JSON.parse; that regex was not // string-aware and corrupted https:// URLs, making the helper falsely refuse // perfectly valid pi/mcp.json. The merged-tip implementation uses - // jsonc-parser's tokenizer (string-aware), and this test locks that in. + // The bundled parser is string-aware, and this test locks that in. test('A7: preserves https:// URLs and other `//` substrings inside string values', () => { const userConfig = [ '{', diff --git a/uninstall.sh b/uninstall.sh index 54acec2..61e283c 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -430,6 +430,7 @@ run_bun_unlink() { remove_opencode() { local config="$OPENCODE_CONFIG_DIR/opencode.json" + local config_failed=false if [[ -f "$config" ]]; then if [[ "$DRY_RUN" == "true" ]]; then @@ -439,7 +440,7 @@ remove_opencode() { log_success "Removed recall-memory from $config" else log_error "Failed to remove recall-memory from $config (existing config is invalid or unsupported — left unchanged)" - return 1 + config_failed=true fi fi fi @@ -478,6 +479,11 @@ remove_opencode() { run rm -f "$guide" log_success "Removed $guide" fi + + if [[ "$config_failed" == "true" ]]; then + return 1 + fi + return 0 } # ── Pi removal ─────────────────────────────────────────────────────────────── @@ -679,7 +685,9 @@ main() { if [[ "$SKIP_OPENCODE" != "true" ]]; then log_info "Removing OpenCode integration..." - remove_opencode + if ! remove_opencode; then + log_warn "OpenCode config was left unchanged; continuing uninstall" + fi echo "" fi