fix(sandbox): normalize UnixLocal snapshot special files and symlinks

[coderdailyone]

Summary

This pull request fixes UnixLocal workspace snapshots containing FIFOs or eligible absolute symlinks to files inside the workspace. Persistence now omits FIFO/device entries and rebases eligible internal symlink targets so the archive can hydrate into a different workspace root.

Symlink headers are deferred until archive capture completes. The rebase preserves symlink and parent-step semantics and proves containment against the original captured member/link graph, so source mutations during traversal cannot approve a different topology. File payloads are written once. External, dangling, excluded, cyclic, or otherwise unprovable targets retain the existing strict hydration behavior from #3094. Public signatures and hydration policy are unchanged.

The branch incorporates current main and reuses the hardlink archive filter and validation before clearing the live workspace from #5188. Duplicate hardlink conversion and its redundant branch-specific assertions have been removed.

Test plan

• Deterministic regressions cover intermediate-link and later-target changes during archive capture, including rejection before modifying the destination.
• Focused UnixLocal/archive suites: 109 passed, 6 native macOS tests skipped in the Codex sandbox.
• Full required verification passed: formatting, lint, mypy, pyright, and tests (11,326 passed, 66 skipped across parallel and serial runs).
• Two independent reviews of the final complete diff passed, covering filesystem trust and persistence lifecycle. Final source fingerprints match the reviewed and verified content.
• Native macOS sandbox coverage remains enabled in CI; it is intentionally skipped only inside the Codex sandbox.

Issue number

No new closing issue. Hardlink handling is already covered by #5188; Docker's corresponding archive behavior is tracked separately in #4834.

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all local verification steps pass
• If using Codex, I've completed the required independent final review

[sylvesterkaczmarek]

Normalising at archive creation is the right boundary because hydrate already has a strict, security-relevant contract. Converting hardlinks to payload-bearing regular members and relativising only absolute targets proven inside the workspace preserves that contract without weakening extraction.

[seratch]

Thanks for the contribution. Fixing archive creation is the right direction, and the hardlink restore failure warrants a focused fix.

Before merging, please correct the absolute-symlink rewrite so it preserves path semantics. With current -> releases/v1, a link to <root>/current/../config refers to <root>/releases/config, but normpath() collapses it to <root>/config. The restored link can therefore silently read a different file.

Please preserve the target components when replacing the workspace-root prefix, avoiding normalization that collapses .. across symlink components. Add a persist/hydrate regression with different contents at those two paths and assert that the restored link reads the original contents.

[coderdailyone]

Thanks — you're right that normpath() changed the meaning. Fixed in 1e1806d4: the rewrite now only replaces the workspace-root prefix with the climb out of the link's own archive directory and keeps the target's components verbatim, so <root>/current/../config becomes current/../config (or ../../current/../config from releases/v1/) and the kernel still resolves the .. against the current symlink's target after restore. Regression test_rebased_symlink_keeps_parent_steps_after_symlink_components sets up current -> releases/v1, config = "wrong", releases/config = "right", persists and hydrates into a new root, and asserts both restored links read "right"; on the previous revision the link was rewritten to config.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e1806d47c

ℹ️ 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".

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-13T22:39:53.398411Z	ede33d1	New commits
🔒 Security Review	✅ Completed 2026-09-13T22:44:37.817238Z	ede33d1	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[sylvesterkaczmarek]

Re-reviewed current ede33d1 after the symlink rewrite fix and rebase. Absolute in-workspace targets are now rebased without collapsing .. across symlink components, the regression pins the current -> releases/v1 semantic case, and the later separator-run fix preserves the same boundary handling. Current CI and Codex/security reviews are clean. No remaining blocker from me.

[coderdailyone]

One more commit, 06365a87 (plus a ruff-format follow-up), for the containment gap Codex flagged on #4834: the verbatim rebase turns <root>/a/link/../tmp into a/link/../tmp, which hydrate's lexical check accepts, yet with a/link -> .. it names /tmp after restore. _restorable_tar_member now walks the rebased target through the workspace's own relative links first (applying .. to a link's target as the kernel does) and keeps the absolute form, which hydrate refuses as before, when the walk leaves the root, hops through an absolute-target link, or loops. test_rebased_symlink_that_escapes_through_a_link_stays_absolute covers those three plus a chain that resolves inside; the current -> releases/v1 case is unchanged.

[sylvesterkaczmarek]

Re-reviewed current 53638a7. The new source-workspace symlink walk closes the archive-owned containment case, but the destination-state gap identified on #4834 also applies here. UnixLocalSandboxSession.hydrate_workspace() extracts into an existing root, while _symlink_target_stays_under() proves containment only against the source workspace at persist time. A path component absent from the source snapshot can already be a symlink in the destination and change where the rebased target resolves after hydration. safe_extract_tarfile() checks existing symlinks in the destination path of the new link itself, but not symlinks later traversed by that link's target. Please account for destination state during hydration, or only rebase targets whose intermediate components are established by the snapshot itself. This remains a blocker for the strict external-target guarantee.

[coderdailyone]

