🤖 feat: keep shared sub-agent notebooks safe across workspace removal - #4217
🤖 feat: keep shared sub-agent notebooks safe across workspace removal#4217ThomasK33 wants to merge 4 commits into
Conversation
…oved Removing an intermediate sub-agent would leave its children's parentWorkspaceId chain dangling, so they would fall back to private notebooks. Before any destructive step, removal records memoryOwnerWorkspaceId on each surviving direct child (the owner it resolves to now), verified by reading the config back; the resolver honors a pin only once the recorded parent is gone, and a pin whose owner is gone leaves the child on its own store. A non-forced removal aborts (workspace intact) when the pin does not persist. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$18.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=18.74 -->
…s tombstoned Reads have no commit guard, so a removed child's (or owner's) stream in another backend could keep viewing the shared store. Every workspace-scope entry point now opens the store through a tombstone gate on the acting workspace, the store's physical owner and an optional guardedWorkspaceId (a child whose consolidation run sweeps under the owner's identity), and re-checks before any bytes or listing leave the service (view, index, hot set, UI reads, pin toggles). The commit guard binds the owner to the resolved store instead of re-resolving it. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$18.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=18.74 -->
…ted removals Once the session directory is gone, retryable harvest records can never be retried: removal marks them terminal (finalizeHarvestsForRemoval) and a residual retryable write can no longer reopen a terminal record — only a genuine completion may replace it. A removal that aborts before its point of no return lifts the in-process teardown gate (releaseRemovalCancellation) so the surviving workspace consolidates again. Redirected child runs carry guardedWorkspaceId so the child's tombstone refuses the owner-keyed run. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$18.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=18.74 -->
--- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$18.74`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=18.74 -->
ThomasK33
changed the title
|
@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: ec06f19e3a
ℹ️ 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 review |
|
Codex Review: Didn't find any major issues. Breezy! 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. |
1 participant
Summary
Removal safety for the shared sub-agent notebook introduced in #4210. Removing a workspace in a task tree no longer strands or leaks its shared memory: surviving descendants are pinned to the owner they were using, reads and lists refuse once the owner or actor is tombstoned (no exposure of a removed workspace's data), and a removed child's harvest state is finalized without new provider work while an aborted removal releases the teardown gate again.
Stack: layer 2 of the split of #4139 (base:
memory-sharing-l1, #4210). Addresses the two removal-related findings deferred from #4210's review; rollback of a child's refinements through the owner root follows in the next layer.Background
With #4210 alone, removing an intermediate task made its children's
parentWorkspaceIddangle, which resolved them to an empty private store and dropped them out of the tree's notebook. A tombstoned owner also still served reads to its children, and a child's retryable harvest records could be retried after its teardown. The review scope rule sends these to their own stacked PR because they need a persisted field (memoryOwnerWorkspaceId) and removal-path changes.Implementation
memoryWorkspaceOwner.ts: the resolver honors amemoryOwnerWorkspaceIdpin only while the recorded parent is gone (a live parent always wins, so a stale pin heals on the next removal instead of redirecting a child into another tree).pinDescendantWorkspaceMemoryOwnersrecords, for each direct child of the workspace being removed, the owner it resolves to right now.WorkspaceService.removeWorkspace: pins descendants before any destructive step and verifies the pins persisted (a non-forced removal aborts and stays retryable if they did not); the resolved owner is reused for the owner store lock;finalizeHarvestsForRemovalruns after the session directory is removed;releaseRemovalCancellationruns when the removal aborts before committing.MemoryService:openWorkspaceStoregates every workspace-store resolve on the actor's and owner's tombstones; reads (view,readFileWithSha, index and hot-set listing) re-check after reading so a workspace tombstoned mid-read exposes nothing.assertMutationCommittablebinds the owner from the store path and checks every guarded workspace (guardedWorkspaceIdlets a redirected consolidation run act on a removed child's behalf safely).MemoryConsolidationService:finalizeHarvestsForRemovalmarks a removed workspace's outstanding harvest records terminal so recovery never retries them; terminal records are never overwritten;releaseRemovalCancellationlifts the in-process teardown gate for an aborted removal.Known limitations (later layers)
config.jsonis unreadable at removal time the child resolves to itself and the owner store lock is not taken; the strict-load abort is part of the multi-backend layer.XUM_ALLOW_MULTIPLE_INSTANCES), a child registered between the pin pass and deregistration is not pinned, and harvest finalization is serialized only in-process; both are closed by the multi-backend layer.memoryOwnerWorkspaceId; notes a pinned child writes during a downgrade land in its private store and are picked up by the legacy-adoption layer on re-upgrade.Validation
Ported and new tests: pin resolution (live parent wins, dangling parent uses pin, pin whose owner is gone falls back to self), grandchild keeps the root store after the middle node is removed, non-persisting pin aborts a non-forced removal with nothing destructive run, reads refused once the owner is tombstoned, mid-read tombstone withholds the result (including hot set), pin toggle refused on a tombstoned owner, removal-finalized harvest records stay terminal with no model calls, teardown gate released only on abort. Sibling suites: 1787 pass.
Risks
Medium, scoped to task-tree workspaces. New persisted field
memoryOwnerWorkspaceIdis optional and ignored by older builds (they fall back to per-workspace behavior). Read gates add tombstone probes on memory reads for workspace scope.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:high• Cost:$1012.00