Skip to content

fix(recovery): drop the dead opencode session store, wire recovery into the bridge - #35

Open
lukeaus wants to merge 1 commit into
MeroZemory:mainfrom
lukeaus:32-session-recovery-reads-an-opencode-era-storage-path
Open

lukeaus wants to merge 1 commit into
MeroZemory:mainfrom
lukeaus:32-session-recovery-reads-an-opencode-era-storage-path

Conversation

@lukeaus

@lukeaus lukeaus commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Closes #32.

What was wrong

Session recovery read and wrote an opencode-era storage tree:

$XDG_DATA_HOME|~/.local/share  +  factory-droid/storage/{message,part}

opencode kept a session as a directory of per-message and per-part JSON files, which is why storage.ts was shaped the way it was: list a directory to find messages, write one more file to inject a part, unlinkSync a file to strip a thinking block. Factory Droid keeps one append-only .jsonl transcript per session under ~/.factory/, and never creates that tree at all.

The path arrived with the oh-my-claudecode port (a0b693a) and was renamed to say factory-droid by the 359-file rename sweep in 789dd80, which made it look native. It wasn't. And because getMessageDir() opened with:

if (!existsSync(MESSAGE_STORAGE)) return '';

the failure was silent. Every finder returned [], every mutator returned false, and recovery reported "nothing needed fixing" without ever logging a complaint. All 464 lines had been unreachable since the day they were ported. Verified absent on a real machine, including the XDG_DATA_HOME variant.

Why B, not the recommended A

The issue recommends A (comment + rename) and warns that B is a real project. Fair warning, and I did A first — then reconsidered, because A leaves 464 lines of permanently unreachable code in a Factory Droid utility library and the rename only makes the tombstone more legible.

What made B tractable is that B has two halves and only one of them is the "real project":

  • Deleting the opencode store: mechanical, and the audit below shows nothing depended on it.
  • Rewiring recovery to .jsonl: genuinely a rewrite, since the readers assume a listable directory per message id. Not attempted here.

So recovery keeps its detection half and loses its mutation half. handleSessionRecovery now classifies the error and returns corrective guidance for the next turn, which is exactly how its two siblings in the same module already behave — handleEditErrorRecovery and handleContextWindowRecovery never touched a session store either. RECOVERY_MESSAGES were reworded to match, since they advertised mutations that no longer happen ("Stripping thinking blocks..." → "Thinking is disabled for this session. Reply with visible content only and no thinking blocks.").

Also fixes the second half of the issue

Recovery is exported from src/hooks/index.ts but has no case in processHook. It is library-only surface.

Now wired, on two entry points:

  • post-tool-use — this case already existed but its body was dead: it read directory and toolInput into unused variables and returned { continue: true }, with a comment admitting it was a placeholder. It now runs handleEditErrorRecovery, so a failed Edit ("oldString not found", "found multiple times", "must be different") gets the corrective reminder.
  • recovery — new hook type routing through handleRecovery, preserving the documented priority order. Verified against built output rather than assumed:
Error errorType Guidance
prompt is too long: 200000 tokens > 180000 maximum token_limit_exceeded_string CONTEXT WINDOW LIMIT REACHED…
messages: empty content is not permitted empty_content The last turn produced no content…
ENOENT: no such file attempted: false, silent

Both read either the SDK's snake_case payload (tool_name, tool_response, possibly an object) or the legacy camelCase shape, matching what templates/hooks/post-tool-use.mjs already does at line 108. Every path returns continue: true — recovery advises, never blocks.

Deleted

  • storage.ts in full, and the constants that only fed it: MESSAGE_STORAGE, PART_STORAGE, getDataDir, THINKING_TYPES, META_TYPES, CONTENT_TYPES, PLACEHOLDER_TEXT (empty-message-sanitizer keeps its own copy)
  • the Stored* types and MessageData, which only described that on-disk shape
  • the four recover* mutators, one of which (recoverToolResultMissing) was already a self-described "placeholder showing the recovery intent" that returned true without acting

Breaking changes

Public API removals: 9 *Recovery* storage aliases, RECOVERY_PLACEHOLDER_TEXT, RecoveryMessageData and the 5 RecoveryStored* types.

handleSessionRecovery and handleRecovery / createRecoveryHook().onError lost their failedMessage / message parameters, which existed only to locate rows in the deleted store. No caller in the repo passed them, and package.json exports only .dist/index.js, so no deep import could reach them.

Audit for siblings

Since the interesting part of #32 is ported code whose host assumption changed, I swept for more of the same:

  • Every hardcoded home-relative path root in src/ — all 30+ resolve to ~/.omd/ (this tool) or ~/.factory/ (Factory Droid). The deleted one was the only outlier.
  • The silent-dead-guard pattern (if (!existsSync(root)) return []) — 3 hits, all legitimate cache-miss handling on files the code writes itself.
  • src/hooks/rules-injector/storage.ts, the other Ported from oh-my-opencode storage module — writes to ~/.omd/rules-injector, which it creates. Working code, opencode ancestry, no opencode assumption.

Remaining opencode mentions are attribution (README/AGENTS/CHANGELOG), ~14 Ported from provenance comments, and a user-selectable opencode HUD statusline preset. All intentional; left alone.

Per the issue

  • typos.toml's preceeding kept — recovery error matching still relies on it (session-recovery.ts:84).
  • Transcript scanner untouched; it was already correct.

Verification

  • npm test → 86 files, 2107 passed / 8 skipped (+26 new)
  • npm run build → tsc + 3 bundles clean
  • npm run lint → 0 errors; warnings 200 → 198, the two dead post-tool-use variables
  • Windows-safe: no new hardcoded separators; path assertions use join(), per c3666a3

