HOMELAB-2634: feat(herdr): clean up background sessions, count sessions, notice only failures #94

Merged

pi-landing merged 4 commits from plane/HOMELAB-2634-janitor-background into main 2026-11-04 21:36:54 +00:00

Conversation 6 Commits 4 Files changed 4 +2003 -22

aaron commented 2026-11-04 19:14:47 +00:00

Owner

Copy link

HOMELAB-2634 — the cleaner learns background sessions, session counts and notices

Follow-up to #93 (merged 945d97c), which armed the herdr session janitor. Aaron, 2026-11-04 18:37Z: "make it so it can keep cleaning up our space. we need to be clean" and "this way the operators (jesus and me) only interact with active supervisors".

1. Background Claude sessions with no pane

The janitor only knew herdr panes. There are also claude --background sessions: processes that outlive their pane and stay alive with no tab and no name. They are now collected from claude agents --json and reported:

BACKGROUND id=9e011d8b name=… state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker

A background session may be stopped only when all of these hold: a retire marker exists for it (retire/<name> or retire/<id>); kind == background; the process is alive; status != busy; state == done; no claude attach <id> client; the name is not protected (ld-hypervisor/el-director, case-folded); and its cwd is under repos/homelab or repos/pi-extensions. Anything else is report-only.

The write is claude stop <id> — its conversation is kept and it is resumable. This code never calls claude rm, never claude daemon stop, never kill, and never disturbs an attach client. Liveness and the attach check read /proc; no signal is ever sent to a foreign process. The allowlist permits exactly agents --json and stop <id>; rm, daemon stop and stop --all are refused before a subprocess exists.

2. One-line session count

Before the per-pane detail:

SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0

An unreadable source increments unreachable — it is never silently 0.

3. A notice only when something could not be closed

NOTICE: is emitted only for a failed pane close, a SKIP_CHANGED revalidation, a marker whose close was refused, a failed claude stop, or an unreadable source. When everything either closed cleanly or was refused by rule, there is no NOTICE: at all. An ordinary REPORT row is the normal state and stays silent.

4. Herdr sessions that do not fit the session-rules table

Per working-files/ldv0/analisis-macro/change-package/session-rules.md, the only herdr sessions that should exist are El Director / project supervisors / Jesús's Director, and epic orchestrators. A named herdr session that fits neither is reported:

RULE name=<name> workspace=<w> why=<not-a-director-or-an-orchestrator>

A juez-/judge- named session is specifically reported as why=judge-should-be-a-subagent. Report only — it never closes one.

Safety contract unchanged

Dry-run by default; real actions need --apply and the non-symlink activation file containing HOMELAB-2634; every close re-decided from fresh state immediately before the destructive call; never wS/wH; never a focused, unnamed or protected pane; the 600-second stillness floor still applies to both the successor and the retire-marker paths.

Evidence

89 → 117 tests, 28 new (tests/test_herdr_janitor_sessions.py, fake claude on PATH and a fake /proc), full suite green, py_compile clean. Live dry-run against the real fleet: SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0, zero CLOSE_CANDIDATE, no NOTICE:.

Not proven

No background session has been stopped for real — proved on fakes only. Two caveats are recorded in docs/herdr-janitor.md: /proc liveness can in theory be fooled by pid reuse (the code then reports STOP_FAILED and does not retry), and the RULE line is a name-grammar heuristic that reports rather than acts.

## HOMELAB-2634 — the cleaner learns background sessions, session counts and notices Follow-up to #93 (merged `945d97c`), which armed the herdr session janitor. Aaron, 2026-11-04 18:37Z: *"make it so it can keep cleaning up our space. we need to be clean"* and *"this way the operators (jesus and me) only interact with active supervisors"*. ### 1. Background Claude sessions with no pane The janitor only knew herdr panes. There are also `claude --background` sessions: processes that outlive their pane and stay alive with no tab and no name. They are now collected from `claude agents --json` and reported: ``` BACKGROUND id=9e011d8b name=… state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker ``` A background session may be stopped **only** when all of these hold: a retire marker exists for it (`retire/<name>` or `retire/<id>`); `kind == background`; the process is alive; `status != busy`; `state == done`; **no `claude attach <id>` client**; the name is not protected (`ld-hypervisor`/`el-director`, case-folded); and its cwd is under `repos/homelab` or `repos/pi-extensions`. Anything else is report-only. The write is **`claude stop <id>`** — its conversation is kept and it is resumable. This code never calls `claude rm`, never `claude daemon stop`, never `kill`, and never disturbs an attach client. Liveness and the attach check read `/proc`; no signal is ever sent to a foreign process. The allowlist permits exactly `agents --json` and `stop <id>`; `rm`, `daemon stop` and `stop --all` are refused before a subprocess exists. ### 2. One-line session count Before the per-pane detail: ``` SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0 ``` An unreadable source increments `unreachable` — it is never silently 0. ### 3. A notice only when something could not be closed `NOTICE:` is emitted only for a failed `pane close`, a `SKIP_CHANGED` revalidation, a marker whose close was refused, a failed `claude stop`, or an unreadable source. When everything either closed cleanly or was refused by rule, there is no `NOTICE:` at all. An ordinary REPORT row is the normal state and stays silent. ### 4. Herdr sessions that do not fit the session-rules table Per `working-files/ldv0/analisis-macro/change-package/session-rules.md`, the only herdr sessions that should exist are El Director / project supervisors / Jesús's Director, and epic orchestrators. A named herdr session that fits neither is reported: ``` RULE name=<name> workspace=<w> why=<not-a-director-or-an-orchestrator> ``` A `juez-`/`judge-` named session is specifically reported as `why=judge-should-be-a-subagent`. **Report only** — it never closes one. ### Safety contract unchanged Dry-run by default; real actions need `--apply` **and** the non-symlink activation file containing `HOMELAB-2634`; every close re-decided from fresh state immediately before the destructive call; never `wS`/`wH`; never a focused, unnamed or protected pane; the 600-second stillness floor still applies to both the successor and the retire-marker paths. ### Evidence 89 → **117 tests**, 28 new (`tests/test_herdr_janitor_sessions.py`, fake `claude` on PATH and a fake `/proc`), full suite green, `py_compile` clean. Live dry-run against the real fleet: `SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0`, zero `CLOSE_CANDIDATE`, no `NOTICE:`. ### Not proven **No background session has been stopped for real** — proved on fakes only. Two caveats are recorded in `docs/herdr-janitor.md`: `/proc` liveness can in theory be fooled by pid reuse (the code then reports `STOP_FAILED` and does not retry), and the `RULE` line is a name-grammar heuristic that reports rather than acts.

aaron added 1 commit 2026-11-04 19:14:47 +00:00

HOMELAB-2634: report and retire background Claude sessions, census the fleet, notice only failures ... 507a985c99

The janitor only knew herdr panes, so a claude --background session that
outlived its pane was invisible. It is now a first-class row.

- BACKGROUND rows from `claude agents --json`; the only write is
  `claude stop <id>` (conversation kept, resumable). `claude rm`,
  `claude daemon stop`, `kill`, and every attach client are refused by an
  argv allowlist, not by intention. Attach clients and liveness are read from
  /proc, so noticing one cannot disturb it.
- A stop needs ALL of: retire marker by name or id, kind=background, live pid,
  status!=busy, state=done, no attach client, unprotected name (folded), cwd
  under homelab/ or pi-extensions/. Any doubt is report-only with its reason.
  Dead pid reports process=gone. Revalidated before the single write.
- SESSIONS census line before per-pane detail; an unreadable source counts as
  unreachable, never as a silent 0.
- NOTICE lines only when the janitor could not do what it intended: failed
  close, revalidation refusal, failed stop, unreadable source. Rule refusals
  and ordinary REPORT rows stay silent.
- RULE rows for named herdr sessions that session-rules.md says should not
  exist; `juez-*`/`judge-*` report why=judge-should-be-a-subagent. Report
  only, never closed.

tests/test_herdr_janitor_sessions.py drives a fake `claude` on PATH and a fake
/proc; the real binary is never invoked. 89 -> 117 tests, all green.

aaron added 1 commit 2026-11-04 20:20:44 +00:00

HOMELAB-2634: recognise analyst sessions as a named role e01532f3e2

ld-reviewer requested changes 2026-11-04 20:29:34 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: REQUEST_CHANGES

model: omniroute/deepseek-flash
head: e01532f3e2
session: homelab-2634-review-5

Review of plane/HOMELAB-2634-janitor-background at e01532f3e21bcb7d4adca90c49f9f4b34b73c556 (base origin/main = 945d97c786f87cfcfbab3fd1833997ec1678bb11). I reviewed the commit objects in my own detached worktree; I did not use a shared checkout. Session homelab-2634-review-5, model omniroute/deepseek-flash.

What I verified myself

Verified on this VM, with the real binaries. I ran the janitor --dry-run into an isolated mktemp state dir against the live herdr and the live claude. It printed SESSIONS herdr=10 named=6 background=2 background-alive=1 unreachable=0, ten REPORT rows, zero CLOSE_CANDIDATE/WOULD_CLOSE, zero NOTICE: and zero RULE lines. I then re-ran it with a logging wrapper installed as --claude; the only argv the dry-run ever issued was agents --json, and stop was invoked zero times, which is the read-only claim demonstrated rather than asserted.

Tests. python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py' — 120 tests, OK, 0 failures, 0 errors.

CI on the exact head. Runs #266 and #267 are both [pass] at e01532f3e2; the python job is green on both, and the full extension suite is green on #266.

The 600-second floor is untouched. This is the cleanest result of the review: git diff 945d97c e01532f -- scripts/herdr_janitor/janitor.py removes no line from decide, screen_stable, successor_age or min_successor_seconds, and touches none of them at all. The only removed behaviour-bearing lines are the old two-member PROTECTED_NAMES literal and safe_markers being superseded by markers_with_state (a strict superset that additionally reports an unreadable directory). screen_stable is still called on both herdr branches, still requires now - hash_since >= config.min_successor_seconds, the default is still 600, and --min-successor-seconds 599 still refuses with exit 2. Nothing #93 fixed was loosened.

The argv allowlist cannot be widened. claude_allowed_argv admits exactly ["agents","--json"] and ["stop", <one token>]. I brute-forced every argv of length 1–3 over the token set stop rm daemon all agents attach --json -a --all x "" and no combination escaped. rm, daemon stop, stop --all, stop --daemon, extra arguments, any leading -, /, .., a/b, empty and whitespace ids are all refused. ClaudeCLI._run re-checks the allowlist so the subprocess cannot be reached by another path, and it refused rm x, daemon stop and stop --all when I called it directly. There is no os.kill, no signal, no terminate and no shell in the file; the only subprocess.run calls are the two allowlisted ones.

/proc access is genuinely read-only. Processes uses only is_dir(), iterdir() and read_bytes() on /proc/<pid>/cmdline. Nothing is signalled, so noticing an attach client cannot disturb it.

Every "never stop" condition refuses, and names itself. I broke each one at a time against the real decision function: status=busy → status-busy; state=running → state-running; missing state → state-unknown; attach by id and attach by full name → attach-client-present; no marker → no-retire-marker; a marker naming a different session → no-retire-marker; analista-macro, Analista-Macro, ld-hypervisor, El-Director → protected-name; cwd=/home/aaron/repos/other, empty cwd and the near-miss /home/aaron/repos/homelabX → cwd-not-allowed; pid=None and a non-existent pid → process-gone; kind=interactive with a marker → kind-interactive; kind=interactive without a marker is not even reported. A cwd genuinely under either root (/home/aaron/repos/homelab/sub/dir) is allowed. Marker matching by id and by name both work. MARKER_NAME_RE refuses .., ../../etc/passwd, /etc/passwd, a/b and a leading -, so the unlink cannot escape the retire dir, and .. cannot even be created as a marker.

The marker is deleted only after a successful stop. _execute_background sets STOP_FAILED and continues before the unlink; the marker survives a failed stop and test_failed_stop_keeps_the_marker_and_notices proves it. A delete that fails after a successful stop becomes STOPPED_MARKER_DELETE_FAILED plus a NOTICE:.

unreachable is never a silent zero. I forced each source to fail: claude raising OSError → 1; claude returning bad JSON → 1; claude not wired in at all → 1; retire dir chmod 000 → 1; unreadable roster → 1; one failed pane status read → 1. A missing retire dir correctly stays 0, because nobody has asked for a retirement. Notice emission matches: REPORT, STOP_CANDIDATE and STOPPED are silent, while STOP_FAILED, SKIP_CHANGED, STOPPED_MARKER_DELETE_FAILED and every unreadable source each emit exactly one NOTICE:.

The RULE change works as described. analista-* in any casing, Analista-Macro, ANALISTA-MACRO, analista-riesgo and a hypothetical analista-nuevo-x produce no RULE line. juez-macro, Juez-Outliers, judge-1, JUDGE, juez and judge.x produce why=judge-should-be-a-subagent. stray-session and scratch still produce not-a-director-or-orchestrator, so the silencing is scoped to the prefix and did not widen. analista-macro is in PROTECTED_NAMES and is protected on both the pane path (_base_protections) and the background path, in every casing I tried, including accent-folded Ánalista-Mácro; near-misses like analista-macro-2 and xanalista-macro are correctly not protected.

Findings

F1 — the attach-client guard fails open on two invocations that claude --help documents as normal. BLOCKING.

This is the one thing that must change before merge. The attach check is the only guard standing between the janitor and a human's live terminal, and it can be walked around without any exotic input. claude --help on this box lists -d, --debug, --debug-file and --verbose as top-level flags, and claude attach --help says verbatim: "Part of the session name works in place of the id." The janitor's /proc scan, however, only records an attach token when it is not argv[0] and the immediately preceding argv element contains claude, and it then compares the collected string for exact equality against session.agent_id and session.name.

I put the real background row from this VM — id=9e011d8b, name=Ghosttly-in-the-shell Android port of rootshell, kind=background, state=done, status=idle, live pid, cwd /home/aaron/repos/homelab — through the real decide_background with a matching marker and these cmdlines:

• claude attach 9e011d8b → detected → REPORT, attach-client-present. Correct.
• claude attach 'Ghosttly-in-the-shell Android port of rootshell' → detected, and correctly refused only because claude's own attach path wants the name quoted as one argv, which is what I tested. Correct.
• claude attach ghosttly → the scan collects ghosttly; ghosttly != 9e011d8b and ghosttly != "Ghosttly-in-the-shell Android port of rootshell" → STOP_CANDIDATE. A person watching that session in their terminal, having typed the name prefix claude's help tells them to type, is stopped.
• claude attach 'Ghosttly-in-the-shell' → same, STOP_CANDIDATE.
• claude --debug attach 9e011d8b → the scan skips the attach token because argv[i-1] is --debug, not a claude element → empty attached set → STOP_CANDIDATE.
• claude --verbose attach 9e011d8b → STOP_CANDIDATE.
• sh -c 'claude attach 9e011d8b' → no argv element equals attach → STOP_CANDIDATE.
• node /opt/claude/bin/claude.js attach 9e011d8b → detected. Correct.

So five of the eight spellings I tried stop a session a human is attached to, and two of those five are invocations claude itself advertises. Nothing else in the pipeline catches this: claude attach keeps the session running "either way", so state stays done and status stays idle, and every other guard passes. The docs/herdr-janitor.md sentence added by this PR — "no attach client for that id (claude attach <id> in any cmdline)" — is therefore false in exactly the cases that matter, and it is the sentence a future reader will rely on. The stop is resumable so nothing is destroyed, but this is a safety claim the change makes and does not honour; it is cheap to fix (match the id as a prefix, match the name by substring/prefix, and treat any attach occurrence in a cmdline that also mentions claude as an attach client) and should be fixed here rather than in a follow-up.

F2 — status is compared for exact equality, so the busy guard fails open on any unrecognised value. Medium, not blocking. session.status == "busy" with no folding is asymmetric with state != "done", which fails closed. Busy, active, waiting, working, or an absent status all pass the busy check. On this VM the real wire format is lowercase idle/busy, and a background row can omit status entirely — the live state: blocked row has no status key — so today there is no exposure, and the state == "done" requirement covers it. But the guard is fail-open on a vocabulary nothing pins down, and the next status name claude invents silently widens the stop. The same treatment state gets (compare against a known-safe set) would make it fail closed like its neighbour.

F3 — an unreadable /proc makes the janitor more willing, not less, and is not counted. Low, not blocking. Processes.attached_ids() returns frozenset() on OSError from iterdir() and skips any /proc/<pid>/cmdline it cannot read. An empty attach set means "stop everything", so this is the one read failure in the file that errs permissive, and it is not included in unreachable — every other unreadable source is. On a single-user box with a readable /proc this is theoretical; it becomes real the day an attach client runs as another user whose cmdline is unreadable.

