Skip to content

test(agents): restore portable sandbox resolver coverage (BRNS-DESK-059) - #91

Merged
stefan-ssv-labs merged 1 commit into
devfrom
test/desk-059-portable-sandbox-path-tests
Aug 14, 2026
Merged

test(agents): restore portable sandbox resolver coverage (BRNS-DESK-059)#91
stefan-ssv-labs merged 1 commit into
devfrom
test/desk-059-portable-sandbox-path-tests

Conversation

@stefan-ssv-labs

Copy link
Copy Markdown

Finding

BRNS-DESK-059

Requirement → change

  • AC1: adds a portable temporary-directory test for canonical required sandbox roots and ancestor reconstruction of optional roots.
  • AC2: exercises a missing required root through SandboxRoots::params() and asserts SandboxError::Path, without calling policy().
  • AC3: keeps the real Seatbelt wrapper, preflight, and headless confinement integration coverage macOS-only.
  • AC4: changes test coverage only; sandbox policy text, readiness, paths, environment gates, and runtime behavior are unchanged.

Verification

  • cargo fmt --all --check — passed.

  • cargo clippy --workspace --all-targets -- -D warnings — passed (only the pre-existing block v0.1.6 future-incompatibility notice).

  • cargo test --target-dir /private/tmp/brains-desk-059-refactor-target -p brains-local-agents --lib — passed: 109 tests, including both new portable resolver tests.

  • npm run lint:size — passed; sandbox.rs is 545 lines.

  • cargo test --workspace -q — all ordinary suites passed. The two existing macOS Keychain-dependent headless_sandbox tests failed locally:

    • a_confined_run_reads_its_login_but_writes_no_secret
    • a_scheduled_run_is_confined_and_still_does_its_work

    The same two failures reproduce on clean origin/dev with CI-equivalent Recall bundle stubs, so this is a non-regression environment baseline.

Platform evidence and gaps

  • Exercised locally on macOS from source.
  • Windows was not run locally; the existing Windows CI Rust workspace suite is the cross-platform proof point.
  • No packaged-build or GUI validation is needed for this Rust test-only change.

Risks and non-goals

This does not broaden non-macOS Seatbelt support or alter the order of sandbox policy construction. It only restores portable coverage at the path-resolution seam.

Shipping impact

No packaging, capabilities, updater, persisted-state, or release-action impact.

Regression provenance

  • Related review context: PR #47 and its coverage discussion.
  • Prior portable-test approach: PR #62.
  • This finding restores the omitted portable resolver coverage without changing the macOS-specific confinement coverage.

Reproduction

  1. Build the local-agents library on a non-macOS target or with macOS-only policy construction unavailable.
  2. Run the portable resolver tests.
  3. Confirm required temporary roots canonicalize and missing required roots return SandboxError::Path before any Seatbelt policy is constructed.

Expected: resolver coverage runs portably while actual Seatbelt enforcement remains macOS-only.

Merge

Squash merge after review and required checks pass.

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AC4 checks out where it matters: both removals sit inside #[cfg(test)] mod tests, and the only non-test edit is the #[path] module declaration — no policy text, resolver, or gate moved. For a change touching sandbox.rs that's the claim worth proving rather than accepting.

The trade is a real gain on the second test. an_unresolvable_root_refuses_to_produce_a_policy could only ever run on macOS, because policy() checks the platform before the roots — off macOS it passed for the wrong reason. Going through params() instead means a missing required root is now pinned as SandboxError::Path on every platform, which is strictly more coverage than what was deleted.

  • 🟢 The resolution assertion is only load-bearing where the temp path actually contains a symlink. It compares params() against std::fs::canonicalize(path), so on a symlink-free root that's a tautology and a resolver returning its input unchanged would pass. In practice macOS tempdirs land under /var/folders (a symlink), so the platform that runs Seatbelt still exercises it — the old /tmp/private/tmp assertion just failed louder by naming the answer. A symlink in the fixture pointing at the real dir would restore that property portably.

Checked: that every removed line is inside the test module, the new tests' assertions against what the deleted ones covered, and that params() is reached without policy(). Not read: the macOS-only Seatbelt/preflight tests that stay put — unchanged here.

Merge: ✅ into dev.

@sebastian-ssvlabs sebastian-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-at: 651657a

@stefan-ssv-labs stefan-ssv-labs left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ review-pr (self-review, cannot approve): clean — BRNS-DESK-059 requirements are satisfied; no blockers or criticals.

@stefan-ssv-labs
stefan-ssv-labs merged commit 2cbe65a into dev Aug 14, 2026
6 of 7 checks passed
@stefan-ssv-labs
stefan-ssv-labs deleted the test/desk-059-portable-sandbox-path-tests branch August 14, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants