Allow runner to execute without a sandbox

[epompeii]

This changeset adds the concept of an operating system (os) and sandbox to the spec.

This allows the bare metal runner to be used without Firecracker and non-Linux operating systems.

[github-actions]

🤖 Claude Code Review

PR: #720
Base: devel
Head: u/ep/local-runner
Commit: acc6801a6b99c6e77b8dfe84822c582a4f837c7e

---

Now I have a thorough understanding of the PR. Here's my review:

---

PR Review: Add OS and Sandbox fields to spec, enable non-sandboxed runner execution

Overview

This PR adds os (operating system) and sandbox fields to the spec model, introduces a non-sandboxed execution mode for the runner (executing benchmarks directly on the host), and includes the --danger-allow-no-sandbox CLI flag to gate this mode. ~2900 lines added, ~860 removed across 65 files.

---

Strengths

Security design is solid:

• canonicalize_within_rootfs() (local.rs:375-461) correctly re-roots absolute symlinks and clamps .. traversal — this is the right approach for rootfs containment
• pidfd_open usage (local.rs:228-234) prevents PID reuse races when killing child processes
• Environment is cleared before execution (local.rs:77), only OCI-derived vars are set
• --danger-allow-no-sandbox naming makes the security implications unmistakable
• check_sandbox_allowed() (up/job.rs:16-27) enforces the flag at job execution time with a clear rejection message

Code quality is high and follows project conventions:

• thiserror used throughout, no anyhow in library/service code
• #[expect(...)] used instead of #[allow(...)] with reason strings
• camino::Utf8PathBuf used for all paths
• Strong types (OperatingSystem, Sandbox, Cpu, Memory) — no stringly-typed values
• CLI parser in separate parser module, handler logic in runner module
• No num_args on the --danger-allow-no-sandbox flag

Types and schema well-implemented:

• OperatingSystem and Sandbox enums follow the established typed_string! macro pattern
• Migration safely backfills existing rows with 'linux' / 'firecracker' defaults
• Option<Sandbox> correctly maps to Nullable<Text> in the schema
• Good test coverage including serde roundtrips and invalid input rejection

---

Issues

1. Polling loop in wait_with_timeout — busy-wait with 100ms sleep (local.rs:147-172)

The timeout/cancel polling loop sleeps 100ms per iteration. This is functional but means up to 100ms latency on process completion. Consider using child.try_wait() in a loop or wait_with_output on a thread with a Receiver::recv_timeout pattern. Minor issue — acceptable for benchmark workloads.

2. format!("{e}") instead of .to_string() (local.rs:363)

hint: format!("{e}"),

Should be e.to_string() — format! with a single Display arg is a clippy lint (to_string_in_format_args). This appears in a few places.

3. dyn Any downcast in panic handler (local.rs:175-179)

let msg = panic
    .downcast_ref::<&str>()
    .copied()
    .or_else(|| panic.downcast_ref::<String>().map(String::as_str))
    .unwrap_or("unknown panic");

CLAUDE.md says "Do NOT use dyn std::any::Any without explicit justification." This is a JoinHandle::join result which inherently returns Box<dyn Any>, so the usage is justified — but it would be good to add a comment noting this is the standard pattern for extracting panic messages from thread::JoinHandle.

4. Migration hardcodes 'firecracker' for all existing specs (up.sql)

All existing specs get sandbox = 'firecracker' — this is correct given that all prior specs were Firecracker-based, but if any spec was created outside the normal flow, the migration will silently assign the wrong sandbox. Consider adding a comment in the migration noting this assumption.

5. collect_output_files silently skips failures (local.rs:481-484, 490-492)

Warnings are printed to stderr but callers have no way to distinguish "no output files requested" from "all output files failed to read." This is a minor observability gap — the current Option<HashMap> return type loses this information.

6. ChildHandle doesn't implement Send/Sync explicitly (local.rs:211-216)

The ChildHandle holds a raw pidfd (i32). Since it's only used from the main thread and kill() is called before handle.join(), this is safe in practice. But if the usage pattern ever changes, consider adding a SAFETY comment or explicit Send impl.

---

Minor Nits

• operating_system.rs:86-88 and sandbox.rs:78: Tests use assert_eq!(true, ...) — prefer assert!(is_valid_operating_system("linux")) for readability
• The WaitOutput struct (local.rs:120-124) could potentially reuse RunOutput to reduce duplication, though the types differ slightly (no output_files)

---

Verdict

Approve with minor fixes. The security model is well-thought-out, the code follows project conventions closely, and the type system is used effectively to prevent misuse. The issues identified are minor and don't block merging. The most important ones to address are #2 (clippy lint) and adding comments for #3 and #4.

---

_{Model: claude-opus-4-6}

[github-actions]

Bencher Report

Branch	u/ep/local-runner
Testbed	ubuntu-22.04

Click to view all benchmark results

Benchmark	Latency	Benchmark Result microseconds (µs) (Result Δ%)	Upper Boundary microseconds (µs) (Limit %)
Adapter::Json	📈 view plot 🚷 view threshold	3.92 µs (+10.49%) Baseline: 3.55 µs	4.68 µs (83.73%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	3.90 µs (+10.73%) Baseline: 3.52 µs	4.60 µs (84.73%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	24.96 µs (-2.15%) Baseline: 25.50 µs	31.53 µs (79.15%)
Adapter::Rust	📈 view plot 🚷 view threshold	2.89 µs (+2.76%) Baseline: 2.81 µs	3.26 µs (88.49%)
Adapter::RustBench	📈 view plot 🚷 view threshold	2.87 µs (+2.15%) Baseline: 2.81 µs	3.26 µs (87.93%)
head_version_insert/batch/10	📈 view plot 🚷 view threshold	104.83 µs (+4.11%) Baseline: 100.69 µs	118.74 µs (88.29%)
head_version_insert/batch/100	📈 view plot 🚷 view threshold	246.78 µs (+3.58%) Baseline: 238.25 µs	265.55 µs (92.93%)
head_version_insert/batch/255	📈 view plot 🚷 view threshold	469.83 µs (+1.41%) Baseline: 463.32 µs	499.30 µs (94.10%)
head_version_insert/batch/50	📈 view plot 🚷 view threshold	164.95 µs (+2.15%) Baseline: 161.47 µs	183.04 µs (90.12%)
threshold_query/join/10	📈 view plot 🚷 view threshold	149.98 µs (+3.18%) Baseline: 145.35 µs	169.62 µs (88.42%)
threshold_query/join/20	📈 view plot 🚷 view threshold	167.65 µs (+4.84%) Baseline: 159.92 µs	186.54 µs (89.87%)
threshold_query/join/5	📈 view plot 🚷 view threshold	144.48 µs (+5.10%) Baseline: 137.47 µs	160.38 µs (90.09%)
threshold_query/join/50	📈 view plot 🚷 view threshold	206.85 µs (+2.80%) Baseline: 201.22 µs	231.48 µs (89.36%)

🐰 View full continuous benchmarking report in Bencher