🤖 feat: reconcile downgrade-time edits of adopted sub-agent notebooks - #4224
Open
ThomasK33 wants to merge 7 commits into
Open
🤖 feat: reconcile downgrade-time edits of adopted sub-agent notebooks#4224ThomasK33 wants to merge 7 commits into
ThomasK33 wants to merge 7 commits into
Conversation
…s, escape child ids in import paths Three adoption-pass hardening items carried from #4220 review: - BOM: both strict decoders (legacy source, owner destination) now pass ignoreBOM so a leading U+FEFF survives into the copy and its hash, and a BOM-less owner note no longer compares equal to a BOM-prefixed source. - Strict walk: LocalMemoryStore.listFiles classifies a dirent whose type the filesystem did not report (DT_UNKNOWN, every predicate false) by lstat instead of dropping it, so removal's handover cannot see a complete listing that omits a note or a subtree. - Import segment: imported/<childId>/ is built through legacyImportSegment — ids the memory path grammar admits are used verbatim, the rest (a legacy id keeping a project basename's `~`, `..`, `%2e`, control or XML characters) are escaped per UTF-8 byte as `=XX`; a verbatim segment never contains `=`, so the forms cannot collide. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$86.05`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=86.05 -->
…ed legacy notes The adoption pass now follows what a downgraded build does to a legacy note after its first adoption, using the generation stamps the manifest already records as provenance: - In-place replacement: legacy bytes edited while the copy THIS adoption created is still its own generation (targetStamp, or replacementStamp on the far side of an interrupted replacement) replace that copy — staged, receipted, and installed only if the target is still the generation the decision was made against. A copy the owner edited or recreated keeps the owner's bytes and the edit lands under imported/. The pending prior record carries replacementContent/replacementStamp so a retry recognizes the copy on either side of an interrupted write; a copy the owner replaced is recorded `replaced` so the child's pin toggles no longer reach it. - Deletion/rename reconciliation: a source proven gone (ENOENT/ENOTDIR) removes the adoption-created copy only while it is unchanged and still this adoption's generation; owner-authored or owner-edited files stay. A rename onto the path of its own conflict copy transfers provenance to the successor record (with the generation actually on disk) instead of deleting the file from under it. pendingDeletion is written before the removal so a crash between removal and tombstone is recovered as "removed by us", never as owner-changed; the record stays as a `deleted` tombstone (rollback mapping) and a reappearing source is adopted afresh. Target inspection goes through the R5 destination helper: non-regular entries, over-cap and non-UTF-8 files are owner state, transient failures keep the pass unmemoized. - Manifest: replaced/replacementContent/replacementStamp/pendingDeletion/ deleted fields parse fail-closed like the existing flags; records written by the previous layer read unchanged and are upgraded in place. Tests are the FINAL reconciliation suite (17), adapted only where this layer differs (lstat-based target inspection; a fresh sidecar instance for a cross-instance pin read). --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$90.30`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=90.30 -->
…ed copy A copy the adoption created has no owner choice behind its pin: the descendants whose legacy notes it represents own it together, so their pins now fold as an OR. Two pre-sharing descendants with the same note, the first adopted unpinned: the second's pin is no longer dropped as "the owner's choice". Both pinned, one unpinned on the old build: the other's pin still protects the note, and a later unpin on that one is the transition that clears it. Ownership is decided by the LIVE generation, never by target path and flags alone: this pass's record (a fresh write or a copy still stamped as ours), or another descendant's settled record whose receipt (targetStamp/replacementStamp) matches the stamp of the file on disk. An owner-edited or recreated generation is the owner's — the owner's pin stands and the existing transition rule applies. Only a true aggregate is applied; with every owning pin off, an unpin remains this child's own transition, so a mere view never clears a pin the owner set. Sibling manifests are read tolerantly (nothing contributes on failure), once per pass and only when a note has a sidecar entry to fold. MemoryMetaService.mergeKeys gains `pinned: "on"` for the aggregate. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$95.19`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=95.19 -->
ThomasK33
added this pull request to stack #4218
September 11, 2026 18:38
ThomasK33
changed the title
ThomasK33
marked this pull request as ready for review
September 11, 2026 18:39
Member
Author
|
@codex review |
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. |
🛡️ 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. |
![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: 8a20160125
ℹ️ 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".
…hared copy This reverts commit 8a20160 (mergeKeys `pinned: "on"`, the OR aggregate over sibling manifests, its tests). Review of #4224 showed the shared-copy model it aggregated over is itself the problem: one owner file standing for several descendants' notes lets one descendant's in-place replacement destroy a sibling's bytes, one descendant's source deletion remove the copy the others still need, a reused generation never be marked replaced, and an unreadable sibling manifest change pin outcomes. The follow-up commit makes an adoption-created target belong to exactly one descendant instead, so pins fold per copy under the existing rule. The FINAL pin test that exercises that rule (owner-owned generation untouched, a real child toggle folds) is kept. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$104.71`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=104.71 -->
…an identical sibling's An adoption-created target now belongs to exactly one descendant. A descendant whose legacy note is byte-identical to an existing owner file reuses it only when that file is NOT another descendant's adoption-created copy — decided by the LIVE generation: a sibling's settled `created` record naming the path whose receipt (targetStamp, or replacementStamp on the far side of an interrupted replacement) equals the stamp on disk. Otherwise the note gets its own copy under imported/<child>/ (created: true), and its pin folds into that copy under the existing per-copy rule. The owner's own identical notes keep the previous reuse (created: false, pin "target"); an owner-edited generation is nobody's to reuse against. The sibling manifests are read strictly, once per pass and only when a candidate is identical: an unreadable or malformed one leaves the question unanswered, so the note is skipped transiently (no reuse, no pin change, pass not memoized) rather than reused against a copy that may be a sibling's. The check runs both at placement (legacyImportTarget) and when a prior record's target turns out not to be ours. By construction this removes the failure modes of a shared copy: one child's in-place replacement rewriting a sibling's bytes, one child's source deletion removing a copy others still need, a reused generation that could never be marked replaced, and pin outcomes depending on a sibling's manifest being readable. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$104.71`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=104.71 -->
…ed notes The deletion/rename reconciliation now runs BEFORE the listed notes are adopted, and the slot a removed copy frees is credited to the same pass. A downgraded rename in an owner store at capacity therefore lands at once: previously the new name was skipped as "full" (and the pass memoized) while the old copy still held the slot, so the rename never completed until something else changed. Reconciling first also makes the successor-provenance transfer unnecessary: a note renamed onto the path of its own conflict copy finds the path free (the old copy removed under its own record's provenance, tombstoned created: true) and is adopted as a fresh copy with its own generation, instead of one file being handed between two records. The two FINAL successor tests are adapted to that outcome. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$104.71`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=104.71 -->
…hem as unsettled A `deleted` tombstone is now written with `pending: true`. The previous build knows neither the flag nor the reconciliation: it ignored the tombstone's unknown fields and, finding the settled hash and sidecar, took a reappearing source as "folded in earlier" — its forced handover then reported completion and deleted the child session while no copy existed. Pending, that build re-adopts the source (its target is free). Here `deleted` takes precedence, so a tombstone never reads as an interrupted adoption. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$104.71`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=104.71 -->
Member
Author
|
@codex review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22861779b1
ℹ️ 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".
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
Reconciliation for adopted legacy notebooks (#4220). When an older build edits, deletes, or renames a sub-agent's pre-sharing note after it was adopted into the shared store, the adoption pass now updates the adoption-created copy in place (never an owner-authored file), removes or moves adoption-created copies whose source is gone, recovers idempotently from an interrupted replacement or deletion, and gives every descendant its own adoption-created copy (a note identical to another descendant's copy lands under
imported/<child>/, so replacement and deletion never touch a sibling's copy). It also closes the three hardening items carried from #4220's last review round: BOM-preserving byte-exact compare and copy,lstatclassification of unknown dirent types in strict walks, and escaping of child IDs inimported/<child>/paths.Stack: layer 4 of the split of #4139 (base:
memory-sharing-l3a, #4220). With this layer the minimum shippable bundle is complete:#4210 + #4217 + #4220 + #4224.Background
#4220 kept downgrade behavior simple (an edited note re-adopts as an
imported/conflict copy; a deleted source keeps its copy). Codex showed a concrete data-loss path in that simplification: a second edit of a note already underimported/<child>/could not be placed, so non-forced removal refused and a forced removal deleted the only copy. Every deferred thread on #4220 that named this layer is closed here.Implementation
replaced,replacementContent,replacementStamp,pendingDeletion, anddeleted(fail-closed flag parsing; 🤖 feat: adopt pre-sharing sub-agent notebooks into the shared store #4220-format records are read unchanged and upgraded when rewritten).imported/<child>/as before. Capacity is not charged for a replacement.pendingDeletionis written before the sidecar and file are removed so an interrupted deletion recovers; a rename transfers provenance to the successor with the on-disk generation.{ fatal: true, ignoreBOM: true }; strictlistFilesclassifies unknown dirent types vialstat;legacyImportSegmentescapes child IDs the memory path grammar rejects (=XXper UTF-8 byte).Known limitations (later layers)
Not merge-ready on its own: a stabilization layer (L3a-3) stacked on this PR owns the second review round's findings: one-time migration of parent-format shared receipts before destructive reconciliation, DT_UNKNOWN classification in the bounded fingerprint walk, downgrade safety of an interrupted in-place replacement, and treating an unavailable live stamp as transient.
Refinement-row migration and rollback path remapping for adopted files (L3b); rollback of a child's own rows still fails closed.
Throttled full-store fingerprint for over-cap legacy stores, cross-process sidecar locking, second-backend removal races (L4).
Deletion tombstones are written with
pending: trueso the previous build treats them as unsettled; a descendant whose older record reused a copy that turns out to be sibling-owned migrates to its own copy on its next non-trivial pass.Validation
26 new tests: in-place edit reflected without a duplicate, owner-edited target left alone, legacy deletion and rename follow adoption-created copies only, owner note recreated at a deleted target preserved, interrupted replacement and deletion recovery, pending-deletion target never read, reappeared identical source re-adopted, provenance transfer on rename, sibling-owned identical target forced to its own copy with owner-authored control, unreadable sibling manifest waits, rename at capacity lands in one pass, tombstones read as unsettled by the previous build, BOM survives byte-exact, DT_UNKNOWN dirents adopted,
proj~1-child→imported/proj=7E1-child/. Each part was red-checked against the previous code. Sibling suites: 1803 pass.Risks
Medium, confined to the adoption pass and manifest. Replacement and deletion touch only files whose provenance stamp proves adoption created them; owner-authored files are never modified or removed.
Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:high• Cost:$1117.25