Conversation
…stination generation as transient - LocalMemoryStore.listFiles classifies DT_UNKNOWN dirents by lstat in the bounded walk as well (still capped), not only the strict one: the legacy store fingerprint otherwise dropped such entries, so a downgraded edit under an untyped directory never changed the check key and the memoized pass never picked it up. A bounded walk treats an unclassifiable entry as "other" as before; the strict walk still fails on it. - An identical adoption destination whose generation stamp cannot be read (the lstat fails after the content read) is an unanswered sibling-owner question, not "nobody's": legacyImportTarget and the prior-record reuse branch now throw, and the caller skips the note transiently (no reuse, no record, pass not memoized) instead of settling a reuse record against a file that may be another descendant's copy. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$112.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=112.74 -->
…efore the creator reconciles it The previous layers let a descendant's identical note settle on another descendant's adoption-created copy (a created: false record naming that target). Under the one-owner invariant such a record is not settled: the unchanged-record fast path now checks a created: false record's target against the strict sibling scan and, when a sibling's live generation owns it, falls through to placement — the note is copied from its legacy bytes into imported/<child>/ (created: true, fresh stamp), the sibling's file untouched. Removal's forced handover runs the same pass, so it performs the migration too. On the creator's side, the deletion loop keeps a copy that a sibling's settled reuse record still names (transient skip, pass unmemoized) until that sibling has migrated, so a source deletion never pulls a note out from under a sibling. Both checks read sibling manifests strictly; an unanswerable one makes the note wait. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$112.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=112.74 -->
…dopting an interrupted in-place edit An in-place replacement interrupted between its pending manifest write and the install, followed by a downgrade, left two visible copies: the prior build reads the pending record as an interrupted first adoption, finds the prior copy holding other bytes, and settles the edited note anew under imported/ — the original stays, named by no record, and this build could not tell it from the owner's own note. The replacement now also writes a superseded-original marker record beside its pending record, keyed under a control-character prefix no legacy note can be listed as, carrying the replaced generation (hash and stamp) in the fields every build preserves. The prior build keeps records it does not list, so once this build runs again it finds the marker, sees the note's record moved elsewhere (or tombstoned), and removes the original while it is still that generation; an owner-edited one is the owner's and only the marker is dropped. A marker whose record still names the same target is a replacement in flight (finished by the pass) or one resolved there (stale, dropped); settling drops it, and the install's record restore drops it too. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$112.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=112.74 -->
… unanswered generation probes transient Four in-layer fixes from review of the reconciliation stabilization: - The superseded-original cleanup applies the deletion loop's sibling reliance guard (shared siblingReliesOn): while any sibling's settled reuse receipt still names the original's generation, the file and the marker stay (transient skip) until that sibling has migrated. - In the same path, a generation probe that fails (unreadable lstat) is a transient skip that retains the marker, instead of dropping it unconditionally — the cleanup completes on the next pass. - The unchanged-record fast path's shared-receipt check treats an unreadable target generation as an unanswered question (transient skip, no memoization) rather than a settled receipt, mirroring the prior-record branch; an absent target is settled as before. - LocalMemoryStore.listFiles classifies DT_UNKNOWN entries by lstat BEFORE sorting, so an untyped directory sorts with its "name/" key and the bounded fingerprint's capped subset stays a global lexicographic prefix (a sibling file such as a.md is not pushed out by a large untyped a/). --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$127.20`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=127.20 -->
…crash, bound dirent typing, heal malformed sibling manifests Four follow-ups on the reconciliation stabilization: - A note migrating off a shared copy in a full owner store no longer deadlocks with the copy's creator: when the creator has lost its legacy source (the copy would follow it out, but cannot while this note relies on it) the file is taken over by rename — no slot, same generation as the receipt. While the creator keeps its source the note stays on the shared copy as a transient (unmemoized) skip, so freed space or the creator's deletion is picked up on the next access instead of after a restart. - While a migration's new copy is not yet installed, the receipt naming the old copy is retained as a created: false superseded-marker record, so the creator's reliance check still sees it: a crash in that window can no longer let the creator's source deletion remove the only copy. The marker is dropped when the migration settles or is restored; the marker step skips any marker whose record is pending. - LocalMemoryStore.listFiles types DT_UNKNOWN dirents with at most 16 lstats in flight (classification still precedes sorting). - The sibling scan quarantines a MALFORMED sibling manifest through the same self-healing step the sibling's own pass uses (it then claims nothing), instead of blocking this child's every pass and removal until a sibling that may never run a pass heals it; an UNREADABLE manifest still fails the question. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$136.97`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=136.97 -->
…d spies Fills the classified-entry array by index instead of preallocating it (consistent-generic-constructors / no-unsafe-assignment), and types the readFile spy targets as strings (no-base-to-string). --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$136.97`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=136.97 -->
… plain receipt, drop take-over renames and marker-key reliance Review of the previous layer found the two mechanisms it added for shared-receipt migration to be the problem: taking the creator's copy over by rename left every other descendant's receipt pointing at nothing (their fast path read that as settled), left the owner metadata keyed by the old path, and — after a crash between the pending manifest write and the rename in a full store — a permanent capacity skip that also blocked forced removal; and retaining the reliance receipt under the superseded-marker control key was tombstoned by the previous build's unlisted-key reconciliation on downgrade, after which the creator could delete the only shared copy. Both are removed. A migration now installs the descendant's own copy FIRST while the manifest still holds the plain created: false receipt naming the shared copy — the form every build reads, and the sibling's reason to keep that copy (siblingReliesOn) — and only then flips the record to created: true for the new target, in the pass's single manifest write. No pending record precedes the install (it would replace the receipt), so a retry recognizes a copy installed before the flip by its identical bytes at this descendant's own import slot and claims it instead of writing a second one; a crash before the install leaves nothing to recover. A downgrade in the window sees the receipt exactly as before plus, at worst, one redundant file this build claims on return. The install is exempt from the capacity check — at most one extra file per outstanding shared receipt, bounded by prior state — and the creator's file is never renamed, so its normal deletion (removeKeys included) returns the slot once nobody relies on it. The sibling scan reads the config strictly: an unreadable or malformed config.json no longer answers "no other descendants" (which let the deletion and superseded-copy paths remove a generation a sibling still names); callers treat the throw as transient. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$153.06`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=153.06 -->
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
|
@codex review |
ThomasK33
added this pull request to stack #4218
September 14, 2026 07:48
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 151590e705
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…any identical occupied candidate A migration retry that found an identical file at the descendant's own import slot marked it adoption-created. No receipt was recorded before that install, so the file was indistinguishable from one the owner wrote there — the slot is inside the writable notebook — and after the legacy source was deleted, reconciliation trusted that provenance and removed the owner's file. The claim is dropped. The retry now follows the same rule as first-time placement for a byte-identical occupied candidate: reused non-destructively with a plain created: false receipt naming it, never claimed. Destructive provenance is only ever taken from a receipt recorded before an install (targetStamp), never from a byte match. The scenario still converges in one pass with no second copy; the bounded outcome of a crash between install and flip is one redundant copy that adoption never deletes. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$171.20`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=171.20 -->
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
ThomasK33
added this pull request to the merge queue
Sep 14, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Sep 14, 2026
Member
Author
ThomasK33
removed this pull request from stack #4218
September 14, 2026 12:17
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 14, 2026
…rden guarded reads, removal pinning and legacy adoption (coder#4250) ## Summary Final layer of the split of coder#4139. coder#4210–coder#4245 landed on `main` through the merge queue; this PR carries the two remaining layers as one diff (the queue's head-branch auto-delete closed coder#4248 and dropped this PR mid-stack, so they were rebased onto `main` and collapsed here): the install-before-flip migration that replaced take-over-by-rename (former coder#4248 / coder#4254), the post-rebase hardening of guarded reads, removal pinning and legacy adoption, and the recovery round from this PR's own review against `main`. ## Implementation **Migration off a shared adoption copy** (former coder#4248/coder#4254) 1. *Install before flip.* A note migrating off a sibling-owned copy stages and installs its own copy at `imported/<child>/<rel>` while the manifest still holds the plain `created: false` receipt naming the shared copy — the shape every build reads and what `siblingReliesOn` checks. Only after the install does the record flip, in the pass's single manifest write. No control key is ever written for a migration, so a downgrade in the window sees exactly the receipt it already understands. 2. *Retry.* Crash before install: nothing changed. Crash after install, before flip: an identical file at the child's own import slot is reused non-destructively (plain `created: false` receipt) — never claimed as adoption-created, because without a receipt recorded before the install it cannot be told from an owner-authored note. If a downgraded build then edits the legacy note in that window, the pass fails closed (documented limitation at `legacyImportTarget`): the edit stays in the legacy session store, the receipt keeps protecting the shared copy, and the non-forced handover rejects until the redundant slot is cleared by hand. 3. *Capacity.* A migration at the per-scope cap is a transient, unmemoized skip: the note keeps standing on the shared copy, the creator's deletion waits, and the migration completes once a slot frees. The store never exceeds the cap; take-over by rename and the earlier cap exemption are both gone, so no receipt is ever left naming an absent target and no owner metadata is stranded. 4. *Strict sibling scan.* `descendantAdoptionRecords` reads the config with `throwOnError`; an unreadable or malformed `config.json` is a transient skip, never "no siblings". **Hardening** (former layer 7 + this PR's review rounds) 5. Guarded reads: the acting/`guardedWorkspaceId` tombstone check runs before and after reads in every scope, with a final all-scope gate over the assembled hot set, index and root listing; a late tombstone first observed in the second gate also clears global/project items. 6. Descendant pinning during removal is a config-only edit that runs before the metadata lookup (phantom-cleanup path included); a non-forced removal aborts when the pin cannot be verified. 7. Legacy adoption: names the parser only admits after normalization are a reported permanent skip; a non-regular entry at the old source counts as deletion; manifest keys and receipt targets are validated for containment before any probe; excluded dot entries skip `DT_UNKNOWN` classification. 8. Sibling checks use file identity (`ino:size:mtimeNs`) rather than spelling, with the receipt identities probed once per pass (cache evicted for paths the pass mutates). A same-inode match counts as relying when it is a single directory entry (`nlink 1`) or the spellings fold to the same name; an independent hard link does not block the creator's deletion. 9. Read-side re-ranking emits the memory change event with `reason: "access"` (optional field on the in-memory/IPC event) so `MemoryBrowser` no longer labels unchanged notes as agent-edited. No new persisted fields, files or modules. ## Validation Every fix has a test that was red on the code it replaced, including: three descendants at cap migrate one by one as slots free and the store never exceeds the cap; a crash between stage and install leaves the manifest byte-identical; a copy installed before the flip is reused without being claimed and an owner-authored identical file survives the later source deletion; an edit made after a crash between install and flip is left unfolded with nothing overwritten; hot set, index and root listing all withhold already-accumulated global items on a late tombstone; receipt-identity probes are bounded by the receipt count; hard link vs case alias. ## Known limitations (later work) - Crash between a migration's install and its receipt flip followed by a downgraded edit of the same note: fail-closed, manual slot clean-up (see `legacyImportTarget`). - Cross-process ownership revalidation of an `AgentSession`'s cached memory context after another backend removes the owner (multi-backend layer). - Rollback owner-root resolution for adopted rows; throttled full-store fingerprint; sidecar locking; concurrent removal across backends (as replied on the earlier layers' threads). ## Risks Low to medium, confined to the adoption pass, manifest, and read gating (fail-closed in both directions). No destructive decision is ever taken from byte identity alone; every destructive step requires a receipt recorded before the install. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$1260.46`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=1260.46 -->
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
Replaces the two mechanisms that #4245's second review round introduced for migrating a descendant off a shared adoption copy — take-over-by-rename at the file cap, and a reliance record under the superseded-marker control key — with a simpler install-before-flip migration. The five findings from #4245's third review round (multi-sibling take-over, crash before the take-over rename, marker tombstoned on downgrade, tolerant config read in a destructive check, stale owner metadata after take-over) disappear by construction;
memoryService.tsshrinks by 51 lines.Stack: layer 6 of the split of #4139 (base:
memory-sharing-l3a3, #4245). Closes the open review threads on #4245; the shippable bundle is#4210 + #4217 + #4220 + #4224 + #4245 + this PR.Implementation
imported/<child>/<rel>while the manifest still holds the plaincreated: falsereceipt naming the shared copy — the shape every build reads and exactly whatsiblingReliesOnchecks. Only after the install does the record flip tocreated: truefor the new target, in the pass's single manifest write. No pending record precedes the install, so no control key is ever written for a migration.created: falsereceipt naming it) — it is never claimed as adoption-created, because without a receipt recorded before the install it cannot be told from an owner-authored note. Destructive provenance comes only from atargetStamprecorded before an install.imported/<child>/that this build reuses (never deletes) on return; older builds do not reconcile unlisted files underimported/.remainingCapacityis still decremented). The creator's file is never renamed, so its normal deletion loop (removeKeys+remove) returns the slot once no receipt relies on it. Migration skips are transient only.descendantAdoptionRecordsreads the config withthrowOnError, so an unreadable or malformedconfig.jsonthrows (callers already treat that as a transient skip) instead of yielding an empty project map that reports "no siblings" to the deletion paths.Removed:
siblingOwner/siblingSourceGone/migratingFrom/takeOver, the take-over staging/installable/restore branches, theDescendantAdoptionRecordscan shape, and the reliance-marker write. Kept from #4245: the deletion-loop reliance guard and superseded-copy cleanup, marker preserved when its generation is unreadable, boundedDT_UNKNOWNclassification, malformed sibling manifest → quarantine, transient null stamps.Validation
Four new tests, each red on
memory-sharing-l3a3: three descendants at cap all migrate off one shared copy and the creator's deletion then clears the copy and its pin; a crash between stage and install leaves the manifest byte-identical (no pending record, no control keys) and the creator's deletion keeps the copy until the retry finishes; a copy installed before the flip is reused without being claimed, and an owner-authored identical file at the import slot survives the migration and the later source deletion; an unreadable config leaves everything in place and the pass unmemoized. The two #4245 tests that encoded take-over and marker reliance are replaced by these scenarios.Risks
Low to medium, confined to the adoption pass and manifest. The one residual outcome is a single redundant identical file under a descendant's own import slot after a crash between install and flip; adoption reuses it as a non-owned receipt and never deletes it.
Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:high• Cost:$1197.33