#673 [tests][mutants] 変異試験が防御を外したまま kill(-1) などを実行し、利用者の全プロセスを止める

.cargo/mutants.toml

@@ -0,0 +1,27 @@
+# cargo-mutants configuration (Issue #673).
+#
+# Incident (2026-10-14, macOS): a `cargo mutants` run replaced
+#   src/bounded_process.rs: `replace signalable_process_group -> Option<i32>
+#   with Some(1)`
+# and the mutated test binary then executed `kill(-1, SIGTERM)`, terminating
+# every process the user owned (the launchd log recorded 102 SIGTERM exits).
+# The product guard was correct; the mutant removed it. The same shape exists
+# in `group_is_signalable` (src/bin/gui_server/trial_process.rs), where a
+# `-> true` mutant turns `send_group_signal(1, Kill)` into `kill(-1, SIGKILL)`.
+#
+# Exclusion is the immediate mitigation (Issue #673 対処の方向 1). It is a
+# *listing* filter: cargo-mutants applies `exclude_re` to the mutation
+# description, and the command line `--re` does not override it. Removing these
+# entries requires the isolated-container evidence from 対処の方向 4: the mutant
+# must actually be enumerated and executed inside the container, the dangerous
+# value must be refused by the last, non-mutatable defense in the `kill`
+# wrapper, no dangerous syscall may be observed, the same-UID sentinel must
+# survive, and only then may the exclusion be dropped (対処の方向 4, last item).
+#
+# The names match cargo-mutants' description text, for example
+#   `replace signalable_process_group -> Option<i32> with Some(1)`
+#   `replace group_is_signalable -> bool with true`
+exclude_re = [
+    "signalable_process_group",
+    "group_is_signalable",
+]

docs/dev/parallel-dev-harness.md

@@ -301,6 +301,39 @@ Approval is always written, never implied: the PM's dispatch brief says
    (`gh pr list --state merged --head <branch>`), then
    `git push origin --delete <branch>`.
 
+## 3a. Mutation-testing rules
+
+`cargo mutants` may run only under these rules. They exist because a macOS run
+on 2026-10-14 mutated a guard (`signalable_process_group -> Some(1)`) and the
+mutated test binary executed `kill(-1, SIGTERM)`, stopping every process the user
+owned (102 SIGTERM exits in the launchd log; #673).
+
+- Scope the run to the functions named in the brief. Widening it needs the PM's
+  confirmation.
+- Use a fresh run script and a fresh output directory for every run. Before
+  running, produce `cargo mutants --list` and have the PM match every mutant's
+  name and replacement value (a count alone is not enough).
+- Mutations that touch signal, deletion, or the child session always run in an
+  isolated environment, whether or not the last, non-mutatable defense is in
+  place (Docker; a macOS-only test on a disposable VM or a dedicated account).
+  The Docker container is non-root (uid 1000) with `--network none`, no mounts,
+  and no `--privileged`, `--pid=host`, or Docker socket; the tree enters only
+  through the `docker build` context or `docker cp`.
+- `cargo mutants`' `--timeout` does not reap processes a test left behind (for
+  example a child that ignores `TERM`), so each run uses a disposable container
+  and the whole container is discarded afterwards (`docker rm -f`). On the
+  2026-10-15 confirmation run a `TERM`-ignoring child survived inside the
+  container after the timeout.
+- Host tests run with the narrowest test-name filter, a dedicated secret-free
+  HOME and Git configuration, and known executables. Set `GITHUB_STEP_SUMMARY`
+  only to an unset value or a file the new run owns. The outer `GIT_*` is dropped
+  by the bounded child's allowlist, so verify the actual child environment.
+- `--jobs 1`, the heavy lock, and the user being at the keyboard are not
+  substitutes for isolation or for reaping the processes a run started.
+
+The signal sites, their defenses, and the functions to compare against
+`cargo mutants --list` are listed in `docs/dev/dangerous-commands.md`.
+
 ## 4. auto-yes policy
 
 | Session | auto-yes |

src/bin/gui_server/session_paths.rs

