🤖 fix: preserve staged attachments across snapshot archives - #4167
🤖 fix: preserve staged attachments across snapshot archives#4167ibetitsmike wants to merge 8 commits into
Conversation
Snapshot archives captured only git-visible state, so the git-excluded .xum/user-attachments uploads were lost when the worktree was removed. Copy them into the archive-state artifacts at capture time, restore them into the recreated checkout (re-adding the info/exclude entry), and stop reporting the attachments' container directory as lossy untracked data. Fixes #3947
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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 review |
This comment has been minimized.
This comment has been minimized.
![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: bdfcedbadd
ℹ️ 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".
…ing checkouts, strict stat, symlink-safe restore - Store the upload copies in a sibling archive-attachments dir so an older build's restore (which deletes archive-state) cannot destroy them. - Restore missing uploads into an already-matching checkout before the snapshot is cleared. - Only ENOENT/ENOTDIR count as an absent staging dir; other stat errors abort capture. - Check the restore target through symlinks so a repo-controlled link cannot redirect the copy outside the checkout.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c5814d02
ℹ️ 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 comment has been minimized.
This comment has been minimized.
…ng dirs by contents - clearSnapshotState removes only the artifact entries the snapshot references (pruning empty parents) instead of the whole archive-attachments dir, so copies stranded by a downgrade cycle survive; capture merges into the tree rather than replacing it. - Capture copies from the resolved real path so a symlinked staging directory yields its files, not a link that dangles after the worktree is removed; restore likewise copies into the resolved target.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a675381d30
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: a675381d30
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…g dirs, scope the lossy filter - Capture writes entries to a temp dir and replaces exactly those paths before the metadata commit, so a stale same-path copy cannot resurrect a deleted upload while unrelated copies stay untouched. - A staging directory that resolves outside the checkout fails capture instead of being skipped: the link goes with the worktree, so the uploads would not come back. - The lossy-untracked check only drops .xum/.mux containers whose contents are all ignored; empty directories elsewhere keep their warning.
…the captured uploads A .xum/ (or .mux/) entry leaves the lossy-untracked warning only when its ignored contents are exactly the staged attachment directory the snapshot preserves; other excluded files in there (e.g. a workspace MCP override) are still lost with the worktree and keep the warning.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d2a28085
ℹ️ 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".
Git lists neither ignored files nor empty directories, so decide whether a .xum/ entry holds nothing but the captured staged attachment directory with a readdir instead of an --ignored listing; an empty sibling directory keeps the lossy warning.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce35787049
ℹ️ 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 comment has been minimized.
This comment has been minimized.
… attachment paths - Capture fails when the staged attachment tree contains a symlink; the archived link would point into the deleted checkout. - repoRelativeDir must name a .xum/.mux staged attachment directory and artifactPath must resolve inside archive-attachments/, so tampered config can neither write into tracked directories nor delete other session files.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd1e952fdf
ℹ️ 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".
| await fsPromises.rm(artifactDir, { recursive: true, force: true }); | ||
| await fsPromises.mkdir(path.dirname(artifactDir), { recursive: true }); | ||
| await fsPromises.rename(tempArtifactDir, artifactDir); |
There was a problem hiding this comment.
When an archive created by this build is unarchived by an older build, that build leaves the sibling artifact here but recreates the checkout without its contents. If the user later upgrades, stages any new upload, and archives again, the same storageKey/repoRelativeDir is reused and this unconditional removal permanently deletes the sole copy of every older attachment before installing only the new staging tree. Fresh evidence beyond the stale-artifact thread is that this staging absence results from the older restorer ignoring stagedAttachmentDirs, not from the user deleting those files; adopt or preserve such downgrade orphans before replacement.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changed in this round; escalating this one for a human decision because it reverses the guidance from the earlier "Replace stale attachment artifacts before capture" thread (P2), which asked for exactly this atomic same-path replacement so a stale copy cannot resurrect a deleted upload. The two requests conflict: a same-path leftover is either a downgrade orphan (should be preserved/adopted) or a stale copy from a restore whose snapshot-state writeback failed (should be replaced), and nothing on disk distinguishes them. Options on the table: (A) keep replacement (current), documenting that a downgrade-then-re-archive of the same path supersedes the orphan; (B) merge the leftover into the new entry so nothing is ever deleted, accepting that a deleted upload can reappear after that writeback-failure case; (C) quarantine the leftover under a sibling name that restore also merges back. I will apply whichever the maintainer picks; leaving this thread open until then.
…ot, sweep stale temp dirs - A malformed stagedAttachmentDirs entry is skipped with a warning (its artifact left for manual recovery) instead of failing every unarchive. - The archive-attachments root must be a plain directory; a symlink there is treated as absent so nothing outside the session dir is written or deleted through it. - Capture sweeps temp directories left by an interrupted capture.
|
@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. |
1 participant
Summary
Snapshot archives (the
snapshotworktree archive behavior) capture only git-visible state before removing the managed worktree, so chat uploads staged into the git-excluded.xum/user-attachmentsdirectory were silently lost: after unarchive, persisted chat/draft attachment paths pointed at files that no longer existed. The snapshot now copies those directories into the session dir at capture time and restores them into the recreated checkout.Fixes #3947
Background
WorkspaceService.stageAttachmentwrites uploads into.xum/user-attachmentsunder the workspace execution path and excludes that directory through the worktree'sinfo/exclude, so the agent can read them with ordinary filesystem tools.WorktreeArchiveSnapshotService.captureSnapshotForArchiveonly records a format-patch mailbox plus staged/unstaged diffs, and ignored paths never reach any of those. Origin: Codex round 19 P2 on #3940 (placement leg; #3940 closed the in-flight race).Implementation
Of the three directions in the issue, this takes option 2 (include the attachment directories in the snapshot) rather than moving staged writes into the session dir (option 1) or rewriting chat references at archive time (option 3):
bash,file_read) inside the workspace runtime. For SSH, Coder, Docker and devcontainer workspaces the session dir lives on the xum host, not on the runtime, so attachments have to stay inside the checkout there. Option 1 would need runtime-conditional placement plus a compat story for every persisted.xum/user-attachments/...reference (chat notices, drafts, fork copies, download). Option 3 rewrites persisted chat history.WorktreeArchiveSnapshotProjectSchema, written under a temp dir and renamed with the metadata).appendSubProjectRelativePath), which also covers sub-project workspaces.Capture copies each existing
.xum/user-attachments(and legacy.mux/user-attachments) directory (resolved through symlinks, so a linked staging directory yields its contents rather than a link that dangles once the worktree is gone) into<sessionDir>/archive-attachments/<storageKey>/<repo-relative dir>and records{ repoRelativeDir, artifactPath }entries in the new optionalstagedAttachmentDirsfield. Entries are written to a temp dir and each one atomically replaces the same path in that tree before the metadata is committed; clearing a snapshot removes only the entries it references (empty parents pruned). A staging directory that resolves outside the checkout, or one containing a symlink, fails the capture: the link would be lost with the worktree and the payload could not be restored. PersistedrepoRelativeDirvalues must name a.xum/.muxstaged attachment directory andartifactPathvalues must resolve (through symlinks) inside a realarchive-attachments/directory; a malformed entry is skipped with a warning (its artifact left in place for manual recovery) instead of blocking every unarchive, and a symlinked artifact root is treated as absent so nothing outside the session dir is read or deleted. Temp directories left by an interrupted capture are swept at the start of the next one. Restore copies them back after the patch replay and re-runsensureGitInfoExclude, because the recreated worktree starts with an emptyinfo/excludeand the uploads would otherwise surface as untracked files. The existing-checkout fast path (git state already matches the snapshot) restores missing uploads before the snapshot is cleared. A referenced artifact that is missing on a fresh restore fails it like the existing patch artifacts do (checkout cleaned up, snapshot kept for retry); a stat failure other than ENOENT/ENOTDIR aborts capture instead of treating the directory as absent; and the restore target is checked through symlinks (deepest existing ancestor realpath) so a repo-controlled link cannot redirect the copy outside the checkout.listUnsupportedUntrackedFilesdrops a.xum/or.mux/container entry only when the directory holds nothing but the staged attachment directory the snapshot now preserves (checked on the filesystem, since git lists neither ignored files nor empty directories).git ls-files --others --directoryreports a directory whose contents are all ignored, so every workspace with a staged attachment triggered the "lossy untracked files" confirmation for.xum/; anything else in there (for example an ignored workspace MCP override or an empty directory) is still lost with the worktree and keeps the warning, as do empty directories elsewhere.Upgrade/downgrade: the snapshot version stays 1 and the new field is optional. The attachment copies deliberately live beside
archive-state, not inside it: an older build restoring a newer snapshot ignores the field and deletesarchive-state, so keeping the uploads in a sibling directory means the downgrade restores the checkout exactly as that build always did (without attachments) while the copies stay recoverable instead of being destroyed. A newer build restoring an older snapshot sees no field and skips the step. Because entries are replaced and removed individually, copies stranded by such a downgrade cycle are neither deleted by a later capture of other paths nor by clearing a snapshot that does not reference them; they stay in the session dir until the workspace is deleted or a later capture of the same path supersedes them.Validation
WorktreeArchiveSnapshotServicetests stage a real attachment throughstageWorkspaceAttachment, capture, remove the worktree with git, restore, and assert the bytes are back at the same relative path with a cleangit status(workspace root and sub-project execution path). Further cases: missing-artifact failure, reconciliation into an existing matching checkout, capture abort on an EACCES stat, refusal to restore through a tracked symlink that leaves the checkout, unreferenced copies surviving a snapshot clear, a stale same-path copy replaced while unrelated copies survive, capture refusal when the staging directory resolves outside the checkout, a symlinked staging directory captured by contents, empty directories still reported as lossy while an attachments-only.xum/is not, a.xum/holding another ignored file or an empty sibling directory still reported, a symlink inside the staged tree refusing capture, tamperedartifactPath/repoRelativeDirmetadata neither deleting session files nor writing into tracked directories (and not blocking the restore), a symlinked artifact root never being written through or deleted through, and stale capture temp directories being swept. Each guard was verified red first by removing it.Risks
Low. The change is confined to the snapshot capture/restore path and only adds work when an attachment directory exists. The lossy-untracked filter only affects
.xum/and.mux/entries whose ignored contents are exactly the captured uploads, so it removes a misleading warning, not data.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:xhigh• Cost:$8.42