Thanks, addressed in 04c7d513 by taking the second option: only targets whose intermediate components are established by the snapshot itself are rebased. The walk now requires every non-link component to exist in the workspace, not be excluded by the persist skip list, and be a directory unless it is the leaf (which must be a regular file or directory); relative links are still followed with .. applied to their targets. Anything the snapshot does not create, a dangling leaf included, keeps its absolute form for hydrate to refuse as before, so a destination that already holds a symlink at such a path cannot change where a rebased link resolves. test_rebased_symlink_through_components_the_snapshot_does_not_create_stays_absolute covers an absent intermediate component, a dangling leaf, a file component, and a skipped directory; the same rule landed on #4834 for the archive-side walk.

[sylvesterkaczmarek]

Re-reviewed current 04c7d513. The destination-state containment gap is closed by requiring every rebased target component to be established by the snapshot itself: present, not skipped, and directory-shaped on intermediate steps, while relative source-workspace links are still resolved before the proof succeeds. Targets that cannot be proven safe keep their absolute form for hydration to reject. The new regression covers missing, dangling, file, skipped, and valid in-snapshot paths. No remaining blocker from my review.

[github-actions]

This PR is stale because it has been open for 10 days with no activity.

[sylvesterkaczmarek]

Rechecked current 04c7d513, which is unchanged since my last technical review. The destination-state containment blocker I raised remains resolved by requiring rebased target components to be established by the snapshot itself. No remaining blocker from my review.

[jbeckwith-oai]

Maintainer assessment of 04c7d51388f2a03b20ecb59ff74a3ac924351fc5 against the full merge-base diff and current main (c42f3c6e).

Need status: demonstrated, with the hardlink portion now already covered. A successfully persisted workspace can fail strict hydration because its archive contains hardlinks, FIFOs, or absolute workspace-local symlinks. The contributor's reported persist/hydrate cases and the producer/extractor paths establish the mismatch. This is a worthwhile snapshot-reliability problem. However, #5188 has now merged the hardlink fix and validation before clearing the live workspace, so this PR is no longer needed for those outcomes.

Recommendation: merge-worthy after focused changes. Repository readiness: rebase or conflict resolution required. Please rebase onto current main and remove this branch's duplicate hardlink conversion, using #5188's archive filter and retaining its pre-clear validation. The remaining change should cover FIFO/device omission and conservative relocation of eligible internal absolute symlinks, with the corresponding tests. This is an integration/scope adjustment, not an additional runtime defect found in the submitted implementation.

Implementation review: no actionable defects found in the complete two-file diff after two rounds, each with independent correctness and architecture reviewers. I inspected archive production, exclusion handling, strict validation/extraction, snapshot lifecycle, cancellation ownership, and the changed tests. The latest code addresses the earlier separator, symlink/parent-step, and destination-state review concerns. The tests inspect emitted payloads and perform persist/hydrate round trips into a new root. Public signatures and the strict external-link policy from #3094 remain unchanged; this does not make arbitrary symlink layouts or system-target links restorable.

Caller-created relative links and workspace cleanup can work when applications control every generated entry, but they do not automatically repair snapshots of ordinary generated workspaces. Exclusions also omit content. Producer-side normalization is the appropriate layer; weakening hydration or introducing a general resolver is unnecessary. The current bounded walk has a concrete role in preserving link semantics and checking snapshot-owned target components.

Validation limits and next action: this was a desk review, including test-source inspection, not execution of PR code, tests, or runtime probes. Contributor-reported test results were not independently reproduced. Current-head GitHub workflows for Tests and CodeQL report action_required; there are no completed check-run results establishing a passing gate. After the focused rebase, the new head needs review and passing required CI before merge. No fixes or other PR changes were made by this review.

[jbeckwith-oai]

Addressed the scope/integration recommendation in be23af51: merged current main without rewriting contributor history, reused #5188's hardlink archive filter and pre-clear snapshot validation, and removed the duplicate hardlink converter and redundant branch-specific assertions. The remaining diff covers FIFO/device omission and eligible internal absolute symlink relocation.

Two independent final reviews passed. Local formatting, lint, mypy, pyright, and the full test stack passed (11,324 tests passed, 66 skipped). The PR description now reflects the final implementation. Tests and CodeQL have been approved to run on this head; CI is being monitored.

[markstuart-oai]

Reviewed be23af510c4de3d830835e565d8ede6fa77df1f5. The FIFO/device handling and reuse of the existing hardlink filter look sound. One archive-integrity issue remains in the symlink proof: it needs to establish the topology that will actually be restored.

Source-only review; all 21 hosted checks passed on this commit. The concurrency case below is established by source tracing and was not executed locally.

[dpiet-oai]

Blocking finding is inline.

[markstuart-oai]

Re-reviewed 08b98fdf92a75f98104df11371421714f7de2f1f. The archive-topology finding is addressed: containment is now checked against the completed captured member graph, and copied symlink headers keep earlier rewrites from changing later proofs. The deterministic regressions cover both capture orders and verify that rejected archives leave the destination unchanged.

No remaining actionable findings in the full two-file diff. Existing hardlink handling, exclusions and strict hydration validation are preserved. Source-only review; all 21 hosted checks passed on this exact commit. I did not run tests locally.

Dismissing this earlier changes-requested review at the maintainer’s request. The archive-topology finding was addressed in 08b98fd with deterministic regression coverage; the thread is resolved, all 21 CI checks pass, and markstuart-oai and seratch have approved this exact commit.