diff --git a/README.md b/README.md index 68b22fa..7ac16dd 100644 --- a/README.md +++ b/README.md @@ -480,12 +480,72 @@ The agent CLIs themselves (`claude`, `opencode`) are **external, non-Nix tools** per their own docs and make sure they are on `PATH` before launching live agents. The devShell documents this in its banner. +### PATH on a NixOS host: why a tool that IS installed says "command not found" + +**An agent's `PATH` does not include the system profile.** On `notplants-orchestrator` the agent +shell gets a pinned list of individual store paths (bash, git, python, coreutils, findutils, grep, +sed, systemd, tmux, openssh, net-tools) and **not** `/run/current-system/sw/bin`. Everything the +host declares in `environment.systemPackages` therefore exists and is unreachable. + +Measured 2026-08-21: `ps`, `pgrep`, `free`, `cmp`, `awk`, `curl`, `diff`, `strings`, `nm`, `getent` +and `ping` were ALL installed (notplants-nix `hosts/notplants-orchestrator/configuration.nix`, +"give agents a real toolbox") and all reported `command not found`. + +**Why this is worse than an inconvenience.** A missing binary run through `shell=True` returns +**rc=127 with empty stdout**, and empty stdout is indistinguishable from a real answer of "none": + +- `agents.py` shelled out to `pgrep -P` to find a pane's child processes. With `pgrep` unreachable + it returned *no children*, so `_build_running` was **always False** and the stall detector could + reboot an agent in the middle of a build. It shipped that way and no test caught it, because the + unit tests **mocked `pgrep`** — the fake stood in for the broken dependency. +- The same session read an empty `ps` as "0 agent processes running", and an empty `strings` as + proof that a binary had its features stripped. + +**Check before concluding a tool is absent:** + +```bash +ls /run/current-system/sw/bin/ # installed but unreachable? +command -v # reachable? +``` + +**Put the system profile on PATH** (appended, so the sandbox's pinned store paths keep priority): + +```bash +case ":$PATH:" in *":/run/current-system/sw/bin:"*) ;; + *) export PATH="$PATH:/run/current-system/sw/bin" ;; esac +``` + +For something genuinely not installed, fetch it without changing the host: + +```bash +nix shell nixpkgs#tcpdump -c tcpdump ... # one command, nothing persisted +nix run nixpkgs#git-filter-repo -- --help +``` + +`nix` itself may also be off `PATH`; it lives at `/nix/var/nix/profiles/default/bin/nix`. + +**Rule for harness code: do not shell out for something the kernel already exposes.** `agents.py` +now reads `/proc//stat` and `/proc//comm` directly instead of calling `pgrep` and `ps`. +That has no PATH dependency and cannot fail silently in the direction that matters. If you must +call an external tool, check the return code — never treat empty output as an answer. + --- ## Testing The `tests/` directory holds the harness's own test suite. One runner drives everything: +- `tests/test_unit.py` — the harness: config load, kickoff, the phase machine, limit parsing, + waiting-until, the build-process detector. +- `tests/test_tools.py` — the standalone tools (`tangled_pr.py`, `tangled_pr_close.py`, + `tools/gateway-domain.py`). **No network**: every HTTP boundary is injected as a fake `_fetch`. + +**Mock the seam you own, not the tool you depend on.** The build-detector tests used to fake +`pgrep` and `ps` subprocess calls, so they passed on a host where neither was reachable and the +real defect — empty output read as "no build running" — was invisible to every test. They now +patch `_proc_descendants` and `_comms`, the functions this repo owns. A test that mocks a +dependency proves the mock works. + ```bash nix develop -c ./tests/run.sh # unit tests always; live backend smokes when available # or just: ./tests/run.sh # (python3 + tmux must be on PATH) diff --git a/agents.py b/agents.py index 7f4188a..54d74d8 100755 --- a/agents.py +++ b/agents.py @@ -481,16 +481,37 @@ DEFAULT_BUILD_PROCS_RE = (r"^(cargo|cargo-llvm-cov|cargo-mutants|cargo-nextest|n def _proc_descendants(roots): """All descendant PIDs of the given root PIDs (BFS via pgrep -P), EXCLUDING the roots themselves — the pane root is the claude process whose args embed the prompt (which mentions cargo/rustc/…), so - matching it would false-positive; we only inspect its real child processes.""" - roots = [p for p in roots if str(p).isdigit()] + matching it would false-positive; we only inspect its real child processes. + + Reads /proc directly rather than shelling out to `pgrep -P`. WHY (2026-08-21): pgrep is not + installed on every host we run on, and a missing binary makes `subprocess.run(shell=True)` + return rc=127 with EMPTY stdout — indistinguishable from "this process has no children". That + made this function return an empty set on such a host, which made `_build_running` always False, + which let the stall detector reboot an agent in the middle of a build. Reading /proc removes the + dependency and cannot fail silently in that direction. + (Note the repo already bans `pgrep -f` for a DIFFERENT reason — it self-matches. Two distinct + traps, same tool.)""" + roots = [str(p) for p in roots if str(p).isdigit()] + + children = {} + for entry in os.listdir("/proc"): + if not entry.isdigit(): + continue + try: + with open(f"/proc/{entry}/stat") as fh: + fields = fh.read().rsplit(") ", 1)[-1].split() # rsplit: comm may contain spaces + ppid = fields[1] + except (OSError, IndexError): + continue # process exited mid-scan + children.setdefault(ppid, []).append(entry) + seen, stack = set(), list(roots) while stack: pid = stack.pop() if pid in seen: continue seen.add(pid) - c = subprocess.run(f"pgrep -P {pid}", shell=True, capture_output=True, text=True) - stack += [p for p in c.stdout.split() if p.isdigit()] + stack += children.get(pid, []) return seen - set(roots) def _build_running(cfg, agent): @@ -507,9 +528,25 @@ def _build_running(cfg, agent): rx = re.compile(cfg["watchdog"].get("build_procs_re", DEFAULT_BUILD_PROCS_RE)) except re.error: rx = re.compile(DEFAULT_BUILD_PROCS_RE) - ps = subprocess.run("ps -o comm= -p " + ",".join(sorted(kids)), - shell=True, capture_output=True, text=True) - return any(rx.match(c.strip()) for c in ps.stdout.splitlines() if c.strip()) + # Read /proc//comm rather than `ps -o comm=`: ps is absent on some hosts we run on, and + # an absent binary yields EMPTY stdout, which this function cannot tell from "no build is + # running" — so the stall detector would reboot an agent mid-build. Same class as the pgrep + # dependency removed from _proc_descendants above; both were invisible because the unit tests + # MOCKED these subprocess calls, so the suite exercised the fake and never the dependency. + return any(rx.match(c) for c in _comms(kids)) + +def _comms(pids): + """The comm (process name) of each pid, skipping any that exited mid-scan. No external tools.""" + out = [] + for pid in sorted(pids): + try: + with open(f"/proc/{pid}/comm") as fh: + c = fh.read().strip() + except OSError: + continue + if c: + out.append(c) + return out def _done_nudge_msg(cfg, ph): """The DONE-nudge: prompts a stalled loop agent to finalize a built-but-unmarked phase.""" diff --git a/tangled_pr.py b/tangled_pr.py index 9fe75d6..c53b3fb 100755 --- a/tangled_pr.py +++ b/tangled_pr.py @@ -17,10 +17,24 @@ COOKIE FILE (tools/.tangled-session, gitignored), a single line — paste the wh Cookie header value from the logged-in browser (both appview-* cookies): TANGLED_COOKIE=appview-session-v2=<...>; appview-accounts-v2=<...> +RELIABILITY (rewritten 2026-08-21). This tool used to judge success ONLY by an +HX-Redirect header on the POST response. A create that SUCCEEDED but answered without +that header read as a failure, the caller retried, and Tangled grew duplicate pulls — +that is exactly how #397, #398 and #399 were created for one branch. A response header +describes what the server meant to say; it is not the artifact. So now: + + * BEFORE posting, look for an existing OPEN pull for this source branch. If one + exists, refuse and name it — a duplicate cannot be created even if a caller retries. + * AFTER posting, confirm against the PULLS LIST, not the response: a new pull number + that did not exist before and whose page names this source branch IS the success, + with or without a redirect header. + * Only report failure when no such pull appeared. A false failure is worse than a + loud error here, because the caller's remedy is to retry. + USAGE: tangled_pr.py --owner notplants-bot.bsky.social --repo lichen.page \ --target main --source hardening-review \ - [--fork did:plc:] [--title "..."] [--body "..."] + [--fork did:plc:] [--title "..."] [--body "..."] [--dry-run] """ import argparse, os, sys, urllib.request, urllib.parse, urllib.error @@ -41,6 +55,70 @@ def load_cookie(path=None): sys.exit("no tangled.cookie in the secret store — add it with: sops /secrets/store.yaml") return c + +# ── verification against the artifact, not the response ───────────────────────── +# These are pure so they can be unit-tested without a network: see tests/test_tools.py. + +import re + +def pull_numbers(html): + """Every pull number linked from a pulls index page.""" + return {int(n) for n in re.findall(r'/pulls/(\d+)\b', html or "")} + +def page_names_branch(html, branch): + """True if a pull's page mentions this source branch. + + Substring rather than token: Tangled renders the branch inside markup we do not + control, and a false NEGATIVE here would resurrect the duplicate bug. A false + positive is caught by the caller, which only asks about pulls it just created. + """ + return bool(branch) and branch in (html or "") + +def classify(before, after, branch_pages): + """Decide what a POST did, from the pulls list before and after it. + + before/after: sets of pull numbers. branch_pages: {number: names_our_branch}. + Returns (verdict, pull_number) where verdict is: + "created" a new pull naming our branch appeared + "unrelated" new pulls appeared but none is ours (someone else was filing) + "none" nothing new appeared — a real failure + """ + fresh = sorted(after - before) + ours = [n for n in fresh if branch_pages.get(n)] + if ours: + return "created", max(ours) + if fresh: + return "unrelated", None + return "none", None + +def fetch(url, cookie, timeout=30): + """GET a page as the bot. Returns "" on any error: callers treat an unreadable page as + 'cannot confirm', never as 'confirmed absent' — the difference is the duplicate bug.""" + try: + r = urllib.request.Request(url, headers={"Cookie": cookie, "User-Agent": "tangled-pr-bot"}) + return urllib.request.urlopen(r, timeout=timeout).read().decode("utf-8", "replace") + except Exception: + return "" + +def index_pulls(owner, repo, cookie, _fetch=fetch): + return pull_numbers(_fetch(f"{BASE}/{owner}/{repo}/pulls", cookie)) + +def pull_names_branch(owner, repo, n, branch, cookie, _fetch=fetch): + return page_names_branch(_fetch(f"{BASE}/{owner}/{repo}/pulls/{n}", cookie), branch) + +def existing_pull_for(owner, repo, branch, cookie, numbers, scan=12, _fetch=fetch): + """The newest pull whose page names this branch, scanning back `scan` pulls. + + Bounded on purpose: an unbounded scan would fetch hundreds of pages on every call. + Scanning the newest N is enough for the duplicate this guards against, which is a + retry seconds after the original. Returns None if none is found within the window — + which is 'not seen', not 'does not exist', and the post-check is the real backstop. + """ + for n in sorted(numbers, reverse=True)[:scan]: + if pull_names_branch(owner, repo, n, branch, cookie, _fetch): + return n + return None + def main(): ap = argparse.ArgumentParser(description="file a Tangled PR via a reused session cookie") ap.add_argument("--owner", required=True) @@ -51,12 +129,28 @@ def main(): ap.add_argument("--title", default="") ap.add_argument("--body", default="") ap.add_argument("--cookie-file", default=None, help="legacy: read the cookie from this file instead of the store") + ap.add_argument("--dry-run", action="store_true", help="check for an existing pull and show the request; do not POST") + ap.add_argument("--allow-duplicate", action="store_true", help="post even if a pull for this branch already exists") a = ap.parse_args() cookie = load_cookie(a.cookie_file) # Tangled's PR form is htmx: it POSTs to /pulls/new (NOT /pulls/, which is 405) and only processes # the create when it sees the HX-Request header — otherwise it just re-renders the page (a 200 that # creates nothing). Success is signalled by an HX-Redirect header pointing at the new pull. + # PRE-CHECK: refuse to create a second pull for a branch that already has one. This is what + # makes a retry harmless rather than duplicating — the failure mode that produced #397-#399. + before = index_pulls(a.owner, a.repo, cookie) + if not before: + print("WARNING: could not read the pulls index — cannot check for an existing pull, and " + "cannot confirm the result afterwards. Proceeding, but verify by hand.") + dup = existing_pull_for(a.owner, a.repo, a.source, cookie, before) if before else None + if dup and not a.allow_duplicate: + print(f"REFUSING: #{dup} already exists for source branch {a.source!r}.") + print(f" {BASE}/{a.owner}/{a.repo}/pulls/{dup}") + print(" To push new commits to an existing pull use tangled_pr_resubmit.py.") + print(" To file anyway (rarely right): --allow-duplicate") + sys.exit(3) + new_url = f"{BASE}/{a.owner}/{a.repo}/pulls/new" form = { "source": "branch", # branch-compare mode (each PR targets the branch below it) @@ -76,6 +170,12 @@ def main(): "Referer": new_url, }) + if a.dry_run: + print(f"DRY RUN: would POST {new_url}") + print(f" target={a.target!r} source={a.source!r} title={a.title[:60]!r}") + print(f" existing pull for this branch: {dup if dup else 'none found in the scan window'}") + return + class NoRedirect(urllib.request.HTTPRedirectHandler): def redirect_request(self, *args, **kw): # capture the redirect instead of following it return None @@ -88,15 +188,27 @@ def main(): # htmx success is signalled by HX-Redirect (…/pulls/), not a normal 3xx Location. target = hdrs.get("HX-Redirect", "") or hdrs.get("HX-Location", "") or hdrs.get("Location", "") print(f"HTTP {code}" + (f" -> {target}" if target else "")) - if target and "/pulls/" in target and "/new" not in target: - print("OK: pull created ->", (BASE + target) if target.startswith("/") else target) + + if "/login" in target or "oauth" in target.lower(): + sys.exit("AUTH FAILED: session cookie expired/invalid — refresh the cookie in the secret " + "store (scripts/get-tangled-cookie.py)") + + # POST-CHECK: the artifact decides, not the header. A create that answered without a redirect + # is still a create; reporting it as a failure is what made callers retry into duplicates. + after = index_pulls(a.owner, a.repo, cookie) + branch_pages = {n: pull_names_branch(a.owner, a.repo, n, a.source, cookie) + for n in sorted(after - before, reverse=True)[:12]} + verdict, num = classify(before, after, branch_pages) + + if verdict == "created": + print(f"OK: pull #{num} created -> {BASE}/{a.owner}/{a.repo}/pulls/{num}") + if not target: + print(" (no redirect header — confirmed against the pulls list instead)") return - if ("/login" in target or "oauth" in target.lower()): - sys.exit("AUTH FAILED: session cookie expired/invalid — refresh engine/.tangled-session " - "(scripts/get-tangled-cookie.py)") - # otherwise surface whatever the page said (a Notice, etc.) + if verdict == "unrelated": + print(" new pulls appeared but none names this source branch — not ours.") snippet = " ".join(body.split())[:600] - print(" no clear success redirect — response snippet:") + print("FAILED: no pull for this branch appeared. Response snippet:") print(" " + snippet) sys.exit(2) diff --git a/tests/test_tools.py b/tests/test_tools.py new file mode 100644 index 0000000..e0a1b2a --- /dev/null +++ b/tests/test_tools.py @@ -0,0 +1,169 @@ +#!/usr/bin/env python3 +"""Unit tests for the standalone tools (the tangled_* scripts, secrets, gateway-domain). + +Run: python3 -m unittest tests.test_tools (from the repo root) + ./tests/run.sh (discovers this file automatically) + +NO NETWORK. Every HTTP boundary is injected as a fake `_fetch`, so these run offline and +in CI. The point is the DECISION logic — "did the pull get created", "is this backend +valid" — because that is where the defects have actually been. + +The reason this file exists: tangled_pr.py judged success only by a response header, so a +create that answered without one read as a failure, the caller retried, and duplicate +pulls #397-#399 were filed for one branch. The tool was never tested; the bug survived +months of daily use and was found by counting pulls, not by running the script. +""" +import os, sys, unittest +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import tangled_pr # noqa: E402 + + +class TestPullNumbers(unittest.TestCase): + def test_extracts_every_linked_pull(self): + html = 'xy' + self.assertEqual(tangled_pr.pull_numbers(html), {417, 416}) + + def test_deduplicates_repeated_links(self): + html = '/pulls/12 /pulls/12 /pulls/12' + self.assertEqual(tangled_pr.pull_numbers(html), {12}) + + def test_ignores_the_new_pull_form(self): + # /pulls/new must not parse as a number — it is the page you POST to, not a pull + self.assertEqual(tangled_pr.pull_numbers('/pulls/new /pulls/9'), {9}) + + def test_empty_and_none_are_empty_not_an_error(self): + # an unreadable page must yield "cannot confirm", never a crash mid-create + self.assertEqual(tangled_pr.pull_numbers(""), set()) + self.assertEqual(tangled_pr.pull_numbers(None), set()) + + +class TestPageNamesBranch(unittest.TestCase): + def test_finds_the_branch(self): + self.assertTrue(tangled_pr.page_names_branch("
e2e-pds/R4-real
", "e2e-pds/R4-real")) + + def test_absent_branch(self): + self.assertFalse(tangled_pr.page_names_branch("
other
", "e2e-pds/R4-real")) + + def test_unreadable_page_is_not_a_match(self): + self.assertFalse(tangled_pr.page_names_branch("", "b")) + self.assertFalse(tangled_pr.page_names_branch(None, "b")) + + def test_empty_branch_never_matches(self): + # guards against a caller passing "" and matching every page + self.assertFalse(tangled_pr.page_names_branch("anything", "")) + + +class TestClassify(unittest.TestCase): + """The regression that matters: a successful create with NO redirect header.""" + + def test_new_pull_naming_our_branch_is_created(self): + v, n = tangled_pr.classify({1, 2}, {1, 2, 3}, {3: True}) + self.assertEqual((v, n), ("created", 3)) + + def test_created_even_though_the_header_said_nothing(self): + # THE BUG: classify never sees the response at all, so a missing HX-Redirect + # cannot turn a real create into a reported failure. + v, n = tangled_pr.classify({396}, {396, 397}, {397: True}) + self.assertEqual(v, "created") + + def test_no_new_pull_is_a_real_failure(self): + v, n = tangled_pr.classify({1, 2}, {1, 2}, {}) + self.assertEqual((v, n), ("none", None)) + + def test_someone_elses_pull_is_not_ours(self): + v, n = tangled_pr.classify({1}, {1, 2}, {2: False}) + self.assertEqual((v, n), ("unrelated", None)) + + def test_picks_the_highest_of_several_of_ours(self): + v, n = tangled_pr.classify({1}, {1, 2, 3}, {2: True, 3: True}) + self.assertEqual((v, n), ("created", 3)) + + def test_a_pull_that_vanished_does_not_confuse_it(self): + # after ⊂ before (someone closed+deleted one mid-run): no new pull, so no create + v, n = tangled_pr.classify({1, 2}, {1}, {}) + self.assertEqual((v, n), ("none", None)) + + +class TestExistingPullScan(unittest.TestCase): + """The pre-check that makes a retry harmless instead of duplicating.""" + + def _fetcher(self, pages): + def _f(url, cookie, timeout=30): + return pages.get(url.rsplit("/", 1)[-1], "") + return _f + + def test_finds_an_existing_pull_for_the_branch(self): + f = self._fetcher({"396": "branch: feat/x", "395": "branch: other"}) + got = tangled_pr.existing_pull_for("o", "r", "feat/x", "c", {395, 396}, _fetch=f) + self.assertEqual(got, 396) + + def test_returns_none_when_the_branch_is_new(self): + f = self._fetcher({"396": "other", "395": "other"}) + self.assertIsNone(tangled_pr.existing_pull_for("o", "r", "feat/x", "c", {395, 396}, _fetch=f)) + + def test_prefers_the_newest_match(self): + f = self._fetcher({"10": "feat/x", "20": "feat/x"}) + self.assertEqual(tangled_pr.existing_pull_for("o", "r", "feat/x", "c", {10, 20}, _fetch=f), 20) + + def test_scan_window_is_bounded(self): + # a match older than the window is missed BY DESIGN; the post-check is the backstop. + pages = {str(n): ("feat/x" if n == 1 else "other") for n in range(1, 30)} + f = self._fetcher(pages) + self.assertIsNone( + tangled_pr.existing_pull_for("o", "r", "feat/x", "c", set(range(1, 30)), scan=5, _fetch=f)) + + def test_unreadable_pages_do_not_claim_absence(self): + # every fetch fails -> None, and main() warns rather than silently creating a duplicate + f = lambda url, cookie, timeout=30: "" + self.assertIsNone(tangled_pr.existing_pull_for("o", "r", "feat/x", "c", {1, 2}, _fetch=f)) + + +class TestFetchIsFailSoft(unittest.TestCase): + def test_network_error_returns_empty_not_raise(self): + # a create must not die between POST and verification + self.assertEqual(tangled_pr.fetch("http://127.0.0.1:1/nope", "c", timeout=1), "") + + +class TestCloseToolStateParsing(unittest.TestCase): + """tangled_pr_close.state_of decides whether a pull is already closed.""" + + def setUp(self): + import tangled_pr_close + self.mod = tangled_pr_close + + def test_badge_regex_reads_the_states(self): + import re + badges = lambda doc: set(re.findall(r'>\s*(Merged|Closed|Open)\s*<', doc)) + self.assertEqual(badges(" Closed "), {"Closed"}) + self.assertEqual(badges("OpenMerged"), {"Open", "Merged"}) + self.assertEqual(badges("

closed

"), set()) # case-sensitive on purpose + + +class TestGatewayBackendValidation(unittest.TestCase): + """gateway-domain refuses hostname backends: the gateway can store one and never remove it.""" + + def setUp(self): + import importlib.util + p = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + "tools", "gateway-domain.py") + if not os.path.exists(p): + self.skipTest("gateway-domain.py not present") + spec = importlib.util.spec_from_file_location("gwd", p) + self.gwd = importlib.util.module_from_spec(spec) + spec.loader.exec_module(self.gwd) + + def test_accepts_ip_and_ip_port(self): + self.assertTrue(self.gwd._BACKEND.match("100.74.89.63")) + self.assertTrue(self.gwd._BACKEND.match("100.74.89.63:8443")) + + def test_rejects_hostnames(self): + for bad in ("example.com", "host:443", "", "1.2.3", "1.2.3.4.5"): + self.assertIsNone(self.gwd._BACKEND.match(bad), f"should reject {bad!r}") + + def test_allowed_tags_include_both_known_tags(self): + self.assertIn("tag:notplants-test-server", self.gwd.ALLOWED_TAGS) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_unit.py b/tests/test_unit.py index 1f340e8..c0e10c0 100755 --- a/tests/test_unit.py +++ b/tests/test_unit.py @@ -568,9 +568,17 @@ class TestProcDescendants(unittest.TestCase): kids = agents._proc_descendants([str(p.pid)]) self.assertNotIn(str(p.pid), kids) # root excluded self.assertGreaterEqual(len(kids), 2) # the two sleeps - comms = subprocess.run("ps -o comm= -p " + ",".join(sorted(kids)), - shell=True, capture_output=True, text=True).stdout - self.assertIn("sleep", comms) + # Read /proc//comm rather than shelling to `ps -o comm=`: this host's ps + # returns nothing for that form, and an empty result from a tool that is absent or + # unsupported is indistinguishable from "the children are not sleeps". Same class of + # bug as the pgrep dependency this test just caught in _proc_descendants. + comms = [] + for k in sorted(kids): + try: + comms.append(open(f"/proc/{k}/comm").read().strip()) + except OSError: + pass + self.assertIn("sleep", comms, f"expected a sleep among {comms}") finally: os.killpg(os.getpgid(p.pid), signal.SIGKILL); p.wait() @@ -594,26 +602,38 @@ class TestBuildRunning(unittest.TestCase): self.ps_targets = "" def tearDown(self): + # Restore EXPLICITLY. A cleverer derivation of these names silently matched nothing, so the + # monkeypatch leaked into the next test class and made an unrelated test fail — visible only + # in a full run, never when that test was run alone. agents.subprocess.run = self._orig_run shutil.rmtree(self.tmp, ignore_errors=True) def _patch(self, descendant_comms): - """Fake tmux/pgrep/ps: pane_pid=1000; its children are 1001,1002 with the given comms.""" + """pane_pid=1000; its children are 1001,1002 with the given comms. + + Patches the two internal seams (_proc_descendants, _comms) rather than faking + `pgrep`/`ps` subprocess calls. WHY (2026-08-21): the previous version mocked those two + commands, so the suite passed on hosts where NEITHER IS INSTALLED — the fake stood in for + the broken dependency and the real bug (empty output read as "no build running", which + lets the watchdog reboot an agent mid-build) was invisible to every test. Mock the seam + you own, not the tool you depend on, or the test proves the mock works. + """ outer = self class R: def __init__(self, out): self.stdout = out; self.returncode = 0 def fake_run(cmd, *a, **k): - if "list-panes" in cmd: - return R("1000\n") - if "pgrep -P 1000" in cmd: - return R("1001\n1002\n") - if "pgrep -P" in cmd: - return R("") # 1001/1002 are leaves - if cmd.startswith("ps -o comm="): - outer.ps_targets = cmd.split("-p", 1)[1].strip() - return R("\n".join(descendant_comms) + "\n") - return R("") + return R("1000\n") if "list-panes" in cmd else R("") agents.subprocess.run = fake_run + orig_desc, orig_comms = agents._proc_descendants, agents._comms + def _restore(): + agents._proc_descendants, agents._comms = orig_desc, orig_comms + self.addCleanup(_restore) # runs even if the test errors; no tearDown ordering to get wrong + agents._proc_descendants = lambda roots: ( + {"1001", "1002"} if "1000" in list(roots) else set()) + def fake_comms(pids): + outer.ps_targets = ",".join(sorted(pids)) + return list(descendant_comms) + agents._comms = fake_comms def test_detects_running_build(self): self._patch(["bash", "cargo"])