F4 — a failed background stop does not affect the exit code. Low, not blocking. main() returns 1 only when a pane close returned CLOSE_FAILED. A STOP_FAILED or STOPPED_MARKER_DELETE_FAILED on the background path leaves exit 0 — confirmed by my live run, which exited 0 with no notices. The NOTICE: line is there for a human reading report.txt, but anything keying off exit status (cron, a monitor, a caller) will not see a failed background stop.

F5 — nit. rule_violation silences any folded name starting with analista-, including the degenerate analista- itself.

What this does not prove

No background session has ever been stopped by this code for real, and this review does not change that. The live evidence is read-only by construction: the dry-run issued agents --json and nothing else, and zero stop argv. The stop path is proved on fakes — a fake claude installed on PATH in the tests, and my direct calls through decide_background and ClaudeCLI._run — so what is established is that the janitor's decision and its argv surface are correct, not that claude stop <id> behaves as documented, that it is idempotent, what it returns for an unknown or ambiguous id, or that a real stopped session is genuinely resumable afterwards. The docs say this plainly and that honesty is to their credit.

Three further limits: liveness is a /proc/<pid> directory test, so a recycled pid can make a dead entry look alive (the stop then fails and is reported, which is the safe direction, and this is documented); the background path has no continuous-observation floor at all, so a session that finished its turn one second ago is stoppable if a marker exists — defensible, since there is no screen to be unstable and the stop is resumable, but it is a weaker gate than the pane path and nothing says so; and normalise_cell folds case and accents but not separators, so EL DIRECTOR with a space is not in PROTECTED_NAME_KEYS. That last one is pre-existing — the function is untouched by this PR — and on the pane path the name is still not relievable, so it fails closed there; on the background path a space-spelled protected name would rely on the cwd and marker guards alone.

Verdict

REQUEST_CHANGES, on F1 alone. Everything else in this change is careful, narrowly scoped, and honest about its own limits: the allowlist is proven rather than asserted, the 600-second floor is untouched to the character, the census cannot lie about a failed source, notices are exactly as silent as claimed, and the analista-* change is scoped so it does not swallow the RULE report. But this PR's headline promise is that it will not disturb an attach client, and an ordinary claude --debug attach <id> or claude attach <prefix> disturbs one. Fix the detection (or narrow the documentation to the one spelling it actually catches, which I would not accept for a stop-authority guard), add the four spellings above as failing tests, and this is a straightforward re-approve.

## Verdict: REQUEST_CHANGES model: omniroute/deepseek-flash head: e01532f3e21bcb7d4adca90c49f9f4b34b73c556 session: homelab-2634-review-5 Review of `plane/HOMELAB-2634-janitor-background` at `e01532f3e21bcb7d4adca90c49f9f4b34b73c556` (base `origin/main` = `945d97c786f87cfcfbab3fd1833997ec1678bb11`). I reviewed the commit objects in my own detached worktree; I did not use a shared checkout. Session `homelab-2634-review-5`, model `omniroute/deepseek-flash`. ## What I verified myself **Verified on this VM, with the real binaries.** I ran the janitor `--dry-run` into an isolated `mktemp` state dir against the live herdr and the live `claude`. It printed `SESSIONS herdr=10 named=6 background=2 background-alive=1 unreachable=0`, ten `REPORT` rows, zero `CLOSE_CANDIDATE`/`WOULD_CLOSE`, zero `NOTICE:` and zero `RULE` lines. I then re-ran it with a logging wrapper installed as `--claude`; the only argv the dry-run ever issued was `agents --json`, and `stop` was invoked zero times, which is the read-only claim demonstrated rather than asserted. **Tests.** `python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py'` — 120 tests, OK, 0 failures, 0 errors. **CI on the exact head.** Runs #266 and #267 are both `[pass]` at `e01532f3e2`; the `python` job is green on both, and the full extension suite is green on #266. **The 600-second floor is untouched.** This is the cleanest result of the review: `git diff 945d97c e01532f -- scripts/herdr_janitor/janitor.py` removes no line from `decide`, `screen_stable`, `successor_age` or `min_successor_seconds`, and touches none of them at all. The only removed behaviour-bearing lines are the old two-member `PROTECTED_NAMES` literal and `safe_markers` being superseded by `markers_with_state` (a strict superset that additionally reports an unreadable directory). `screen_stable` is still called on both herdr branches, still requires `now - hash_since >= config.min_successor_seconds`, the default is still 600, and `--min-successor-seconds 599` still refuses with exit 2. Nothing #93 fixed was loosened. **The argv allowlist cannot be widened.** `claude_allowed_argv` admits exactly `["agents","--json"]` and `["stop", <one token>]`. I brute-forced every argv of length 1–3 over the token set `stop rm daemon all agents attach --json -a --all x ""` and no combination escaped. `rm`, `daemon stop`, `stop --all`, `stop --daemon`, extra arguments, any leading `-`, `/`, `..`, `a/b`, empty and whitespace ids are all refused. `ClaudeCLI._run` re-checks the allowlist so the subprocess cannot be reached by another path, and it refused `rm x`, `daemon stop` and `stop --all` when I called it directly. There is no `os.kill`, no `signal`, no `terminate` and no shell in the file; the only `subprocess.run` calls are the two allowlisted ones. **`/proc` access is genuinely read-only.** `Processes` uses only `is_dir()`, `iterdir()` and `read_bytes()` on `/proc/<pid>/cmdline`. Nothing is signalled, so noticing an attach client cannot disturb it. **Every "never stop" condition refuses, and names itself.** I broke each one at a time against the real decision function: `status=busy` → `status-busy`; `state=running` → `state-running`; missing `state` → `state-unknown`; attach by id and attach by full name → `attach-client-present`; no marker → `no-retire-marker`; a marker naming a different session → `no-retire-marker`; `analista-macro`, `Analista-Macro`, `ld-hypervisor`, `El-Director` → `protected-name`; `cwd=/home/aaron/repos/other`, empty cwd and the near-miss `/home/aaron/repos/homelabX` → `cwd-not-allowed`; `pid=None` and a non-existent pid → `process-gone`; `kind=interactive` with a marker → `kind-interactive`; `kind=interactive` without a marker is not even reported. A cwd genuinely under either root (`/home/aaron/repos/homelab/sub/dir`) is allowed. Marker matching by id and by name both work. `MARKER_NAME_RE` refuses `..`, `../../etc/passwd`, `/etc/passwd`, `a/b` and a leading `-`, so the unlink cannot escape the retire dir, and `..` cannot even be created as a marker. **The marker is deleted only after a successful stop.** `_execute_background` sets `STOP_FAILED` and `continue`s before the `unlink`; the marker survives a failed stop and `test_failed_stop_keeps_the_marker_and_notices` proves it. A delete that fails after a successful stop becomes `STOPPED_MARKER_DELETE_FAILED` plus a `NOTICE:`. **`unreachable` is never a silent zero.** I forced each source to fail: `claude` raising `OSError` → 1; `claude` returning bad JSON → 1; `claude` not wired in at all → 1; retire dir `chmod 000` → 1; unreadable roster → 1; one failed pane status read → 1. A *missing* retire dir correctly stays 0, because nobody has asked for a retirement. Notice emission matches: `REPORT`, `STOP_CANDIDATE` and `STOPPED` are silent, while `STOP_FAILED`, `SKIP_CHANGED`, `STOPPED_MARKER_DELETE_FAILED` and every unreadable source each emit exactly one `NOTICE:`. **The `RULE` change works as described.** `analista-*` in any casing, `Analista-Macro`, `ANALISTA-MACRO`, `analista-riesgo` and a hypothetical `analista-nuevo-x` produce no `RULE` line. `juez-macro`, `Juez-Outliers`, `judge-1`, `JUDGE`, `juez` and `judge.x` produce `why=judge-should-be-a-subagent`. `stray-session` and `scratch` still produce `not-a-director-or-orchestrator`, so the silencing is scoped to the prefix and did not widen. `analista-macro` is in `PROTECTED_NAMES` and is protected on both the pane path (`_base_protections`) and the background path, in every casing I tried, including accent-folded `Ánalista-Mácro`; near-misses like `analista-macro-2` and `xanalista-macro` are correctly not protected. ## Findings **F1 — the attach-client guard fails open on two invocations that `claude --help` documents as normal. BLOCKING.** This is the one thing that must change before merge. The attach check is the *only* guard standing between the janitor and a human's live terminal, and it can be walked around without any exotic input. `claude --help` on this box lists `-d, --debug`, `--debug-file` and `--verbose` as top-level flags, and `claude attach --help` says verbatim: "Part of the session name works in place of the id." The janitor's `/proc` scan, however, only records an `attach` token when it is not argv[0] and the immediately preceding argv element contains `claude`, and it then compares the collected string for exact equality against `session.agent_id` and `session.name`. I put the real background row from this VM — `id=9e011d8b`, `name=Ghosttly-in-the-shell Android port of rootshell`, `kind=background`, `state=done`, `status=idle`, live pid, cwd `/home/aaron/repos/homelab` — through the real `decide_background` with a matching marker and these `cmdline`s: - `claude attach 9e011d8b` → detected → `REPORT`, `attach-client-present`. Correct. - `claude attach 'Ghosttly-in-the-shell Android port of rootshell'` → detected, and correctly refused only because claude's own `attach` path wants the name *quoted as one argv*, which is what I tested. Correct. - `claude attach ghosttly` → the scan collects `ghosttly`; `ghosttly != 9e011d8b` and `ghosttly != "Ghosttly-in-the-shell Android port of rootshell"` → **`STOP_CANDIDATE`**. A person watching that session in their terminal, having typed the name prefix claude's help tells them to type, is stopped. - `claude attach 'Ghosttly-in-the-shell'` → same, **`STOP_CANDIDATE`**. - `claude --debug attach 9e011d8b` → the scan skips the `attach` token because argv[i-1] is `--debug`, not a `claude` element → empty attached set → **`STOP_CANDIDATE`**. - `claude --verbose attach 9e011d8b` → **`STOP_CANDIDATE`**. - `sh -c 'claude attach 9e011d8b'` → no argv element equals `attach` → **`STOP_CANDIDATE`**. - `node /opt/claude/bin/claude.js attach 9e011d8b` → detected. Correct. So five of the eight spellings I tried stop a session a human is attached to, and two of those five are invocations claude itself advertises. Nothing else in the pipeline catches this: `claude attach` keeps the session running "either way", so `state` stays `done` and `status` stays `idle`, and every other guard passes. The `docs/herdr-janitor.md` sentence added by this PR — "no attach client for that id (`claude attach <id>` in any `cmdline`)" — is therefore false in exactly the cases that matter, and it is the sentence a future reader will rely on. The stop is resumable so nothing is destroyed, but this is a safety claim the change makes and does not honour; it is cheap to fix (match the id as a prefix, match the name by substring/prefix, and treat any `attach` occurrence in a `cmdline` that also mentions `claude` as an attach client) and should be fixed here rather than in a follow-up. **F2 — `status` is compared for exact equality, so the busy guard fails open on any unrecognised value. Medium, not blocking.** `session.status == "busy"` with no folding is asymmetric with `state != "done"`, which fails closed. `Busy`, `active`, `waiting`, `working`, or an absent `status` all pass the busy check. On this VM the real wire format is lowercase `idle`/`busy`, and a background row can omit `status` entirely — the live `state: blocked` row has no `status` key — so today there is no exposure, and the `state == "done"` requirement covers it. But the guard is fail-open on a vocabulary nothing pins down, and the next status name claude invents silently widens the stop. The same treatment `state` gets (compare against a known-safe set) would make it fail closed like its neighbour. **F3 — an unreadable `/proc` makes the janitor more willing, not less, and is not counted. Low, not blocking.** `Processes.attached_ids()` returns `frozenset()` on `OSError` from `iterdir()` and skips any `/proc/<pid>/cmdline` it cannot read. An empty attach set means "stop everything", so this is the one read failure in the file that errs permissive, and it is not included in `unreachable` — every other unreadable source is. On a single-user box with a readable `/proc` this is theoretical; it becomes real the day an attach client runs as another user whose `cmdline` is unreadable. **F4 — a failed background stop does not affect the exit code. Low, not blocking.** `main()` returns 1 only when a pane close returned `CLOSE_FAILED`. A `STOP_FAILED` or `STOPPED_MARKER_DELETE_FAILED` on the background path leaves exit 0 — confirmed by my live run, which exited 0 with no notices. The `NOTICE:` line is there for a human reading `report.txt`, but anything keying off exit status (cron, a monitor, a caller) will not see a failed background stop. **F5 — nit.** `rule_violation` silences any folded name starting with `analista-`, including the degenerate `analista-` itself. ## What this does not prove No background session has ever been stopped by this code for real, and this review does not change that. The live evidence is read-only by construction: the dry-run issued `agents --json` and nothing else, and zero `stop` argv. The stop path is proved on fakes — a fake `claude` installed on `PATH` in the tests, and my direct calls through `decide_background` and `ClaudeCLI._run` — so what is established is that the janitor's *decision* and its *argv surface* are correct, not that `claude stop <id>` behaves as documented, that it is idempotent, what it returns for an unknown or ambiguous id, or that a real stopped session is genuinely resumable afterwards. The docs say this plainly and that honesty is to their credit. Three further limits: liveness is a `/proc/<pid>` directory test, so a recycled pid can make a dead entry look alive (the stop then fails and is reported, which is the safe direction, and this is documented); the background path has no continuous-observation floor at all, so a session that finished its turn one second ago is stoppable if a marker exists — defensible, since there is no screen to be unstable and the stop is resumable, but it is a weaker gate than the pane path and nothing says so; and `normalise_cell` folds case and accents but not separators, so `EL DIRECTOR` with a space is not in `PROTECTED_NAME_KEYS`. That last one is pre-existing — the function is untouched by this PR — and on the pane path the name is still not relievable, so it fails closed there; on the background path a space-spelled protected name would rely on the cwd and marker guards alone. ## Verdict `REQUEST_CHANGES`, on F1 alone. Everything else in this change is careful, narrowly scoped, and honest about its own limits: the allowlist is proven rather than asserted, the 600-second floor is untouched to the character, the census cannot lie about a failed source, notices are exactly as silent as claimed, and the `analista-*` change is scoped so it does not swallow the `RULE` report. But this PR's headline promise is that it will not disturb an attach client, and an ordinary `claude --debug attach <id>` or `claude attach <prefix>` disturbs one. Fix the detection (or narrow the documentation to the one spelling it actually catches, which I would not accept for a stop-authority guard), add the four spellings above as failing tests, and this is a straightforward re-approve.

ld-reviewer approved these changes 2026-11-04 20:31:54 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: e01532f3e2
session: homelab-2634-review-6

