feat(bwrap): proc_mount backend key — :bind for masked-proc hosts (k8s rootless)#75
Conversation
…the guard
The periodic measurement caught only {:noproc, _} exits. Under agent load
the SwarmManager GenServer.call can exit with :timeout (seen live: 'Error
when calling MFA defined by measurement: Genswarms.Telemetry
:measure_swarms []'), and a swarm entry missing :agent_count would raise —
both escaped and logged poller errors every tick. rescue-all + :exit-wide
catch, agent_count via Map.get. Tests pin the no-raise contract and the
guard shape.
…or masked-proc hosts (k8s) On Kubernetes the runtime masks /proc paths (procMount: Default) and the kernel refuses a fresh procfs mount in a userns over a masked source — every rootless spawn died with 'bwrap: Can't mount proc: Operation not permitted' (exit 1). proc_mount: :bind recursively ro-binds the outer /proc instead: the masks ride along (nothing gets unmasked) and --unshare-pid still blocks cross-namespace signaling; the trade is the sandbox can see the outer pid namespace's process list. Default :new keeps the fresh procfs everywhere else. Config-whitelisted as a backend key.
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesConfigurable /proc mount in bwrap backend
Telemetry polling error hardening
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82073a49b
ℹ️ 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".
| max_turns store extra_store_paths seccomp env volumes cmd container_name | ||
| subzeroclaw_src cpu_limit memory_swap pids_limit request_extra | ||
| compact_extra endpoint api_key model privilege_mode nice)a | ||
| compact_extra endpoint api_key model privilege_mode nice proc_mount)a |
There was a problem hiding this comment.
Adding proc_mount to the generic backend-key allowlist makes it reachable from the dynamic add-agent surface: parse_agent_spec/1 atomizes request config with this list, AgentServer.init/1 passes backend keys through, and IR.OpPolicy currently forbids only subzeroclaw_path, extra_ro_binds, extra_rw_binds, and extra_path. In that context a caller can POST an agent with config: {"proc_mount": "bind"} and force bwrap to ro-bind the host /proc, which this patch notes exposes the outer PID namespace process list. Keep this option operator-only by also blocking it in the dynamic-op policy.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/genswarms/telemetry/telemetry.ex (1)
116-130: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider logging before swallowing errors.
Broadening
catchto any:exitand adding a catch-allrescueis reasonable for poller resilience, but swallowing both without any log line means genuine bugs (typos, unexpectedswarmshape, SwarmManager crashes) will never surface in production. ALogger.warning/1call before returning:okwould preserve resilience while keeping the failure observable.♻️ Proposed fix to add logging
+ require Logger + try do swarms = Genswarms.SwarmManager.list() Enum.each(swarms, fn swarm -> :telemetry.execute( [:genswarms, :swarm, :agent_count], %{agent_count: Map.get(swarm, :agent_count, 0)}, %{swarm: swarm.name} ) end) rescue - _ -> :ok + error -> + Logger.warning("measure_swarms rescued: #{Exception.format(:error, error)}") + :ok catch - :exit, _ -> :ok + :exit, reason -> + Logger.warning("measure_swarms caught exit: #{inspect(reason)}") + :ok end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/genswarms/telemetry/telemetry.ex` around lines 116 - 130, The telemetry polling block in Genswarms.Telemetry.telemetry/0 is swallowing both rescued exceptions and caught exits without any visibility. Update the try/rescue/catch around Genswarms.SwarmManager.list and the Enum.each telemetry execution to log a warning with Logger.warning/1 before returning :ok, so unexpected swarm shape issues or manager crashes are still observable while keeping the poller resilient.lib/genswarms/backends/bwrap_backend.ex (1)
650-654: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider warning on unrecognized
proc_mountvalues instead of silently falling back.Any value other than
:bind/"bind"(including a typo like:bndor"binf") silently selects the fresh-procfs branch. On a Kubernetes host with masked/proc, that reintroduces the exactbwrap: Can't mount proc: Operation not permittedfailure this option is meant to fix — with no diagnostic. This mirrors the existingmax_turnshandling, which logs a warning on bad input rather than dropping it silently.♻️ Optional: warn on unexpected values
proc_args = case Map.get(config, :proc_mount, :new) do b when b in [:bind, "bind"] -> ["--ro-bind", "/proc", "/proc"] - _ -> ["--proc", "/proc"] + b when b in [:new, "new", nil] -> ["--proc", "/proc"] + other -> + Logger.warning( + "bwrap: unrecognized proc_mount #{inspect(other)} — defaulting to fresh procfs (--proc)" + ) + ["--proc", "/proc"] end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/genswarms/backends/bwrap_backend.ex` around lines 650 - 654, The proc_mount selection in the proc_args case currently treats every non-bind value as the default branch, which silently hides typos and misconfiguration. Update the logic around Map.get(config, :proc_mount, :new) to explicitly accept only :bind/"bind", and for any other unexpected value emit a warning similar to the max_turns handling before falling back to the default proc mounting behavior. Keep the warning and fallback close to the proc_args construction so the source of the bad value is easy to trace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/genswarms/telemetry/telemetry_test.exs`:
- Around line 14-27: The test is checking raw source text in telemetry.ex
instead of verifying measure_swarms/0 behavior. Replace the
File.read!/String.split/String.slice assertions with a behavioral test that
stubs or mocks Genswarms.SwarmManager.list/0 to exit or raise, then assert
measure_swarms/0 still returns :ok. Use the measure_swarms/0 and
Genswarms.SwarmManager.list/0 symbols to locate the code and keep the test
resilient to refactors.
---
Nitpick comments:
In `@lib/genswarms/backends/bwrap_backend.ex`:
- Around line 650-654: The proc_mount selection in the proc_args case currently
treats every non-bind value as the default branch, which silently hides typos
and misconfiguration. Update the logic around Map.get(config, :proc_mount, :new)
to explicitly accept only :bind/"bind", and for any other unexpected value emit
a warning similar to the max_turns handling before falling back to the default
proc mounting behavior. Keep the warning and fallback close to the proc_args
construction so the source of the bad value is easy to trace.
In `@lib/genswarms/telemetry/telemetry.ex`:
- Around line 116-130: The telemetry polling block in
Genswarms.Telemetry.telemetry/0 is swallowing both rescued exceptions and caught
exits without any visibility. Update the try/rescue/catch around
Genswarms.SwarmManager.list and the Enum.each telemetry execution to log a
warning with Logger.warning/1 before returning :ok, so unexpected swarm shape
issues or manager crashes are still observable while keeping the poller
resilient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b4695ad-b6eb-4b72-9337-b6e7903de5e5
📒 Files selected for processing (4)
lib/genswarms/backends/bwrap_backend.exlib/genswarms/config/swarm_config.exlib/genswarms/telemetry/telemetry.extest/genswarms/telemetry/telemetry_test.exs
| test "swallows exits from a busy/absent SwarmManager (guard is :exit-wide)" do | ||
| # Simulate the live failure: a caller that exits with something OTHER | ||
| # than {:noproc, _} (the only clause the old guard matched). We can't | ||
| # easily force a GenServer.call timeout here without slowing the suite, | ||
| # so pin the guard shape instead: the rescue/catch in measure_swarms | ||
| # must cover :exit of ANY shape and any raise. | ||
| source = File.read!(Path.join([__DIR__, "..", "..", "..", "lib", "genswarms", "telemetry", "telemetry.ex"])) | ||
| [_, body] = String.split(source, "def measure_swarms do", parts: 2) | ||
| guard_window = String.slice(body, 0, 1_200) | ||
|
|
||
| assert guard_window =~ "catch" | ||
| assert guard_window =~ ~r/:exit,\s*_/, "the :exit guard must be shape-agnostic (timeouts, not just :noproc)" | ||
| assert guard_window =~ "rescue", "raises from swarm-entry shape drift must be swallowed too" | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test asserts on source text, not behavior.
This test greps the raw source of telemetry.ex for "catch", a :exit, _ regex, and "rescue" rather than exercising the function under a simulated SwarmManager exit/crash. It breaks on harmless refactors (renaming, reformatting, extracting a helper) and doesn't actually prove the guard swallows exits — it only proves the words are present in the source.
Prefer stubbing/mocking Genswarms.SwarmManager.list/0 to raise or exit and asserting measure_swarms/0 still returns :ok, exercising real behavior instead of parsing source text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/genswarms/telemetry/telemetry_test.exs` around lines 14 - 27, The test
is checking raw source text in telemetry.ex instead of verifying
measure_swarms/0 behavior. Replace the File.read!/String.split/String.slice
assertions with a behavioral test that stubs or mocks
Genswarms.SwarmManager.list/0 to exit or raise, then assert measure_swarms/0
still returns :ok. Use the measure_swarms/0 and Genswarms.SwarmManager.list/0
symbols to locate the code and keep the test resilient to refactors.
1 participant
On Kubernetes the runtime masks /proc paths (procMount: Default) and the kernel refuses a fresh procfs mount in a userns over a masked source — every rootless spawn dies with
bwrap: Can't mount proc: Operation not permitted(exit 1; found live on the devexp cluster via agent_stopped buffer_tail forensics).proc_mount: :bindrecursively ro-binds the outer /proc instead: the k8s masks ride along (nothing gets unmasked) and--unshare-pidstill blocks cross-namespace signaling; the trade is the sandbox can see the outer pid namespace's process list. Default:newkeeps the fresh procfs everywhere else — zero behavior change unless opted in.Config-whitelisted as a backend key.
test/genswarms/config/63/63 green; backends suite run locally.Note: branched from fix/telemetry-measure-guard (#72), so that commit appears here until #72 merges — rebase or merge #72 first and this reduces to one commit.
Summary by CodeRabbit
New Features
/procis mounted.Bug Fixes
Tests