| 1 | 1. **P1 — Restore can still block indefinitely on a FIFO.** [restore.go:22](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/region-ha-1b-next-build/internal/hostfiles/restore.go:22) |
| 2 | After `realParents` checks a child directory, a tenant can rename it aside and replace it with a FIFO before `syncDir` calls `root.Open`. That open has neither `O_DIRECTORY` nor `O_NONBLOCK`, so it waits indefinitely without a writer. The install holds the host guard during this operation; cancellation does not interrupt the open. This is an inherited defect still reached through the new `RestoreRoot` call. Use a descriptor-relative directory open that cannot block, and test the replacement at the existing `syncDir` seam. |
| 3 | |
| 4 | 2. **P2 — The absence check still confuses a vanished directory with a missing file.** [relocationflush.go:184](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/region-ha-1b-next-build/internal/host/relocationflush.go:184), also [present.go:63](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/region-ha-1b-next-build/internal/hostfiles/present.go:63) |
| 5 | `StatDir` closes its descriptor before the subsequent read reopens the directory. If the directory disappears between those operations, `ReadBeneathLimit` returns `ENOENT` from opening the directory, and `readInstanceFile` reports `absent=true`. A transient rename followed by restoration can therefore let a flush commit deletion of an existing env or adoption item. `Present` repeats this pattern and can incorrectly accept absence. Keep one directory descriptor and recognize absence only from the file lookup beneath it. |
| 6 | |
| 7 | **Verdict: request changes.** The root-opening fix is sound, but the requested protection against blocking and check/use races is incomplete. |
| 8 | |
| 9 | The other conclusions from the brief: |
| 10 | |
| 11 | - `OpenRootBeneath` keeps the original descriptor open until `os.OpenRoot` acquires its own; renaming the pathname cannot redirect that handoff. |
| 12 | - I found no outside-root symlink write through `RestoreRoot`. Internal symlink redirection remains possible, but bounded by [Go’s `os.Root` confinement](https://pkg.go.dev/os@go1.26.0#Root). |
| 13 | - Hard-linked destinations are replaced rather than truncated. Reads still accept hardlinks; cross-tenant protection depends on the existing isolation and migration controls. |
| 14 | - `Restore` and `SyncDir` retain their shared implementations and existing caller behavior. No second low-level safe-open implementation was introduced, although the flawed absence-check sequence is duplicated. |
| 15 | |
| 16 | Targeted tests could not start: the read-only sandbox prevented Go from creating its build directory. Findings are from source inspection. |
| 17 | |
| 18 | --- |
| 19 | |
| 20 | **FIRST PROMPT:** |
| 21 | “Follow the review brief docs/superpowers/plans/CODEX-REVIEW-REGION-HA-TENANT-DIR-BRIEF.md. Review the diff cfb4314d..3194d674 on branch region-ha-1b-next-build.” |
| 22 | |
| 23 | **WHAT:** |
| 24 | Review tenant-directory safety in the requested diff. |
| 25 | |
| 26 | **STATUS:** |
| 27 | Review complete: one P1 and one P2; tests blocked by the sandbox. |
| 28 | |
| 29 | **NEEDED:** |
| 30 | Nothing for this review. |
| 31 | |
| 32 | **RECOMMENDED:** |
| 33 | Fix both races and add deterministic regression tests before approval. |
| 34 | |
| 35 | **SESSION:** |
| 36 | Safe to close. No files changed or processes left running; the read-only sandbox prevented saving a handoff file. |
amadan