@@ -46,6 +46,10 @@ pub(super) struct RunRecordPaths {
 
 pub(super) struct SessionPaths {
     run_root: PathBuf,
+    /// The `runs` directory that owns `run_root`. Kept separately from
+    /// `run_root` so a recursive delete can re-check the real path against an
+    /// independent reference (Issue #673).
+    runs_root: PathBuf,
     execution_workspace: PathBuf,
     selected_binding: Option<WorkingDirectoryBinding>,
 }
@@ -73,8 +77,10 @@ impl SessionPaths {
             || workspace.join(SESSION_WORKSPACES_DIRECTORY).join(id),
             |binding| binding.canonical_path.clone(),
         );
+        let runs_root = commandagent::runtime_paths::runs_dir(workspace);
         Ok(Self {
-            run_root: commandagent::runtime_paths::runs_dir(workspace).join(id),
+            run_root: runs_root.join(id),
+            runs_root,
             execution_workspace,
             selected_binding,
         })
@@ -105,6 +111,7 @@ impl SessionPaths {
             );
             return Ok(Some(Self {
                 run_root,
+                runs_root,
                 execution_workspace,
                 selected_binding,
             }));
@@ -297,7 +304,34 @@ impl SessionPaths {
     }
 
     pub(super) fn rollback_unstarted(&self) -> anyhow::Result<()> {
-        match std::fs::remove_dir_all(&self.run_root) {
+        // Issue #673: re-check the recursive delete target against the real
+        // path of the runs root that owns it, so a mutation that moves
+        // `run_root` outside `runs/` (a `..`, an ancestor symlink, the root
+        // itself, or the filesystem root) is refused before anything is
+        // removed. The checked real path is the value that is removed. A
+        // missing target left nothing to roll back and stays `Ok`.
+        match std::fs::symlink_metadata(&self.run_root) {
+            Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
+            Err(error) => {
+                return Err(error).with_context(|| {
+                    format!(
+                        "inspect unstarted session directory {}",
+                        self.run_root.display()
+                    )
+                });
+            }
+            Ok(_) => {}
+        }
+        let Some(checked) = commandagent::tools::dir_fd::strict_descendant_real_path(
+            &self.runs_root,
+            &self.run_root,
+        ) else {
+            anyhow::bail!(
+                "refusing to remove a session run outside the runs root: {}",
+                self.run_root.display()
+            );
+        };
+        match std::fs::remove_dir_all(&checked) {
             Ok(()) => Ok(()),
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()),
             Err(error) => Err(error).with_context(|| {

src/bin/gui_server/trial_process.rs

@@ -426,6 +426,11 @@ enum Signal {
 
 /// Guards the negative `kill` targets: group id 1 is init and 0 is the
 /// caller's own group, so only a real child group id may be signalled.
+///
+/// This is a value-returning selector, so cargo-mutants can replace the whole
+/// body with `true` (Issue #673). It is listed in `.cargo/mutants.toml`, and
+/// every `kill` caller re-checks the converted target with a non-mutatable
+/// `assert!(-process_group < -1)` in the function that calls `kill`.
 #[cfg(unix)]
 fn group_is_signalable(process_group: i32) -> bool {
     process_group > 1
@@ -436,6 +441,15 @@ fn send_group_signal(process_group: i32, signal: Signal) -> anyhow::Result<()> {
     if !group_is_signalable(process_group) {
         bail!("refusing to signal process group {process_group}");
     }
+    // Last, non-mutatable defense (Issue #673), written directly in the
+    // function that calls `kill`: even a mutated selector
+    // (`group_is_signalable -> true`) panics here instead of executing
+    // `kill(-1, ...)` (every signalable process) or `kill(0, ...)` (the
+    // caller's own group). The `bail!` path above is unchanged.
+    assert!(
+        -process_group < -1,
+        "refusing to signal process group {process_group}"
+    );
     let signal = match signal {
         Signal::Interrupt => libc::SIGINT,
         Signal::Kill => libc::SIGKILL,
@@ -464,6 +478,12 @@ fn process_group_exists(process_group: i32) -> bool {
     if !group_is_signalable(process_group) {
         return false;
     }
+    // Last, non-mutatable defense (Issue #673), written directly in the
+    // function that calls `kill`.
+    assert!(
+        -process_group < -1,
+        "refusing to signal process group {process_group}"
+    );
     // SAFETY: `process_group` is checked to be greater than 1 and is derived
     // from the delegated child PID; signal 0 only probes for its existence.
     let result = unsafe { libc::kill(-process_group, 0) };
@@ -517,4 +537,18 @@ mod tests {
         assert!(send_group_signal(1, Signal::Kill).is_err());
         assert!(!process_group_exists(1));
     }
+
+    /// Issue #673 対処の方向 4: pass 0 (the caller's own group), 1 (init), and
+    /// -1 straight to the functions that call `kill`, skipping the selector's
+    /// own assertion, so a mutated `group_is_signalable` reaches the
+    /// non-mutatable `assert!` in front of `kill` and panics there instead of
+    /// signalling `kill(0)` / `kill(-1)`. Runs only in the isolated container.
+    #[cfg(unix)]
+    #[test]
+    fn dangerous_value_send_group_signal_refuses_init_caller_and_negative_groups() {
+        for process_group in [0, 1, -1] {
+            assert!(send_group_signal(process_group, Signal::Kill).is_err());
+            assert!(!process_group_exists(process_group));
+        }
+    }
 }

src/bounded_process.rs

@@ -401,6 +401,11 @@ pub fn terminate_process_group(child: &mut Child) {
 /// signal with a negative `kill` target. The child is spawned with
 /// `setsid()`, so its PID equals its group id, but `-1` would signal
 /// every signalable process and `0`/`1` would target the caller or init.
+///
+/// This is a value-returning selector, so cargo-mutants can replace the whole
+/// body with `Some(1)` (Issue #673). It is listed in `.cargo/mutants.toml`, and
+/// every `kill` caller re-checks the converted target with a non-mutatable
+/// `assert!(-pgid < -1)` in the function that calls `kill`.
 #[cfg(unix)]
 fn signalable_process_group(pid: u32) -> Option<i32> {
     let pgid = i32::try_from(pid).ok()?;
@@ -415,6 +420,16 @@ pub fn terminate_process_group_by_pid(pid: u32) -> io::Result<()> {
                 "refusing to signal a process group outside the child range",
             ));
         };
+        // Last, non-mutatable defense (Issue #673). cargo-mutants does not
+        // rewrite comparisons inside `assert!`, and this is written directly in
+        // the function that calls `kill`, so a mutated selector (`Some(0)`,
+        // `Some(1)`, `Some(-1)`) panics here instead of executing `kill(0, ...)`
+        // (the caller's own group) or `kill(-1, ...)` (every signalable
+        // process). The `Err` path above is unchanged.
+        assert!(
+            -pgid < -1,
+            "refusing to signal a process group outside the child range"
+        );
         // SAFETY: `pgid` is greater than 1 and derived from a child spawned
         // with `setsid()`, so `-pgid` names only that child's own
         // process group, never pid 0/1 or the caller's group.
@@ -475,6 +490,12 @@ fn terminate_process_group_with_grace<F, C>(
     #[cfg(unix)]
     {
         if let Some(pgid) = signalable_process_group(child.id()) {
+            // Last, non-mutatable defense (Issue #673), written directly in the
+            // function that calls `kill`.
+            assert!(
+                -pgid < -1,
+                "refusing to signal a process group outside the child range"
+            );
             // SAFETY: `pgid` is greater than 1 and was created with
             // `configure_child_session()` for this child, so `-pgid` targets
             // only the child's process group, never pid 0/1 or the caller's.
@@ -497,6 +518,12 @@ fn terminate_process_group_with_grace<F, C>(
     #[cfg(unix)]
     {
         if let Some(pgid) = signalable_process_group(child.id()) {
+            // Last, non-mutatable defense (Issue #673), written directly in the
+            // function that calls `kill`.
+            assert!(
+                -pgid < -1,
+                "refusing to signal a process group outside the child range"
+            );
             // SAFETY: the same checked child-derived group identifier as above.
             unsafe {
                 libc::kill(-pgid, libc::SIGKILL);
@@ -861,6 +888,95 @@ mod tests {
         assert!(terminate_process_group_by_pid(1).is_err());
     }
 
+    /// Issue #673 対処の方向 4: pass 0 (the caller's own group) and 1 (init)
+    /// straight to the function that calls `kill`, skipping the selector's own
+    /// assertions, so a mutated `signalable_process_group` reaches the
+    /// non-mutatable `assert!` in front of `kill` and panics there instead of
+    /// signalling. Runs only in the isolated container.
+    #[cfg(unix)]
+    #[test]
+    fn dangerous_value_terminate_process_group_by_pid_refuses_init_and_caller_group() {
+        assert!(terminate_process_group_by_pid(0).is_err());
+        assert!(terminate_process_group_by_pid(1).is_err());
+    }
+
+    /// Issue #673 sentinel test. It exercises a real `kill(-pgid, ...)`, so it
+    /// must run only inside the isolated container: it panics unless
+    /// `COMMANDAGENT_ISSUE673_CONTAINER=1` is set, and it is `#[ignore]`d so
+    /// `cargo test` never runs it in a host process. Docker command (from the
+    /// repository root of a container that shares no host PID namespace):
+    ///
+    /// ```text
+    /// COMMANDAGENT_ISSUE673_CONTAINER=1 \
+    ///   cargo test --lib -- --ignored --exact \
+    ///   bounded_process::tests::dangerous_sentinel_same_uid_other_group_survives
+    /// ```
+    ///
+    /// It proves the scope of the signal: a same-UID process in a **different**
+    /// process group survives, while the target child's group and its
+    /// grandchild are terminated.
+    #[cfg(unix)]
+    #[test]
+    #[ignore = "run only inside the isolated container (COMMANDAGENT_ISSUE673_CONTAINER=1)"]
+    fn dangerous_sentinel_same_uid_other_group_survives() {
+        if std::env::var("COMMANDAGENT_ISSUE673_CONTAINER")
+            .ok()
+            .as_deref()
+            != Some("1")
+        {
+            panic!("run only inside the isolated container");
+        }
+        use std::os::unix::process::CommandExt;
+
+        // Sentinel: same UID, its own process group, and not the target group.
+        let mut sentinel = Command::new("sh");
+        sentinel
+            .arg("-c")
+            .arg("sleep 30")
+            .process_group(0)
+            .stdin(Stdio::null())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null());
+        let mut sentinel = sentinel.spawn().expect("spawn the sentinel");
+
+        // Target: a `setsid` child whose `sh` also leaves a grandchild in the
+        // same process group.
+        let mut target = Command::new("sh");
+        target
+            .arg("-c")
+            .arg("sh -c 'sleep 30' & sleep 30")
+            .stdin(Stdio::null())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null());
+        let mut target = spawn_child(&mut target).expect("spawn the target");
+        let target_pid = target.id();
+        thread::sleep(Duration::from_millis(300));
+
+        terminate_process_group_by_pid(target_pid).expect("reap the target group");
+        let _ = target.wait();
+        assert!(
+            !group_exists(target_pid),
+            "the target group must be gone after the reap"
+        );
+        assert!(
+            sentinel.try_wait().expect("poll the sentinel").is_none(),
+            "the same-UID sentinel in another group must survive"
+        );
+
+        let _ = sentinel.kill();
+        let _ = sentinel.wait();
+    }
+
+    #[cfg(unix)]
+    fn group_exists(pgid: u32) -> bool {
+        let Ok(pgid) = i32::try_from(pgid) else {
+            return false;
+        };
+        // SAFETY: signal 0 only probes for the group's existence.
+        let rc = unsafe { libc::kill(-pgid, 0) };
+        rc == 0
+    }
+
     struct TestEnvGuard {
         keys: Vec<&'static str>,
     }

src/model_probe.rs

@@ -212,16 +212,22 @@ enum ProbeTaskKind {
 }
 
 struct ScratchWorkspace {
+    /// The system temp directory that owns `path`. Kept separately from `path`
+    /// so the recursive delete in [`ScratchWorkspace::cleanup`] can re-check the
+    /// real path against an independent reference (Issue #673).
+    root: PathBuf,
     path: PathBuf,
     cleaned: bool,
 }
 
 impl ScratchWorkspace {
     fn create() -> anyhow::Result<Self> {
-        let path = std::env::temp_dir().join(format!("anvil-model-probe-{}", Uuid::now_v7()));
+        let root = std::env::temp_dir();
+        let path = root.join(format!("anvil-model-probe-{}", Uuid::now_v7()));
         fs::create_dir_all(&path)
             .with_context(|| format!("failed to create scratch workspace {}", path.display()))?;
         Ok(Self {
+            root,
             path,
             cleaned: false,
         })
@@ -231,7 +237,27 @@ impl ScratchWorkspace {
         if self.cleaned {
             return true;
         }
-        self.cleaned = match fs::remove_dir_all(&self.path) {
+        // Issue #673: only remove a scratch directory that is a strict
+        // descendant of the temp dir it was created in, so a mutation that
+        // moves `path` cannot recursively delete an unrelated directory.
+        match std::fs::symlink_metadata(&self.path) {
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+                self.cleaned = true;
+                return true;
+            }
+            Err(_) => {
+                self.cleaned = false;
+                return false;
+            }
+            Ok(_) => {}
+        }
+        let Some(checked) =
+            crate::tools::dir_fd::strict_descendant_real_path(&self.root, &self.path)
+        else {
+            self.cleaned = false;
+            return false;
+        };
+        self.cleaned = match fs::remove_dir_all(&checked) {
             Ok(()) => true,
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => true,
             Err(_) => false,