From b50ea033849ddc08b5941407896150c2c70daef1 Mon Sep 17 00:00:00 2001
From: Adam Perry <anp@openai.com>
Date: Wed, 24 Jun 2026 20:24:13 +0000
Subject: [PATCH] codex: address PR review feedback (#29892)

---
 .../testing/bwrap/fixtures/bwrap_smoke.rs     |  6 ++++
 bazel/rules/testing/bwrap/src/lib.rs          | 35 +++++++++++--------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/bazel/rules/testing/bwrap/fixtures/bwrap_smoke.rs b/bazel/rules/testing/bwrap/fixtures/bwrap_smoke.rs
index 8229c6607c..25468a4684 100644
--- a/bazel/rules/testing/bwrap/fixtures/bwrap_smoke.rs
+++ b/bazel/rules/testing/bwrap/fixtures/bwrap_smoke.rs
@@ -12,6 +12,12 @@ use std::process::Command;
 use anyhow::Context;
 use anyhow::Result;
 
+/// Namespace classes whose topology is part of this harness's contract.
+///
+/// The outer bwrap isolates user, mount, IPC, and UTS, but deliberately inherits
+/// PID plus writable procfs for nested bwrap setup and network for loopback.
+/// Cgroup and time namespaces stay executor-owned; `*_for_children` entries are
+/// alternate PID/time handles rather than separate namespace classes.
 const NAMESPACES: &[&str] = &["user", "mnt", "pid", "ipc", "uts", "net"];
 
 fn main() -> Result<()> {
diff --git a/bazel/rules/testing/bwrap/src/lib.rs b/bazel/rules/testing/bwrap/src/lib.rs
index 3cbbdd9d7a..56a9ddb271 100644
--- a/bazel/rules/testing/bwrap/src/lib.rs
+++ b/bazel/rules/testing/bwrap/src/lib.rs
@@ -86,7 +86,12 @@ impl BwrapTestCommand {
         fs::create_dir(&private_tmp)
             .with_context(|| format!("create private tmp directory {}", private_tmp.display()))?;
         fs::set_permissions(&private_tmp, fs::Permissions::from_mode(0o1777)).with_context(
-            || format!("set permissions on private tmp directory {}", private_tmp.display()),
+            || {
+                format!(
+                    "set permissions on private tmp directory {}",
+                    private_tmp.display()
+                )
+            },
         )?;
         let home = private_tmp.join("home");
         let codex_home = private_tmp.join("codex-home");
@@ -104,15 +109,19 @@ impl BwrapTestCommand {
             },
         )?;
 
-        // Keep the VM's PID namespace and expose its existing procfs
-        // read-write: a nested bwrap must write UID/GID maps before mounting
-        // the fresh procfs for its own PID namespace. Mounting a fresh procfs
-        // here instead leaves locked child mounts that make the nested mount
-        // fail Linux's mount_too_revealing check. A private directory backs
-        // /tmp so remote fixtures cannot alias files owned by the test runner.
-        // The Bazel workspace is the other deliberate writable carveout:
-        // production bwrap setup needs to materialize missing mount targets
-        // below a writable command cwd.
+        // Firecracker supplies the host PID boundary, so the outer wrapper
+        // deliberately shares the disposable VM's PID namespace and rebinds
+        // its procfs read-write. Nested unprivileged bwrap writes UID/GID maps
+        // through the caller's /proc before mounting a fresh procfs for its
+        // own PID namespace. Giving the outer wrapper a fresh procfs on this
+        // executor leaves locked child mounts that make the nested proc mount
+        // fail Linux's mount_too_revealing check; making the inherited procfs
+        // read-only instead makes UID/GID map setup fail.
+        //
+        // A private directory backs /tmp so remote fixtures cannot alias files
+        // owned by the test runner. The Bazel workspace is the other deliberate
+        // writable carveout: production bwrap setup needs to materialize
+        // missing mount targets below a writable command cwd.
         //
         // Map the caller to a non-root ID and leave the final process with no
         // capabilities. Bubblewrap rejects a non-root caller that already has
@@ -133,11 +142,7 @@ impl BwrapTestCommand {
             ])
             .arg(&private_tmp)
             .arg("/tmp")
-            .args([
-                "--dev",
-                "/dev",
-                "--bind",
-            ])
+            .args(["--dev", "/dev", "--bind"])
             .arg(&workspace)
             .arg(&workspace)
             .args([