I reviewed this as an independent seat on head e01532f3e21bcb7d4adca90c49f9f4b34b73c556 (PR #94, base origin/main = 945d97c), from my own worktree, not a shared one. The change adds a second kind of thing to the janitor: claude --background sessions, processes with no pane that outlive their tab, plus a fleet census line, a notice policy, and a RULE report for named herdr sessions that fit none of the allowed shapes. The bar I applied is the one that matters for this change: can anything here stop, kill or disturb a live session that should have been left alone, and can any write other than claude stop <id> reach the shell.

What I verified myself, not trusted from the PR text. I read the whole diff against origin/main, not against a local main (my first pass used a stale local branch and over-attributed existing code to this PR; the corrected diff touches only PROTECTED_NAMES, the new background/census/notice/RULE machinery, Snapshot, _execute's revalidation and render_report). The pane-close decision function, screen_stable, the 600-second floor and the marker branch are byte-identical between origin/main and this head — I diffed them and confirmed the PR does not edit decide() or decide_background's sibling screen_stable at all. _execute becomes strictly stricter: revalidation now also compares directory and marker. So the answer to "did this loosen the 600-second stillness floor on the herdr-pane paths" is a proven no: nothing in the pane close path moved, and the only new pane-path change is an extra equality check before herdr pane close.

Allowlist probes. I drove claude_allowed_argv directly over 24 argv shapes and confirmed agents --json and stop <id> are the only two that pass. rm <id>, rm --force <id>, daemon stop, stop --all, stop all, stop daemon, stop rm, stop attach, stop agents, stop <id> --force, stop -<id>, kill <id>, agents --json extra, agents, stop ../etc/passwd, stop a/b, stop "a b" and [] are all refused. I then replaced subprocess.run with a spy that raises on entry and called ClaudeCLI._run with the destructive forms: every one raised ValueError and the spy recorded zero invocations, so the refusal happens before any subprocess exists. claude stop ALL is the one shape that passes the shape check, because the deny list is case-sensitive while MARKER_NAME_RE allows uppercase; that is not an escalation (it would be an unknown session id and the command errors), but the deny check should fold case — informational, non-blocking.

Decision-function probes. I drove decide_background with constructed fixtures for every refusal the ticket asked about. Confirmed STOP_CANDIDATE only on the clean case (marker + kind=background + state=done + status=idle + live pid + allowed cwd + no attach). Each single deviation produced REPORT with the reason named and no stop: status=busy → status-busy, state=working/blocked/empty → state-*, attach client by id or by exact name → attach-client-present, no marker → no-retire-marker, El-Director/LD-Hypervisor/EL-DIRECTOR/Analista-Macro → protected-name, cwd /home/aaron/repos/intellimation or /opt/sendrast-director or / or empty → cwd-not-allowed, /home/aaron/repos/homelabX and /home/aaron/repos/homelab-other → cwd-not-allowed (the parent test is a real containment test, not a startswith), a dead pid, pid=None, pid=True, pid=0 and pid=-1 → process-gone. A kind=interactive row with a marker naming it is reported as kind-interactive and never stopped; without a marker it never enters the table at all. A path under an allowed root that is a symlink resolving outside is refused, because cwd_allowed resolves before comparing.

Liveness and attach detection are read-only. Processes.alive is a /proc/<pid> is_dir() test; attached_ids reads /proc/*/cmdline and decodes it. No os.kill, no signal, no write anywhere in the class. I confirmed against the live table: attached_ids() returned [] on the real fleet right now, and nothing in this path can disturb what it reads.

Marker lifetime. I ran the full janitor in apply mode with a fake claude on PATH and a fake /proc. With claude stop failing, the row became STOP_FAILED reasons=retire-marker|claude-stop-failed, the marker file was still there, and a NOTICE: claude stop failed id=9e011d8b name=epic-work was emitted. With claude stop succeeding, the single id was written to the stops log and the marker was removed. In dry-run nothing was stopped and the marker stayed. Revalidation refuses rather than closes when the marker set becomes unreadable, because the marker then reads as None and no longer equals the original.

Census and notices. SESSIONS counts an unreachable source as non-zero in every case I could construct: claude agents failing → unreachable=1, an unreadable retire directory → unreachable=1, a failed pane read → unreachable=1, and Janitor(claude=None) → unreachable=1 rather than a quiet zero. I did not find a path where a failed source silently reports unreachable=0.

Tests. I ran the suite myself: python3 -m unittest discover -s tests → 120 tests, OK (32 in test_herdr_janitor.py, 58 in test_herdr_janitor_relief.py, 30 in test_herdr_janitor_sessions.py). The PR body says 117; the real count on this head is 120. CI on the exact head is green: runs #266 and #267 for e01532f3e2 all pass.

Live read-only dry-run. Isolated state dir under mktemp -d, mode 0700, no --apply, real fleet:

SESSIONS herdr=10 named=6 background=2 background-alive=1 unreachable=0
BACKGROUND id=83e833d6 name=Intellimation SWOT and AI strategy state=blocked status=unknown process=gone cwd=/home/aaron/repos/intellimation action=REPORT reasons=cwd-not-allowed|process-gone|state-blocked|no-retire-marker
BACKGROUND id=9e011d8b name=Ghosttly-in-the-shell Android port of rootshell state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker
summary REPORT=10 total=10 directory_unreadable_refusals=0

Zero CLOSE_CANDIDATE, zero WOULD_STOP, zero NOTICE: lines. The live el-director pane came back reasons=protected-name|focused-pane and analista-macro came back protected-name|directory-current, which is the fail-closed behaviour the operator asked for. The intellimation session is exactly the case the cwd rule exists for and it is refused three ways over.

Plainly: no real background session has been stopped. Not by me, not by this code. All my stop-path evidence comes from a fake claude and a fake /proc. The live dry-run issued no stop. The two live background sessions are still alive after my review.

Findings, none of which I consider blocking.

The attach-guard is exact-match only — medium, non-blocking. claude attach --help on this machine says "Part of the session name works in place of the id". The detector collects the raw args after attach and compares them for equality against the session id or the whole name. I fed it a fake /proc with six shapes and it caught claude attach <id> via the symlink and via the real claude.exe path, and the full name; it did not catch claude attach Ghosttly (a documented partial name), claude attach 9e01 (an id prefix), or claude attach <id> with flags before attach. In those cases a human is attached and the guard is inert, so the session remains a STOP_CANDIDATE. The consequence is bounded: a stop still requires an operator retire marker for that exact id or name, and claude stop keeps the conversation resumable, so the worst case is a live terminal closing under a watching operator, not lost work. Not blocking, but the comment in Processes and the doc line "an attach client is something a person is looking at" overstate what the exact-match check proves, and matching on containment rather than equality would close it.

The protected-name list does not cover every Director spelling the code itself accepts — medium, non-blocking. DIRECTOR_NAMES includes el director, jesus-delivery, jesus-director and sendrast-director; PROTECTED_NAMES covers only ld-hypervisor, el-director and the newly added analista-macro. I confirmed el director (space) reaches STOP_CANDIDATE on the background path while El-Director is refused. It is only reachable through a by-<id> marker, since MARKER_NAME_RE forbids spaces so retire/el director cannot be created — which is why I am not blocking. But the two lists should be one set, because "the single source of truth for protected names" currently disagrees with the code's own definition of who a Director is, and session names on background sessions are free text.

The RULE analyst suppression is prefix-based and slightly broad — low, non-blocking. analista- as a bare prefix silences analista-, analista-ghost-2 and analista-macro junk copy. That matches the documented analista-<x> grammar and, importantly, this branch can only ever suppress a report line, never authorise or refuse a close, so the failure direction is a non-conforming session going unlisted rather than anything being touched. Fine to leave.

A failed pane read is counted but not noticed — low, non-blocking. With herdr=1 named=1 unreachable=1 and a failing agent read, the report produced no NOTICE: line, because the notice only fires when the pane actually reached a close branch. The doc's Notices section promises a notice when "a source could not be read". The census still tells the truth, so this is a documentation-versus-behaviour gap, not silence where it matters.

The roster is not consulted on the new stop path — informational. decide_background takes no Directory, so the operator's roster, which is otherwise a cross-check on every close, constrains nothing here. Given that the roster's aliases are role names and background session names are free text, it would rarely fire; worth knowing rather than fixing now.

The "not armed" framing is stale for this fleet — informational but worth saying out loud. docs/herdr-janitor.md opens with "The implementation PR does not install or enable the timer and does not authorize a real close", and the PR body says the same. That is already untrue here: herdr-janitor.timer is enabled and has been running --apply every 15 minutes since 19:05Z with a valid activation file, and the deployed copy at ~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py is byte-identical to origin/main. So the deployed code cannot stop a background session today, but the deploy step is routine — merging this and refreshing the deployed copy puts background stops live on a 15-minute schedule with no further decision. The retire marker stays the explicit operator act, which is why I still do not treat this as blocking; the doc sentence should be corrected so nobody reads "not armed" as a safety property of this change.

Two smaller notes. A duplicate id in claude agents --json yields two STOP_CANDIDATE rows and would issue two stops, but the per-row revalidation turns the second into SKIP_CHANGED because the session is already gone, so it is benign; parse_claude_sessions could still dedupe. And Config(directory=None) (library use only, not reachable from the CLI, which always resolves a path) yields a readable roster with no rows, so the roster gate passes silently; parse_directory(None) returning readable=True is the one shape I would tighten.

Verdict: APPROVE. The risk this change carries — stopping a live background session — is gated behind an explicit operator retire marker, and every doubt I could construct is report-only with the reason named. The write surface is exactly two argv, refused before a subprocess exists. The pane-close path, including the 600-second stillness floor, is untouched. My findings are about the completeness of two guards under conditions that each already require an explicit operator act, so I am not blocking on them; I would want the attach matcher widened and the director name sets merged before anyone relies on this in a hurry.

## Verdict: APPROVED model: omniroute/deepseek-flash head: e01532f3e21bcb7d4adca90c49f9f4b34b73c556 session: homelab-2634-review-6 I reviewed this as an independent seat on head `e01532f3e21bcb7d4adca90c49f9f4b34b73c556` (PR #94, base `origin/main` = `945d97c`), from my own worktree, not a shared one. The change adds a second kind of thing to the janitor: `claude --background` sessions, processes with no pane that outlive their tab, plus a fleet census line, a notice policy, and a `RULE` report for named herdr sessions that fit none of the allowed shapes. The bar I applied is the one that matters for this change: can anything here stop, kill or disturb a live session that should have been left alone, and can any write other than `claude stop <id>` reach the shell. **What I verified myself, not trusted from the PR text.** I read the whole diff against `origin/main`, not against a local `main` (my first pass used a stale local branch and over-attributed existing code to this PR; the corrected diff touches only `PROTECTED_NAMES`, the new background/census/notice/RULE machinery, `Snapshot`, `_execute`'s revalidation and `render_report`). The pane-close decision function, `screen_stable`, the 600-second floor and the marker branch are byte-identical between `origin/main` and this head — I diffed them and confirmed the PR does not edit `decide()` or `decide_background`'s sibling `screen_stable` at all. `_execute` becomes strictly stricter: revalidation now also compares `directory` and `marker`. So the answer to "did this loosen the 600-second stillness floor on the herdr-pane paths" is a proven no: nothing in the pane close path moved, and the only new pane-path change is an extra equality check before `herdr pane close`. **Allowlist probes.** I drove `claude_allowed_argv` directly over 24 argv shapes and confirmed `agents --json` and `stop <id>` are the only two that pass. `rm <id>`, `rm --force <id>`, `daemon stop`, `stop --all`, `stop all`, `stop daemon`, `stop rm`, `stop attach`, `stop agents`, `stop <id> --force`, `stop -<id>`, `kill <id>`, `agents --json extra`, `agents`, `stop ../etc/passwd`, `stop a/b`, `stop "a b"` and `[]` are all refused. I then replaced `subprocess.run` with a spy that raises on entry and called `ClaudeCLI._run` with the destructive forms: every one raised `ValueError` and the spy recorded **zero** invocations, so the refusal happens before any subprocess exists. `claude stop ALL` is the one shape that passes the shape check, because the deny list is case-sensitive while `MARKER_NAME_RE` allows uppercase; that is not an escalation (it would be an unknown session id and the command errors), but the deny check should fold case — informational, non-blocking. **Decision-function probes.** I drove `decide_background` with constructed fixtures for every refusal the ticket asked about. Confirmed STOP_CANDIDATE only on the clean case (marker + `kind=background` + `state=done` + `status=idle` + live pid + allowed cwd + no attach). Each single deviation produced REPORT with the reason named and no stop: `status=busy` → `status-busy`, `state=working`/`blocked`/empty → `state-*`, attach client by id or by exact name → `attach-client-present`, no marker → `no-retire-marker`, `El-Director`/`LD-Hypervisor`/`EL-DIRECTOR`/`Analista-Macro` → `protected-name`, cwd `/home/aaron/repos/intellimation` or `/opt/sendrast-director` or `/` or empty → `cwd-not-allowed`, `/home/aaron/repos/homelabX` and `/home/aaron/repos/homelab-other` → `cwd-not-allowed` (the parent test is a real containment test, not a `startswith`), a dead pid, `pid=None`, `pid=True`, `pid=0` and `pid=-1` → `process-gone`. A `kind=interactive` row with a marker naming it is reported as `kind-interactive` and never stopped; without a marker it never enters the table at all. A path under an allowed root that is a symlink resolving outside is refused, because `cwd_allowed` resolves before comparing. **Liveness and attach detection are read-only.** `Processes.alive` is a `/proc/<pid>` `is_dir()` test; `attached_ids` reads `/proc/*/cmdline` and decodes it. No `os.kill`, no signal, no write anywhere in the class. I confirmed against the live table: `attached_ids()` returned `[]` on the real fleet right now, and nothing in this path can disturb what it reads. **Marker lifetime.** I ran the full janitor in apply mode with a fake `claude` on PATH and a fake `/proc`. With `claude stop` failing, the row became `STOP_FAILED reasons=retire-marker|claude-stop-failed`, the marker file was **still there**, and a `NOTICE: claude stop failed id=9e011d8b name=epic-work` was emitted. With `claude stop` succeeding, the single id was written to the stops log and the marker was removed. In dry-run nothing was stopped and the marker stayed. Revalidation refuses rather than closes when the marker set becomes unreadable, because the marker then reads as `None` and no longer equals the original. **Census and notices.** `SESSIONS` counts an unreachable source as non-zero in every case I could construct: `claude agents` failing → `unreachable=1`, an unreadable retire directory → `unreachable=1`, a failed pane read → `unreachable=1`, and `Janitor(claude=None)` → `unreachable=1` rather than a quiet zero. I did not find a path where a failed source silently reports `unreachable=0`. **Tests.** I ran the suite myself: `python3 -m unittest discover -s tests` → **120 tests, OK** (32 in `test_herdr_janitor.py`, 58 in `test_herdr_janitor_relief.py`, 30 in `test_herdr_janitor_sessions.py`). The PR body says 117; the real count on this head is 120. CI on the exact head is green: runs #266 and #267 for `e01532f3e2` all pass. **Live read-only dry-run.** Isolated state dir under `mktemp -d`, mode 0700, no `--apply`, real fleet: ``` SESSIONS herdr=10 named=6 background=2 background-alive=1 unreachable=0 BACKGROUND id=83e833d6 name=Intellimation SWOT and AI strategy state=blocked status=unknown process=gone cwd=/home/aaron/repos/intellimation action=REPORT reasons=cwd-not-allowed|process-gone|state-blocked|no-retire-marker BACKGROUND id=9e011d8b name=Ghosttly-in-the-shell Android port of rootshell state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker summary REPORT=10 total=10 directory_unreadable_refusals=0 ``` Zero `CLOSE_CANDIDATE`, zero `WOULD_STOP`, zero `NOTICE:` lines. The live `el-director` pane came back `reasons=protected-name|focused-pane` and `analista-macro` came back `protected-name|directory-current`, which is the fail-closed behaviour the operator asked for. The intellimation session is exactly the case the cwd rule exists for and it is refused three ways over. **Plainly: no real background session has been stopped.** Not by me, not by this code. All my stop-path evidence comes from a fake `claude` and a fake `/proc`. The live dry-run issued no stop. The two live background sessions are still alive after my review. **Findings, none of which I consider blocking.** *The attach-guard is exact-match only — medium, non-blocking.* `claude attach --help` on this machine says "Part of the session name works in place of the id". The detector collects the raw args after `attach` and compares them for equality against the session id or the whole name. I fed it a fake `/proc` with six shapes and it caught `claude attach <id>` via the symlink and via the real `claude.exe` path, and the full name; it did **not** catch `claude attach Ghosttly` (a documented partial name), `claude attach 9e01` (an id prefix), or `claude attach <id>` with flags before `attach`. In those cases a human is attached and the guard is inert, so the session remains a STOP_CANDIDATE. The consequence is bounded: a stop still requires an operator retire marker for that exact id or name, and `claude stop` keeps the conversation resumable, so the worst case is a live terminal closing under a watching operator, not lost work. Not blocking, but the comment in `Processes` and the doc line "an attach client is something a person is looking at" overstate what the exact-match check proves, and matching on containment rather than equality would close it. *The protected-name list does not cover every Director spelling the code itself accepts — medium, non-blocking.* `DIRECTOR_NAMES` includes `el director`, `jesus-delivery`, `jesus-director` and `sendrast-director`; `PROTECTED_NAMES` covers only `ld-hypervisor`, `el-director` and the newly added `analista-macro`. I confirmed `el director` (space) reaches STOP_CANDIDATE on the background path while `El-Director` is refused. It is only reachable through a by-`<id>` marker, since `MARKER_NAME_RE` forbids spaces so `retire/el director` cannot be created — which is why I am not blocking. But the two lists should be one set, because "the single source of truth for protected names" currently disagrees with the code's own definition of who a Director is, and session names on background sessions are free text. *The `RULE` analyst suppression is prefix-based and slightly broad — low, non-blocking.* `analista-` as a bare prefix silences `analista-`, `analista-ghost-2` and `analista-macro junk copy`. That matches the documented `analista-<x>` grammar and, importantly, this branch can only ever suppress a report line, never authorise or refuse a close, so the failure direction is a non-conforming session going unlisted rather than anything being touched. Fine to leave. *A failed pane read is counted but not noticed — low, non-blocking.* With `herdr=1 named=1 unreachable=1` and a failing `agent read`, the report produced no `NOTICE:` line, because the notice only fires when the pane actually reached a close branch. The doc's Notices section promises a notice when "a source could not be read". The census still tells the truth, so this is a documentation-versus-behaviour gap, not silence where it matters. *The roster is not consulted on the new stop path — informational.* `decide_background` takes no `Directory`, so the operator's roster, which is otherwise a cross-check on every close, constrains nothing here. Given that the roster's aliases are role names and background session names are free text, it would rarely fire; worth knowing rather than fixing now. *The "not armed" framing is stale for this fleet — informational but worth saying out loud.* `docs/herdr-janitor.md` opens with "The implementation PR does not install or enable the timer and does not authorize a real close", and the PR body says the same. That is already untrue here: `herdr-janitor.timer` is enabled and has been running `--apply` every 15 minutes since 19:05Z with a valid activation file, and the deployed copy at `~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py` is byte-identical to `origin/main`. So the deployed code cannot stop a background session today, but the deploy step is routine — merging this and refreshing the deployed copy puts background stops live on a 15-minute schedule with no further decision. The retire marker stays the explicit operator act, which is why I still do not treat this as blocking; the doc sentence should be corrected so nobody reads "not armed" as a safety property of this change. Two smaller notes. A duplicate id in `claude agents --json` yields two STOP_CANDIDATE rows and would issue two stops, but the per-row revalidation turns the second into `SKIP_CHANGED` because the session is already gone, so it is benign; `parse_claude_sessions` could still dedupe. And `Config(directory=None)` (library use only, not reachable from the CLI, which always resolves a path) yields a readable roster with no rows, so the roster gate passes silently; `parse_directory(None)` returning `readable=True` is the one shape I would tighten. **Verdict: APPROVE.** The risk this change carries — stopping a live background session — is gated behind an explicit operator retire marker, and every doubt I could construct is report-only with the reason named. The write surface is exactly two argv, refused before a subprocess exists. The pane-close path, including the 600-second stillness floor, is untouched. My findings are about the completeness of two guards under conditions that each already require an explicit operator act, so I am not blocking on them; I would want the attach matcher widened and the director name sets merged before anyone relies on this in a hurry.

aaron added 1 commit 2026-11-04 20:46:18 +00:00

HOMELAB-2634: fail closed on attach clients and unknown session status (review F1/F2/F3) ... bca96b3f31

An attach client is now detected by CONTENT, not by argv shape: every readable
/proc/<pid>/cmdline is joined with spaces, and a session counts as attached when
any command line mentions `attach` anywhere and names it -- full id, id prefix
(`claude attach <prefix>`), or the session name. `claude --debug attach <id>`,
`claude --verbose attach <id>`, and `sh -c '... attach <id>'` are all seen now;
the shape guard missed them. The session's own daemon (its `pid` from
`claude agents --json`) and that pid's parent are excluded: running a session is
not looking at it.

Fail closed everywhere the guard was permissive:
- an unlistable /proc or an unreadable cmdline marks the scan unreachable, so no
  background session is stopped, and it is counted in `unreachable` (F3) and
  noticed;
- any status that is not exactly `idle` refuses the stop, so `busy`, `working`,
  `""` and unknown statuses no longer slip through (F2).

F-new: PROTECTED_NAMES is now the whole director family (ld-hypervisor,
el-director, analista-macro, sendrast-director) plus a `jesus` prefix rule, and
it is the single source of truth -- DIRECTOR_NAMES is gone. `name_key` folds
case, accents, spacing AND separators, so `El Director` and `EL_DIRECTOR` are
protected too.

Low: a failed pane read is counted in `unreachable`, so it now also emits a
NOTICE instead of being a silent zero.

Tests: 131 pass (was 120). The relief test that asserted the old three-name set
encoded the narrow set that was the bug; it now asserts the family and the
separator folding.

ld-reviewer approved these changes 2026-11-04 21:07:26 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: bca96b3f31
session: homelab-2634-review-7

How this was verified

Fresh review at head bca96b3f3195c1fe772e345d8315431bcc10f02a (base origin/main = 945d97c), read from the git object in a private git worktree under /tmp. Every decision function was imported and driven with fake process tables (Processes.proc_root pointed at a temp tree) — 46 probes, no real /proc dependency, and no claude stop, herdr close, marker or session was touched at any point.

Probes 1–8 requested by the brief, and their results:

1. Attach spellings — all refuse, each with a named reason (attach-client-present): claude attach <id>, claude --debug attach <id>, claude --verbose attach <id>, sh -c 'claude attach <id>', bash -lc 'exec claude attach <prefix10>', id prefix (8 chars), full UPPERCASE id, NUL-separated argv (claude\0--debug\0attach\0<prefix6>), the session name, and claude --debug attach --force <id>. 10/10.
2. No false positive from the session's own processes: with pid 4242 (the session) and pid 1111 (its parent, per stat) both carrying attach <id> in their cmdlines, the scan returns an empty attach set and the session remains a STOP_CANDIDATE; a third unrelated pid carrying the same text is caught. The exclusion is by pid and parent, not by text.
3. Fail closed on unreadable sources: a /proc that cannot be listed ⇒ unreachable=True, reason attach-scan-unreachable, unreachable counted in the census, and NOTICE: source unreadable attach-scan: …. A cmdline that exists but is unreadable (mode 000) ⇒ the same. A failed pane read (texts[pane]=None) ⇒ no CLOSE_CANDIDATE, counted, and NOTICE: source unreadable pane-read: <pane>.
4. Status/state fail closed: "", unknown, working, busy, IDLE, "idle ", done all refuse with status-<value>; state of "", running, busy refuse with state-<value>. Only exactly idle + done is safe.
5. Protected family, both paths: sendrast-director, Sendrast-Director, jesus, jesus-anything, El Director, El-Director, EL_DIRECTOR, él director, LD-Hypervisor, ld.hypervisor, Analista-Macro — every one is REPORT with protected-name on the background path and REPORT/protected-name on the herdr-pane path, even with the name itself as a retire marker. One list (PROTECTED_NAMES + jesus prefix) folded through name_key, no second narrower list.
6. Positive control still stops: unattached, marker-bearing, kind=background, state=done, status=idle, live pid, allowed cwd ⇒ STOP_CANDIDATE. Without a marker, with a foreign cwd (/tmp/elsewhere), or with kind=pane, no stop and (for the last) not even a row.
7. Subprocess allowlist holds before any subprocess: with a fake claude on the path that would touch a sentinel, all of rm <id>, rm --all, daemon stop, stop --all, stop, stop <id> --force, stop -x, stop all, stop attach, attach <id>, agents, agents --json --all, stop <id> extra, kill <id>, --debug stop <id>, stop ../etc, stop "" raise ValueError and the sentinel never appears. agents --json and stop <8-hex id> are the only two accepted forms; the module contains no os.kill, no SIGTERM, no Popen.
8. The 600-second stillness floor is untouched: screen_stable and decide are byte-identical to origin/main (945d97c); the only herdr-path change is _base_protections switching to is_protected_name (strictly wider protection). --min-successor-seconds 120 still exits 2 with refusing successor observation below 600 seconds, and --apply without an activation file is refused.

Test suite

python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py' ⇒ 131 tests, OK (2.3 s).

Live read-only dry run (isolated state dir)

SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0

Zero CLOSE_CANDIDATE, zero STOP_CANDIDATE, no NOTICE line. The two real background rows: 83e833d6 (cwd /home/aaron/repos/intellimation, state=blocked, no pid) ⇒ REPORT reasons=cwd-not-allowed|process-gone|status-unknown|state-blocked|no-retire-marker; 9e011d8b (cwd /home/aaron/repos/homelab, state=done status=idle, process=alive) ⇒ REPORT reasons=no-retire-marker. That second row is the healthy case and it is held by the marker alone — as designed, and worth the operator knowing.

No real background session has been stopped by this code, and none can be until it is installed. Live context I checked: the user timer herdr-janitor.timer runs --apply every 15 minutes against the deployed copy at ~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py, which is md5-identical to origin/main (c454094c…, no background code at all); at 21:00:07Z that deployed copy closed one real pane (wT:p13 infra-stab-orchestrator-5, evidence=retire-marker) and the retire dir is now empty. So the background path has never executed against a real session, and the moment this PR is installed the same armed 15-minute timer starts exercising it unattended.

Findings

F-1 — attach reference floor (severity: low; blocking: no). The content scan is a real fix and defeats every spelling I could construct (probe 1). Two spellings still escape: an id prefix shorter than MIN_ATTACH_REFERENCE = 4 (claude attach 9e0), and an id glued to a flag with = (claude --session=9e011d8b attach — REFERENCE_STRIP does not strip =). I could not establish reachability for either without risking a real attach: claude attach zz and claude attach zzq both resolve (the CLI complains about no job matching, not about a short argument), so a 2–3 character prefix is not rejected by length, but I would not point one at a live id to see whether id prefixes resolve at all. What I can say is that the code comment at MIN_ATTACH_REFERENCE is inverted: it says a sub-4-character token "is only ever a false positive (the safe direction)", but skipping the match is the unsafe direction — a real attach client that names the session goes unseen. Lowering the floor costs only extra refusals (fail-safe), so I recommend doing exactly that and correcting the comment, in a follow-up. Not blocking here because every concrete spelling in the brief refuses, and the guard is one of several (state==done, status==idle, marker, live pid, allowed cwd).

F-2 — the doc understates the guard it documents (severity: low; blocking: no). docs/herdr-janitor.md still states that a background stop requires status != "busy" and state == "done", and describes attach detection as "claude attach <id> in any cmdline". The code now requires exactly idle (F2 of this PR) and detects attach by scanning for attach plus a full id / ≥4-char prefix / session name, with an unreadable scan refusing every stop. The code is stricter than the doc in both places, so this is not a fail-open — but a supervisor reading the doc would believe working sessions are stoppable, and a future maintainer "aligning" code to the doc would erode the guard. The protected-name list in the same section (ld-hypervisor, el-director) also omits sendrast-director, analista-macro and the jesus prefix. Please fix the bullets in a follow-up.

Residual risks I could not close

• An attach that is not a separate process. The scan reads /proc; an operator who attaches from inside an already-running interactive Claude process (the agent view's own attach) would not appear as a new cmdline. Whether that shows up in claude agents --json (state/status) is untested — I would not attach to a live session to find out — so treat the attach scan as the primary barrier, not as one of two.
• /proc is only the local pid namespace, and a recycled pid can make a dead entry look alive (documented; the stop then fails and is reported STOP_FAILED, never retried).
• A rename or a marker spelled differently from both the id and the name yields no-retire-marker — fail-safe, but silent; the SESSIONS … unreachable= census is the only signal, and it counts sources, not markers.

Trusted, not verified

• The claude agents --json schema: I read it live (keys observed: id, sessionId, name, kind, state, status, pid, cwd, startedAt), and parse_claude_sessions consumes exactly those; id is preferred over sessionId when both exist, and the 8-char id is a prefix of sessionId, so a human attaching with the full UUID is still caught as a substring.
• That a real claude stop <id> succeeds and keeps the conversation resumable: asserted by the CLI's own --help, never executed by me or by this code.
• That is_protected_name's accent folding (él director) is what normalise_cell actually produces — I verified it through the decision output, not by unit-reading the fold.

I approve this head. The F1 fix is complete for every spelling I was asked to probe and materially stronger than what the previous round rejected; F-1 and F-2 are real but narrow, and both fixes are strictly safety-increasing.

## Verdict: APPROVED model: omniroute/deepseek-flash head: bca96b3f3195c1fe772e345d8315431bcc10f02a session: homelab-2634-review-7 ### How this was verified Fresh review at head `bca96b3f3195c1fe772e345d8315431bcc10f02a` (base `origin/main` = `945d97c`), read from the git object in a private `git worktree` under `/tmp`. Every decision function was imported and driven with **fake process tables** (`Processes.proc_root` pointed at a temp tree) — 46 probes, no real `/proc` dependency, and no `claude stop`, `herdr close`, marker or session was touched at any point. Probes 1–8 requested by the brief, and their results: 1. **Attach spellings — all refuse, each with a named reason** (`attach-client-present`): `claude attach <id>`, `claude --debug attach <id>`, `claude --verbose attach <id>`, `sh -c 'claude attach <id>'`, `bash -lc 'exec claude attach <prefix10>'`, id prefix (8 chars), full UPPERCASE id, NUL-separated argv (`claude\0--debug\0attach\0<prefix6>`), the session *name*, and `claude --debug attach --force <id>`. 10/10. 2. **No false positive from the session's own processes**: with pid 4242 (the session) and pid 1111 (its parent, per `stat`) both carrying `attach <id>` in their cmdlines, the scan returns an empty attach set and the session remains a `STOP_CANDIDATE`; a third unrelated pid carrying the same text *is* caught. The exclusion is by pid and parent, not by text. 3. **Fail closed on unreadable sources**: a `/proc` that cannot be listed ⇒ `unreachable=True`, reason `attach-scan-unreachable`, `unreachable` counted in the census, and `NOTICE: source unreadable attach-scan: …`. A `cmdline` that exists but is unreadable (mode 000) ⇒ the same. A failed pane read (`texts[pane]=None`) ⇒ no `CLOSE_CANDIDATE`, counted, and `NOTICE: source unreadable pane-read: <pane>`. 4. **Status/state fail closed**: `""`, `unknown`, `working`, `busy`, `IDLE`, `"idle "`, `done` all refuse with `status-<value>`; `state` of `""`, `running`, `busy` refuse with `state-<value>`. Only exactly `idle` + `done` is safe. 5. **Protected family, both paths**: `sendrast-director`, `Sendrast-Director`, `jesus`, `jesus-anything`, `El Director`, `El-Director`, `EL_DIRECTOR`, `él director`, `LD-Hypervisor`, `ld.hypervisor`, `Analista-Macro` — every one is `REPORT` with `protected-name` on the background path *and* `REPORT`/`protected-name` on the herdr-pane path, even with the name itself as a retire marker. One list (`PROTECTED_NAMES` + `jesus` prefix) folded through `name_key`, no second narrower list. 6. **Positive control still stops**: unattached, marker-bearing, `kind=background`, `state=done`, `status=idle`, live pid, allowed cwd ⇒ `STOP_CANDIDATE`. Without a marker, with a foreign cwd (`/tmp/elsewhere`), or with `kind=pane`, no stop and (for the last) not even a row. 7. **Subprocess allowlist holds before any subprocess**: with a fake `claude` on the path that would `touch` a sentinel, all of `rm <id>`, `rm --all`, `daemon stop`, `stop --all`, `stop`, `stop <id> --force`, `stop -x`, `stop all`, `stop attach`, `attach <id>`, `agents`, `agents --json --all`, `stop <id> extra`, `kill <id>`, `--debug stop <id>`, `stop ../etc`, `stop ""` raise `ValueError` and the sentinel never appears. `agents --json` and `stop <8-hex id>` are the only two accepted forms; the module contains no `os.kill`, no `SIGTERM`, no `Popen`. 8. **The 600-second stillness floor is untouched**: `screen_stable` and `decide` are **byte-identical** to `origin/main` (`945d97c`); the only herdr-path change is `_base_protections` switching to `is_protected_name` (strictly wider protection). `--min-successor-seconds 120` still exits 2 with `refusing successor observation below 600 seconds`, and `--apply` without an activation file is refused. ### Test suite `python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py'` ⇒ **131 tests, OK** (2.3 s). ### Live read-only dry run (isolated state dir) ``` SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0 ``` Zero `CLOSE_CANDIDATE`, zero `STOP_CANDIDATE`, no `NOTICE` line. The two real background rows: `83e833d6` (cwd `/home/aaron/repos/intellimation`, `state=blocked`, no pid) ⇒ `REPORT reasons=cwd-not-allowed|process-gone|status-unknown|state-blocked|no-retire-marker`; `9e011d8b` (cwd `/home/aaron/repos/homelab`, `state=done status=idle`, `process=alive`) ⇒ `REPORT reasons=no-retire-marker`. That second row is the healthy case and it is held by the marker alone — as designed, and worth the operator knowing. **No real background session has been stopped by this code, and none can be until it is installed.** Live context I checked: the user timer `herdr-janitor.timer` runs `--apply` every 15 minutes against the *deployed* copy at `~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py`, which is md5-identical to `origin/main` (`c454094c…`, no background code at all); at 21:00:07Z that deployed copy closed one real pane (`wT:p13 infra-stab-orchestrator-5`, `evidence=retire-marker`) and the retire dir is now empty. So the background path has never executed against a real session, and the moment this PR is installed the same armed 15-minute timer starts exercising it unattended. ### Findings **F-1 — attach reference floor (severity: low; blocking: no).** The content scan is a real fix and defeats every spelling I could construct (probe 1). Two spellings still escape: an id prefix shorter than `MIN_ATTACH_REFERENCE = 4` (`claude attach 9e0`), and an id glued to a flag with `=` (`claude --session=9e011d8b attach` — `REFERENCE_STRIP` does not strip `=`). I could not establish reachability for either without risking a real attach: `claude attach zz` and `claude attach zzq` both resolve (the CLI complains about *no job matching*, not about a short argument), so a 2–3 character prefix is not rejected by length, but I would not point one at a live id to see whether id prefixes resolve at all. What I can say is that the code comment at `MIN_ATTACH_REFERENCE` is **inverted**: it says a sub-4-character token "is only ever a false positive (the safe direction)", but skipping the match is the *unsafe* direction — a real attach client that names the session goes unseen. Lowering the floor costs only extra refusals (fail-safe), so I recommend doing exactly that and correcting the comment, in a follow-up. Not blocking here because every concrete spelling in the brief refuses, and the guard is one of several (`state==done`, `status==idle`, marker, live pid, allowed cwd). **F-2 — the doc understates the guard it documents (severity: low; blocking: no).** `docs/herdr-janitor.md` still states that a background stop requires `status != "busy"` and `state == "done"`, and describes attach detection as "`claude attach <id>` in any `cmdline`". The code now requires **exactly** `idle` (F2 of this PR) and detects attach by scanning for `attach` plus a full id / ≥4-char prefix / session name, with an unreadable scan refusing every stop. The code is stricter than the doc in both places, so this is not a fail-open — but a supervisor reading the doc would believe `working` sessions are stoppable, and a future maintainer "aligning" code to the doc would erode the guard. The protected-name list in the same section (`ld-hypervisor`, `el-director`) also omits `sendrast-director`, `analista-macro` and the `jesus` prefix. Please fix the bullets in a follow-up. ### Residual risks I could not close - **An attach that is not a separate process.** The scan reads `/proc`; an operator who attaches from inside an already-running interactive Claude process (the agent view's own `attach`) would not appear as a new cmdline. Whether that shows up in `claude agents --json` (`state`/`status`) is untested — I would not attach to a live session to find out — so treat the attach scan as the primary barrier, not as one of two. - **`/proc` is only the local pid namespace**, and a recycled pid can make a dead entry look alive (documented; the stop then fails and is reported `STOP_FAILED`, never retried). - **A rename or a marker spelled differently from both the id and the name** yields `no-retire-marker` — fail-safe, but silent; the `SESSIONS … unreachable=` census is the only signal, and it counts sources, not markers. ### Trusted, not verified - The `claude agents --json` schema: I read it live (keys observed: `id`, `sessionId`, `name`, `kind`, `state`, `status`, `pid`, `cwd`, `startedAt`), and `parse_claude_sessions` consumes exactly those; `id` is preferred over `sessionId` when both exist, and the 8-char `id` is a prefix of `sessionId`, so a human attaching with the full UUID is still caught as a substring. - That a real `claude stop <id>` succeeds and keeps the conversation resumable: asserted by the CLI's own `--help`, never executed by me or by this code. - That `is_protected_name`'s accent folding (`él director`) is what `normalise_cell` actually produces — I verified it through the decision output, not by unit-reading the fold. I approve this head. The F1 fix is complete for every spelling I was asked to probe and materially stronger than what the previous round rejected; F-1 and F-2 are real but narrow, and both fixes are strictly safety-increasing.

ld-reviewer requested changes 2026-11-04 21:09:05 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: REQUEST_CHANGES

model: omniroute/deepseek-flash
head: bca96b3f31
session: homelab-2634-review-8

Fresh, independent review of plane/HOMELAB-2634-janitor-background at bca96b3f3195c1fe772e345d8315431bcc10f02a (base origin/main = 945d97c), from my own detached worktree under $(mktemp -d) — not a shared checkout. Session homelab-2634-review-8, model omniroute/deepseek-flash. I did not read the other seats' bodies before forming this verdict.

I did not run --apply, and no real background session was stopped by me. I did not even attach to one. The only things I ran against the live fleet were reads: herdr pane list, claude agents --json, claude attach --help, claude stop --help, one dry run into an isolated state dir, and a read of /proc.

Verified myself (not trusted from the PR text)

The attach guard, live /proc, real processes. I built a fake claude on PATH and a fake /proc and drove Processes.attachments / names_session directly, then repeated the important cases against the real /proc with real live child processes. Detected: plain claude attach <id>; claude --debug attach <id> (flag before the verb); sh -c '… attach …'; bash -lc …; claude ATTACH <id>; Claude Attach; id prefixes of 4 and 6 chars; claude reattach <id> (attach inside a longer token — over-detection, the safe direction); claude agents attach --id <id>; env FOO=1 claude attach <id>; nohup …; ./claude attach <id>; the full session name; and claude at"tach" <id> / claude at$'tach' <id> — both of which the scan text misses but which is recovered because bash removes the quotes before exec, so the child's own cmdline reads … attach …. That recovery is real: I confirmed it live with real processes, so the spellings that defeated earlier heads are closed.

Fail-closed holes are real. An unreadable /proc/<pid>/cmdline (I made cmdline a directory) gives unreachable=True and reports every session as attached; an unlistable /proc does the same. unreachable then increments in the census.

The restore of the 600-second floor is untouched. git diff 945d97c bca96b3 -- scripts/herdr_janitor/janitor.py removes no line mentioning min_successor_seconds, screen_stable, OLD_SAFE_STATUSES, NEW_SAFE_STATUSES, NEVER_WORKSPACES or DEFAULT_WORKSPACES. screen_stable is still required on the successor path and on the retire-marker path, still compares the ~40-line hash and still demands now - hash_since >= 600. --min-successor-seconds 599 still exits 2 with refusing successor observation below 600 seconds. The only removals are the old two-member PROTECTED_NAMES literal and normalise_cell-based protection, both replaced by a strictly wider rule. Nothing #93 fixed was loosened.

The argv allowlist cannot be widened. 22 shapes driven through claude_allowed_argv: exactly ["agents","--json"] and ["stop",<one token>] pass. Refused: rm <id>, daemon stop, stop --all, stop all, stop daemon, stop agents, stop attach, stop <id> --force, stop --force <id>, stop "", stop -x, stop "abc; rm -rf /", stop "abc && rm -rf /", stop "$(id)", stop ../../etc/passwd, stop "9e011d8b\n", kill <id>, agents, agents --json -a. ClaudeCLI._run re-checks the allowlist, so the subprocess is unreachable by another path. There is no shell=True, no os.kill, no signal, no os.remove, no shutil.rmtree, no .kill( and no pkill anywhere in the file; the only two subprocess.run calls are the two allowlisted argv.

Read-only liveness. Processes only uses is_dir(), iterdir(), read_bytes(), read_text(). Nothing is signalled, so noticing an attach client cannot disturb it. The dry run wrote exactly report.txt and janitor.log into the isolated state dir and no seen.json.

Marker lifecycle. With a fake claude whose stop raises: action=STOP_FAILED, reasons=…|claude-stop-failed, and the marker is still present. With a stop that succeeds: action=STOPPED, marker removed. So the marker survives a failed stop and is removed only on success — as documented.

Notices. Four ordinary refusal rows (protected name, attach client present, no marker + foreign cwd, status-busy) plus a protected pane refusal produce [] — silent, as promised. A failed claude stop, a SKIP_CHANGED, a failed pane read, an unreadable claude agents --json, an unreachable attach scan, an unreadable retire dir and an unreadable roster each produce their own NOTICE:.

unreachable is never a silent zero. Each source broken one at a time: claude unreadable → unreachable=1; attach-scan unreachable → 1; markers unreadable → 1; roster unreadable → 1; one failed pane read → 1. Clean run → 0. A list_panes failure prints to stderr and exits 1 with no census line at all, which is loud rather than quiet.

Protected names, every spelling I could construct. Protected: sendrast-director, Sendrast-Director, SENDRAST_DIRECTOR, sendrast director, jesus, jesus-director, Jesús-Director, jésus-director, JESUS-orchestrator-9, El Director, el-director, EL.DIRECTOR, el__director, " El Director ", LD-Hypervisor, ld_hypervisor, Ld.Hypervisor, Analista-Macro, ANALISTA_MACRO. name_key folds case, accents, spacing and the - _ . separators, and the jesus prefix protects the whole person family. On the pane path a live el-director tab reports reasons=protected-name|focused-pane|role-not-relievable.

Tests, CI, dry run

python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py' → 131 tests, OK, 0 failures, 0 errors. py_compile clean. CI on the exact head bca96b3: runs #269 (python, pass) and #268 (all 15 jobs, pass). Nothing red at this head.

Live read-only dry run, isolated mktemp state dir, real herdr and real claude:

SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0

Zero CLOSE_CANDIDATE / WOULD_CLOSE (nine REPORT rows, summary REPORT=9 total=9), zero NOTICE:, zero RULE lines. The one live, alive, idle/done, homelab-cwd background session (9e011d8b) reports reasons=no-retire-marker — i.e. exactly one marker away from being stoppable, which is why F1 below matters.

Findings

F1 — BLOCKING. The attach guard misses every partial session reference, including the form the CLI documents. claude attach --help on the real binary (v2.1.318) says:

Usage: claude attach <id|name>
  … Part of the session name works in place of the id.

Processes.names_session models only: the full id as a substring, an id prefix of ≥4 chars, and the full name as a substring. It does not model part of a name. Against a fabricated copy of the live session (name=Ghosttly-in-the-shell Android port of rootshell, status=idle, state=done, alive, allowed cwd, retire marker present), each of these produced action=STOP_CANDIDATE with no attach reason at all:

• claude attach Ghosttly-in-the-shell (first word of the name)
• claude attach Ghostt (six chars of the name)
• claude attach "Ghosttly-in"
• claude attach (bare — the agent picker)

while claude attach <full name> and claude attach 9e011d correctly gave REPORT + attach-client-present. In --apply mode each of the missed forms would have issued claude stop <id> on a session a human is looking at. This is the exact invariant the PR claims to have repaired ("an attach client is something a person is looking at: none of those exist here"; "a guard a flag defeats is not a guard") and it is still defeated — not by a shell trick, but by a human typing the first word of a long name, which is what the CLI's own help invites for names like the two live ones. The test suite only covers the full-name case (("name", ["claude", "--debug", "attach", "epic-work"])), so nothing catches this.

Fix I would accept: also treat any token ≥4 chars that is a prefix of name_key(session.name) as a reference, and fail closed (unreachable) for an attach-bearing command line that cannot be attributed to any session — a bare claude attach names someone, just not in its argv. The bounded stop is recoverable (claude stop keeps the conversation), and nothing is armed until the activation file exists, which is why I ask for a fix and not a redesign; but the guard must be right before the timer is ever armed.

F2 — Low, non-blocking. Two documented safeguards are described more loosely than they are implemented. docs/herdr-janitor.md and the PR description say a stop requires status != "busy" and lists the protected set as ld-hypervisor/el-director. The code is stricter than the doc (status == "idle" exactly, and six protected spellings plus a jesus prefix). Stricter-than-documented is the safe direction, so this is not a risk to sessions — but a reader of the doc would believe a working background session is stoppable, and would not know that sendrast-director, analista-macro and jesus-* are protected.

F3 — Low, non-blocking. el–director (en dash) and hybrid role names are not protected. name_key folds - _ . but not –/—, so el–director is unprotected; and because protection is tested on the whole name, el-director-orchestrator-1 and sendrast-director-orchestrator-1 are a relievable family and do reach CLOSE_CANDIDATE on the pane path (I reproduced both; jesus-director-orchestrator-1 is still caught by the prefix, and ld-hypervisor is caught because it fits no role family). The live fleet's director tab is named el-director, so I do not believe this is reachable today; it is a hardening gap against the stated contract, not a demonstrated stop.

F4 — Low, non-blocking. Exit status does not reflect a failed stop. main() returns 1 only for a pane CLOSE_FAILED. A background STOP_FAILED, SKIP_CHANGED or STOPPED_MARKER_DELETE_FAILED still exits 0. The NOTICE: line carries it for a human, but the machine-readable signal a timer or alert would read says success.

F5 — Informational. analista-* is a blanket RULE exemption. rule_violation returns None for any folded name starting with analista-, so analista-<anything> never appears as a RULE row even when it is not the operator's named analyst. RULE is report-only, so nothing can be stopped through it; the cost is reporting precision, which the operator can tighten if they want it.

What I trusted rather than proved

claude attach's acceptance of a partial name is the CLI's own help text; I did not attach to a live session to observe it (I judged touching 9e011d8b to be the exact disturbance this review is meant to prevent). The dashless-uuid form (claude attach 9e011d8b9e01… without dashes) is also missed by names_session and equally unverified against a real session — if the CLI resolves it, it is the same hole by another route. claude stop <id>'s own behaviour (conversation kept, resumable) is documented in claude stop --help and not exercised. The pid-reuse caveat is the PR's own and remains true.

The change is close: the argv allowlist, the fail-closed scan, the marker lifecycle, the census, the notices, the protected family and the untouched 600-second floor are all what they claim to be, and I verified each of them rather than reading them. I am asking for changes for one reason: the attach guard repairs the argv axis and still misses the reference axis, and that is the one thing this change must not get wrong.

## Verdict: REQUEST_CHANGES model: omniroute/deepseek-flash head: bca96b3f3195c1fe772e345d8315431bcc10f02a session: homelab-2634-review-8 Fresh, independent review of `plane/HOMELAB-2634-janitor-background` at `bca96b3f3195c1fe772e345d8315431bcc10f02a` (base `origin/main` = `945d97c`), from my own detached worktree under `$(mktemp -d)` — not a shared checkout. Session `homelab-2634-review-8`, model `omniroute/deepseek-flash`. I did not read the other seats' bodies before forming this verdict. I did **not** run `--apply`, and **no real background session was stopped by me**. I did not even attach to one. The only things I ran against the live fleet were reads: `herdr pane list`, `claude agents --json`, `claude attach --help`, `claude stop --help`, one dry run into an isolated state dir, and a read of `/proc`. ## Verified myself (not trusted from the PR text) **The attach guard, live `/proc`, real processes.** I built a fake `claude` on `PATH` and a fake `/proc` and drove `Processes.attachments` / `names_session` directly, then repeated the important cases against the **real** `/proc` with real live child processes. Detected: plain `claude attach <id>`; `claude --debug attach <id>` (flag before the verb); `sh -c '… attach …'`; `bash -lc …`; `claude ATTACH <id>`; `Claude Attach`; id prefixes of 4 and 6 chars; `claude reattach <id>` (`attach` inside a longer token — over-detection, the safe direction); `claude agents attach --id <id>`; `env FOO=1 claude attach <id>`; `nohup …`; `./claude attach <id>`; the **full** session name; and `claude at"tach" <id>` / `claude at$'tach' <id>` — both of which the *scan text* misses but which is recovered because bash removes the quotes before `exec`, so the child's own `cmdline` reads `… attach …`. That recovery is real: I confirmed it live with real processes, so the spellings that defeated earlier heads are closed. **Fail-closed holes are real.** An unreadable `/proc/<pid>/cmdline` (I made `cmdline` a directory) gives `unreachable=True` and reports every session as attached; an unlistable `/proc` does the same. `unreachable` then increments in the census. **The restore of the 600-second floor is untouched.** `git diff 945d97c bca96b3 -- scripts/herdr_janitor/janitor.py` removes **no** line mentioning `min_successor_seconds`, `screen_stable`, `OLD_SAFE_STATUSES`, `NEW_SAFE_STATUSES`, `NEVER_WORKSPACES` or `DEFAULT_WORKSPACES`. `screen_stable` is still required on the successor path and on the retire-marker path, still compares the ~40-line hash and still demands `now - hash_since >= 600`. `--min-successor-seconds 599` still exits 2 with `refusing successor observation below 600 seconds`. The only removals are the old two-member `PROTECTED_NAMES` literal and `normalise_cell`-based protection, both replaced by a strictly wider rule. Nothing #93 fixed was loosened. **The argv allowlist cannot be widened.** 22 shapes driven through `claude_allowed_argv`: exactly `["agents","--json"]` and `["stop",<one token>]` pass. Refused: `rm <id>`, `daemon stop`, `stop --all`, `stop all`, `stop daemon`, `stop agents`, `stop attach`, `stop <id> --force`, `stop --force <id>`, `stop ""`, `stop -x`, `stop "abc; rm -rf /"`, `stop "abc && rm -rf /"`, `stop "$(id)"`, `stop ../../etc/passwd`, `stop "9e011d8b\n"`, `kill <id>`, `agents`, `agents --json -a`. `ClaudeCLI._run` re-checks the allowlist, so the subprocess is unreachable by another path. There is no `shell=True`, no `os.kill`, no `signal`, no `os.remove`, no `shutil.rmtree`, no `.kill(` and no `pkill` anywhere in the file; the only two `subprocess.run` calls are the two allowlisted argv. **Read-only liveness.** `Processes` only uses `is_dir()`, `iterdir()`, `read_bytes()`, `read_text()`. Nothing is signalled, so noticing an attach client cannot disturb it. The dry run wrote exactly `report.txt` and `janitor.log` into the isolated state dir and no `seen.json`. **Marker lifecycle.** With a fake `claude` whose `stop` raises: `action=STOP_FAILED`, `reasons=…|claude-stop-failed`, and the marker **is still present**. With a `stop` that succeeds: `action=STOPPED`, marker removed. So the marker survives a failed stop and is removed only on success — as documented. **Notices.** Four ordinary refusal rows (protected name, attach client present, no marker + foreign cwd, `status-busy`) plus a protected pane refusal produce `[]` — silent, as promised. A failed `claude stop`, a `SKIP_CHANGED`, a failed pane read, an unreadable `claude agents --json`, an unreachable attach scan, an unreadable retire dir and an unreadable roster each produce their own `NOTICE:`. **`unreachable` is never a silent zero.** Each source broken one at a time: claude unreadable → `unreachable=1`; attach-scan unreachable → `1`; markers unreadable → `1`; roster unreadable → `1`; one failed pane read → `1`. Clean run → `0`. A `list_panes` failure prints to stderr and exits 1 with no census line at all, which is loud rather than quiet. **Protected names, every spelling I could construct.** Protected: `sendrast-director`, `Sendrast-Director`, `SENDRAST_DIRECTOR`, `sendrast director`, `jesus`, `jesus-director`, `Jesús-Director`, `jésus-director`, `JESUS-orchestrator-9`, `El Director`, `el-director`, `EL.DIRECTOR`, `el__director`, `" El Director "`, `LD-Hypervisor`, `ld_hypervisor`, `Ld.Hypervisor`, `Analista-Macro`, `ANALISTA_MACRO`. `name_key` folds case, accents, spacing and the `- _ .` separators, and the `jesus` prefix protects the whole person family. On the pane path a live `el-director` tab reports `reasons=protected-name|focused-pane|role-not-relievable`. ## Tests, CI, dry run `python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py'` → **131 tests, OK, 0 failures, 0 errors**. `py_compile` clean. CI on the exact head `bca96b3`: runs **#269** (`python`, pass) and **#268** (all 15 jobs, pass). Nothing red at this head. Live read-only dry run, isolated `mktemp` state dir, real herdr and real `claude`: ``` SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0 ``` Zero `CLOSE_CANDIDATE` / `WOULD_CLOSE` (nine `REPORT` rows, `summary REPORT=9 total=9`), zero `NOTICE:`, zero `RULE` lines. The one live, alive, `idle`/`done`, homelab-cwd background session (`9e011d8b`) reports `reasons=no-retire-marker` — i.e. exactly one marker away from being stoppable, which is why F1 below matters. ## Findings **F1 — BLOCKING. The attach guard misses every partial session reference, including the form the CLI documents.** `claude attach --help` on the real binary (v2.1.318) says: ``` Usage: claude attach <id|name> … Part of the session name works in place of the id. ``` `Processes.names_session` models only: the full id as a substring, an id **prefix** of ≥4 chars, and the **full** name as a substring. It does not model *part of a name*. Against a fabricated copy of the live session (`name=Ghosttly-in-the-shell Android port of rootshell`, `status=idle`, `state=done`, alive, allowed cwd, retire marker present), each of these produced `action=STOP_CANDIDATE` with **no** attach reason at all: - `claude attach Ghosttly-in-the-shell` (first word of the name) - `claude attach Ghostt` (six chars of the name) - `claude attach "Ghosttly-in"` - `claude attach` (bare — the agent picker) while `claude attach <full name>` and `claude attach 9e011d` correctly gave `REPORT` + `attach-client-present`. In `--apply` mode each of the missed forms would have issued `claude stop <id>` on a session a human is looking at. This is the exact invariant the PR claims to have repaired ("an attach client is something a person is looking at: none of those exist here"; "a guard a flag defeats is not a guard") and it is still defeated — not by a shell trick, but by a human typing the first word of a long name, which is what the CLI's own help invites for names like the two live ones. The test suite only covers the full-name case (`("name", ["claude", "--debug", "attach", "epic-work"])`), so nothing catches this. *Fix I would accept:* also treat any token ≥4 chars that is a prefix of `name_key(session.name)` as a reference, and fail closed (`unreachable`) for an `attach`-bearing command line that cannot be attributed to any session — a bare `claude attach` names someone, just not in its argv. The bounded stop is recoverable (`claude stop` keeps the conversation), and nothing is armed until the activation file exists, which is why I ask for a fix and not a redesign; but the guard must be right before the timer is ever armed. **F2 — Low, non-blocking. Two documented safeguards are described more loosely than they are implemented.** `docs/herdr-janitor.md` and the PR description say a stop requires `status != "busy"` and lists the protected set as `ld-hypervisor`/`el-director`. The code is *stricter* than the doc (`status == "idle"` exactly, and six protected spellings plus a `jesus` prefix). Stricter-than-documented is the safe direction, so this is not a risk to sessions — but a reader of the doc would believe a `working` background session is stoppable, and would not know that `sendrast-director`, `analista-macro` and `jesus-*` are protected. **F3 — Low, non-blocking. `el–director` (en dash) and hybrid role names are not protected.** `name_key` folds `- _ .` but not `–`/`—`, so `el–director` is unprotected; and because protection is tested on the whole name, `el-director-orchestrator-1` and `sendrast-director-orchestrator-1` are a relievable family and **do** reach `CLOSE_CANDIDATE` on the pane path (I reproduced both; `jesus-director-orchestrator-1` is still caught by the prefix, and `ld-hypervisor` is caught because it fits no role family). The live fleet's director tab is named `el-director`, so I do not believe this is reachable today; it is a hardening gap against the stated contract, not a demonstrated stop. **F4 — Low, non-blocking. Exit status does not reflect a failed stop.** `main()` returns 1 only for a pane `CLOSE_FAILED`. A background `STOP_FAILED`, `SKIP_CHANGED` or `STOPPED_MARKER_DELETE_FAILED` still exits 0. The `NOTICE:` line carries it for a human, but the machine-readable signal a timer or alert would read says success. **F5 — Informational. `analista-*` is a blanket `RULE` exemption.** `rule_violation` returns `None` for any folded name starting with `analista-`, so `analista-<anything>` never appears as a `RULE` row even when it is not the operator's named analyst. `RULE` is report-only, so nothing can be stopped through it; the cost is reporting precision, which the operator can tighten if they want it. ## What I trusted rather than proved `claude attach`'s acceptance of a partial name is the CLI's **own help text**; I did not attach to a live session to observe it (I judged touching `9e011d8b` to be the exact disturbance this review is meant to prevent). The dashless-uuid form (`claude attach 9e011d8b9e01…` without dashes) is also missed by `names_session` and equally unverified against a real session — if the CLI resolves it, it is the same hole by another route. `claude stop <id>`'s own behaviour (conversation kept, resumable) is documented in `claude stop --help` and not exercised. The pid-reuse caveat is the PR's own and remains true. The change is close: the argv allowlist, the fail-closed scan, the marker lifecycle, the census, the notices, the protected family and the untouched 600-second floor are all what they claim to be, and I verified each of them rather than reading them. I am asking for changes for one reason: the attach guard repairs the argv axis and still misses the reference axis, and that is the one thing this change must not get wrong.

aaron added 1 commit 2026-11-04 21:26:59 +00:00

HOMELAB-2634: treat partial attach references as attached, harden protected names (review F1b/F3) ... bcb8ef0e02

F1b: `claude attach` documents that part of the session name works in place of
the id, so the guard now matches any token of three characters or more against
every fragment of a session's name and id, in both directions, instead of the
full id / full name only. An attach-bearing command line that resolves to no
known session -- a bare listing `claude attach`, a flag-only form, an
unrecognised name -- is ambiguity, not evidence of absence: it marks every
background session as attached, stops none for that cycle, and reports
`attach-client-unidentified`. Fail-closed on unreadable /proc and cmdline is
unchanged, as are the own-pid and parent exclusions.

F3: director-family protection is containment on the folded name instead of an
exact match, with en/em dashes added to the single folding rule, so
`el-director-orchestrator-1`, `ld-hypervisor-2`, and `sendrast-director-x` are
protected. The attach guard is documented as deliberately over-conservative.

ld-reviewer approved these changes 2026-11-04 21:34:57 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: bcb8ef0e02
session: homelab-2634-review-9

base: 945d97c786 (origin/main)

I attacked this head as an adversary: my goal was to stop a background session that
should have been left alone. I did not get one. Every fabricated process table that
places a person on a session refuses the stop, and every table I could not resolve
refuses everything rather than nothing.

Verified (I read the code and ran it)

Isolated worktree /tmp/rev94-bcb8ef0 at the head above; all probes use a fabricated
/proc (Processes.proc_root pointed at a temp tree, every cmdline/stat written by
the probe) and fabricated claude agents --json rows. No real session was named,
attached to, or stopped.

F1b — partial attach references. 24 of 25 attack lines refuse the stop; the 25th is
the intended one (an attach line naming a different known session marks that session,
not mine, which is the documented contract).

probe	result
claude attach Ghosttly-in-the-shell (full name)	attach-client-present — refused
claude attach Ghostt (partial name)	refused
claude attach the-shell / in-the	refused
claude attach 'Ghostt' / "Ghosttly" (quoted partial)	refused
id prefix 6 / 8 / 20 chars	refused
id prefix 2 chars (9e)	attach-client-unidentified -> every session attached, refused
sh -c "claude attach --name <full name>" (name inside token)	refused
claude ATTACH <name> (uppercase)	refused
claude --debug attach 9e0117aa	refused
sh -c '... attach ...', env FOO=1 ..., nohup ..., sudo -u aaron ...	refused
claude attach 9e0117aa --verbose (trailing flag)	refused
bare claude attach	attach-client-unidentified -> refused
claude attach --json (flag-only)	attach-client-unidentified -> refused
claude attach some-random-session (unknown name)	attach-client-unidentified -> refused
claude attach th (2-char fragment)	attach-client-unidentified -> refused
claude attach '' (empty arg)	attach-client-unidentified -> refused
/usr/local/bin/claude attach 9e0117aa (path component)	refused
claude attach <other session id prefix>	only the other session marked; mine stoppable — correct

Ambiguity refuses everything. An unresolvable attach-bearing line sets
unidentified and AttachmentScan.attached() then answers True for every id, so
decide_background refuses all of them with attach-client-unidentified. Verified: with
two sessions in the roster and one bare claude attach line, both sessions carried the
refusal. Two sessions with same-family names (sup-a-orchestrator-1,
sup-b-orchestrator-1) plus the ambiguous fragment attach orchestrator marks both.

F3 — protected names. 31 of 31 spellings protected, including every name the ticket
names as a collision: el-director, El-Director, EL_DIRECTOR, el.director,
el director, double space, leading/trailing space, El\u2013Director (en dash),
em/box/figure/hyphen dashes, ld-hypervisor, LD-Hypervisor, ld\u2013hypervisor,
ld-hypervisor-2, ld-hypervisor-99, sendrast-director, sendrast-director-x,
jesus, jesus-director, jesus-anything-1, Jes\u00fas-Director,
el-director-orchestrator-1, el-director-orchestrator-9,
ld-hypervisor-orchestrator-2, sup-el-director-2, analista-macro. A protected
background session with a marker is refused by both marker spellings (name and id):
reasons=protected-name|retire-marker-held. The collision el-director-orchestrator-1
is caught by _base_protections on the pane path too (protected-name -> successor
branch takes successor-close-held), and rule_violation() returns None for it, so it
gets no RULE line either — the two paths agree.

Allowlist — refusal precedes any subprocess. 19 of 19 forbidden argv refused by
claude_allowed_argv before subprocess.run is reached (_run raises first): rm <id>,
stop all, stop --all, stop -x, stop <id> --force, stop alone, agents without
--json, agents --json --x, daemon stop, stop daemon, stop attach, and injection
shapes stop ../../etc/passwd, stop 'x;rm -rf /', stop '$(id)', stop 'a b',
stop '', stop <200 chars>, attach <id>. Only agents --json and stop <one id>
pass.

Read-only on every path. AST scan of the module: no os.kill/os.killpg/signal/
system/popen/rmtree/remove/rename/chmod; the only os.replace is inside
atomic_write, and the only .unlink() sites are the atomic-write temp file and the two
retire-marker deletions that follow a successful close/stop. Processes only calls
iterdir, read_bytes, read_text, is_dir.

Marker survives a failed stop. _execute_background: STOP_FAILED leaves the marker
(even with --apply and a valid activation file: marker_exists=True), STOPPED removes
it (marker_exists=False); a failed delete becomes
STOPPED_MARKER_DELETE_FAILED + NOTICE: retire marker delete failed. Revalidation reads
a fresh roster and re-decides before the single write; a marker source that became
unreadable yields SKIP_CHANGED, not a stop.

600-second stillness floor is identical to origin/main. screen_stable,
successor_age, update_seen, text_hash, live_work, load_seen, save_seen,
valid_first_seen, directory_gate, directory_successor_state are byte-identical
between origin/main and this head (extracted and diffed). A 36-cell differential over
min_successor_seconds in {0,1,599,600,601,3600} x screen age in {0,1,599,600,601,5000}
produced zero divergences. Defaults stay 600 (Config, argparse), and
--min-successor-seconds 0 and --min-successor-seconds 599 still exit 2 with
"refusing successor observation below 600 seconds". The pane-path diff is limited to
is_protected_name replacing the narrower equality check (strictly more protective) and
the report getting SESSIONS/BACKGROUND/RULE/NOTICE lines.

NOTICE discipline and the census. Fabricated-run probes: a plain REPORT
(no marker) -> 0 notices; a quiet refusal (attach-client-present,
attach-client-unidentified, status-busy, cwd-not-allowed, protected-name) -> 0
notices; a real failure (claude agents --json raising, claude stop raising, an
unreadable /proc cmdline, a failed pane read) -> exactly one notice naming the source,
and unreachable increments (1, 1, 1). unreachable is an explicit sum over
claude-agents, the attach scan, the retire dir, the roster, and every failed pane read,
so it cannot collapse to a silent 0; a claude source that is not wired at all counts as
unreachable too.

Test count

python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py' -> Ran 138 tests …
OK (0.40 s) at bcb8ef0.

Dry-run (read-only, isolated state dir)

python3 scripts/herdr_janitor/janitor.py --dry-run --state-dir <mktemp>:

SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0
BACKGROUND id=83e833d6 name=Intellimation SWOT and AI strategy state=blocked status=unknown process=gone cwd=/home/aaron/repos/intellimation action=REPORT reasons=cwd-not-allowed|process-gone|status-unknown|state-blocked|no-retire-marker
BACKGROUND id=9e011d8b name=Ghosttly-in-the-shell Android port of rootshell state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker
summary REPORT=9 total=9 directory_unreadable_refusals=0

Zero CLOSE_CANDIDATE, zero WOULD_CLOSE, zero WOULD_STOP, zero NOTICE: lines, and
the only filesystem effect was report.txt + janitor.log inside the isolated state dir.
The live el-director pane reports protected-name|focused-pane. analista-macro is
directory-current. No real background session has been stopped by this code, ever: the
deployed herdr-janitor.timer copy at ~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py
is the origin/main build (it prints no SESSIONS/BACKGROUND lines), so claude stop
has never been issued from this repository. The first real stop will happen only after this
lands, is deployed, and someone creates a retire marker.

Trusted, not verified

• claude attach's documented rule "part of the session name works in place of the id" is
  taken from the ticket and the docstring; I did not run claude attach to re-derive which
  fragment forms the real CLI accepts. The guard is conservative against that rule, not
  derived from the binary.
• claude agents --json field semantics (kind, state, status, pid, cwd) are
  trusted; the schema I exercised is the one the PR's own parser accepts.
• I did not exercise --apply against a real session, and I did not create a real retire
  marker: every STOPPED/STOP_FAILED probe used a fabricated roster and a spy stop.

Findings (none blocking)

1. low / non-blocking — doc drift in docs/herdr-janitor.md. The background-stop list
   says status != "busy" and state == "done"; the code requires exactly
   status == "idle" (stricter, BACKGROUND_IDLE_STATUS). The document understates the
   guard it describes. Fix with the next touch; it cannot cause a stop.
2. low / non-blocking — the attach token is matched as a raw substring of the whole
   cmdline, so an unrelated process blocks every background stop for its lifetime. A
   line whose argv merely contains attach and resolves to no known session — tmux attach,
   herdr attach, even cat attach.log — sets attach-client-unidentified and refuses
   all stops, not just one. That is the correct fail-closed direction and nothing was
   closed by it, but it means the feature can silently never fire on a host that has such a
   process. I verified this with a fabricated cat attached.log. Worth a follow-up ticket
   (scope the token to a claude/attach argv shape, staying fail-closed) rather than a
   block on this PR.
3. low / non-blocking — residual spellings that defeat name_key. Separator, case,
   accent, and the whole Unicode dash family are folded; these are not:
   eldirector (no separator at all), el\u200bdirector (zero-width space),
   el\u00b7director (middle dot), \u0435l-director (Cyrillic е), el-direct0r. A
   session deliberately named with a zero-width space would escape the protection. A marker
   is still required to reach a stop, and every spelling the ticket names is covered, so I
   do not block; the folding table deserves the next ticket.
4. info — an unnamed background session can be stopped via retire/<id> with no
   protected-name check, because there is no name to fold. Marker-by-id is explicit
   operator authority, so this is the documented contract, not a leak.
5. info — the roster veto does not gate claude stop. directory-unreadable /
   directory-current refuse every pane close, but decide_background never consults
   the Directory. The bounds on a background stop are marker + kind + protected name +
   liveness + status/state + cwd + attach scan. A background session running a numbered
   successor role could therefore be stopped by a marker without a roster cross-check; the
   marker is still explicit human authority. Worth stating in the doc if it is intended.

Verdict

APPROVE. Both fixes the earlier reviewers asked for are present at this head and survive an
adversarial read: partial and quoted attach references, id prefixes of any resolvable
length, ATTACH/--debug/wrapper forms and the whole unresolvable class all refuse, and
the protected-name check now contains rather than equals on a separator-folded key so the
el-director-orchestrator-1 collision is protected on every path. The 600-second stillness
floor that made the pane janitor safe is provably unchanged. CI is green on this exact head.
The findings above are follow-ups, not blockers.

## Verdict: APPROVED model: omniroute/deepseek-flash head: bcb8ef0e02cc2a8bb52914cad124947101a86717 session: homelab-2634-review-9 base: 945d97c786f87cfcfbab3fd1833997ec1678bb11 (origin/main) I attacked this head as an adversary: my goal was to stop a background session that should have been left alone. I did not get one. Every fabricated process table that places a person on a session refuses the stop, and every table I could not resolve refuses *everything* rather than nothing. ### Verified (I read the code and ran it) Isolated worktree `/tmp/rev94-bcb8ef0` at the head above; all probes use a fabricated `/proc` (`Processes.proc_root` pointed at a temp tree, every `cmdline`/`stat` written by the probe) and fabricated `claude agents --json` rows. No real session was named, attached to, or stopped. **F1b — partial attach references.** 24 of 25 attack lines refuse the stop; the 25th is the intended one (an attach line naming a *different* known session marks that session, not mine, which is the documented contract). | probe | result | |---|---| | `claude attach Ghosttly-in-the-shell` (full name) | `attach-client-present` — refused | | `claude attach Ghostt` (partial name) | refused | | `claude attach the-shell` / `in-the` | refused | | `claude attach 'Ghostt'` / `"Ghosttly"` (quoted partial) | refused | | id prefix 6 / 8 / 20 chars | refused | | id prefix 2 chars (`9e`) | `attach-client-unidentified` -> every session attached, refused | | `sh -c "claude attach --name <full name>"` (name inside token) | refused | | `claude ATTACH <name>` (uppercase) | refused | | `claude --debug attach 9e0117aa` | refused | | `sh -c '... attach ...'`, `env FOO=1 ...`, `nohup ...`, `sudo -u aaron ...` | refused | | `claude attach 9e0117aa --verbose` (trailing flag) | refused | | bare `claude attach` | `attach-client-unidentified` -> refused | | `claude attach --json` (flag-only) | `attach-client-unidentified` -> refused | | `claude attach some-random-session` (unknown name) | `attach-client-unidentified` -> refused | | `claude attach th` (2-char fragment) | `attach-client-unidentified` -> refused | | `claude attach ''` (empty arg) | `attach-client-unidentified` -> refused | | `/usr/local/bin/claude attach 9e0117aa` (path component) | refused | | `claude attach <other session id prefix>` | only the *other* session marked; mine stoppable — correct | **Ambiguity refuses everything.** An unresolvable attach-bearing line sets `unidentified` and `AttachmentScan.attached()` then answers `True` for *every* id, so `decide_background` refuses all of them with `attach-client-unidentified`. Verified: with two sessions in the roster and one bare `claude attach` line, both sessions carried the refusal. Two sessions with same-family names (`sup-a-orchestrator-1`, `sup-b-orchestrator-1`) plus the ambiguous fragment `attach orchestrator` marks **both**. **F3 — protected names.** 31 of 31 spellings protected, including every name the ticket names as a collision: `el-director`, `El-Director`, `EL_DIRECTOR`, `el.director`, `el director`, double space, leading/trailing space, `El\u2013Director` (en dash), em/box/figure/hyphen dashes, `ld-hypervisor`, `LD-Hypervisor`, `ld\u2013hypervisor`, `ld-hypervisor-2`, `ld-hypervisor-99`, `sendrast-director`, `sendrast-director-x`, `jesus`, `jesus-director`, `jesus-anything-1`, `Jes\u00fas-Director`, `el-director-orchestrator-1`, `el-director-orchestrator-9`, `ld-hypervisor-orchestrator-2`, `sup-el-director-2`, `analista-macro`. A protected background session with a marker is refused by *both* marker spellings (name and id): `reasons=protected-name|retire-marker-held`. The collision `el-director-orchestrator-1` is caught by `_base_protections` on the pane path too (`protected-name` -> successor branch takes `successor-close-held`), and `rule_violation()` returns `None` for it, so it gets no RULE line either — the two paths agree. **Allowlist — refusal precedes any subprocess.** 19 of 19 forbidden argv refused by `claude_allowed_argv` before `subprocess.run` is reached (`_run` raises first): `rm <id>`, `stop all`, `stop --all`, `stop -x`, `stop <id> --force`, `stop` alone, `agents` without `--json`, `agents --json --x`, `daemon stop`, `stop daemon`, `stop attach`, and injection shapes `stop ../../etc/passwd`, `stop 'x;rm -rf /'`, `stop '$(id)'`, `stop 'a b'`, `stop ''`, `stop <200 chars>`, `attach <id>`. Only `agents --json` and `stop <one id>` pass. **Read-only on every path.** AST scan of the module: no `os.kill`/`os.killpg`/`signal`/ `system`/`popen`/`rmtree`/`remove`/`rename`/`chmod`; the only `os.replace` is inside `atomic_write`, and the only `.unlink()` sites are the atomic-write temp file and the two retire-marker deletions that follow a *successful* close/stop. `Processes` only calls `iterdir`, `read_bytes`, `read_text`, `is_dir`. **Marker survives a failed stop.** `_execute_background`: `STOP_FAILED` leaves the marker (even with `--apply` and a valid activation file: `marker_exists=True`), `STOPPED` removes it (`marker_exists=False`); a failed delete becomes `STOPPED_MARKER_DELETE_FAILED` + `NOTICE: retire marker delete failed`. Revalidation reads a *fresh* roster and re-decides before the single write; a marker source that became unreadable yields `SKIP_CHANGED`, not a stop. **600-second stillness floor is identical to `origin/main`.** `screen_stable`, `successor_age`, `update_seen`, `text_hash`, `live_work`, `load_seen`, `save_seen`, `valid_first_seen`, `directory_gate`, `directory_successor_state` are byte-identical between `origin/main` and this head (extracted and diffed). A 36-cell differential over `min_successor_seconds` in {0,1,599,600,601,3600} x screen age in {0,1,599,600,601,5000} produced **zero** divergences. Defaults stay 600 (`Config`, argparse), and `--min-successor-seconds 0` and `--min-successor-seconds 599` still exit 2 with "refusing successor observation below 600 seconds". The pane-path diff is limited to `is_protected_name` replacing the narrower equality check (strictly more protective) and the report getting `SESSIONS`/`BACKGROUND`/`RULE`/`NOTICE` lines. **NOTICE discipline and the census.** Fabricated-run probes: a plain `REPORT` (no marker) -> 0 notices; a quiet refusal (`attach-client-present`, `attach-client-unidentified`, `status-busy`, `cwd-not-allowed`, `protected-name`) -> 0 notices; a real failure (`claude agents --json` raising, `claude stop` raising, an unreadable `/proc` cmdline, a failed pane read) -> exactly one notice naming the source, and `unreachable` increments (1, 1, 1). `unreachable` is an explicit sum over `claude-agents`, the attach scan, the retire dir, the roster, and every failed pane read, so it cannot collapse to a silent 0; a `claude` source that is not wired at all counts as unreachable too. ### Test count `python3 -m unittest discover -s tests -p 'test_herdr_janitor*.py'` -> **Ran 138 tests … OK** (0.40 s) at `bcb8ef0`. ### Dry-run (read-only, isolated state dir) `python3 scripts/herdr_janitor/janitor.py --dry-run --state-dir <mktemp>`: ``` SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0 BACKGROUND id=83e833d6 name=Intellimation SWOT and AI strategy state=blocked status=unknown process=gone cwd=/home/aaron/repos/intellimation action=REPORT reasons=cwd-not-allowed|process-gone|status-unknown|state-blocked|no-retire-marker BACKGROUND id=9e011d8b name=Ghosttly-in-the-shell Android port of rootshell state=done status=idle process=alive cwd=/home/aaron/repos/homelab action=REPORT reasons=no-retire-marker summary REPORT=9 total=9 directory_unreadable_refusals=0 ``` Zero `CLOSE_CANDIDATE`, zero `WOULD_CLOSE`, zero `WOULD_STOP`, zero `NOTICE:` lines, and the only filesystem effect was `report.txt` + `janitor.log` inside the isolated state dir. The live `el-director` pane reports `protected-name|focused-pane`. `analista-macro` is `directory-current`. **No real background session has been stopped by this code, ever:** the deployed `herdr-janitor.timer` copy at `~/.pi/agent/extensions/scripts/herdr_janitor/janitor.py` is the `origin/main` build (it prints no `SESSIONS`/`BACKGROUND` lines), so `claude stop` has never been issued from this repository. The first real stop will happen only after this lands, is deployed, and someone creates a retire marker. ### Trusted, not verified - `claude attach`'s documented rule "part of the session name works in place of the id" is taken from the ticket and the docstring; I did not run `claude attach` to re-derive which fragment forms the real CLI accepts. The guard is conservative against that rule, not derived from the binary. - `claude agents --json` field semantics (`kind`, `state`, `status`, `pid`, `cwd`) are trusted; the schema I exercised is the one the PR's own parser accepts. - I did not exercise `--apply` against a real session, and I did not create a real retire marker: every `STOPPED`/`STOP_FAILED` probe used a fabricated roster and a spy stop. ### Findings (none blocking) 1. **low / non-blocking — doc drift in `docs/herdr-janitor.md`.** The background-stop list says `status != "busy" and state == "done"`; the code requires exactly `status == "idle"` (stricter, `BACKGROUND_IDLE_STATUS`). The document understates the guard it describes. Fix with the next touch; it cannot cause a stop. 2. **low / non-blocking — the attach token is matched as a raw substring of the whole `cmdline`, so an unrelated process blocks every background stop for its lifetime.** A line whose argv merely contains `attach` and resolves to no known session — `tmux attach`, `herdr attach`, even `cat attach.log` — sets `attach-client-unidentified` and refuses *all* stops, not just one. That is the correct fail-closed direction and nothing was closed by it, but it means the feature can silently never fire on a host that has such a process. I verified this with a fabricated `cat attached.log`. Worth a follow-up ticket (scope the token to a `claude`/`attach` argv shape, staying fail-closed) rather than a block on this PR. 3. **low / non-blocking — residual spellings that defeat `name_key`.** Separator, case, accent, and the whole Unicode dash family are folded; these are not: `eldirector` (no separator at all), `el\u200bdirector` (zero-width space), `el\u00b7director` (middle dot), `\u0435l-director` (Cyrillic е), `el-direct0r`. A session deliberately named with a zero-width space would escape the protection. A marker is still required to reach a stop, and every spelling the ticket names is covered, so I do not block; the folding table deserves the next ticket. 4. **info — an unnamed background session can be stopped via `retire/<id>`** with no `protected-name` check, because there is no name to fold. Marker-by-id is explicit operator authority, so this is the documented contract, not a leak. 5. **info — the roster veto does not gate `claude stop`.** `directory-unreadable` / `directory-current` refuse *every pane close*, but `decide_background` never consults the `Directory`. The bounds on a background stop are marker + `kind` + protected name + liveness + `status`/`state` + cwd + attach scan. A background session running a numbered successor role could therefore be stopped by a marker without a roster cross-check; the marker is still explicit human authority. Worth stating in the doc if it is intended. ### Verdict APPROVE. Both fixes the earlier reviewers asked for are present at this head and survive an adversarial read: partial and quoted attach references, id prefixes of any resolvable length, `ATTACH`/`--debug`/wrapper forms and the whole unresolvable class all refuse, and the protected-name check now contains rather than equals on a separator-folded key so the `el-director-orchestrator-1` collision is protected on every path. The 600-second stillness floor that made the pane janitor safe is provably unchanged. CI is green on this exact head. The findings above are follow-ups, not blockers.

ld-reviewer approved these changes 2026-11-04 21:35:22 +00:00

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: bcb8ef0e02
session: homelab-2634-review-10
tier: premium

I reviewed head bcb8ef0e02cc2a8bb52914cad124947101a86717 (base origin/main = 945d97c) from the git object and a throwaway worktree, as an independent session. Verdict: APPROVE. Everything the head claims is fixed, is fixed; the attach guard refuses every form I could construct; the protected director family cannot be closed under any spelling I probed, on either path. I record three findings below; none of them stops a session that the operator's limits protect, and none is a merge blocker. F-A must be closed before the timer is ever activated.

No real background session has been stopped by me. Every probe used a fabricated process table, a fake claude, and a fake herdr; the only live work was one read-only dry run in an isolated state directory.

Verified, not trusted

• Attach guard (F1b claim). 20 fabricated /proc tables, each with one live kind=background, state=done, status=idle, allow-listed-cwd session carrying a retire marker, so that anything the guard misses reaches STOP_CANDIDATE. All 20 refused: full id, full name, 3-char name prefix, mid-name prefix, 3-char id prefix, 1-char id prefix (too short to match a name, so the line resolves to nobody and the ambiguity rule fires), quoted fragment, upper-case trick, sh -c wrapper, env FOO=1 wrapper, nohup, --debug before and after the subcommand, bare claude attach, flag-only claude attach --json, unknown name, name suffix only, glob token, upper-case-with-dashes. Seven of those are refused by attach-client-present (the line matched a fragment), thirteen by attach-client-unidentified (the line named nobody we know and every session was marked attached). Both directions of the refusal hold a stop for the cycle and leave the marker in place.
• Ambiguity in the unsafe direction. I tried to make the guard conclude "nothing attached" from a line it did not understand. I could not, with one exception (F-B). A token shorter than three characters, a flag-only line, a bare listing, an unrecognisable name, and a token that is not a substring of the name or id in either direction all set unidentified and refuse every session. A token split across argv cannot hide after exec (the words are rejoined in cmdline).
• Unreadable source ≠ quiet fleet. complete=False (an unreadable cmdline) marks every session attached and adds unreachable=1. claude agents --json failing, a failed pane read, an unreadable roster and an unreadable marker directory each add to unreachable and each emit their own NOTICE:; a normal refusal (no marker, protected name, live work, unstable screen, ambiguity) emits none. Verified on both the census line and the tail.
• Protected director family (F3 claim). Containment on the folded name holds for el-director, El-Director, EL_DIRECTOR, el director, El–Director (en dash), El—Director, el‐director/el‑director, el.director, el-director-orchestrator-1, el-director-orchestrator-12, ld-hypervisor-2, sendrast-director-x, analista-macro, Analista-Macro, jesus, jesus-voice, Jesús-director, and a padded " EL-DIRECTOR ". On the herdr-pane path those panes reach REPORT with protected-name, never CLOSE_CANDIDATE, even with a retire marker present and a 600-second-stable screen. On the background path the same names refuse with protected-name even when the retire marker is named after the agent id, so a marker cannot launder a protected session. Names outside the family (sup-gits-e1-5, gits-e0r-orchestrator-4, analista-ventas, epic-work) stay eligible, so the guard did not simply block everything.
• Verbs. claude accepts exactly agents --json and stop <one id>; refused: stop --all, stop all, stop daemon, stop stop, stop attach, stop <id> <id>, stop <id>;rm -rf /, stop $(id), stop <id> --force, rm <id>, daemon stop, logs <id>. herdr accepts exactly pane list, agent list, pane close <id>, agent read <id> --source recent-unwrapped --lines 240 --format text; refused: server stop, bare pane close, kill, agent prompt, upgrade, and a shortened agent read. There is no source line containing os.kill, signal., SIGTERM, SIGKILL, kill(, pkill, terminate(, rmtree, os.system, shell=True, or shutil. — so no path signals anything or writes anywhere but report.txt, janitor.log, seen.json and a marker unlink.
• Marker lifecycle. Stop fails → STOP_FAILED, marker kept, NOTICE: claude stop failed, no stop argv reached the fake (worst case: claude stop runs and fails, nothing else). Stop succeeds → STOPPED, marker deleted, exactly one stop <id> call. Same for panes: pane close raising → CLOSE_FAILED, marker kept, notice; success → CLOSED, marker gone.
• 600-second stillness floor is byte-identical to origin/main. screen_stable, successor_age, update_seen, LIVE_WORK_RES, the --min-successor-seconds parser default (600), Config.min_successor_seconds = 600, and main()'s if args.min_successor_seconds < 600 refusal are unchanged blocks. The decide/_base_protections diff touches only the protected-name call, and widens it.
• cwd containment uses Path parents, not string prefixes: /home/aaron/repos/homelab-fork and /home/aaron/repos/homelab/../intellimation are refused, as are /home/aaron/repos and "".
• Status gating refuses busy, working, empty, state=running and empty state; only exact idle + done goes on to the marker check.

Tests and CI

138 tests, all passing, with the repo's own command (python3 -m unittest discover -s tests): 32 + 58 + 48 across test_herdr_janitor.py, test_herdr_janitor_relief.py, test_herdr_janitor_sessions.py. CI on the exact head bcb8ef0e02cc2a8bb52914cad124947101a86717: run #270 (14 jobs, python, pr-title, and every gated package) and #271 (python) are both green. No red job on this head.

Dry run (read-only, isolated state dir)

SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0

Zero CLOSE_CANDIDATE, zero WOULD_STOP, 9 REPORT rows, no NOTICE:. The two background rows are Intellimation SWOT and AI strategy (foreign cwd, blocked, gone — refused four ways) and Ghosttly-in-the-shell Android port of rootshell (done/idle, alive, allow-listed cwd — held only by no-retire-marker, which is the documented authority model). The live panes named el-director (protected-name|focused-pane) and analista-macro (protected-name|directory-current) are refused on the pane path too. Only report.txt and janitor.log were written.

Findings

F-A — medium, not a merge blocker, must be fixed before activation. The background path never consults the operator's roster: decide_background has no Directory parameter, and directory_gate is only applied in decide. With --directory /nonexistent/directorio.md, the report prints directory=unreadable EVERY close is refused and every pane is refused with directory-unreadable, while the background row still reads action=WOULD_STOP reasons=retire-marker. That is false against docs/herdr-janitor.md ("directory-unreadable and directory-current block every close path") and against the header line printed into report.txt. This is the file's own stated principle — "a name protected on one path and closable on another is not protected at all" — applied to the roster instead of to a name. The absolute limits still hold on that path (the director family is protected by name, and a marker is still required), which is why I do not block the merge; but the timer must not be enabled while a stale or renamed roster silently stops nothing on one path and everything on the other. Suggested fix: thread directory into decide_background, refuse on unreadable, and refuse a session the roster names as its role's current session, exactly as directory_gate does.

F-B — low. Processes.attachments exempts a line whose pid is the session's own pid or its direct parent, and does so before the ambiguity test: if any(pid in pids for pids in own.values()): continue. An attach-bearing line sitting at either pid therefore neither marks a session attached nor sets unidentified. My probe: one session (pid 4001, ppid 4000) and the line claude attach <id> at pid 4000 → STOP_CANDIDATE. test_the_sessions_own_process_still_never_sets_the_flag asserts this deliberately (with --attach-capable in the parent's argv). Exempting the daemon's own pid is necessary — its argv may legitimately contain the token — but the parent exemption is not: a parent whose argv says claude attach <id> is the attach client, and the only way to reach it is a pane shell that execs into the attach client (or a recycled ppid). Narrow, pre-existing, and given the operator's "never disturb an attach client" absolute I would tighten it: keep the own-pid exemption, and for the parent build the same reference test before exempting it, or set unidentified when an exempt line references the session by fragment.

F-C — low, informational. The retire-marker namespace is shared between the two paths with no namespace separation: a marker named after a herdr role (retire/sup-gits-e1-5) also authorises a claude stop of a background session that happens to carry the same name. My probe: STOP_CANDIDATE. This is documented ("a marker at retire/<name> or retire/<id>") and names are unique in the fleet today, so it is a note rather than a defect; a <kind>- prefix on marker names would remove the coupling.

Nits. docs/herdr-janitor.md states the stop condition as status != "busy" and state == "done"; the code requires exactly status == "idle". The code is the stricter of the two, so the doc understates the guard. Separately, the background path has no 600-second stillness floor (only done+idle); documented as such, and claude stop keeps the conversation resumable, so the blast radius of a wrong stop here is small — but it is a real asymmetry with the pane path worth stating in "What this cannot prove".

What I could not prove

The real semantics of claude attach's partial-name resolution — I tested the code against the documented claim, never against the real binary, and I deliberately did not attach to any live session. Nothing has been run against a real attach client, so the guard's behaviour on a genuine one is inferred from fabricated command lines, not observed. And no live background stop has been executed by this head or by me.

## Verdict: APPROVED model: omniroute/deepseek-flash head: bcb8ef0e02cc2a8bb52914cad124947101a86717 session: homelab-2634-review-10 tier: premium I reviewed head `bcb8ef0e02cc2a8bb52914cad124947101a86717` (base `origin/main` = `945d97c`) from the git object and a throwaway worktree, as an independent session. Verdict: **APPROVE**. Everything the head claims is fixed, is fixed; the attach guard refuses every form I could construct; the protected director family cannot be closed under any spelling I probed, on either path. I record three findings below; none of them stops a session that the operator's limits protect, and none is a merge blocker. F-A must be closed before the timer is ever activated. No real background session has been stopped by me. Every probe used a fabricated process table, a fake `claude`, and a fake `herdr`; the only live work was one read-only dry run in an isolated state directory. ## Verified, not trusted - **Attach guard (F1b claim).** 20 fabricated `/proc` tables, each with one live `kind=background`, `state=done`, `status=idle`, allow-listed-cwd session carrying a retire marker, so that anything the guard misses reaches `STOP_CANDIDATE`. All 20 refused: full id, full name, 3-char name prefix, mid-name prefix, 3-char id prefix, 1-char id prefix (too short to match a name, so the line resolves to nobody and the ambiguity rule fires), quoted fragment, upper-case trick, `sh -c` wrapper, `env FOO=1` wrapper, `nohup`, `--debug` before and after the subcommand, bare `claude attach`, flag-only `claude attach --json`, unknown name, name suffix only, glob token, upper-case-with-dashes. Seven of those are refused by `attach-client-present` (the line matched a fragment), thirteen by `attach-client-unidentified` (the line named nobody we know and every session was marked attached). Both directions of the refusal hold a stop for the cycle and leave the marker in place. - **Ambiguity in the unsafe direction.** I tried to make the guard conclude "nothing attached" from a line it did not understand. I could not, with one exception (F-B). A token shorter than three characters, a flag-only line, a bare listing, an unrecognisable name, and a token that is not a substring of the name or id in either direction all set `unidentified` and refuse every session. A token split across argv cannot hide after `exec` (the words are rejoined in `cmdline`). - **Unreadable source ≠ quiet fleet.** `complete=False` (an unreadable `cmdline`) marks every session attached and adds `unreachable=1`. `claude agents --json` failing, a failed pane read, an unreadable roster and an unreadable marker directory each add to `unreachable` and each emit their own `NOTICE:`; a normal refusal (no marker, protected name, live work, unstable screen, ambiguity) emits none. Verified on both the census line and the tail. - **Protected director family (F3 claim).** Containment on the folded name holds for `el-director`, `El-Director`, `EL_DIRECTOR`, `el director`, `El–Director` (en dash), `El—Director`, `el‐director`/`el‑director`, `el.director`, `el-director-orchestrator-1`, `el-director-orchestrator-12`, `ld-hypervisor-2`, `sendrast-director-x`, `analista-macro`, `Analista-Macro`, `jesus`, `jesus-voice`, `Jesús-director`, and a padded `" EL-DIRECTOR "`. On the herdr-pane path those panes reach `REPORT` with `protected-name`, never `CLOSE_CANDIDATE`, even with a retire marker present and a 600-second-stable screen. On the background path the same names refuse with `protected-name` even when the retire marker is named after the **agent id**, so a marker cannot launder a protected session. Names outside the family (`sup-gits-e1-5`, `gits-e0r-orchestrator-4`, `analista-ventas`, `epic-work`) stay eligible, so the guard did not simply block everything. - **Verbs.** `claude` accepts exactly `agents --json` and `stop <one id>`; refused: `stop --all`, `stop all`, `stop daemon`, `stop stop`, `stop attach`, `stop <id> <id>`, `stop <id>;rm -rf /`, `stop $(id)`, `stop <id> --force`, `rm <id>`, `daemon stop`, `logs <id>`. `herdr` accepts exactly `pane list`, `agent list`, `pane close <id>`, `agent read <id> --source recent-unwrapped --lines 240 --format text`; refused: `server stop`, bare `pane close`, `kill`, `agent prompt`, `upgrade`, and a shortened `agent read`. There is no source line containing `os.kill`, `signal.`, `SIGTERM`, `SIGKILL`, `kill(`, `pkill`, `terminate(`, `rmtree`, `os.system`, `shell=True`, or `shutil.` — so no path signals anything or writes anywhere but `report.txt`, `janitor.log`, `seen.json` and a marker unlink. - **Marker lifecycle.** Stop fails → `STOP_FAILED`, marker kept, `NOTICE: claude stop failed`, no `stop` argv reached the fake (worst case: `claude stop` runs and fails, nothing else). Stop succeeds → `STOPPED`, marker deleted, exactly one `stop <id>` call. Same for panes: `pane close` raising → `CLOSE_FAILED`, marker kept, notice; success → `CLOSED`, marker gone. - **600-second stillness floor is byte-identical to `origin/main`.** `screen_stable`, `successor_age`, `update_seen`, `LIVE_WORK_RES`, the `--min-successor-seconds` parser default (600), `Config.min_successor_seconds = 600`, and `main()`'s `if args.min_successor_seconds < 600` refusal are unchanged blocks. The `decide`/`_base_protections` diff touches only the protected-name call, and widens it. - **cwd containment** uses `Path` parents, not string prefixes: `/home/aaron/repos/homelab-fork` and `/home/aaron/repos/homelab/../intellimation` are refused, as are `/home/aaron/repos` and `""`. - **Status gating** refuses `busy`, `working`, empty, `state=running` and empty state; only exact `idle` + `done` goes on to the marker check. ## Tests and CI 138 tests, all passing, with the repo's own command (`python3 -m unittest discover -s tests`): 32 + 58 + 48 across `test_herdr_janitor.py`, `test_herdr_janitor_relief.py`, `test_herdr_janitor_sessions.py`. CI on the exact head `bcb8ef0e02cc2a8bb52914cad124947101a86717`: run #270 (14 jobs, `python`, `pr-title`, and every gated package) and #271 (`python`) are both green. No red job on this head. ## Dry run (read-only, isolated state dir) `SESSIONS herdr=9 named=5 background=2 background-alive=1 unreachable=0` Zero `CLOSE_CANDIDATE`, zero `WOULD_STOP`, 9 `REPORT` rows, no `NOTICE:`. The two background rows are `Intellimation SWOT and AI strategy` (foreign cwd, blocked, gone — refused four ways) and `Ghosttly-in-the-shell Android port of rootshell` (`done`/`idle`, alive, allow-listed cwd — held only by `no-retire-marker`, which is the documented authority model). The live panes named `el-director` (`protected-name|focused-pane`) and `analista-macro` (`protected-name|directory-current`) are refused on the pane path too. Only `report.txt` and `janitor.log` were written. ## Findings **F-A — medium, not a merge blocker, must be fixed before activation.** The background path never consults the operator's roster: `decide_background` has no `Directory` parameter, and `directory_gate` is only applied in `decide`. With `--directory /nonexistent/directorio.md`, the report prints `directory=unreadable EVERY close is refused` and every *pane* is refused with `directory-unreadable`, while the background row still reads `action=WOULD_STOP reasons=retire-marker`. That is false against `docs/herdr-janitor.md` ("`directory-unreadable` and `directory-current` block **every** close path") and against the header line printed into `report.txt`. This is the file's own stated principle — "a name protected on one path and closable on another is not protected at all" — applied to the roster instead of to a name. The absolute limits still hold on that path (the director family is protected by name, and a marker is still required), which is why I do not block the merge; but the timer must not be enabled while a stale or renamed roster silently stops nothing on one path and everything on the other. Suggested fix: thread `directory` into `decide_background`, refuse on `unreadable`, and refuse a session the roster names as its role's current session, exactly as `directory_gate` does. **F-B — low.** `Processes.attachments` exempts a line whose pid is the session's own pid **or its direct parent**, and does so before the ambiguity test: `if any(pid in pids for pids in own.values()): continue`. An attach-bearing line sitting at either pid therefore neither marks a session attached nor sets `unidentified`. My probe: one session (pid 4001, ppid 4000) and the line `claude attach <id>` at pid 4000 → `STOP_CANDIDATE`. `test_the_sessions_own_process_still_never_sets_the_flag` asserts this deliberately (with `--attach-capable` in the parent's argv). Exempting the daemon's own pid is necessary — its argv may legitimately contain the token — but the parent exemption is not: a parent whose argv says `claude attach <id>` *is* the attach client, and the only way to reach it is a pane shell that `exec`s into the attach client (or a recycled ppid). Narrow, pre-existing, and given the operator's "never disturb an attach client" absolute I would tighten it: keep the own-pid exemption, and for the parent build the same reference test before exempting it, or set `unidentified` when an exempt line references the session by fragment. **F-C — low, informational.** The retire-marker namespace is shared between the two paths with no namespace separation: a marker named after a herdr role (`retire/sup-gits-e1-5`) also authorises a `claude stop` of a *background* session that happens to carry the same name. My probe: `STOP_CANDIDATE`. This is documented ("a marker at `retire/<name>` or `retire/<id>`") and names are unique in the fleet today, so it is a note rather than a defect; a `<kind>-` prefix on marker names would remove the coupling. **Nits.** `docs/herdr-janitor.md` states the stop condition as `status != "busy" and state == "done"`; the code requires exactly `status == "idle"`. The code is the stricter of the two, so the doc understates the guard. Separately, the background path has no 600-second stillness floor (only `done`+`idle`); documented as such, and `claude stop` keeps the conversation resumable, so the blast radius of a wrong stop here is small — but it is a real asymmetry with the pane path worth stating in "What this cannot prove". ## What I could not prove The real semantics of `claude attach`'s partial-name resolution — I tested the code against the documented claim, never against the real binary, and I deliberately did not attach to any live session. Nothing has been run against a real attach client, so the guard's behaviour on a genuine one is inferred from fabricated command lines, not observed. And no live background stop has been executed by this head or by me.

pi-landing merged commit bc4e0c6487 into main 2026-11-04 21:36:54 +00:00

pi-landing referenced this pull request from a commit 2026-11-04 21:36:56 +00:00

Merge pull request 'HOMELAB-2634: feat(herdr): clean up background sessions, count sessions, notice only failures' (#94) from plane/HOMELAB-2634-janitor-background into main

Sign in to join this conversation.

Reviewers

No reviewers

ld-reviewer

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!94

No description provided.