tangled_pr: verify against the pulls list, not a response header; and a tools test suite
THE BUG THAT PROMPTED THIS. tangled_pr.py judged success ONLY by an HX-Redirect header on the POST. A create that SUCCEEDED but answered without that header read as a failure, so the caller retried and Tangled grew duplicates — that is exactly how #397, #398 and #399 were filed for one branch. A response header describes what the server meant to say; it is not the artifact. Now it checks the artifact, in both directions: * BEFORE posting, refuse if an open pull already exists for this source branch, naming it. A retry cannot duplicate, whatever the response said. (--allow-duplicate to override.) * AFTER posting, confirm against the pulls list: a new pull number that did not exist before, whose page names this source branch, IS the success — with or without a redirect header. * Failure is reported only when no such pull appeared. A false failure is worse than a loud error here, because the caller's remedy is to retry. Verified live: a dry-run against a branch that already has a pull refuses with rc=3, naming #417. TWO REAL DEFECTS FOUND BY WRITING THE TESTS. agents.py shelled out to `pgrep -P` and `ps -o comm=`. Neither is on the agent PATH on this host, and a missing binary under shell=True returns rc=127 with EMPTY stdout — indistinguishable from "this process has no children" and "no build is running". So _build_running was ALWAYS False and the stall detector could reboot an agent mid-build. Both now read /proc directly: no PATH dependency, and it cannot fail silently in that direction. That shipped because the unit tests MOCKED pgrep and ps. The fakes stood in for the broken dependency, so the suite passed on a host where neither tool was reachable and never exercised the real path. The tests now patch _proc_descendants and _comms — the seams this repo owns. A test that mocks a dependency proves the mock works. Also fixed a monkeypatch leak those tests had: restoration used a name derivation that silently matched nothing, so the patch escaped into another test class and failed an unrelated test — only in a full run, never when that test ran alone. Now addCleanup, which cannot be ordered wrong. NEW: tests/test_tools.py, 24 tests over tangled_pr, tangled_pr_close and gateway-domain, with every HTTP boundary injected so they run offline. Mutation-checked: breaking classify(), the pull-number regex, the branch match, or the scan bound each turns the suite red. Suite is 93 tests, green, and order-stable across repeated runs. README: a "PATH on a NixOS host" section. Every one of ps, pgrep, free, cmp, awk, curl, diff, strings, nm, getent and ping is INSTALLED here and simply not on the agent PATH, so each reports "command not found" and reads as a missing package. Documents how to check before concluding a tool is absent, how to add the system profile, `nix shell` for what is genuinely missing, and the rule that harness code should not shell out for what the kernel already exposes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3LdmEL7CvCYTNpoBq1kce
This commit is contained in:
@@ -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/<pid>/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."""
|
||||
|
||||
Reference in New Issue
Block a user