fix: stop marking unpersisted extractions as complete, harden uninstall#249
Merged
Conversation
…n, uninstall status
…nt OpenCode uninstall
…n dual-write retries
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix the four concrete post-merge review defects discovered after Recall PR #248 merged: classify lowercase OpenCode markdown extraction failures as failed and retryable; make SQLite WAL writer failures visible and prevent batch extraction from recording success without persisted records; make uninstall resilient to malformed OpenCode JSONC while preserving user config and continuing remaining integrations without a node_modules/jsonc-parser runtime dependency; and verify the real OpenCode event plugin boundary and hook-name contract. Add focused regressions for classification, persistence and lock visibility and retry/success, resilient full uninstall and JSONC preservation, and the real OpenCode boundary. Preserve Phase 4 scope; do not expand into semantic issues #240, #241, or #226, Codex lifecycle capture, broad installer cleanup, release/version work, or local installation reconciliation. Work from updated main on fm/recall-pr248-review-fixes-r10, keep coherent fixes separately committed, and validate with focused and full tests, build, lint, shell checks, and the complete no-mistakes pipeline; never merge the PR.
What Changed
runExtractCorereturns a newpersistence_failedoutcome when the SQLite dual-write reports any per-table failure; both the Stop and markdown paths inRecallExtract.tslog the failure details and callmarkAsFailed(24h retry) instead ofmarkAsExtracted.RecallBatchExtract.tsroutes both classification sites through a sharedisExtractionFailureOutputmatcher that catches lowercase[FabricExtract] (markdown) extraction failed,sqlite write failed, andpersistence failedwhile leaving the non-fatal provider-cascade messages alone, and a successful extraction whose tracker write fails is now counted as failed and retried rather than as extracted. Because that seam is now replayed, the four plain-INSERT writers inhooks/lib/sqlite-writers.tstake an opt-inskipDuplicateskeyed on(session_id, content)— content-scoped so in-session windows and the correction writer are unaffected.jsonc-parserruntime dependency. The inlinebun -eblocks inlib/install-lib.share replaced by a new dependency-freelib/jsonc-mcp.ts(merge/removesubcommands) that preserves comments, trailing commas, sibling MCP entries, and custom Recall entry fields, and refuses to write malformed or unwritable configs;jsonc-parseris dropped frompackage.jsonandbun.lock.remove_opencodenow records the config failure but still removes plugins, agent, and guide — returning0on the clean path — andmain()continues the remaining integrations with a warning instead of aborting.scripts/e2e-opencode.tsasserts the OpenCode plugin factory exposes a realeventfunction and no obsoletesession.idlekey before invoking the payload. New tests cover batch success accounting, DB-lock persistence visibility, dual-write dedupe on replay, a full resilient uninstall, and JSONC preservation.docs/architecture.md,docs/installation.md,docs/OPENCODE_INTEGRATION.md,AGENTS.md, andhooks/AGENTS.mdare updated for the persistence check, the retry window's new failure class, the bundled parser, and theskipDuplicatescontract.Risk Assessment
Testing
Baseline first: the full
bun testsuite is green (1277 pass, 0 fail) on the target commit, so the ~10 worktree lifecycle failures tracked in issue #243 did not reproduce here and nothing red pre-existed. On top of that I exercised each of the four defects the way an operator would experience it, running every scenario twice — once against agit archiveexport of the base commit's hooks/lib/uninstall tree and once against the target — so the transcripts show the actual behavior change rather than just a passing assertion. The classification fix turns a crashed markdown extraction from "SUCCESS: Extracted and tracked, never revisited, session lost" into a FAILED row with retry_after that later recovers the archive; the persistence fix makes a SQLite writer failure print the per-table failures and stops the batch from claiming success with zero persisted records; the uninstall fix leaves a malformed opencode.json byte-identical while continuing to clean up Pi and Claude and reaching "Uninstall Complete" with exit 0, and the bundled JSONC helper merges/removes entries with no node_modules on disk; the OpenCode boundary was verified against the live OpenCode 1.18.4 CLI via the repo e2e (which also ranbun run buildclean) plus a direct factory-surface check. I added the one regression the intent enumerates but the PR lacked — batch retry/success accounting — and confirmed it fails against the base implementation. Per the task rules I did not run linters/tsc/bash -n, and did not re-run the no-mistakes pipeline (this run is that validation step). These are CLI, lifecycle-hook, and installer surfaces with no rendered UI, so no screenshot, GIF, or rendered-HTML artifact applies; the reviewer-visible evidence is the command transcripts, persisted SQLite tracker/memory state, and the preserved config files. Everything passed; the only open item is the author-decision warning about duplicate rows on the newly enabled retry path. All temp sandboxes were removed and the worktree contains only the intentional test-file addition.Evidence: Defect 1 — lowercase markdown extraction failure: base marks it extracted forever and loses the session; fixed records a retryable failure and recovers
[BASE] SUCCESS: Extracted and tracked | tracker extracted_at set | next run: (file was never revisited) | DISTILLED.md: NO — session lost [FIXED] QUALITY GATE FAILED or extraction failed ... FAILED: Will retry after 24h cooldown tracker: {"extracted_at":null,"failed_at":"...","retry_after":"2026-07-24 ..."} next run after fault cleared: SUCCESS: Extracted and tracked | DISTILLED.md: YES — session archived memory records: crashed attempt {loa_entries:1,decisions:1} -> after retry {loa_entries:2,decisions:2}Evidence: Defect 2 — SQLite write failures are visible and never reported as success
A. read-only Recall DB [BASE] (silent) [FabricExtract] Appended markdown extraction to DISTILLED.md [FIXED] [FabricExtract] SQLite write failed (markdown): {"sessions":"attempt to write a readonly database","decisions":...,"loa":...} archive preserved in both; 0 memory records persisted in both B. every memory write fails, tracker writable [BASE] SUCCESS: Extracted and tracked | records {loa_entries:0,...} | tracker extracted_at set | never revisited [FIXED] FAILED: Will retry after 24h cooldown | retry_after set | after fault cleared: SUCCESS, records {loa_entries:1,extraction_sessions:1,decisions:1}Evidence: Defect 2 (retry/success accounting) — tracker write failure must not count as extracted
[BASE] WARN: failed to mark extracted in SQLite ... no such table: extraction_tracker SUCCESS: Extracted and tracked <- false success [FIXED] WARN: failed to mark extracted ... / WARN: failed to mark failed ... FAILED: Extraction completed but tracking failed; will retry after 24h cooldown New regression added (tests/hooks/RecallBatchExtract.test.ts): 17 pass / 0 fail; base tree prints '1 extracted, 0 failed' for the same input, so the test is non-vacuous.Evidence: Defect 3 — malformed OpenCode JSONC preserved, uninstall continues, dependency-free JSONC helper
[BASE] ✗ Failed to remove recall-memory ... — exit status 1 pi/mcp.json still has recall-memory: 1 | reached 'Uninstall Complete': NO [FIXED] ⚠ OpenCode config was left unchanged; continuing uninstall ✓ Removed recall-memory from .../pi/mcp.json → ║ Uninstall Complete ║ — exit status 0 malformed opencode.json byte-for-byte identical in BOTH runs; user's github entry preserved Packaged layout (only lib/jsonc-mcp.ts, node_modules present: NO): merge exit 0 / remove exit 0 — // comments and JSON5 trailing comma surviveEvidence: Defect 4 — OpenCode plugin exposes the real `event` hook boundary
hooks exposed by the plugin factory : ["event"] typeof plugin.event : function obsolete 'session.idle' hook key : absent (correct) session.idle payload -> drop file created: .../MEMORY/opencode-sessions/ses_boundary_demo.md non session.idle event -> no drop created, existing drop untouchedEvidence: Real OpenCode 1.18.4 e2e (includes build) — boundary asserts, export retry, concurrent WAL writers, installer JSONC rollback
ESM ⚡️ Build success in 79ms / DTS ⚡️ Build success in 2052ms opencode.version=1.18.4 opencode.search_markers=OPENCODE_PHASE4_SEARCHABLE_MARKER,... opencode.export_failure_retry=verified opencode.concurrent_wal_writers=verified opencode.installer_jsonc_rollback=verified isolation.production_db_opened=false EXIT=0Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 11 issues found → auto-fixed ✅
hooks/RecallBatchExtract.ts:78- The broadened case-insensitive alternative/extraction failed/ialso matches non-fatal provider-cascade messages that RecallExtract's child prints on a SUCCESSFUL run:[FabricExtract] Claude CLI extraction failed: ...(hooks/lib/hosts/claude/extraction-provider.ts:94). The cascade order is [claude, ollama], so on any machine where the Claude CLI is broken and Ollama works, every extraction succeeds, the child calls markAsExtracted, and then extractFile() returns false -> the batch calls recordFailure(). markAsFailed isINSERT OR REPLACE(hooks/lib/extraction-tracker.ts:53) with noextracted_atcolumn, so it wipes the success record and sets a 24h retry -> the same conversation is re-extracted forever, appending duplicate rows to decisions/learnings/breadcrumbs (writeDecisionsBatch is a plain INSERT with no dedup) and duplicate blocks to DISTILLED.md / DECISIONS.log. This is systematic, not an edge case, on ollama-fallback setups. The old case-sensitiveoutput.includes('Extraction failed')did not match these lowercase provider messages. Fix constraint: do NOT simply revert — the lowercase markdown case is exactly what the PR set out to fix. Drop only the bareextraction failedalternative and replace it with an anchored form such as/\[FabricExtract\] (?:markdown )?extraction failed/i, keepingquality gate failed|all extraction methods failed|sqlite write failed|persistence failedintact soMarkdown extraction failed(RecallExtract.ts:839) andExtraction failed:(RecallExtract.ts:697) still classify as failures.lib/jsonc-mcp.ts:205- The single-property removal branch splices out onlycurrent.keyStart..current.value.endand leaves a preceding/following trailing comma behind. Verified: input{\n "mcp": {\n "recall-memory": { "type": "local" },\n }\n}produces{\n "mcp": {\n ,\n }\n}and the process exits 0, so uninstall.sh logsRemoved recall-memory from ...while having written a config that no JSON or JSONC parser accepts. The parser deliberately accepts trailing commas (object() continues past,before}), and the docs updated in this same diff claim the helper 'preserves ... trailing commas' and 'leaves a malformed OpenCode config unchanged' — so this directly inverts the intent's requirement to make uninstall 'preserv[e] user config'. jsonc-parser's modify() handled the separator; the replacement does not. Needs a separator-aware removal range (extend the deleted span through a following comma, or through a preceding one when it is the last property).uninstall.sh:483-[[ "$config_failed" == "true" ]] && return 1is the last statement in remove_opencode(), so on the SUCCESS path the[[ ]]test fails, the&&short-circuits, and the function's exit status is 1. main() then takes theif ! remove_opencodebranch and printsOpenCode config was left unchanged; continuing uninstallon every uninstall, including ones where the config was removed cleanly. Verified withbash -euo pipefail. Neither new test catches it (both only assert overall status 0 / 'Uninstall Complete'). Fix:if [[ "$config_failed" == "true" ]]; then return 1; fifollowed by an implicit success, or appendreturn 0.lib/jsonc-mcp.ts:209- When recall-memory is not the last property, removal splicescurrent.keyStart .. properties[currentIndex+1].keyStart, which swallows every comment sitting between the removed entry and the NEXT key — comments that belong to the surviving sibling. Verified: input containing"recall-memory": {...},\n // GitHub MCP - hand tuned, do not touch\n "github": {...}comes back as{ "mcp": { "github": { "type": "local" } } }with the comment gone. The mirror branch at line 212 (recall-memory last) deletes from the previous entry's value end, eating any comment attached after it too. This contradicts the contract asserted in tests/opencode-integration.test.ts and the docs updated here ('preserves comments'), on the uninstall path the intent requires to preserve user config.lib/jsonc-mcp.ts:158- insertProperty() applies the separator comma at the closing brace rather than immediately after the previous property's value, so the comma lands on its own line. Verified:{\n "$schema": "...",\n "theme": "dark"\n}becomes...\n "theme": "dark"\n,\n "mcp": {...}\n}. This is the common first-install path (an opencode.json with$schema/themebut nomcpkey), so most users' configs get visibly mangled formatting that jsonc-parser's modify() did not produce. Fix: emit the separator as a separate splice atlastProperty.value.endand keep the new property insertion atclose.hooks/RecallExtract.ts:689- Onpersistence_failedthe code still runs every markdown archive side-effect (DISTILLED.md append, HOT_RECALL, SESSION_INDEX, LoA transcript, DECISIONS/REJECTIONS/ERROR_PATTERNS) and only then calls markAsFailed and returns — deliberate per commit 4d4a4fa. But dualWriteToSqlite is not transactional: each table has its own try/catch (hooks/lib/extraction-parsers.ts:143-215), so a partial failure (e.g. sessions+decisions written, loa failed) still yieldspersistence_failed. The 24h retry then re-runs the whole thing: writeDecisionsBatch / writeLearningsBatch / writeBreadcrumbsBatch are plain INSERTs with no dedup (only writeExtractionSession is INSERT OR REPLACE), and the markdown files are append-only. Result: duplicated memory rows and duplicated archive blocks. The pre-change behavior was silent loss; the new behavior trades it for silent duplication. Worth deciding explicitly — e.g. treat only a total write failure (failures._db) as retryable, or skip the archive writes when the entry will be re-extracted. Same pattern at line 831 for the markdown path.hooks/lib/insession.ts:470- The in-session path now returnsdualWritealongsideoutcome: 'persistence_failed', but hooks/lib/insession.ts contains no logging at all and its only production caller (hooks/RecallInSession.ts:194) discards the return value entirely. So a WAL/lock write failure during in-session extraction is still completely invisible — no console.error, no log line — while the Stop and markdown paths in RecallExtract.ts do surface it. The intent requires 'make SQLite WAL writer failures visible'; that requirement is met for the Stop/markdown paths but not for this one. Either log the failure in RecallInSession or state that the in-session path is out of scope.tests/install/uninstall.test.ts:582- The new full-uninstall test writes a fakebunshim that hardcodesexec /opt/homebrew/bin/bun "$@". That path only exists on Apple-Silicon Homebrew installs — on Linux CI, Intel Homebrew (/usr/local), or a~/.bun/bininstall the shim exec fails and the test breaks for reasons unrelated to what it asserts. Resolve the real bun once (e.g.process.execPathorwhich bun) and interpolate it into the shim.tests/hooks/extract-core.test.ts:155- The new lock test creates/tmp/recall-busy-<pid>.db(plus its journal) and never removes it, and it hardcodes/tmpinstead of the mkdtempSync pattern used elsewhere in this suite. The test is also named 'surfaces a WAL database lock' but setsPRAGMA journal_mode = DELETE, so it exercises the rollback-journal lock rather than WAL — worth renaming or switching the pragma so it matches the defect it is pinning.AGENTS.md:31-lib/is documented as 'shared bash for the install / update / uninstall lifecycle scripts', but this change addslib/jsonc-mcp.ts— a shipped TypeScript runtime helper the lifecycle scripts shell out to. Line 180 also scopes root ownership to 'lifecycle scripts ... + their shared lib/install-lib.sh'. The repo's DOX contract requires the owning doc to be updated when durable structure changes; docs/OPENCODE_INTEGRATION.md was updated but AGENTS.md was not.package.json:54-jsonc-parseris now referenced nowhere in src/, hooks/, lib/, or scripts/ — the only remaining mentions are a comment in lib/jsonc-mcp.ts and a negative assertion in tests/opencode-integration.test.ts. It is still declared underdependencies, so every install pulls it. Either drop it or note why it is retained.🔧 Fix: fix batch failure classification, JSONC insertion, uninstall status
✅ Re-checked - no issues remain.
hooks/RecallExtract.ts:839- Retrying an archive-crash extraction duplicates SQLite memory rows. The new lowercase-failure classification correctly makes[FabricExtract] Markdown extraction failedretryable, but that is the only failure class whererunExtractCorealready returned'extracted'(dual-write persisted) before the archive append threw. The retry re-runs the whole core, so the same session is written twice: in the end-to-end runloa_entrieswent 1 -> 2 anddecisions1 -> 2 after the retry (extraction_sessionsstayed 1, PK-idempotent). Base lost the archive silently instead; whether duplicate loa/decision rows are the better trade is an author call. Evidence: defect1-classification.txt, FIXED branch, 'memory records persisted by the crashed attempt' vs 'memory records after the retry'.bun test(full suite, 1277 pass / 0 fail; issue #243's worktree lifecycle failures did not reproduce)bun test tests/hooks/RecallBatchExtract.test.ts tests/hooks/extract-core.test.ts tests/hooks/extraction-parsers.test.tsbun test tests/install/uninstall.test.ts tests/opencode-integration.test.ts tests/pi-integration.test.tsbun run test:e2e:opencode(includesbun run build; real OpenCode 1.18.4 CLI, assertstypeof plugin.event === 'function'and nosession.idlekey)Added regressiontests/hooks/RecallBatchExtract.test.ts->batch extraction success accounting(drives the real script; asserts a tracker-write failure is not counted as extracted)Non-vacuity check: same staged sandbox run against the base-commithooks/RecallBatchExtract.tsprintsSUCCESS: Extracted and tracked/1 extracted, 0 failedManual base-vs-fixed harnessevidence-classification.ts— OpenCode drop file whose markdown extraction crashes (read-only DISTILLED.md)Manual base-vs-fixed harnessevidence-persistence.ts— read-only Recall DB, and batch extraction with every memory-table write abortingManual base-vs-fixed harnessevidence-tracker-write.ts— extraction succeeds but the extraction_tracker write failsManual base-vs-fixed harnessevidence-uninstall.sh—bash uninstall.sh --no-confirm --skip-ompagainst a malformed opencode.json plus a real Pi/Claude install, andbun run lib/jsonc-mcp.ts merge|removein a packaged layout with no node_modulesManual harnessevidence-opencode-boundary.ts— plugin factory hook surface plus a realsession.idleevent payload producing the markdown drop✅ **Document** - passed
✅ No issues found.
package.json:54-jsonc-parseris now an orphaned runtime dependency. This change replaced its only consumer (therequire(process.env.REPO_DIR + "/node_modules/jsonc-parser")calls inlib/install-lib.sh) with the dependency-freelib/jsonc-mcp.ts; a repo-wide grep finds zero remaining imports or requires. It still ships independencies, so every npm install pulls dead weight — and dropping the runtime dependency was an explicit goal of this change. Not fixed here: removing a dependency touchespackage.jsonplusbun.lockand is a functional change, outside a documentation-and-lint pass.🔧 Fix: remove orphaned jsonc-parser dep, dedupe extraction dual-write retries
1 info still open:
hooks/lib/sqlite-writers.ts:10- Residual limitation of the idempotency fix, surfaced for visibility rather than left silent. The archive-crash retry path re-runs the extraction model (extractAndAppend -> runExtractCore -> runExtractionCascade), and the cascade sets no temperature (no temperature/max_tokens anywhere in hooks/lib/extract-model.ts, extraction-provider.ts, or hosts/claude/extraction-provider.ts). The new guard keys on (session_id, content), so it eliminates duplicates when the rerun yields identical text -- the case the finding describes -- but a materially different model rerun would still insert new rows. A model-independent guarantee needs a persistence marker in extraction_tracker so dualWrite is skipped entirely for an already-persisted conversation; that is larger than the requested smallest correct fix, so it is noted as follow-up, not done.✅ **Push** - passed
✅ No issues found.