fix: probe isolation toolchain compatibility before timing - #551
Conversation
The macOS kernel refuses a nested sandbox_apply from any process whose sandbox-exec policy contains a deny rule, and it refuses to execute /bin/ps from any sandboxed process. Under the runner policy this breaks SwiftPM manifest and plugin sandboxes and agent-device's simulator recording identity lookup. Dispatch now runs both probes under the exact policy before the clock starts, records the results with the run, and refuses an iOS run when either fails and no native compatibility adapter is pinned.
janicduplessis
left a comment
There was a problem hiding this comment.
Verdict: NEEDS CHANGES (finding 1 is actionable; GitHub refuses a request-changes review from the author account, so this is posted as a comment review).
Fresh review of the diff vs main, #469, the tests and the docs, with the sandbox claims re-run locally on macOS 26.5 (25F71).
Findings
1. The nested-sandbox probe cannot prove what it is named and documented as proving (scripts/agent-benchmark/runner-isolation.mjs:156, README.md:157-162, docs/agent-benchmark.md:111-113)
The probe nests a deny-free inner profile, (version 1)(allow default). SwiftPM's manifest and plugin sandbox is not deny-free: Xcode's swift-package carries /usr/bin/sandbox-exec, (deny default), (import "system.sb") (strings on Toolchains/XcodeDefault.xctoolchain/usr/bin/swift-package). On this kernel a deny-containing inner profile is refused from any sandboxed process, deny-free outer profile included:
outer (allow default) / inner (allow default) -> 0
outer (allow default) / inner (deny default)(import "system.sb")(allow file-read*)
(allow process*)(allow sysctl*) -> 71 sandbox_apply: Operation not permitted
outer (allow default) / inner (allow default)(deny file-read-data (literal ...)) -> 71
outer = runnerIsolationPolicy shape / same SwiftPM-shaped inner -> 71
same SwiftPM-shaped inner, not nested -> 0 (profile is valid)
outer (allow default) / inner (allow default)(allow file-read*)(allow process*) -> 0 (deny-free inner still nests)
Consequences:
nestedSandbox.permitted: true(the value the darwin test asserts for an(allow default)profile atrunner-isolation.test.mjs:257-261) does not mean SwiftPM's sandbox would apply; the SwiftPM-shaped profile still exits 71 under that outer profile. The refusal text and the record name say "SwiftPM manifest and plugin sandboxes", so a pass is a false positive for the incompatibility the PR detects.runnerIsolationPolicyalways emits twodenyrules, so dispatch cannot hit the pass today; the retained evidence and the docs are what are wrong.README.md:160-162("only a deny-free profile permits nesting") anddocs/agent-benchmark.md:111-113state a kernel rule that is incomplete in a way that matters for the #469 decision: nosandbox-execouter profile at all can host SwiftPM's sandbox. Option 2 in the issue write-up ("Drop every deny rule, since a deny-free profile permits nesting") has a false premise; it is rejected there for another reason, but the maintainer is choosing from that list, and the corrected fact is that the trade-off is not isolation vs nesting. The write-up needs the same correction.
Suggested fix: probe with a deny-containing inner profile that mirrors SwiftPM's (the SwiftPM-shaped one above runs /usr/bin/true standalone with exit 0), correct the two doc passages and the issue comment, and change the darwin test's positive control to the same inner command run unsandboxed (plain execFileSync), which is the only control that exists on this kernel and still catches a broken invocation.
2. Refusal evidence is not retained, and the refusal lands after the expensive setup (driver.mjs:1367-1371, docs/agent-benchmark.md:117)
On refusal the function throws before the assignment, so preflight.isolationCompatibility is never written anywhere and the only trace is the error on stderr; the docs sentence "the probe results are retained with the private run metadata" holds only for runs that pass. By that point results/<stage>/<runId> already holds the copied golden control-tmp, the copied golden stim-home (stim arm), runner-home, prompt.txt, devices-before.json, runner-isolation.sb and, for launch-crash, the fixture checkout, with no meta.json or run.json (report() filters on run.json, so it ignores the directory; collect() would throw on it). That shape is pre-existing for any throw after the run directory is created, but this PR adds a refusal that is deterministic on this kernel for every iOS dispatch without the adapter, after waitForLoadGate (up to 10 minutes) and the golden copies. Non-blocking; at minimum attach the record to the error or write it into the run directory before throwing, and drop the "retained" wording for refused runs.
3. smoke <arm> does not run the probes (driver.mjs:1097-1134)
smoke has no platform parameter, so it cannot apply the iOS refusal, and dispatch does gate every timed run before the clock, which is what #469 asks for. The gap is that an operator cannot see the two probe results without dispatching; recording them in smoke.json would close it. Non-blocking.
Verified
- Claim 1 (outer deny rules):
(allow default)alone nests/usr/bin/truewith exit 0; plus a singledenyoffile-read-data(canonicalliteral, with a control showing the deny blocks a directcat),mach-lookup, orsysctl-read-> 71sandbox-exec: sandbox_apply: Operation not permitted; a deny plus(allow process-exec* (with no-sandbox) (literal "/usr/bin/sandbox-exec"))-> nested exit 0 and the nestedcatread the protected file while a directcatunder the same profile stayed denied. Three deny-free levels nest fine. - Claim 2 (
ps):/bin/psis-rwsr-xr-x root wheel;sandbox-exec -p '(version 1)(allow default)' /bin/ps -p $$ -o lstart=->execvp() of '/bin/ps' failed: Operation not permitted, exit 71; the PR's exact/bin/sh -c '/bin/ps -p "$$" -o lstart='->/bin/sh: /bin/ps: Operation not permitted, exit 126. agent-device 0.20.10 (global install, version checked)dist/src/host-process.jssetsi = '/bin/ps'andd(pid, field)runs/bin/ps -p <pid> -o lstart=|command=;platform-runtime-screen-recording-apple-simulator-host.jspolls it 81 times before throwingsimctl recordVideo did not expose a complete process identity. Importing the unpatchedc/s/i/aexports under(allow default)and under arunnerIsolationPolicy-generated profile gives{"start":null,"command":null,"zombie":false,"listError":"spawn EPERM"}; unsandboxed they return the real start time, command and 945 processes. - Claim 3 (wiring): both probes go through
isolatedRunnerInvocation(digest check,launcher -p <policy> ...); the driver'srun()isexecFileSyncwith piped stderr andencoding: 'utf8', so a non-zero exit throws with.stderr; the call sits afterprepareRunIsolationand beforeprobeNativeCompatibility,verifyRunnerProfile,prepareAgentDeviceRun,dispatchAtand both spawns, so a refusal precedes any timing; the record reachesmeta.preflighton success. - Exemptions:
verifyNativeCompatibilityreturnsnullunlessSTIM_BENCH_NATIVE_COMPAT_MANIFESTis set and hash-verified (and throws when a pin expects one), so the exemption needs a verified adapter. The adapter covers both mechanisms:psvia the patchedhost-process.js->ps-bridge.mjs-> compiled non-setuidnative-process, whichprobeNativeCompatibilityasserts under the same isolated invocation; nesting via thecompat/xcodebuildwrapper symlinked intoallowed-binahead of/usr/binon the runnerPATHplus the patchedbuild-xcframework.sh. Stim runsxcodebuildPATH-resolved (packages/stim-cli/src/engine/xcode.ts:680). The wrapper only covers PATH-resolvedxcodebuild, so anxcrun xcodebuild, absolute-path or user-script-sandbox nesting would still fail inside the timed window, but that fails the build and the existing app/device proof marks such a run invalid; it cannot yield a valid-looking timing. Android: Gradle applies no sandbox and agent-device's Android recording identity reads/proc/<pid>/statand/proc/<pid>/cmdlineoveradb shell, not hostps. - Tests: the decision test has no darwin dependency (
verifiedIsolationwrites a profile;isolatedRunnerInvocationonly hashes it), the darwin test isskipIf, the unit CI job isubuntu-latest, fixture roots are removed inafterEach, and realsandbox-execin the suite matches the three pre-existing darwin tests. - Commands:
pnpm exec vitest run scripts/agent-benchmark/runner-isolation.test.mjs7/7;pnpm run lint,pnpm run format:check,pnpm run knip,pnpm run build,pnpm run typecheckclean;pnpm testafter build: 3704 passed, 2 failed inengine-ios-device.test.tsfrom stalestim-devicectl-*entries inTMPDIR, matching the description. - Not exercised by me either: real
simctl recordVideo, realxcodebuildpackage resolution, the XCFramework build,dispatchend to end, and where the agent-device daemon that performs the recording is spawned relative to the sandbox (the issue's observed failure is the evidence that it runs inside it).
The macOS kernel refuses a nested sandbox_apply of any profile that differs from the one the process already runs under, with or without deny rules on either side. A deny-free inner probe therefore reported nesting as permitted under policies that still refuse SwiftPM's deny-default profile. The probe now applies the head of SwiftPM's profile, a refusal carries the probe record and dispatch writes it to the run directory, and the docs state the measured rule.
|
Addressed the review in f4951d8.
|
janicduplessis
left a comment
There was a problem hiding this comment.
Verdict: CLEAR (second-round review of f4951d8; no blocking finding, one non-blocking doc correction. Posted as a comment review because GitHub refuses approve and request-changes from the author account.)
Fresh review of the diff vs main, the first review, the fix-up, #469 and its correction comment, with the sandbox claims re-measured on macOS 26.5 (25F71).
Findings
1. The README states the nesting rule as "equivalent in effect"; the kernel is stricter than that (non-blocking) — scripts/agent-benchmark/README.md:160-164
The paragraph says a nested sandbox_apply succeeds for a profile "equivalent to the one the process already runs under" and that "a profile that differs in effect is refused". The second half held in every row I ran; the first does not. A profile that differs only by a rule with no effect is refused too:
outer (allow default) / inner (allow default)(deny file-read-data (literal "/etc/hosts")) -> 71 sandbox_apply: Operation not permitted
outer (allow default)(deny file-read-data (literal "/etc/hosts")) / inner (allow default) -> 71
outer (allow default) / inner (allow default)(deny file-read-data (literal "/nonexistent/never")) -> 71
Both denies are dead: the sandbox matches the canonical /private/etc/hosts, so a direct cat /etc/hosts under either profile exits 0 (with (literal "/private/etc/hosts") it exits 1). The two profiles in each row are identical in effect and still refused, while reordered, duplicated, redundant-allow, (version 2) and (regex #"^/private/etc/hosts$") variants of a live deny all nest (exit 0). What nests is a profile that compiles to the same sandbox as the applied one, not one equivalent in effect. The operative conclusion is unchanged and if anything firmer: SwiftPM's per-invocation profile can never match a runner policy. docs/agent-benchmark.md:111-113 ("any profile that differs from the one a process already runs under") already says it correctly; the README sentence, the PR body's measurements preamble and the #469 correction comment carry the "equivalent"/"differs in effect" wording. Suggested README wording: "unless the inner profile compiles to the same sandbox the process already runs under (identical, reordered, duplicated or redundant rules all nest); any other profile is refused in both directions, even one that only adds a rule with no effect."
First-round findings
- Finding 1 (deny-free inner profile): resolved.
runner-isolation.mjs:155-161nests(version 1)(deny default)(import "system.sb")(allow file-read*)(allow process*)throughisolatedRunnerInvocation, so the innersandbox-execruns under the exact verified policy. Those five fragments are, in that order, the first five profile strings after/usr/bin/sandbox-execin Xcode'sToolchains/XcodeDefault.xctoolchain/usr/bin/swift-package(followed by the twomach-lookupgrants, the cache-write regex, the network block,file-write*andsysctl*), and matchmacOSSandboxProfilein Sandbox.swift at the linked commit. On this kernelpermitted: truecan only arise when the runner policy compiles to the same sandbox as that head, which no policy with a write grant can, so the probe cannot report a false pass under anyrunnerIsolationPolicyoutput; the darwin test pins the refusal. - Finding 2 (evidence retention): resolved. The refusal error carries
isolationCompatibility;driver.mjs:1367-1381writesisolation-compatibility.jsonintorunDirfrom the driver process (nosandbox-execinvolved) and rethrows. Only a refusal writes it; a passing record reachesmeta.preflight. Nothing lists a run directory expecting a fixed file set (reportfilters onrun.json,collectreadsmeta.json/run.json, the only run-directory listing isproof/). The probe order stays after the golden copies: the policy's scoped grants come fromcrash.fixtureCheckout,claudeGuidance.path,env.STIM_HOMEandrunDir/tmp, andprepareRunnerIsolationneedsrunDir/tmpto exist, so an earlier probe would verify a different policy. - Finding 3 (
smoke): left as is, non-blocking, as agreed.
Verified
- Nesting matrix,
/usr/bin/sandbox-exec -p <outer> /usr/bin/sandbox-exec -p <inner> /usr/bin/true,swiftpm= the probe's inner profile:(allow default)/(allow default)0, with a redundant(allow process-exec*)0;(allow default)/swiftpm 71;(allow default)/(allow default)+live deny 71; live deny/itself 0, reordered 0, duplicated 0, plus redundant allow 0, regex form 0,(version 2)0; deny X/deny Y 71; deny X/deny X+Y 71; deny X+Y/deny X 71; deny X/(allow default)71; swiftpm/swiftpm 0 and three levels deep 0; swiftpm/swiftpm+(allow file-write* (subpath "/private/tmp"))71 and the reverse 71; swiftpm/(allow default)71; swiftpm and swiftpm+write standalone 0. ExactrunnerIsolationPolicyoutput, generated through the module with temporary roots: /swiftpm 71, /(allow default)71, /itself 0,/bin/sh -c '/bin/ps -p "$$" -o lstart='126/bin/sh: /bin/ps: Operation not permitted, standalone/usr/bin/true0. Escape row with a live deny: directcatexit 1, nested(allow default)catunder(allow process-exec* (with no-sandbox) (literal "/usr/bin/sandbox-exec"))read the file. ps:/bin/psis-rwsr-xr-x root wheel; the probe command exits 126 under(allow default)and under the exact policy, 0 unsandboxed. Unpatched agent-device 0.20.10host-process.jsexports (c/s/i/a) under(allow default):{"start":null,"command":null,"zombie":false,"listError":"spawn EPERM"}; unsandboxed: real start time, command, 948 processes. Android recording identity reads/proc/<pid>/statand/proc/<pid>/cmdline(platform-runtime-screen-recording-android-host.js), so the Android exemption holds. The adapter exemption still needsverifyNativeCompatibility(null withoutSTIM_BENCH_NATIVE_COMPAT_MANIFEST, throws when the pin expects one), andprobeNativeCompatibilitythen asserts the patched identity path and thexcodebuildwrapper under the same isolated invocation, so it cannot admit an unproven timing.- Tests:
pnpm exec vitest run scripts/agent-benchmark/runner-isolation.test.mjs7/7 on darwin with realsandbox-exec, nostim-runner-isolation-*left inTMPDIR; the injected-execute test has no darwin dependency and the darwin test isskipIf. Two throwaway mutants of the unit test (isolationCompatibility: { bogus: true }, and a wrongstringContainingmessage) each fail with "expected error to match asymmetric matcher", so thetoThrow(expect.objectContaining(...))assertion is load-bearing under vitest 5. The darwin positive control runs/usr/bin/sandbox-exec -p <swiftpm> /usr/bin/trueunsandboxed and asserts no throw; the nested run asserts the exactsandbox_applystderr from real output. pnpm run lint,format:check,knip,build,typecheckclean;pnpm test: 3704 passed, 1 skipped, 2 failed inengine-ios-device.test.ts(stalestim-devicectl-*inTMPDIR, pre-existing, as the description says).gh pr checks 551: test (node 22), test (node 24) and published runtime pass; device jobs skipped.- Docs and issue: README and
docs/agent-benchmark.mdname the two mechanisms and where refused-run results live, and the existing sentence that this does not replace a real untimed build and recording test stands; the #469 correction identifies option 2's false premise correctly. The one added code comment names the external source (SwiftPM's Sandbox.swift) and the constraint, which fits the comment policy. The PR body describes the final diff without review-round narration. - Not exercised by me either:
bench.mjs dispatchend to end, a realsimctl recordVideo, realxcodebuildpackage resolution or the XCFramework build; I booted no simulator.
|
Second-round follow-up: the README now states the rule as "compiles to the same sandbox", with the dead-deny case called out; the description's measurement preamble uses the same wording, and a precision note is posted on #469. Docs-only commit, no code change. |
Description
Refs #469 (does not close it: whether the pinned adapter is the accepted design, and whether a real untimed build/recording campaign check is still wanted on top of these probes, stays open there). The benchmark's
sandbox-execrunner policy passes its filesystem probes but breaks two ordinary iOS operations, and nothing checked either before the clock started. This PR adds detection, not a new isolation design: dispatch refuses an iOS run before timing when the exact policy fails either probe and no native compatibility adapter is pinned, instead of failing inside the timed build or recording. Policy rules, grants, device ownership, version pins, the frozen campaign and published results are untouched.The two operations:
xcodebuildin the SDK 58 ExpoModulesJSI XCFramework build) fail withsandbox-exec: sandbox_apply: Operation not permitted.simctl recordVideo did not expose a complete process identitybecause it reads the recorder's identity through/bin/ps, and the kernel refuses to exec/bin/psfrom any sandboxed process.No profile rule fixes the nesting. On macOS 26.5 only a profile that compiles to the same sandbox as the one already applied nests, and SwiftPM generates its own
(deny default)profile per invocation, so no runner policy can match it; the pinned adapter from #483 (-IDEPackageSupportDisable*Sandbox=1,-disable-sandbox, thepsbridge) stays the only compensation.Measurements: outer profile / nested profile / exit code
/usr/bin/sandbox-exec -p <outer> /usr/bin/sandbox-exec -p <inner> /usr/bin/trueon macOS 26.5 (25F71). A profile that does not compile to the same sandbox as the outer one is refused in both directions, whether or not either profile contains adenyrule and even when the only difference is a rule with no effect; the same sandbox nests at any depth.swiftpmbelow is(version 1)(deny default)(import "system.sb")(allow file-read*)(allow process*), the head of the profile Sandbox.swift generates with per-invocation write grants; it exits 0 on its own.(allow default)(allow default), with or without redundantallows(allow default)(allow default)(allow default)plus onedenyrunnerIsolationPolicyoutput(allow default), or any other profile tried(allow default)plusdenyXallows(allow default)plusdenyX(allow default)plusdenyY, or plusdenyX and Y(allow default)plusdenyX and Y(allow default)plusdenyX(allow file-write* ...), and the reversedenyplus(allow process-exec* (with no-sandbox) (literal "/usr/bin/sandbox-exec"))(allow default)Solution
verifyIsolationCompatibilityruns two untimed probes throughisolatedRunnerInvocation, so they execute under the run's exact verified policy: a nestedsandbox-execapplying the head of SwiftPM's profile, and/bin/sh -c '/bin/ps -p "$$" -o lstart=', the query agent-device makes for the recorder. The nested probe applies SwiftPM's(deny default)shape rather than a deny-free profile because only a profile that compiles to the same sandbox nests: a deny-free inner profile passes under any deny-free outer while SwiftPM's still fails there, so itspermitted: truewould not mean SwiftPM can build.Dispatch calls it right after
prepareRunIsolation, before the adapter probe, the profile smoke and the clock, and stores the record in the run'spreflight.isolationCompatibility. It refuses only aniosrun with no verified adapter (preflight.nativeCompatibilitynull); Android and adapter-pinned runs keep the record as evidence (Gradle does not nest sandboxes, Android recording reads/procon the device, and the adapter's own probe proves the patched identity path). A refusal writes the record toisolation-compatibility.jsonin the run directory (driver) because a refused run never gets ameta.json; the error names the failing probe and its stderr and carries the record.The probe cannot run earlier: the policy grants the run's fixture checkout, guidance file,
STIM_HOMEand tmp paths, which the setup before it creates, so a refused iOS dispatch still pays for the load gate and the golden copies, like every other refusal after the run directory exists.Test plan
scripts/agent-benchmark/runner-isolation.test.mjs, injectedexecute: every probe call goes to/usr/bin/sandbox-exec -p <exact policy>and the nested probe applies a(deny default)profile (catches a probe that runs outside the policy or with a deny-free inner profile and reports a false pass); the refusal message and the record attached to the error; no refusal with the adapter pinned or on Android; a clean record when both pass.sandbox-exec: sandbox_apply: Operation not permittedand/bin/sh: /bin/ps: Operation not permitted, iOS without the adapter throws, and the probe's own inner command run unsandboxed exits 0, so the refusal is the nesting rather than a broken invocation. The refusal assertions pin current kernel behavior on purpose: if Apple changes it, the adapter's premise changed and the test says so.execFileSyncoptions (status 71 and 126 with those messages under the exact profile); unpatched agent-device 0.20.10host-process.jsidentity primitives under the exact profile:{"start":null,"command":null,"listError":"spawn EPERM"}.bench.mjs dispatchend to end (needs a benchmark root, pins, golden state and a runner), so theisolation-compatibility.jsonwrite is covered only by the unit test of the record on the error; a realsimctl recordVideo(no simulator was booted, and I did not boot one); realxcodebuildpackage resolution and the full ExpoModulesJSI XCFramework build. The probes cover the mechanism each operation depends on, not the operation itself.pnpm test: 3704 passed; the 2 failures inengine-ios-device.test.tsare pre-existing on this machine (stalestim-devicectl-*directories inTMPDIR) and unrelated to this diff.