Two new test files: session-recovery.test.ts (11) covers all four classifications, nested error unwrapping, null/undefined, custom message override, no-match. bridge-recovery.test.ts (15) covers both payload casings, structured tool_response, successful Edit, edit-error text from a non-Edit tool, priority ordering, fallthrough, absent error, absent session id.

Not in scope

Rewiring recovery to ~/.factory/**/*.jsonl (the other half of B). Also worth flagging separately: no installed hook script calls processHook at all — not one of the 7 templates/hooks/*.mjs or 16 scripts/*.mjs; they each reimplement standalone and import narrow helpers from dist/. So this makes recovery reachable through the router, but a shell entry point is still needed for it to fire on a real install. Relatedly, docs/hooks-implementation.md:138 documents an error-recovery.mjs with a settings.json entry that does not exist in scripts/ — pre-existing, happy to follow up.

@lukeaus

lukeaus commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

hold up the commit history is a mess

…to the bridge

Closes MeroZemory#32.

Session recovery read and wrote an opencode-era storage tree:

  $XDG_DATA_HOME|~/.local/share + factory-droid/storage/{message,part}

opencode kept a session as a directory of per-message and per-part JSON files,
so storage.ts could list a directory to find messages, write one more file to
inject a part, and unlink a file to strip a thinking block. Factory Droid keeps
one append-only .jsonl transcript per session under ~/.factory/, and never
creates that tree at all.

The path arrived here with the oh-my-claudecode port (a0b693a) and was renamed
to say "factory-droid" by the 359-file rename sweep in 789dd80, which made it
look native. It was not: getMessageDir() opened with

  if (!existsSync(MESSAGE_STORAGE)) return '';

so every finder returned [], every mutator returned false, and recovery
reported "nothing needed fixing" without ever logging a complaint. All 464
lines had been unreachable since the day they were ported.

Repathing was not an option. The readers assume a directory per message id
that can be listed and unlinked file by file; an append-only transcript has no
such structure, so honouring the layout would mean a rewrite rather than a new
constant.

Deleted:
- storage.ts in full, and the constants that only fed it (MESSAGE_STORAGE,
  PART_STORAGE, getDataDir, THINKING_TYPES, META_TYPES, CONTENT_TYPES,
  PLACEHOLDER_TEXT; empty-message-sanitizer keeps its own copy)
- the Stored* types and MessageData, which only described that on-disk shape
- the four recover* mutators in session-recovery.ts, one of which
  (recoverToolResultMissing) was already a self-described placeholder that
  returned true without acting

handleSessionRecovery is now advisory: classify the error, return the
corrective guidance for the next turn. That matches its two siblings in the
same module, handleEditErrorRecovery and handleContextWindowRecovery, neither
of which ever touched a session store. RECOVERY_MESSAGES were reworded
accordingly, since they advertised mutations that no longer happen.

Recovery also had no case in processHook, so none of it was reachable from a
hook at all. The existing post-tool-use case was dead too: it read directory
and toolInput into unused variables and returned continue, with a comment
admitting it was a placeholder. It now runs handleEditErrorRecovery, and a new
'recovery' hook type routes through handleRecovery, preserving the documented
priority order (context window, then session, then edit). Both read either the
SDK's snake_case payload (tool_name, tool_response, possibly an object) or the
legacy camelCase shape, matching what templates/hooks/post-tool-use.mjs
already does. Every path returns continue: true; recovery advises, never
blocks.

Public API removals: 9 *Recovery* storage aliases, RECOVERY_PLACEHOLDER_TEXT,
RecoveryMessageData and the 5 RecoveryStored* types. handleSessionRecovery and
handleRecovery/createRecoveryHook().onError lost their failedMessage/message
parameters, which existed only to locate rows in the deleted store. No caller
in the repo passed them and package.json exports only "." -> dist/index.js.

Tests: 2107 passed, 8 skipped (+26). Build clean. Lint 0 errors, warnings
200 -> 198 as the two dead post-tool-use variables are gone.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@lukeaus

lukeaus commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Merge order note (3 of 4)

Merge after #36. This is the one PR in the set that needs a real conflict resolution.

Shared with #36:

After #36 merges, rebase onto main and resolve bridge.ts in three places:

  1. Import blockfix(hooks): align registrations and payload handling with documented contracts #36 drops the processSubagentStart and processSetup imports. Keep them dropped; add this PR's handleRecovery / handleEditErrorRecovery import.
  2. HookType union — take fix(hooks): align registrations and payload handling with documented contracts #36's pruned union (no subagent-start, setup-init, setup-maintenance) and append 'recovery' to it. Do not restore the pruned members.
  3. processHook switch — keep fix(hooks): align registrations and payload handling with documented contracts #36's deletions of the subagent-start / setup-* cases, and keep this PR's new recovery case.

Worth a look while rebasing: this PR adds TypeScript-side getToolName / getToolOutput helpers to bridge.ts. #36 adds normalizeHookInput / extractResponseText, but only in the .mjs script layer (scripts/lib/hook-input.mjs), so there is no duplicate symbol and nothing breaks. It does mean two normalizers with the same job in different layers, which may be worth unifying in a follow-up rather than here.

Shared with #38: src/hooks/recovery/types.ts, auto-merges (this PR deletes ~99–176; #38 edits ~44).

Independent of #34.

Order: #36#34#35#38.

Verified by replaying each merge with git merge-tree --write-tree against main at 0e4d6a1, in that order and several others. bridge.ts conflicts in every ordering tried; every other shared file auto-merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: recovery storage path is opencode-era and unused

1 participant