fix(memory): make Project Memory process-safe and identity-safe (MEM-PR-01) - #234
Merged
Conversation
Phased reconstruction of the lost ProjectMemoryAuthority redesign on top of the d7b011738 process-safe baseline. 8 phases (P1-P8), each a commit. P1=identity+atomic store API, P6=fromDirectory cutover (1C), P7=crash harness (1A). Co-Authored-By: Claude <noreply@anthropic.com>
…rotocol Adversarial review (5 critics, ultracode) found the lost ADR-0004 is unrecoverable, so 'faithful reconstruction' is unverifiable. Add blocking product decisions D1-D4 (Policy-in-Home vs ADR-0001, source-Home preserve retention, retirement-as-merge/lineage, new journal/Revision/ChangeMemory surface) requiring user sign-off, a mandatory P0 (recreate ADR-0004 + CONTEXT for approval before P1), technical revisions (P4 before P3, FK per-table rules, mutation-gate fixes, ledger locations, phase enum), and a resume protocol for fresh sessions. Co-Authored-By: Claude <noreply@anthropic.com>
…osed) P0 design-of-record for the ProjectMemoryAuthority redo, reconstructing the lost ADR-0004 (uncommitted WIP, /tmp-cleaned). Resolves the §7.A product decisions D1-D4 and encodes the user principles confirmed 2026-08-12: one shared Memory per Project (worktrees share it, no per-worktree memory), Memory never forks, and identity upgrade is imperceptible. Status: Proposed (P0 gate) — P1 is blocked until the user approves this ADR. Co-Authored-By: Claude <noreply@anthropic.com>
Fix #1) migrateProjectId deleted the old ProjectTable row, which ON DELETE CASCADE silently destroyed every DAG workflow and every saved permission whenever a repo gained its first remote (root -> first-remote identity upgrade). Repoint both project_id FKs inside the existing immediate transaction before the old row is deleted. A (newID, action, resource) collision on permission fails the transaction closed (no data loss). Extended 'migrates cached root project data when origin becomes available' to seed a workflow + permission and assert both survive the upgrade. Mutation gate: removing the repointing flips the test Red (rows cascade-deleted). Co-Authored-By: Claude <noreply@anthropic.com>
…04 redesign User applied Occam's Razor: the 8-phase ProjectMemoryAuthority redesign is over-engineered for the real needs (shared/no-fork memory already in baseline; imperceptible upgrade + no data loss via small in-place fixes). ADR-0004 → Rejected. Plan §10 = 4 targeted fixes; Fix #1 already done. Co-Authored-By: Claude <noreply@anthropic.com>
…h complete Re-assessed the remaining Occam fixes against the actual code: - #3 (ABBA): unreachable — migrateHome is one-way (root→remote), no reverse caller, so the two project flocks are never taken in opposite orders. - #4 (destructive rescan): already handled — worktree reconcileLegacyMemory invalidates the admission cache before ensure, forcing a fresh rescan. - #2 (typed errors): deferred — full propagation is a multi-file cascade for a marginal HTTP-status gain on a rare conflict (.orDie preserves the diagnostic in the Die cause). Awaits user decision (Occam cut vs invariant #5). Fix #1 (the real data-loss gap) stands; full regression green. Co-Authored-By: Claude <noreply@anthropic.com>
…entity (MEM-PR01-00) Every commit-less repository resolves to the same ProjectV2.ID.global, and the branch keys Memory Home by project ID. Before this change an enabled global config activated Memory for all commit-less repos at once: one shared Home leaked topics across unrelated repositories, and the first commit moved the identity to root/remote while migrateProjectId never migrates away from global — silently orphaning everything written pre-commit. Fix with the minimal Occam seam: one fail-closed guard in Memory.configuration (the single activation gate behind active/prepare/search/checkpoint/setEnabled) returning undefined while the project identity is global. Memory activates normally once the repository gains a real identity; migrating the shared bucket is structurally infeasible (no per-repo provenance) and pre-existing orphans belong to the deferred retention/GC decision. - Red: search must report "unavailable" and /memory on must stay off for a commit-less repo even with an enabled global config and seeded topics - Green: single guard; identity-scoped (repos with a commit activate normally) - Mutation: removing the guard turns both Red tests red again - Domain regression: memory+project suites 162 pass / 0 fail; opencode+core typecheck clean - redo plan: record Fix #5 decision; reopen #3 (ABBA reachable via remote→remote identity change, MEM-PR01-R1-24) - remove leftover no-assertion diagnostic scaffold (repro-scope-finding); its scenario is captured in finding MEM-PR01-R1-06 for the M-C slice Co-Authored-By: Claude <noreply@anthropic.com>
…erge, FK collision, deadlock-freedom (MEM-PR01 M-A)
Two-round review confirmed four P2 defects in the identity-upgrade path; all
fixed at the existing seams with Red→Green→mutation evidence per finding.
R1-12 inspectHome wedged every upgrade after a crash: the store's own
atomicWrite residue (manifest.json.<pid>.<uuid>.tmp) was rejected as
foreign state. Tolerate the store's own temp pattern; foreign files
still fail closed (pinned).
R1-15 The merge compared full topic JSON, so controller metadata drift from
MemoryStore.markMatched (last_matched_at/match_count/revision/
updated_at) registered as a user-visible ConflictError and wedged the
upgrade. Compare content only; the target's own copy stays
authoritative; real content differences still conflict (pinned).
R1-11 Permission FK repoint used a bulk UPDATE that violated the unique
(project_id, action, resource) index whenever the successor identity
already held the same (action, resource) — the immediate transaction
died and the whole upgrade wedged on every retry. Repoint per row;
on collision the successor row wins and the duplicate old row is
dropped; disjoint rows still repoint.
R1-24 The redo plan claimed ABBA unreachable because retirement was "one-way"
— false: a changed origin URL yields remote→remote transitions, and the
old lock structure (hold flock(old) across the merge while
updateTopics locks flock(new) inside) deadlocks opposite-direction
migrations (Red: 20 s test timeout on the legacy structure). Sorted
pre-acquisition is impossible because the flock is non-reentrant, so
fix by construction: a sorted pair lock serializes the two directions
and the merge is restructured into three phases that never hold more
than one memory-project:* lock at a time. A source that changed
mid-merge now fails closed with retryable SourceChangedError instead
of risking deletion of new data. Crash-retry convergence pinned
(R1-13).
- Domain regression: memory+project suites 169 pass / 0 fail; opencode+core
typecheck clean; lint ratchet unchanged (0 new warnings).
Co-Authored-By: Claude <noreply@anthropic.com>
…t reconcile, TOCTOU revalidation, retired-identity inertness (MEM-PR01 M-C) R1-06 (blocking): worktree remove/reset reconciled admission against a SINGLE directory, so a lone sandbox legacy config could be promoted to the project config past disagreeing siblings (order-dependent, silent effective-config flip). Both call sites now pass the complete snapshot (primary + every registered sandbox); disagreeing siblings fail closed with no promotion. R1-03: configuration() fell back to the stale instance context when the identity row was gone, letting a process holding a retired identity fork a Home under it. The fallback is removed: missing row = inert. R1-04: admission deleted scanned legacy topic/config files without re-reading them; a writer outside the admission flock (older runtime, hand edit) landing between scan and delete lost content. Each file is now re-read and compared immediately before removal; changed content is preserved and surfaced as a conflict. Deterministic TOCTOU test pins the scan→delete window via the store flock. R1-08: worktree remove/reset migration ran for uninitialized projects despite the memory path's inertness rule; reconcile is now gated on time.initialized (residue still fails closed). Existing migration tests stamp initialized. R1-10: admission's explicit-config choice used a localeCompare sort that put memory.json before memory.jsonc, disagreeing with MemoryConfig.load. The scan now keeps loader precedence and a jsonc/json fork in the project directory is diagnosed as config.conflict instead of silently picking a side; legacy configs equal only to the non-effective file are no longer deleted as duplicates. Pins: R1-07 (/memory writes the project config to the project worktree from a non-primary instance context) and R1-23 (runtime admission snapshot covers every registered sandbox). - Domain regression: memory+project suites 176 pass / 0 fail; opencode+core typecheck clean; lint ratchet unchanged (0 new warnings). Co-Authored-By: Claude <noreply@anthropic.com>
…to remove (MEM-PR01 M-D) R1-16 (blocking): list() ran `git worktree prune` and deregistered sandboxes for every merely-prunable entry. "prunable" does not prove a worktree is gone — git also marks inaccessible directories (unmounted volume, locked parent) and broken gitdir links whose directories still exist, so a read call could destroy git admin data and live registrations. list() is now a pure observation path: prunable entries stay hidden from the listing but are otherwise untouched. The destructive cleanup moves to the action path, where each case can be proven: - remove() gains a prunable branch: reconcile legacy memory fail-closed, prune the admin data, remove the directory if it still exists, delete the branch, drop the registration. - remove() gains a git-unknown recovery branch (R1-18): a registered worktree with no git record previously failed forever with a false "not registered" error and no remediation; it now reconciles legacy memory fail-closed and drops the stale registration without ever deleting the directory. - Registration cleanup drops every canonically-equal entry, not just the first — symlinked paths (/var vs /private/var) could register the same worktree twice and leave a zombie entry that broke serialized removal. Pins (both mutation-proven): - R1-17: reset fails closed over invalid legacy memory and preserves it. - R1-19 (blocking): reset/remove invalidate the admission cache before the rescan; a reset-primed clean cache must never hide a legacy file that appears before a later destructive operation. - Updated the prune-era list test to the new semantics (list hides but does not touch; explicit remove cleans up). - Domain regression: memory+project suites 180 pass / 0 fail; opencode+core typecheck clean; lint ratchet unchanged (0 new warnings). Co-Authored-By: Claude <noreply@anthropic.com>
…em_count, torn-commit (MEM-PR01 M-E) R1-02 (P2 test-gap): the corrupt-manifest fail-closed guards had no test, so reverting them would let migrateHome delete an unread Memory Home. Now pinned: an invalid manifest and a manifest referencing a missing generation both fail readSnapshot, and migrateHome fails closed on the merge path with the source Home preserved. Both guards proven load-bearing by mutation (fail-open revert turns the test Red). R1-20 (P3 test-gap): decodeTopic item_count/items.length consistency was only covered by a since-deleted test. Re-pinned at both the decoder and the writeSnapshot generation gate (mutation-proven). R1-21 (P3 test-gap): a crash mid-writeSnapshot leaves an orphaned staging generation whose manifest was never published; pinned that it never shadows the committed generation and the store still commits cleanly (mutation-proven). Test-only change; no production code touched. - memory+project suites 183 pass / 0 fail; opencode+core typecheck clean; lint ratchet unchanged (0 new warnings). Co-Authored-By: Claude <noreply@anthropic.com>
…-process commit conflict (MEM-PR01 M-F) R2-02: the branch collapses MEMORY config onto one project-primary file, written by three paths under mutually disjoint locks — /memory on|off (in-process KeyedMutex), admission promotion (memory-admission flock), and readConfig's normalization rewrite (no lock). atomicWrite prevents torn bytes but not whole-document last-writer-wins across worktrees/processes. All config file writes now serialize on a per-file cross-process flock (memory-config:<file>): writeProject, writeGlobal, and the normalization rewrite. Pinned by a blocking-observation test; mutation-proven (dropping the lock lets a concurrent writer complete while the lock is held). Residual, documented rather than fixed (Occam): decision-level read-modify-write across processes is not CAS-protected — only the write primitives are serialized. A full cross-process RMW protocol would be over-engineering for the exposure. R2-03: the cross-process commit protocol's explicit-conflict guarantee (ADR-0002) was only exercised within one process. A new spawned-worker test commits with a stale expectedRevision from a second OS process and observes CommitConflictError deterministically (the pre-existing updateTopics race test only overlaps probabilistically). - memory+project suites 185 pass / 0 fail; opencode+core typecheck clean; lint ratchet unchanged (0 new warnings). Co-Authored-By: Claude <noreply@anthropic.com>
…esign (MEM-PR01 M-G)
R1-01 (blocking): CONTEXT.md still shipped the rejected ADR-0004 authority
design as the domain's governing self-doc. Rewritten to the actual authority
structure (Store/Config/Admission/migrateHome/worktree guard + project
identity migration): rejected-design glossary and invariants removed (Identity
Alias, Canonical Project ID, tombstone retirement, opaque Revision, destruction
guard); source Home described as migrate-then-remove with retention deferred;
Project Configuration described as the unversioned .opencode/memory.jsonc; the
read-leniency split stated (runtime read projects empty, strict reads and
migration fail closed); ADR-0001's policy clause restored as live; ADR-0004
marked Rejected; the M-A…M-F behaviors reflected (global inertness,
content-only conflicts, non-destructive list, fail-closed reset/remove,
per-file config lock).
R1-25: redo-plan internal consistency — header status no longer says PLANNING;
the §10 resume protocol is marked superseded (no pending autonomous fix, only
user decisions remain).
R1-09 (spec-gap → decision): the git-exclusion narrowing to the two config
candidates is intentional and now documented: preserved fail-closed legacy
topic files stay visible in git status and committable; surfacing repair-
pending files beats silently excluding user data.
R1-14 (spec-gap → requirement): the openspec workspace is untracked, so this
plan now carries the identity-upgrade requirement ("Identity upgrade preserves
Project Memory and Project-owned references") with its scenarios, pinned by the
M-A/M-B/M-C/M-E tests.
Docs-only; no production code touched.
Co-Authored-By: Claude <noreply@anthropic.com>
…oped prune, proof-after-hook (MEM-PR01 M-H) R3-P2-a in-flight old-identity writer could recreate a retired Home: writers (prepare/search/checkpoint) now hold a cross-process memory-identity:<id> flock around their whole read-modify-write and re-check identity liveness inside it; migrateHome takes the same identity lock inside the sorted pair lock, so it waits for in-flight writers and moves their writes with the Home. Lock order admission -> migrate(pair) -> identity -> project is cycle-free. R3-P2-d git worktree prune is repo-global; it now runs only when the removed entry is the sole prunable one (else stale admin data is left for explicit cleanup), so sibling worktrees' admin data is not destroyed. R3-P2-e the WorktreeRemove hook now fires BEFORE the reconcile proof on all remove paths, so the proof observes everything the hook produced. R3-P2-f remove/reset no longer fall back to the stale instance identity (?? ctx.project); they fail closed when the identity row is gone. R3-P2-b cleanupLegacyDirectory re-checks the listing immediately before removing each dir. Pins (mutation-proven): SourceChanged verify-before-delete guard (R3-P2-c); store write paths fail closed on a corrupt manifest (R4-P2-a); unresolved admission results are never cached (R4-P2-b). Docs: ADR-0002 updated to the three-phase merge + identity-lock protocol; rejected ADR-0004 no longer claims to supersede live clauses; redo-plan #3 ABBA narrative corrected. Also drops two no-op non-null assertions (session/summary.ts, format/index.ts) surfaced by type-aware churn from the identity-migration FK fix, returning the tree to the 4852 lint ratchet with no behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
…-PR01 M-I) Closes the two Round 5+6 convergence findings: - P1-a (writer fence in the wrong lock dir): memory.ts's writer fence (prepare/search/checkpoint) now passes home.locks so it lives in the same lock namespace as identity migration. Previously it fell back to the default XDG-state lock dir, a DIFFERENT directory, so the writer fence and the migration fence never actually serialized. - flock-leak P2 (fence released before row deletion): ProjectIdentityMigration .migrate now holds memory-identity:<oldID> for the WHOLE retirement — the Memory Home migration AND the caller's reference/row retirement — via a retireReferences callback. The fence is no longer released between the Home move and the old-row deletion, so an in-flight writer under oldID cannot slip into the gap. Callers pass their row retirement as the callback and no longer touch the fence themselves (single authority for the fence). Mutation check: removing retireReferences() from inside the fence turns the MEM-PR01-R1-11 permission-collision test Red (FK repoint no longer happens), confirming the seam wiring is load-bearing. Co-Authored-By: Claude <noreply@anthropic.com>
…(MEM-PR01 M-J) Round 7 converged on one residual P1 (F1) and one P2 (F2), both pre-existing: - F1 (P1): admission.ensure took the memory-identity fence but never re-checked identity liveness inside it. A retirement could complete while admission waited on the fence, and admission would then import legacy topics into the re-created retired Home AND delete the legacy source files (permanent orphaning — the identity cache already points at the successor, so no migration would ever run for the pair again). - F2 (P2): the phase-1 Home rename raced a concurrent newID writer creating the target between existsSafe and rename (ENOTEMPTY), dying fromDirectory. Self-healing (next boot retries into the merge path), no data loss. The identity-race TOCTOU class has now been found in three consecutive review rounds, and the memory-identity protocol (key + lock dir + in-fence liveness recheck) was hand-duplicated at four sites across three modules — exactly why admission could diverge from the writer discipline. Per the redesign rule this is a seam redesign, not a patch: - New MemoryIdentityFence (memory/identity-fence.ts) is the single authority for the protocol: key() builds the lock key, withLiveIdentity() holds the fence on home.locks AND re-checks the identity row inside the fence (None = retired, callers fail closed). A future path cannot forget the recheck. - Writers (prepare/search/checkpoint) and admission route through it; the retirement seam (ProjectIdentityMigration) stays the only raw fence holder (it deletes the row inside the fence) and builds its key from key(). - admission.ensure now fails with a tagged IdentityRetired error when the row is gone; configuration() stays inert, the worktree guard proceeds (the migration is moot after a completed retirement). - F2: rename failure with a target that appeared falls through to the snapshot-merge path; genuine FS failures still rethrow. Verification: Red test MEM-PR01-R7-F1 (ensure after row retirement must not import nor delete the legacy files) confirmed Red before, Green after; mutation removing the in-fence recheck turns it Red again. memory+project 189 pass / 0 fail; typecheck clean; lint flat at 4852. F2's mutation is registered as a test-gap: the race window is between two file ops with no observable state between them, so no deterministic public-seam test exists (the fallback routes into the already-tested merge path). Co-Authored-By: Claude <noreply@anthropic.com>
…ve (MEM-PR01 M-K) Round 9 product-invariant review found P2-A (introduced by M-J): the reconcileLegacyMemory guard returned undefined on IdentityRetired, so a worktree remove could proceed past the fence and `git worktree remove --force` would destroy legacy .opencode/memory content that was never admitted into any Home. The window sits between removeLocked's own row-liveness check and the admission fence recheck — widened by the WorktreeRemove hook (user scripts) that runs between the two. Fix: on IdentityRetired the guard now returns a blocker message (fail closed), matching the reset path's existing stance. A retry under the successor identity imports the legacy content first and then removes safely. Red-first + mutation evidence: - New test MEM-PR01-R9-P2A (worktree-remove.test.ts): holds the memory-admission flock so the remove blocks inside ensure after its own row check passed, retires the identity row, then releases — asserting the removal fails and the never-admitted legacy file survives. Red before the fix, Green after; reverting the blocker to undefined turns it Red again. Also fixes a standards-P2: reindents the retirement transaction body in project.ts (pure whitespace, no behavior change). Registered, not fixed here (out of PR scope): EffectFlock stale-break can silently lose a cross-process update (P2-B, pre-existing core infra, recorded as a residual for the final audit). Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Make Project Memory process-safe and identity-safe. Memory is now inert when it
cannot safely operate (shared global identity, retired identity), its on-disk Home
migrates atomically with the project identity under a single cross-process fence, and
the worktree lifecycle no longer destroys state as a side effect of listing.
Branch
chore/worktree-lifecycle-audit→dev.Fixes #233
What changed (by slice)
unavailable,refuses
/memory on, writes no project config) when the project resolves to theshared
ProjectV2.ID.global(commit-less repo), so topics can no longer leak acrossrepositories or be orphaned by the first commit.
residue, merges by content identity (match-controller metadata churn is not a
conflict), repoints FKs, and is deadlock-free for opposite-direction migrations.
TOCTOU revalidation, retired-identity inertness.
list()is non-destructive; cleanup moves toremove.item_countintegrity,torn-commit detection pinned by tests.
commit conflict pinned.
memory-identity:<oldID>fence now covers the WHOLEretirement (Home migration and the caller's reference/row retirement via a
retireReferencescallback) inProjectIdentityMigration.migrate, so the fence isnever released between the Home move and the old-row deletion. Writer fences use the
same
home.locksnamespace as migration so they actually serialize.in-fence liveness recheck) previously lived in four hand-duplicated sites across
three modules; admission was the one path missing the recheck, so a post-retirement
import could re-create a retired Home and delete the legacy source files (P1). Now
MemoryIdentityFenceowns the protocol: every reader/writer goes throughwithLiveIdentity(fence + in-fence row recheck,Option.none= retired, callersfail closed), the retirement seam is the only raw holder and builds its key from
key().admission.ensurefails with a taggedIdentityRetirederror; the phase-1Home rename falls back to the merge path instead of dying when the target appears
concurrently.
Verification
memory+projectdomain tests: 189 pass / 0 fail.opencodetypecheck (tsgo --noEmit): clean.retireReferences()from inside the fence turns MEM-PR01-R1-11(permission uniqueness collision) Red.
legacy topics nor delete the legacy files) was Red before M-J, Green after.
new P1/P2 (see review record).
Intentionally NOT in this PR (hard boundaries honored)
ProjectMemoryAuthority, six-phase journal, alias tombstone, opaqueRevision,or destruction guard — these were reviewed and rejected as over-engineering.
.orDieerror handling is deferred (user-approved, not a merge blocker).Registered deferrals (final cold review, all non-blocking P2 — impact + trigger conditions)
migrateHome's rename fast path holds only the source project lock — adefense-in-depth gap reachable only if the successor row exists while the seam runs
(unreachable in current wiring; trigger: a future
migrateHomecaller or a crash-recovery path with a pre-existing successor row).
MemoryStoreis fence-agnostic — claim 5 holds only because everyproduction caller routes through
MemoryIdentityFence/admission (trigger: a futurewrite path that forgets the fence; the fence module's header documents this).
migrateHomeand theretireReferencestransaction —old-identity readers transiently see an empty Home; no data destroyed, converges on
the next
fromDirectory,orDiemakes the failure loud (closing it would requirethe rejected retirement journal).
against lock-free older-version runtimes (acknowledged in code comments; irreducible
without a lock legacy writers respect).
cross-process
/memorytoggle vs admission promotion is last-writer-wins on thepolicy file (not topic-data loss; disclosed in CONTEXT.md).
flock's 5-minute timeout instead of its own 5-second controller timeout (pre-existing
EffectFlock, liveness only).
orDies during routine open — fail-loud by design andpinned by tests; surfacing it operationally is the trigger condition of the deferred
MEM-TYPED-02.
License note
This PR touches
packages/opencodememory/project/worktree code (MIT portions); nolicense-boundary crossings (see
NOTICE).🤖 Generated with Claude Code