fix(sandbox): match the read deny-list against a rule's resolved path, greening shared macOS/Windows CI

[Hmbown]

Summary

Restores a green shared base: hosted Test (macos-latest) and Test (windows-latest) are red on main at 20e957406 and every open PR riding it (#5712, #5719, #5720, #5721, #5703, #5722) inherits the same failures.

macOS (6 failures, sandbox::read_guard::tests) — hosted runners give tempfile a /var/folders/... directory that is really /private/var/folders/.... The deny-list compared the canonicalized candidate (and the already-canonical cwd) against a literal rule, so the six symlink / denied-tree tests never matched. Locally this only reproduces when TMPDIR is the macOS default (the prior verification ran with TMPDIR on a plain volume).

This is a product bug, not a test bug: on macOS /etc, /var, and /tmp are symlinks into /private, so read_file /private/etc/sudoers walked around the built-in /etc/sudoers rule (the OS-level Seatbelt setter already canonicalized its own copy; the in-process matcher did not). Fix: a subtree rule now remembers its resolved spelling (DenyRule::subtree) and check matches a candidate against either spelling.

Windows (1 failure) — root_parent_traversal_does_not_escape_above_root asserted a Unix /etc while normalize_lexically("/../../etc") correctly resolves against the cwd's drive (D:\etc on the hosted runner). The test now spells the traversal from the current drive root and keeps the Unix literal under cfg!(unix).

Defeat tests added (fail on the unpatched tree regardless of TMPDIR):

• rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling (unix): rule written through a symlinked directory; direct read, symlink read, and relative-from-cwd read are all refused; the real directory's innocent sibling stays readable.
• macos_private_spelling_of_a_machine_wide_rule_is_refused (macOS): /private/etc/sudoers and /private/etc/ssh/… are refused by the /etc/* defaults; /private/etc/hosts stays readable.

Not in this PR (branch-specific, not shared-main): the Windows runtime_api::tests::events_endpoint_respects_since_seq_cursor timeout appeared only on #5712's Windows run (8.91 s against an 8 s ci_scaled budget; #5712 does not touch runtime_api), and #5722's Ubuntu fleet::executor failure is its own.

No-Issue: shared-main CI repair; no tracking issue exists for the hosted-runner symlink shape.

Testing

Focused, hosted-equivalent (RUSTFLAGS=-Dwarnings, RUST_MIN_STACK=16MiB, cargo nextest run -p codewhale-tui --lib --all-features --locked --profile ci, TMPDIR = macOS default /var/folders/…):

• Baseline at 20e957406: 22 tests run: 16 passed, 6 failed — the same six as hosted macOS.
• After the fix, same command and TMPDIR: 24 tests run: 24 passed (22 prior + 2 new).
• Defeat probe (rule resolution disabled, tests kept, then file restored byte-for-byte): plain TMPDIR → 6 run: 4 passed, 2 failed — exactly the two new tests; macOS-default TMPDIR → 6 run: 2 passed, 4 failed — the two new tests plus the two hosted-CI originals in the sample.

Broader local gates on the fixed tree (same flags, nextest --profile ci):

• read_guard under a plain (non-symlinked) TMPDIR: 24 run: 24 passed

• Safety-gate filter (command_safety|auto_review|authority|sandbox): 336 run: 336 passed

• Deny-list consumers (tools::{file,search,file_search,read_media}, config::): 601 run: 601 passed

• Full TUI library suite: 11540 run: 11540 passed, 13 skipped

• cargo clippy -p codewhale-tui --all-targets --all-features --locked with the CI allow list: clean

• Windows-target cargo check (x86_64-pc-windows-msvc) could not run on this macOS host (ring's build script needs a Windows C toolchain), so hosted Test (windows-latest) remains the only Windows compile/run evidence.

• cargo fmt --all -- --check

• cargo clippy -p codewhale-tui --all-targets --all-features --locked (warning-free under the CI allow list; workspace form not re-run — no other crate changed)

• cargo test --workspace --all-features --locked (hosted CI; locally the changed crate's full lib suite passed as above)

Checklist

• This PR adds a new layer/module/abstraction — it names or deletes the layer it replaces (n/a)
• Updated docs or comments as needed
• Added or updated tests where relevant
• Verified TUI behavior manually if UI changes (n/a)
• Harvested/co-authored credit uses a GitHub numeric noreply address (n/a — no harvested work)

🤖 Generated with Claude Code

[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-08-29T22:43:20.958647Z	414eafb	PR opened

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

[claude]

Claude finished @Hmbown's task in 1m 57s —— View job

---

Review complete

• Gather context (PR body, changed files)
• Read crates/tui/src/sandbox/read_guard.rs diff in full
• Check DenyRule::subtree matching logic for correctness
• Check new tests (symlink + macOS /private tests)
• Check CHANGELOG entries
• Post final review

Summary

This is a focused, well-scoped fix. The root cause description matches the code: DenyRule::subtree() (read_guard.rs:143-150) now precomputes canonicalize_best_effort(&path) once at rule-build time and stores it only when it differs from the literal (resolved: (resolved != path).then_some(resolved)), and check() (read_guard.rs:280-300) matches a candidate against either spelling. This correctly closes the macOS /etc → /private/etc symlink gap without changing the OS-level wrapper path (subtree_paths() at line 246 still returns only the literal path, which is intentional — Seatbelt/bubblewrap resolve their own copies, as noted in the PR body).

Correctness

• All three call sites (default_denied_subtrees loop, user extra paths, and the constructor itself) now go through the single DenyRule::subtree() helper — no duplicated canonicalization logic.
• DenyRule::Subtree { .. } is destructured with .. everywhere else in the file (describe(), subtree_paths()), so the new resolved field doesn't break any match arms or exhaustiveness.
• The Windows fix for root_parent_traversal_does_not_escape_above_root correctly derives the traversal from the cwd's actual root/prefix components rather than hardcoding a Unix path, and keeps the original Unix-literal assertion under cfg!(unix) — verified the component math by hand (D:\..\..\etc normalizes to D:\etc under the normalize_lexically component-walk logic at line 561).
• DenyRule still derives PartialEq, Eq; adding resolved to the struct doesn't change any equality-based logic elsewhere since DenyRule isn't compared outside this module (confirmed no other file references DenyRule).

Tests

• rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling (unix) is a good defeat test — it exercises direct/symlink/relative-cwd reads and confirms an innocent sibling stays readable, so it isn't just testing the happy path.
• macos_private_spelling_of_a_machine_wide_rule_is_refused correctly gates on cfg(target_os = "macos") since it depends on real /private/etc/* files existing on the host; it won't run/compile-fail on Linux/Windows CI.
• Both new tests reuse the existing ExpectDenied::unwrap_err_or_panic test helper rather than reinventing assertion boilerplate.

Nothing else flagged. No provider/model-routing code is touched, no unwrap/expect was added to a fallible production path, and the CHANGELOG entries in both CHANGELOG.md and crates/tui/CHANGELOG.md are identical and accurately describe the fix.

I wasn't able to execute cargo test/cargo clippy in this sandbox (build commands require approval this session), so this review is based on static reading of the diff plus manual tracing of normalize_lexically/canonicalize_best_effort/path_is_within; it does not independently re-verify the hosted macOS/Windows CI results claimed in the PR description.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 414eafba49

ℹ️ 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]

Apply exemptions to the resolved rule path

When a built-in rule is spelled through a symlink, exemptions are still compared only with its literal path before DenyRule::subtree adds the resolved spelling. For example, on macOS a denial for /private/etc/sudoers tells the user to add that requested path to sandbox_read_denylist_exempt, but the exemption does not match the literal /etc/sudoers rule, which is retained and resolves back to /private/etc/sudoers; the documented escape hatch therefore cannot reopen the path under the spelling that triggered the denial. Compare each exemption against both the literal and resolved rule paths.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Refresh resolved rules after symlink changes

When a configured deny path is a symlink that is replaced or retargeted during a long-running session, resolved remains a startup-time snapshot. A direct read beneath the new target is then allowed because check compares it only with the literal alias and the old target, while the old target remains denied even after the alias stops pointing there. This can occur when another process atomically rotates a symlinked credential directory, so the rule should be re-resolved during checks or refreshed when its filesystem topology changes.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid changing the process cwd in a parallel unit test

Under the documented cargo test -p codewhale-tui --lib --locked invocation, this test can run concurrently with relative_read_from_inside_a_denied_tree_is_refused, which also changes the process-wide cwd; the local test runner's --help confirms that “By default, all tests are run in parallel.” If either test switches cwd between the other's set_current_dir and relative list.check(...), that check resolves against the wrong temporary directory and intermittently fails. Use a subprocess or otherwise serialize both cwd-mutating tests.

AGENTS.md reference: crates/tui/AGENTS.md:L34-L40

Useful? React with 👍 / 👎.

[claude]

Claude finished @Hmbown's task in 1m 16s —— View job

---

Review of the latest push (c4bbb35)

• Gather context (PR body, prior Codex findings, latest commit)
• Read current crates/tui/src/sandbox/read_guard.rs diff (origin/main...HEAD)
• Check whether the two P2 Codex findings (exemption spelling, serialized cwd tests) were addressed
• Check the "refresh resolved rules after symlink changes" P2 finding for validity
• Post final review

Summary

Commit c4bbb35 is a targeted, correct follow-up to 414eafb that resolves both actionable Codex findings from the earlier review round. I re-read the full diff against origin/main and traced the new matching logic by hand.

1. Exemption spelling gap — fixed correctly (read_guard.rs:145-169, 202-217)
DenyRule::is_within_any now checks a rule's path and its resolved alias against each exempt root, and exempt_normalized itself is expanded via flat_map to carry both the literal and canonicalized spelling of every configured exemption (read_guard.rs:207-217). So exempting /private/etc/sudoers correctly drops the /etc/sudoers default rule, and vice versa — this matches the new exempting_either_spelling_of_a_symlinked_default_rule_reopens_it test (read_guard.rs:1194-1210), which is a good defeat test (fails against the prior commit for the resolved spelling) and also confirms an unrelated rule (/etc/ssh) stays armed.

2. Parallel cwd-mutating tests — fixed correctly (read_guard.rs:725-733)
Both relative_read_from_inside_a_denied_tree_is_refused and rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling now take a shared CWD_LOCK mutex before calling std::env::set_current_dir, closing the intermittent-failure window Codex flagged for cargo test's thread-parallel runner. Confirmed via grep that both call sites acquire lock_cwd() before mutating cwd and every set_current_dir in the file is covered.

3. "Refresh resolved rules after symlink changes" — reasonably deferred, not silently dropped
The commit adds an explicit doc comment (read_guard.rs:60-65) stating rules are resolved when the list is built (startup/config reload), so a symlink retargeted mid-session is seen through the literal spelling only until rebuild. That's a documented, deliberate scope boundary consistent with the module's stated defense-in-depth posture (the OS-level Seatbelt/bubblewrap wrapper is the other layer, and it re-resolves independently) rather than an oversight — reasonable to leave as-is rather than adding live re-resolution for a narrow TOCTOU-adjacent window.

Correctness check on the core matcher

• DenyRule::Subtree gained a resolved: Option<PathBuf> field; every match site (describe(), subtree_paths(), the check() loop) already used ../named-field destructuring, so no non-exhaustive match was introduced — confirmed by grepping every DenyRule::Subtree construction/destructure site.
• subtree_paths() (read_guard.rs:274-282) intentionally still returns only the literal path for the OS wrapper handoff — correct, since Seatbelt/bubblewrap canonicalize their own copy.
• check()'s per-candidate loop (read_guard.rs:308-320) now ORs the literal and resolved rule spellings against both the literal and canonicalized candidate — correct given the goal (macOS /etc → /private/etc).

Nothing further to flag; this is a clean, well-scoped fix to a real cross-platform CI bug, and the two review round-trips (414eafb → c4bbb35) each closed the specific gap raised without scope creep.

[Hmbown]

Codex review findings, each verified before fixing — addressed in c4bbb357 (second commit on this branch; hosted CI restarts at that head):

• Apply exemptions to the resolved rule path → confirmed: a denial for /private/etc/sudoers named an exemption that could not reopen the /etc/sudoers rule it resolved from. Exempt entries now carry both spellings and a rule is exempted when either of its spellings lies under either of theirs (DenyRule::is_within_any). New macOS test exempting_either_spelling_of_a_symlinked_default_rule_reopens_it (fails on 414eafba for the resolved spelling).
• Avoid changing the process cwd in a parallel unit test → confirmed under libtest. Both cwd-moving tests now share CWD_LOCK; verified with three consecutive cargo test -p codewhale-tui --lib --all-features -- read_guard::tests runs (25/25 each) plus nextest 25/25.
• Refresh resolved rules after symlink changes → not re-resolved per check. Documented instead: rule spellings are resolved when the list is built (startup and config reload); a symlink retargeted mid-session is seen through the literal candidate only until the list is rebuilt — inside the module's stated defense-in-depth posture (the OS-level Seatbelt copy has the same snapshot semantics). Re-resolving ~40 rules on every tool read is a cost I did not want to add for that edge without evidence it matters.

Hosted evidence at 414eafba before the push: Test (windows-latest) and Test (ubuntu-latest) passed; the macOS leg was in flight and now re-runs at c4bbb357. Local at c4bbb357: read_guard 25/25 (nextest, CI flags), fmt clean, clippy -p codewhale-tui --all-targets --all-features with the CI allow list clean.

[Hmbown]

Integrator gate: exact head c4bbb35 is mergeable and all required checks are green. The private Ops checker still denies only because root CHANGELOG.md is a Shipwright-owned collision point; the TUI implementation and crates/tui/CHANGELOG.md are Beacon-owned. Please remove only the root CHANGELOG.md delta, retain the crate-local changelog entry, and push a new head. I will rerun exact-head CI and the Beacon checker; there is no implementation or CI blocker beyond that ownership split.