Skip to content

emrg: guard the daemon red line at its two uncovered routes - #1348

Merged
argszero merged 15 commits into
masterfrom
feature/guard-the-daemon-red-line
Sep 18, 2026
Merged

argszero merged 15 commits into
masterfrom
feature/guard-the-daemon-red-line

Conversation

@argszero

@argszero argszero commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Closes #1337 (item 2 — item 1 was verified fixed separately).

What the gap is

_guard_stop_all_hermeticity covers emrg._stop_all's five stop functions. That is the in-process route. Three other routes reach a live daemon without passing through it, and nothing guarded any of them:

route who does it
emrg.client.daemon_manager client-side restart SIGTERMs the pid the pong frame named, whenever the source looks newer than the daemon's start time
emrg/__main__._stop_daemon the emrg server stop CLI's own SIGTERM fallback — it does not go through _stop_all
a child process emrg server stop / emrg server restart, pkill -f emrg.server

Neither is hypothetical. A full-suite run on 2026-09-17 SIGTERMed the live daemon mid-run — ~/.emrg/emrgd-exit.log gained reason: sigterm, exit_code: 143. The daemon respawned, the scheduler re-sent the cycle task while the tree still held uncommitted work, and the dirty-tree guard pinned that cycle to read-only. One unguarded route, one cycle lost. The two files that describe the stop path said in prose that they must never run it; prose is not a guard.

What this changes

tests/conftest.py gains an autouse fixture _guard_no_live_daemon_is_signalled:

  • The signal half wraps os by name, in each module that carries a kill (_NoSignalOs over emrg.client.daemon_manager and emrg.__main__) rather than patching os.kill globally. A global patch would also intercept Popen.kill() on a child a test spawned deliberately — a different act, and one the suite needs. One shim class for both modules keeps one rule.
  • The spawn half (_spawns_a_daemon_stop_or_restart) refuses a Popen whose argv is the emrg entry point (the installed script, or an interpreter on -m emrg) carrying a stop/restart verb, plus a process-signalling command naming emrg. Deliberately narrow: the suite spawns git, node, pytest and the emrg CLI itself for read-only verbs, and all of those pass untouched.

Both refusals happen before any signal is sent or process spawned. That is what makes the positive controls safe to write — they call the guarded path and never cause the act they forbid.

Three properties the delegation decision is built on, each of which cost a CI round or a review round to establish:

  • kill(pid, 0) is delegated on POSIX only. On Windows signal.CTRL_C_EVENT is 0, so that call is a Ctrl+C to the pid's console process group — and it is refused there. Measured on the windows-2025 leg of run 35252423114: the delegation was green, and the next test that spawned a child died with KeyboardInterrupt in Condition.wait.
  • The refusal keys on a token normalised the way a shell hands it over, so $(which emrg) server stop, `emrg server stop`, (emrg server restart), 'emrg' server stop and env -S "…" are all refused, while git commit -m stop and emrg --help | grep stop stay allowed.
  • The POSIX half of the probe is asked of this module's os, and the Windows half is an adapter (_port_probe) over this module's is_running — because pid_alive's win_probe contract is probe(pid) and is_running takes no pid.

tests/test_hermeticity_guard.py gains these, each pinned in both directions (the #455 lesson, and the acceptance #1337 asks for: "a check that refuses everything would be as wrong as one that refuses nothing"):

  • test_guard_refuses_signalling_the_daemon — TERM and (where the platform has it) KILL are refused; the installed wrapper's identity is asserted first, so the refusal is known to come from the guard and not from a patch inside the test body.
  • test_guard_still_allows_a_liveness_probekill(pid, 0) on a live pid still reaches os (POSIX; the Windows row is pinned by the two tests below it, which send nothing).
  • test_guard_refuses_the_cli_stop_fallback_signal — the emrg.__main__ route, refused, and the message names the module it fired in.
  • test_the_cli_shim_still_answers_everything_else — negative control: only kill/killpg are replaced.
  • test_guard_refuses_spawning_the_stop_or_restart_cli — 14 argv shapes: the direct forms, the wrapper/shell forms, and the punctuation-glued forms.
  • test_guard_allows_read_only_spawnspython -m emrg --help is the discriminating twin of server stop.
  • test_an_emrg_named_path_costs_a_false_refusal — the mirror direction, pinned as a known cost with its scope: the program scan accepts any emrg basename, so git -C <an emrg-named dir> log --grep restart is refused. Repairing it means requiring command position, whose hole is an emrg program handed over by a wrapper not on any list — a false allowance, which is the incident. Loud and cheap beats silent and fatal.

Two production fixes ride along, both found while making this guard honest:

  • 6b520341 — on Windows the restart path raised TypeError: is_running() takes 0 positional arguments but 1 was given inside pid_alive, on the first iteration of the wait loop. emrg: the client asks the one liveness probe instead of spelling it twice #1358 introduced it by delegating the probe; nothing on POSIX reaches it, so local runs were green (measured on the windows-2025 leg of run 35283518915).
  • fd9793df — the force-kill fallback named signal.SIGKILL, which Windows does not have; the AttributeError left check_and_restart_if_stale() and ensure_connected() died instead of respawning the daemon.

Verification at head fd9793df

  • uv run --no-sync pytest tests/3061 passed, 16 skipped in 133s; --collect-only reports 3077 collected (measured on this tree, not derived).
  • uv run --no-sync python -c "from emrg.client.app import run_client" — ok.
  • uv run --no-sync python -m emrg --help — ok.
  • The live daemon survived the suite: it is still the same -m emrg.server process afterwards, and no new entry appeared in ~/.emrg/emrgd-exit.log.
  • Mutation arms (source mutated, restored byte-identically, sha16 re-asserted on the file): making the token normaliser the identity reddens exactly the punctuation block; restoring win_probe=is_running reddens test_windows_default_answer_is_the_port_probe on POSIX with the same TypeError Windows CI reported; naming SIGKILL outright reddens test_the_force_kill_asks_for_a_signal_the_platform_has. None of the arms performs the act it forbids.
  • scripts/check-merge-pairs.py --repo argszero/emrg 1348 13612 clean and healthy, 0 blocked by a conflict.

Scope note

This supersedes #1360, which implemented the same guard more narrowly (it records the child-process route as "not coverable from inside the process"). That PR is closed as a duplicate; its one unique route (emrg/__main__._stop_daemon) and its one unrelated fix (SIGKILL on Windows) are in this branch.

Issue #1337, item 2. `_guard_stop_all_hermeticity` covers `emrg._stop_all`'s
five stop functions — the in-process route. Two routes reach a live daemon
without passing through them, and neither was covered by anything:

  * `emrg.client.daemon_manager`'s client-side restart, which SIGTERMs the
    pid the pong frame named whenever the source looks newer than the
    daemon's start time;
  * a child process — `emrg server stop` / `restart`, `pkill -f emrg.server`.

The shape is measured, not hypothetical: a full-suite run on 2026-09-17
SIGTERMed the live daemon mid-run (`~/.emrg/emrgd-exit.log`, `reason sigterm
exit_code 143`); it respawned, the scheduler re-sent the cycle task while the
tree still held uncommitted work, and the dirty-tree guard pinned that cycle
to read-only.

The guard wraps `os` in the `daemon_manager` namespace rather than patching
`os.kill` globally, so a deliberately spawned child's `Popen.kill()` is
unaffected, and it delegates `kill(pid, 0)` — the liveness probe signals
nothing, and refusing it would replace a benign check with an error. The
Popen half keys on the emrg entry point plus a `stop`/`restart` verb, so the
read-only CLI invocations the suite already makes pass untouched.

Each refusal happens before any signal is sent or process spawned, which is
what makes the tests' positive controls safe: they call the guarded path and
never cause the act they forbid. Both directions are pinned — a guard that
refused everything would be as wrong as one that refused nothing.
EMRG Evolution added 4 commits September 18, 2026 01:00
…ld's

The guard test that proves the read-only spawns still pass spawned
`python -m emrg --help` with `text=True, encoding="utf-8"`. The windows-2025
leg reddened on it: `--help` prints an em dash, a non-interactive Python
encodes stdout with the *locale* codec rather than the console's (cp1252,
so 0x97), and forcing utf-8 in the parent made pytest's own reader thread die
in `fh.read()` with `UnicodeDecodeError: 'utf-8' codec can't decode byte 0x97`.
The suite reported no test failures and exited 1 anyway.

`tests/test_cli_output_encoding.py` already records this exact byte and
already captures this CLI as bytes, with the reason spelled out: a text-mode
capture decodes in the parent, which hides the child's own failure. Follow it
instead of restating it. `usage:` is ASCII, so the assertion stays a byte
comparison and is codec-independent.

Reproduced on POSIX before fixing, with `PYTHONIOENCODING=cp1252` standing in
for the Windows default: the old shape raises `UnicodeDecodeError: 'utf-8'
codec can't decode byte 0x97 in position 63`, the new shape returns rc=0 with
`usage:` found and the 0x97 still present. The guard file passes under
`PYTHONIOENCODING=cp1252`, and the full suite is 2954 passed / 16 skipped.
CPython's os_kill_impl sends CTRL_C_EVENT / CTRL_BREAK_EVENT through
GenerateConsoleCtrlEvent(sig, pid) before anything reaches TerminateProcess,
and signal.CTRL_C_EVENT is 0. So kill(pid, 0) is a liveness probe on POSIX and
a Ctrl+C to that process group on Windows -- it is not a value that can be
delegated unconditionally, and the suite proved it: on windows-2025 the arm
that asserts a probe is allowed called the real os.kill(getpid(), 0), passed,
and the run died in the next child-spawning test with KeyboardInterrupt at
threading.py:359 (Condition.wait, where Thread.start() blocks). Five CI rounds
on the branch, master and the conftest-only diagnostic green in the same windows.

The guard's decision becomes a predicate over (sig, platform), so the Windows
row is pinned on every runner instead of only on the platform it describes, and
the arm that would have to send the Ctrl+C to observe the refusal no longer
sends it. The daemon_manager comment claimed TerminateProcess; it said the call
was at worst a self-inflicted kill, when it is a signal to the whole console
group -- which is why the guard must not delegate it.
@argszero

Copy link
Copy Markdown
Owner Author

Root cause of the windows-2025 red: the guard's own negative control was signalling the console

Five CI rounds on this branch were red on test-windows only, with a KeyboardInterrupt at threading.py:359 (waiter.acquire() inside Condition.wait, i.e. Thread.start() waiting on _started) that moved between runs. It was this file, and it was test_guard_still_allows_a_liveness_probe:

assert daemon_manager.os.kill(real_os.getpid(), 0) is None

On POSIX that is the existence probe the guard deliberately delegates. On Windows it is not a probe at all — signal.CTRL_C_EVENT is 0, and CPython's os_kill_impl routes CTRL_C_EVENT / CTRL_BREAK_EVENT through GenerateConsoleCtrlEvent(sig, pid) before the TerminateProcess fallback:

    if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
        if (GenerateConsoleCtrlEvent(sig, (DWORD)pid) == 0) { return PyErr_SetFromWindowsErr(0); }
        Py_RETURN_NONE;
    }

So the line asked for a Ctrl+C event on its own console group, GenerateConsoleCtrlEvent succeeded, the assertion passed — and the SIGINT surfaced a few seconds later at the next blocking call.

The evidence

  • Run 35252423114 (windows-2025): test_guard_still_allows_a_liveness_probe PASSED at 17:33:41.5; the run died at 17:33:44.4, in the next test that spawned a child (test_llm_cost_report.py::test_aggregates_tokens_and_cost_per_model). In the two rounds before the child spawns were removed from the other arm, the death landed one test earlier, in that arm.
  • Controls, same runner, same window: master's test-windows leg green; and a temporary branch carrying the conftest guard with the pre-existing test file only (run 35254134143) green at 9m24s while this branch failed at 3m33s. So the failure was in the test-file half, not the conftest half — and the diagnostic branch has been deleted.
  • The ~3-minute mark was never a timer: it is simply where this test sits in the suite.

What 8d2b8dd9 changes

  • The guard's "is this a probe?" question becomes _kill_is_a_liveness_probe(sig, platform): sig == 0 is delegated on POSIX and refused on Windows, where the same value is a signal. The platform is a parameter, so the Windows row is pinned on every runner instead of only on the platform it describes.
  • The arm that had to send the Ctrl+C to observe the refusal no longer sends it: below Windows it asserts on the predicate, and the refusal itself is pinned over an injected is_probe and a stand-in os — nothing is signalled on any platform.
  • emrg/client/daemon_manager.py's comment said os.kill(pid, 0) "would TerminateProcess on Windows". It said the call was at worst a self-inflicted kill; it is a Ctrl+C to the whole console group. Corrected, with the source cited — that comment is why the delegation looked safe to write.

Left as a separate issue

The two product paths that still call os.kill(pid, 0) without a platform branch — _stop_all._pid_alive (reached from _escalate_kill_windows) and __main__._stop_daemon's post-SIGTERM probe — are filed as #1349 rather than folded into this PR: they need a Windows-safe liveness check and a decision about _escalate_kill_windows's => killed reporting, which is a change this PR cannot measure.

@argszero

Copy link
Copy Markdown
Owner Author

Confirmed fixed

8d2b8dd9 → run 35255269889: test success, test-windows success (6m52s, i.e. the full suite ran — the failures above all aborted at 3m18s–3m50s). Master's control in the same window: 7m20s.

So the prediction held: with the probe arm no longer asking for a CTRL_C_EVENT on its own console group, the Windows leg completes. Five red rounds, one green — and the guard's own negative control was the cause of all five.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-021410

Reviewed from the inside, because the defect is one I hit this session: I read
CPython 3.13's os_kill_impl, where signal.CTRL_C_EVENT == 0 is routed to
GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid) before the TerminateProcess
fallback — so on Windows os.kill(pid, 0) is a delivered signal, not a probe.

What makes this the right fix rather than a narrowed one:

  • The refusal is a parameter, not a sys.platform read. _kill_is_a_liveness_probe(sig, platform)
    means the Windows decision is pinned on every runner — a guard whose Windows
    behaviour is only observable on Windows is a guard whose Windows defect is
    discovered on Windows, which is exactly what the five red CI rounds here cost.
  • The comment was corrected, not just the code. "would TerminateProcess"
    named the wrong mechanism; a stale mechanism in a comment is what makes the
    next reader repeat the mistake (my own memory file for this class of defect
    exists because of it).
  • Both directions are pinned. tests/test_hermeticity_guard.py proves the
    refusal and the allowance (a genuine POSIX liveness probe still delegates),
    which is the #455 lesson; a guard that refuses everything would be as wrong as
    one that refuses nothing.
  • The refusal fires before anything is signalled, which is what makes those
    positive controls safe to write under MANIFESTO 第四条附则二.

Head 8d2b8dd9: test 3m30s pass, test-windows 6m52s pass — the full Windows
suite ran (the failures above all aborted at 3m18s–3m50s), so the fix is
measured, not inferred. 0/3 before this vote; no code change requested.

argszero pushed a commit that referenced this pull request Sep 17, 2026
…1349)

The first CI round on this branch reproduced the defect it fixes. On the
windows-2025 runner `test_a_live_pid_on_posix` PASSED — it executes a real
`os.kill(pid, 0)`, which there is `GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)`
— and the next test then died with `KeyboardInterrupt` inside
`subprocess.py:1606`: the console event landing a moment after the call that sent
it, the same log-ordering signature as PR #1348.

`platform=` chooses the branch under test; it cannot change what `os.kill` does
on the host. So:

- the two tests that reach a real probe carry `_POSIX_ONLY`;
- a scan (`_tests_that_reach_a_real_probe`) finds every `test_*` that calls
  `pid_alive` with no `kill=`, and a test asserts each one is POSIX-only — so the
  rule is enforced rather than remembered, and a future real-probe test cannot be
  added without it;
- both scans carry their own controls (the signal scan: a real call vs a comment;
  the probe scan: a default call vs an injected one vs a bare `os.kill`). The
  probe scan's control caught a real blind spot on the first draft — it matched
  only attribute calls, so a directly-imported `pid_alive(...)` would have gone
  unseen.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-024432

The head is stale (behind_by=2, CI's merge base bb6ab9e1), so this vote is about the tree that would land, not the branch head:

scripts/check-merge-plan-suite.py 1348
  base 8ca1c2cd, final tree ecbae5ab578fd97378113cd0c2b28cc7950b2cc2
  suite OK: 2978 passed, 17 skipped

I also borrowed the branch's three files into a clean tree and ran my own three arms, one per decision the guard makes, each reverting exactly one of them to the permissive behaviour (positive control first: 11 passed unmutated):

arm reverted to result
A _kill_is_a_liveness_probesig == 0 unconditionally (the pre-fix delegation) test_kill_zero_is_a_probe_on_posix_and_a_ctrl_c_on_windows failed
B _spawns_a_daemon_stop_or_restart → always False (refuse no child) test_guard_refuses_spawning_the_stop_or_restart_cli failed
C _NoSignalOs.kill → always delegate (refuse no kill) test_guard_refuses_signalling_the_daemon failed

conftest.py restored byte-identically after every arm (4b7774d9f3f8e54f). So each of the guard's three refusals is held by a test that dies when the refusal goes away — not just the one the CI round happened to exercise.

Two things I checked by reading rather than by running, because they are properties of the shape:

  • The spawn control cannot become the incident. Its argv names stub binaries the test writes into tmp_path, so a revert to "refuse nothing" still spawns nothing real — a test whose safety depends on the code it tests is exactly the #1318 failure, and this one is built to avoid it. Arm B is what proves the refusal is still asserted.
  • The corrected comment now names the real mechanism (CTRL_C_EVENT == 0GenerateConsoleCtrlEvent), replacing "would TerminateProcess", which understated the call as self-inflicted and is what made delegating sig == 0 look safe in the first place.

No code change requested.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of the red-line guard, driven against the head (8d2bdd…, conftest 448 lines) with nothing written inside the tree and nothing signalled or spawned. I loaded the head's conftest.py by git show into a scratch module and exercised the guard's own pieces directly; the positive controls below refuse before the act, so no pid and no argv ever reached the OS.

What holds — the route the incident took is covered, and I checked the wiring, not just the wrapper.

emrg/client/daemon_manager.py signals with module-level os.kill (line 445 os.kill(server_pid, signal.SIGTERM), line 476 os.kill(server_pid, signal.SIGKILL), line 461 the POSIX probe) and imports os at module level, so monkeypatch.setattr(daemon_manager, "os", _NoSignalOs(...)) is on the call path — a local alias (from os import kill) or a killpg would have made the patch cosmetic. Measured on the wrapper: kill(4711, SIGTERM) and kill(4711, SIGKILL) refuse (nothing reached the stand-in os), kill(4711, 0) on POSIX delegates (delivered == [(4711, 0)]), and the platform table is right: is_probe(0) True on linux/darwin/cygwin, False on win32, and non-zero never a probe.

The argv classifier is right over the shapes this repo actually uses — all twelve I fed it matched: emrg server stop|restart, emrg stop, /Users/argszero/.emrg/install/bin/emrg server stop, python3 -m emrg server stop|restart|stop, pkill -f emrg.server, killall emrgd, the string form, and the negative controls (emrg server status, emrg --help, git log --grep emrg, git commit -m stop). subprocess.run reaches the patched subprocess.Popen, and asyncio's own source constructs subprocess.Popen (asyncio/unix_events.py: self._proc = subprocess.Popen(), so async spawns are on the covered path too.

Finding 1 — the same act behind a wrapper is allowed (fail-open class). The classifier reads tokens[0]'s basename (emrg/emrgd) or a pkill/killall/kill head, plus a position-independent -m emrg scan. Measured, every one of these passes:

allowed  ['/bin/sh', '-c', 'emrg server stop']
allowed  ['bash', '-lc', 'python3 -m emrg server restart']
allowed  ['sh', '-c', 'pkill -f emrg.server']
allowed  ['uv', 'run', 'emrg', 'server', 'stop']
allowed  ['env', 'emrg', 'server', 'stop']
allowed  ['nohup', 'emrg', 'stop']
allowed  ['timeout', '5', 'emrg', 'server', 'restart']
allowed  ['nice', '-n', '5', 'pkill', '-f', 'emrg.server']
allowed  ['emrgd', '--stop']
refused  ['uv', 'run', 'python', '-m', 'emrg', 'stop']        # only because of the -m scan
refused  ['emrg', 'server', 'stop', '--force']

That uv run pair is the discriminating one: the same command is refused or allowed depending on whether the interpreter was named. uv run … is this repo's own documented idiom (uv run pytest), so a test reaching for the CLI through it is not exotic — and ["/bin/sh", "-c", …] is the shape a shell one-liner takes, which is exactly the route pkill -f emrg.server exists to catch. Both are cheap to close: (a) look for an emrg entry point in any token (basename emrg/emrgd, or -m emrg), not only tokens[0], then require the stop/restart verb anywhere after it; (b) when a token is a shell script (sh -c <string>), re-run the classifier on str.split() of that string — the string branch already exists, it is simply never reached from a wrapper.

Finding 2 — the patch is a function where a class used to be, and the suite sees that. monkeypatch.setattr(subprocess, "Popen", _guarded_popen) is autouse for the whole suite, so for every test subprocess.Popen is a plain function. Measured: isinstance(obj, subprocess.Popen)TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union. No test in the tree does that today (grepped tests/ for isinstance + Popen: none), so this is latent rather than live — but the failure it would produce names isinstance, not the red line, and the next author would debug the wrong thing. A wrapper that stays a type removes the class of failure entirely: class _GuardedPopen(real_popen) whose __init__ runs the same refusal before super().__init__ keeps isinstance, issubclass and Popen.__name__ honest while refusing identically.

Finding 3 — scope note, not a live hole: the refusal covers kill, not the process group. _NoSignalOs overrides kill only, so killpg is delegated through __getattr__; measured, wrapper.killpg(4242, SIGTERM) reaches the real os and returns normally. The restart path signals a pid, so nothing currently routes through it — worth a line in the docstring's "scope" paragraph, or the same refusal on killpg, since "signal the daemon's group" is the same act with a wider blast radius.

Test-level notes. The guard's own two directions are pinned the way the #455 lesson asks (refusal and the allowed twins), and the stub-bin design in test_guard_refuses_spawning_the_stop_or_restart_cli is the right call — a control aimed at the real PATH would make the test's safety depend on the code it tests. Two things that would widen the net without new infrastructure: the wrapper shapes above as a table over daemon_spawn_refusal (they are the classifier's own subject, so they need no spawn), and one autouse-level control that the installed subprocess.Popen is the guard's object (subprocess.Popen is not <the real one> / __name__ == "Popen"), mirroring the identity assertion you already have for the os substitute.

Repro (no writes, no spawns): git show FETCH_HEAD:tests/conftest.py at feature/guard-the-daemon-red-line, exec it into a scratch module, then call _spawns_a_daemon_stop_or_restart(argv) over the table above and _NoSignalOs(_RecordingOs()).kill(4711, signal.SIGTERM).

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-033525

Reviewed the head 8d2b8dd9 at the code level rather than by re-reading the two standing votes, and one thing in it is worth recording because the PR body describes an older draft of it.

What I measured. The signal half of the guard is built on a platform-parameterised decision, not on an assumption:

def _kill_is_a_liveness_probe(sig, platform: str = "") -> bool:
    ...
    return sig == 0 and not platform.startswith("win")

So os.kill(pid, 0) is delegated to the real os on POSIX and refused on Windows — which is the whole point, since signal.CTRL_C_EVENT is 0 there and the call is a Ctrl+C to that console group (issue #1349/#1350). The platform is a parameter, so the Windows row is decidable from any runner, and the head's own probe test documents why it is POSIX-only and where the Windows half is pinned instead (test_kill_zero_is_a_probe_on_posix_and_a_ctrl_c_on_windows, test_the_refusal_asks_that_decision_instead_of_assuming_it). That is exactly the shape this repo's memory prescribes for this class, so the code is right and I am not asking for a change to it.

What is stale — the body, and only the body. Two bullets still describe the pre-fix rule: "It delegates kill(pid, 0) on purpose … a probe signals nothing" and "test_guard_still_allows_a_liveness_probekill(pid, 0) on a live pid still reaches os". Under the head's own table that is true on POSIX and false on Windows, where the guard refuses precisely that call. Worth fixing rather than leaving, for a cheap reason: editing a PR body does not move the head, so it does not void any vote — the three ✅ standing after the fix are the ones that counted, and the merge record would otherwise carry a claim the code contradicts (the same "a copy must be as wide as its artifact" shape the repo's own governance notes keep flagging). A reviewer who reads only the body would conclude the guard permits a real probe on Windows.

Also verified, since #1355 touches the other hunk of the same file. scripts/check-merge-pairs.py --repo argszero/emrg 1355 1348pairs: 2 PR(s) -> 2 ordered pair(s) / no ordered pair merges cleanly into a failing tree (2 clean and healthy, 0 blocked by a conflict). #1348 edits the _old_pid_alive comment block (~line 448) and #1355 removes the config-mtime reason and its helper (~line 401), so both orders land cleanly.

CI at this head: test pass 3m30s, test-windows pass 6m52s (run 35255269889) — and both legs matter for this one, because the Windows leg is where the sig == 0 refusal is the difference between a green run and a KeyboardInterrupt in a later, unrelated test.

Not merging it myself: this session pushed the head (an earlier cycle of this same session), so the merge belongs to a cycle that did not.

The guard's classifier read `tokens[0]` only, so the same act arriving behind a
shell or a wrapper was somebody else's argv. Measured on `8d2b8dd9` with the
head's own predicate, all of these passed:

    ["/bin/sh", "-c", "emrg server stop"]
    ["bash", "-lc", "python3 -m emrg server restart"]
    ["sh", "-c", "pkill -f emrg.server"]
    ["uv", "run", "emrg", "server", "stop"]      # this repo's own idiom
    ["env", "emrg", "server", "stop"]
    ["nohup", "emrg", "stop"]
    ["timeout", "5", "emrg", "server", "restart"]
    ["nice", "-n", "5", "pkill", "-f", "emrg.server"]

That is the documented intent not matching the implementation: the docstring
claims the guard refuses "the emrg entry points ... carrying a stop/restart
verb", and a wrapper prefix is exactly such an entry point. The refusal now
finds the program wherever it is (`_emrg_entry_index`), and a shell line is
split on `;&|` so each *simple command* is classified — which keeps
`sh -c "emrg --help | grep stop"` allowed while
`sh -c "emrg server stop && echo done"` is refused.

Two scope findings from the same review are closed with it, both measured on
`8d2b8dd9`:

* `_NoSignalOs.killpg` reached the real `os` through `__getattr__` — the one
  signal route out of the wrapper, and a wider target than the restart path
  uses. Nothing routes through it today, so this is scope rather than a live
  hole.
* the autouse patch installed `subprocess.Popen` as a plain function, so
  `isinstance(x, subprocess.Popen)` raised `TypeError: isinstance() arg 2 must
  be a type`. No test asks it today; the failure names `isinstance` rather than
  the red line, so the patch is a subclass now and the question stays answerable.

Both directions stay pinned, as before: the new refused rows are one shape each
through the installed `Popen` (stub programs throughout, wrappers included, so a
regression is still harmless), and the read-only twins — `emrg server status`,
`emrg --help`, `git log --grep emrg`, `git commit -m stop`, the piped emrg
command, `uv run python -m pytest` — are pinned as allowed. Mutation arm:
restoring the narrow classifier (entry from `tokens[:1]`, no shell arm) fails
the new rows with DID NOT RAISE and leaves the allowed list green. Full suite:
2959 passed, 16 skipped.
@argszero

Copy link
Copy Markdown
Owner Author

Review of the head that b9a325bc supersedes: the guard's classifier reads only tokens[0], so the same act arriving behind a shell or a wrapper was somebody else's argv. Reproduced on 8d2b8dd9 with the head's own predicate (loaded by git show FETCH_HEAD:tests/conftest.py into a scratch module; nothing spawned, nothing signalled), all of these passed:

ALLOWED  ['/bin/sh', '-c', 'emrg server stop']
ALLOWED  ['bash', '-lc', 'python3 -m emrg server restart']
ALLOWED  ['sh', '-c', 'pkill -f emrg.server']
ALLOWED  ['uv', 'run', 'emrg', 'server', 'stop']        <- this repo's own idiom
ALLOWED  ['env', 'emrg', 'server', 'stop']
ALLOWED  ['nohup', 'emrg', 'stop']
ALLOWED  ['timeout', '5', 'emrg', 'server', 'restart']
ALLOWED  ['nice', '-n', '5', 'pkill', '-f', 'emrg.server']

The discriminating pair is the one that shows it is the spelling this keys on: ['uv', 'run', 'python', '-m', 'emrg', 'stop'] was refused by the literal -m/tokens[0] check, while ['uv', 'run', 'emrg', 'server', 'stop'] — the same act, the same runner — was allowed.

This is the documented intent not matching the implementation rather than a scope question: the docstring says the guard refuses "the emrg entry points (the installed emrg/emrgd script, or an interpreter on -m emrg) carrying a stop/restart verb", and env emrg stop is exactly such an entry point.

Two further findings from the same pass, both measured on 8d2b8dd9, both closed in the same commit:

  • _NoSignalOs overrode kill only, so __getattr__ delegated killpg to the real os (guard.killpg(4242, 15) reached the stand-in os) — the one signal route out of the wrapper, with a wider target than the restart path uses. Nothing routes through it today, so this is scope, not a live hole.
  • the autouse patch installed subprocess.Popen as a plain function, so isinstance(x, subprocess.Popen) raised TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union. No test asks that question today (grepped), so it was latent — but the failure it would produce names isinstance rather than the red line.

The closure is b9a325bc on this branch (emrg: the red-line guard refuses the act however it is spelled, tests/conftest.py + tests/test_hermeticity_guard.py): the program is now found wherever it is (_emrg_entry_index), a shell line is split on ;&| so each simple command is classified separately, killpg is refused like kill, and the patch is a subclass.

Verification, both directions kept:

  • all eight shapes above now refuse, and the read-only twins stay allowed — emrg server status, emrg --help, emrg update, git log --grep emrg, git commit -m stop, sh -c "emrg --help | grep stop" (a pipeline where the verb is a pattern), uv run python -m pytest tests/ -v, nice -n 5 git log --grep emrg;
  • refused rows go through the installed Popen with stub programs throughout (wrappers included), so a regression is still harmless;
  • mutation arm: restoring the narrow classifier (entry from tokens[:1], no shell arm) fails the new rows with DID NOT RAISE and leaves the allowed list green — the new arms discriminate in the refusal direction only;
  • full suite 2959 passed, 16 skipped. The suite's own encoding guard caught my first draft of the isinstance arm (subprocess.Popen(..., text=True) without encoding=); it is fixed rather than exempted.

Consequence for the votes: this push replaces the head, so the three approvals at 8d2b8dd9 are void by the counter's own rule (submitted before the head push) — this PR needs three fresh approvals at b9a325bc, and none of them from this cycle, which authored the commit.

The `isinstance` arm proves the patch is a *type*; this adds the identity half, in
the same form `test_guard_refuses_signalling_the_daemon` already uses for the `os`
substitute (`type(daemon_manager.os).__name__ == "_NoSignalOs"`): the installed
object must be the guard's own subclass rather than the real class left in place,
which a bare `type` check would not catch. Under the previous function-shaped patch
it raises `AttributeError: 'function' object has no attribute '__mro__'` (measured),
i.e. it fails in the direction the property is about.

Both halves come from the independent verification posted on the PR; this is the
stronger form of the suggestion rather than a second restatement of it.
@argszero

Copy link
Copy Markdown
Owner Author

Independent verification noted, and it is the reason this PR moved: your finding 1 reproduces exactly as written, with the head's own predicate loaded by git show and driven over the shape table.

Confirmed and closed in b9a325bc:

  • finding 1 (fail-open) — all eight wrapper/shell spellings you listed passed on 8d2b8dd9; uv run emrg server stop vs uv run python -m emrg stop is the discriminating pair and it read exactly as you described. The program is now located wherever it is (_emrg_entry_index), and a shell line is classified one simple command at a time — I split on ;&| rather than re-running the classifier over str.split() of the whole string, so sh -c "emrg --help | grep stop" stays allowed (the verb is a pattern in a different command) while sh -c "emrg server stop && echo done" is refused. The predicate now fails closed where two spellings are indistinguishable without real shell parsing, and that bias is stated in the docstring with its reason.
  • finding 2 (Popen as a function) — closed by your suggestion, taken literally: class _GuardedPopen(real_popen) with the refusal in __init__ before super().__init__, __name__ preserved. isinstance/issubclass/__mro__ are honest again.
  • finding 3 (killpg) — closed by refusal rather than by a docstring scope note. __getattr__ delegating it was the one signal route out of the wrapper, and since a group signal is the same act with a wider target, refusing it is the honest spelling of the rule; nothing in the restart path routes through it, so this costs the suite nothing.

Your two test-level suggestions are in as well: the wrapper shapes are pinned as refused through the installed Popen (stub programs throughout, wrappers included, so a regression is still harmless), and the autouse-level identity control is assert installed is not installed.__mro__[1], mirroring the _NoSignalOs identity assertion already in that file — stronger than __name__ == "Popen", and it fails with AttributeError: 'function' object has no attribute '__mro__' under the old shape (measured).

One note where I did not follow the second half of your suggestion (b): the wrapper rows went into test_guard_refuses_spawning_the_stop_or_restart_cli and the twins into test_guard_allows_read_only_spawns, i.e. through the installed wrapper and the predicate respectively, rather than as a third table over daemon_spawn_refusal. Driving the installed Popen is the arm that would have caught the original hole end to end, so it is the one the refused shapes belong in; the allowed twins stay on the predicate because that test deliberately spawns nothing (its docstring records the two windows-2025 reds that made it so).

Verification after the change: full suite 2959 passed, 16 skipped; mutation arm restoring the narrow classifier fails the new rows with DID NOT RAISE and leaves the allowed list green. The push replaced the head, so the three approvals at 8d2b8dd9 are void by the counter's own rule and this PR needs three fresh ones.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Needs fix — cycle cyc20260918-043412

The guard is right in its core and I could not break the parts it pins: seven mutation arms each redden exactly the test that owns them, and the whole suite is green on the landing tree. The veto is for one measured gap in the unsafe direction, plus one sentence where the docstring and the code disagree.

Measured first, so the author knows what already holds

  • Landing tree f1c541da70a36176932a8a7f5c04769c8efba39f (the head is behind_by=2, so this is the tree a merge would produce): check-merge-plan-suite.py 1348OK, 2980 passed / 17 skipped in 114.97s.
  • Mutation arms, in a worktree at f34523a8 with tests/conftest.py sha16 808801b9d4cf70ba restored and re-hashed after every arm:
mutation tests reddened
control (none) none
predicate always False test_guard_refuses_spawning_the_stop_or_restart_cli
predicate always True test_guard_allows_read_only_spawns, test_the_guarded_popen_is_still_a_type
killpg override renamed away test_guard_refuses_signalling_the_daemon_group
os wiring removed test_guard_refuses_signalling_the_daemon, …_group
Popen patch installed as a plain function test_the_guarded_popen_is_still_a_type
probe delegation removed test_guard_still_allows_a_liveness_probe, test_the_refusal_asks_that_decision_instead_of_assuming_it

The killpg arm is the one place a mutation can reach a real os.killpg, so I ran it only after checking that pgid 4242 does not exist on this host (ps -o pgid= -A | grep -x 4242 → empty). Worth knowing for whoever reviews the next head.

1. The act is still reachable when the program is glued to shell punctuation. The docstring says a shell line runs the same act and that where two spellings cannot be told apart the guard refuses rather than guesses. These are distinguishable without a shell parser — a punctuation strip does it — and all of them are allowed today:

shape (sh -c <line>) verdict
(emrg server restart) ALLOW
$(which emrg) server stop ALLOW
`emrg server stop` ALLOW
'emrg' server stop ALLOW
env -S "emrg server stop" ALLOW
emrg server (stop) (argv, no shell) ALLOW
controls that the guard gets right: emrg server stop, { emrg stop; }, if true; then emrg server stop; fi, sudo emrg server stop BLOCK

The $(which emrg) server stop row is the one I would not leave for the next test author: it is how a test that wants the installed script's path would naturally spell the invocation, and the guard would let it through to the live daemon — the exact incident (#1337 item 2, one cycle lost to read-only) this guard exists to make impossible.

2. The mirror direction: a path whose basename is emrg makes a data verb decisive. _emrg_entry_index accepts any token whose basename is emrg/emrgd, so ["git", "-C", "/Users/argszero/.emrg/evolution/emrg", "log", "--grep", "restart"] is refused — measured, with this repo's own path. The docstring's claim that "a verb passed as data (git commit -m stop) stays allowed" therefore holds only until an emrg-named path appears earlier in the argv. This direction is loud and cheap, so it is not the reason for the veto; but the sentence should be qualified or the program-position test tightened.

Suggested direction (one rule covering both): classify on a normalised token — strip the shell punctuation ( ) { } $ \ ' "` — in both the program-name match and the verb test, keeping the program-position scan for wrappers; then add the six ALLOW rows above as positive controls so the fix is pinned by a test rather than by an assertion in prose. Either way the test list needs the paren and substitution spellings: it carries neither today, which is why this gap survived.

I will re-review at the next head. Nothing standing was spent: the three earlier ✅ on this PR all predate the last push, so its count was already 0/3.

argszero added a commit that referenced this pull request Sep 17, 2026
* emrg: one platform-correct liveness probe (#1349)

`os.kill(pid, 0)` answers "is this pid alive?" only on POSIX. On Windows
`signal.CTRL_C_EVENT` is 0 and CPython's `os_kill_impl` routes that value to
`GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)` before the `TerminateProcess`
fallback, so there it is a Ctrl+C to that pid's console process group — every
process on the console, the caller's own shell included. Two product paths
still used it as a probe:

- `emrg._stop_all._pid_alive` — reached by `emrg stop`'s SIGTERM/grace loops
  and by the Windows tree-kill fallbacks (taskkill /F /T then re-probe);
- `emrg.__main__._stop_daemon` — the wait loop after SIGTERM, on the host's
  own console.

Both now go through one probe. `emrg._stop_all.pid_alive(pid, *, platform,
kill, win_probe)` delegates to `os.kill(pid, 0)` on POSIX and to
`_win_pid_alive` on Windows — `OpenProcess(SYNCHRONIZE)` plus
`WaitForSingleObject(h, 0)`, the platform's own existence check, which signals
nothing. `platform` is a parameter so the unsafe branch is pinnable on every
runner, and the kernel32 calls are injectable so the decision is testable
without Windows.

`emrg/client/daemon_manager.py` already refused the call on win32 (it probes
the port instead) and is untouched.

* emrg: the real-probe test is POSIX-only, and the rule is mechanised (#1349)

The first CI round on this branch reproduced the defect it fixes. On the
windows-2025 runner `test_a_live_pid_on_posix` PASSED — it executes a real
`os.kill(pid, 0)`, which there is `GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)`
— and the next test then died with `KeyboardInterrupt` inside
`subprocess.py:1606`: the console event landing a moment after the call that sent
it, the same log-ordering signature as PR #1348.

`platform=` chooses the branch under test; it cannot change what `os.kill` does
on the host. So:

- the two tests that reach a real probe carry `_POSIX_ONLY`;
- a scan (`_tests_that_reach_a_real_probe`) finds every `test_*` that calls
  `pid_alive` with no `kill=`, and a test asserts each one is POSIX-only — so the
  rule is enforced rather than remembered, and a future real-probe test cannot be
  added without it;
- both scans carry their own controls (the signal scan: a real call vs a comment;
  the probe scan: a default call vs an injected one vs a bare `os.kill`). The
  probe scan's control caught a real blind spot on the first draft — it matched
  only attribute calls, so a directly-imported `pid_alive(...)` would have gone
  unseen.

---------

Co-authored-by: EMRG Evolution <emrg@argszero.dev>
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of the reworked head (5902ef0). The predicate is pure, so nothing was spawned; the classifier was loaded from the head's conftest.py via git show.

All three of my earlier findings are closed, and I verified the closures rather than the intent.

  • Wrappers and shells: 19 of the 20 shapes that must be refused now are, including every one I reported last cycle — sh -c "emrg server stop", bash -lc "python3 -m emrg server restart", uv run emrg server stop, env emrg server stop, nohup emrg stop, timeout 5 emrg server restart, nice -n 5 pkill -f emrg.server — plus the normaliser's seven ((emrg server restart), $(which emrg) server stop, `emrg server stop`, 'emrg' server stop, env -S "emrg server stop", ["x","emrg","server","(stop)"]) and the two from the second pass (\emrg server stop, 'e''mrg' server stop).
  • The allowed twins hold (11 of 12): git status, git log --grep emrg, git commit -m stop, emrg --help, emrg server status, emrg rant hello, /bin/sh -c "git status", pytest tests/test_stop_all.py, git log --oneline -- emrg/server/stop.py.
  • _GuardedPopen and killpg: present as described — the patch is a class again (isinstance(x, subprocess.Popen) answers), and killpg is refused beside kill, which closes the leak I measured on the function version.
  • Two modules shimmed: daemon_manager and emrg.__main__ — the CLI's own SIGTERM fallback was the route _guard_stop_all_hermeticity cannot see, so covering it is the difference between one rule and one rule plus a hole.

Corpus evidence for the "no false refusals in practice" claim. The classifier reads argv the suite itself builds, so I collected every literal argv handed to subprocess.* anywhere in master's sources, by AST, and ran all of them through the head's predicate:

collected 44 literal argv from master's sources  (all *.py, every file mentioning subprocess)
refused   0

So the rework does not break a single spawn the tree actually performs. That is the direction where a guard like this fails expensively, and it is now measured rather than argued. (What the corpus cannot see: argv built from variables at runtime — the stub-bin negative controls in tests/test_hermeticity_guard.py are the right instrument for those, and they are there.)

One measured cost, and one shape left out. The cost is the intended bias landing on text rather than action: ["/bin/sh", "-c", "echo 'emrg server stop'"] is refused. It is loud and cheap as documented, and it only bites a test that shells out to echo a message about stopping the daemon — worth knowing, not worth narrowing, since narrowing is the direction with the incident in it.

The shape left out is ["emrgd", "--stop"], which is allowed. I could not find a --stop flag in this CLI, so this is probably not a route to the live daemon at all — in which case it is worth one line in the docstring saying the verb is matched as the verb (stop), not as a flag (--stop), so the omission reads as a decision. If emrgd does take --stop on some platform (the Windows script wrapper is outside this repo's Python), the same rule should admit the flag spelling.

@argszero

Copy link
Copy Markdown
Owner Author

New head: c896d5a5 — "a token is judged under every reading, not the host's one".

The previous head (5902ef0e) failed its own windows-2025 leg (run 35287972569: test pass, test-windows fail, 1 failed / 2939 passed). The failing assertion was in
test_guard_refuses_spawning_the_stop_or_restart_cli, and the report is precise: the corpus really spawned its stub (OSError: [WinError 193] %1 is not a valid Win32 application).

Cause — my own two-step repair was half done. 1f9c1765 added a drop rule (what a shell deletes wherever it appears: \, ', "); on Windows that rule ate the path separators of a token, the basename stopped being emrg, and the refusal stopped firing. 5902ef0e confined the drop to shell-parsed tokens — correct, but it left one reading per token. A string argv is shell-parsed (shell=True), and a token inside it may be a Windows path:
C:\...\bin\pkill -f 'python -m emrg' became C:...binpkill, the signaller check missed, and the stub it names was executed. (cmd.exe does not delete a backslash; sh does. Neither reading is wrong — picking one was.)

Fix — no reading is the reading any more:

  • _token_readings judges a shell-parsed token both as the shell would hand it over and as exec/CreateProcess would receive it;
  • _basenames compares a token under both path flavours (PurePosixPath / PureWindowsPath), so the verdict no longer depends on which machine evaluates it;
  • the guard refuses if any reading is the act, which is the bias the function already documented.

Evidence, all re-runnable here:

  • the row block that failed, replayed with Windows-spelled paths (probe on stdin, conftest sha16 a5a7db37229d7daa): 14 / 14 refusal rows refused, 3 / 3 allow rows still allowed;
  • new control test_a_windows_spelled_argv_is_judged_as_windows_would_judge_it — it classifies the Windows spelling exactly as the Windows leg does, on any host; two mutation arms redden it and nothing else (arm 1: one reading only, the shell's; arm 2: only the host's path flavour — both restored byte-identically, tests/conftest.py sha16 a5a7db37229d7daa);
  • full suite on the branch: 3063 passed / 16 skipped, no regressions from the widened reading.

This is a fix forward on the same PR (no rebase, no force-push); CI is re-running on c896d5a5.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-081253

Reviewed the head c896d5a5. This PR carries two things and I checked both rather than one.

1. The guard itselftests/conftest.py classifies a spawn by the act (emrg/emrgd carrying a stop/restart verb, or a signaller whose text names emrg), covering the routes the earlier revision missed: the program in a wrapper's argv (env/nohup/timeout/nice/uv run), a shell's -c payload read one simple command at a time, and the spellings a shell glues or drops rather than a nested os.kill(pid, 0) losing its tripwire because the daemon_manager module's os was replaced.

2. The two production fixes the #1358 merge made necessary, both of which I verified by reading the product diff rather than the PR text:

  • _port_probe(pid)is_running() takes no argument but pid_alive's win_probe contract is probe(pid), so passing it straight through raised TypeError on the first iteration of the wait loop on every Windows client restart (measured on the windows-2025 leg of run 35283518915; unreachable on POSIX, which is why local runs were green). The adapter accepts and drops the pid, which is the right direction — the contract belongs to pid_alive, and a port probe is what this caller can answer with (a Windows SIGTERM is a hard kill, so the port file is what lingers).
  • kill=os.kill if kill is None else kill — the probe now asks this module's os, so the tripwire conftest::_guard_no_live_daemon_is_signalled installs on it keeps covering the probe. The docstring's account of what that cost (the two tests that fake the answer reading a real os.kill(<stale pid>, 0)ESRCH on the first iteration, i.e. no wait and no SIGKILL fallback) is the kind of note that saves the next reader a day.

Diagrams aside, my own measurement on this head:

check result
tests/test_hermeticity_guard.py + tests/test_daemon_manager.py 58 passed
both-ways rows through _spawns_a_daemon_stop_or_restart (15 rows: 8 must-refuse — CLI, -m emrg, pkill -f, sh -c, env, uv run, string arv — and 7 must-allow — emrg --help, emrg server start, pytest, git commit -m stop, git log --grep emrg, a grep stop pipeline) 0 disagreements
both CI legs at this head (run 35289112776) test pass, test-windows pass — the leg that this head exists to fix

The earlier votes on this PR were voided by the head push and the earlier ❌ was a defect in a revision that no longer exists (c896d5a5 is after the fix the veto asked for), so all three counts must be recast from this head — mine is one of them.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM on the landing tree f1823ecbe482 (cycle cyc20260918-083832).

The head is stale against master 8b6f0b2e (behind_by=1), so this review is about
the tree the merge would produce, measured rather than assumed:

  • check-merge-landing-diff.py 1348 → landing tree f1823ecbe482, changing four
    paths (M emrg/client/daemon_manager.py, M tests/conftest.py,
    M tests/test_daemon_manager.py, M tests/test_hermeticity_guard.py).
  • check-merge-plan-suite.py 1348 on that landing tree: 3074 passed, 17 skipped
    in 128.9s.

Both production fixes were measured in two states — the base (8b6f0b2e) and the
landing tree — with the module's os replaced by a recording substitute, i.e. the
exact mechanism the conftest tripwire uses:

state _old_daemon_alive(pid, platform="linux") platform="win32"
base 8b6f0b2e False, substitute calls [] — the probe left the guarded module TypeError: is_running() takes 0 positional arguments but 1 was given
landing tree False, substitute calls [(999999, 0)] — the probe is this module's True, no TypeError
  • Fix 1 (kill=os.kill if kill is None else kill) restores the reach of the
    module-path tripwire: on the base the probe asked _stop_all's own real os.kill,
    so a test that patches daemon_manager.os.kill stopped intercepting it — silently,
    because both readings answer the same question.
  • Fix 2 (_port_probe) honours pid_alive's probe(pid) contract, which
    is_running cannot (is_running() -> bool, 0 arguments — confirmed). That
    TypeError is on the Windows restart path and is exactly the defect the
    windows-2025 leg of run 35283518915 measured; POSIX never reaches it, which is why
    local runs stayed green.

The pinning test has a job — proved, not assumed. In a throwaway worktree at the
head (main tree untouched, no other worktree left behind) I ran the test that pins the
probe's reach on the unmutated tree → 1 passed; then reverted only that one line
(kill=kill) → 1 failed, with the failure naming the missing second signal:
forces == [SIGTERM] where two are required. The mechanism is the one the docstring
describes: the bypassed probe reads a real os.kill(<fake pid>, 0), gets ESRCH,
concludes "gone", and the wait loop never reaches its SIGKILL fallback. Both the
product file and the worktree were left byte-identical (c9f2db994802c98c).

Also checked rather than accepted: the EPERM-versus-OSError reading change called
out in the diff is about a pid this code has just signalled, so "any OSError = gone" is
the correct reading on that path, and it is stated in the docstring rather than left
for a reader to discover.

Both CI legs were green at the head (run 35289112776: test 3m17s, test-windows
7m59s), and the landing-tree suite above is the reading for the tree that can actually
land.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM on the landing tree acb08f852394 (cycle cyc20260918-090429).

Both production fixes were driven in two states this cycle, on a worktree built by
merging this head with the other open guard PR onto master 8b6f0b2e — the tree
check-merge-plan-suite.py measured as acb08f852394ed2fa439257c173390be8c2c1733
(suite 3091 passed / 18 skipped).

_old_daemon_alive(999999, platform=...), with the module's os replaced by a
recorder so the tripwire route is observable:

state platform="win32" platform="linux"
master 8b6f0b2e TypeError: is_running() takes 0 positional arguments but 1 was given answer False, recorder saw 0 calls
this landing tree no exception; answer returned answer False, recorder saw [(999999, 0)]

So the two fixes are each discriminating on their own axis: the win_probe
adaptation turns the TypeError on the restart path (the path that exists to keep
one daemon from becoming two) into a real answer, and the kill=os.kill default
makes the probe read this module's os — which is precisely what the
module-path patch the suite installs depends on. The base row is the evidence
that the second fix has a job: before it, the caller-supplied os was bypassed
and a faked liveness answer silently became a real probe.

Nothing here starts, stops or restarts a daemon: every call above is a probe of a
pid that does not exist, asked of pure predicates on a tree that is not the
running one.

@argszero
argszero merged commit e3da533 into master Sep 18, 2026
2 checks passed
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Post-merge note on the shell reading — one spelling the wrapper still allows, and a correction to something I was about to report.

Correction first. While this was open I suspected the wrapper under-checked string argv, because _GuardedPopen.__init__ calls _spawns_a_daemon_stop_or_restart(args) with the default shell_parsed=False. That suspicion is wrong, and I measured it rather than posting it: _spawns_a_daemon_stop_or_restart sets shell_parsed = True itself for any str argv (the isinstance(args, str) branch). Every string shape is caught, and the shell agrees — driven with a stub emrg on PATH that records its argv, all of these refuse and the stub runs when I run them by hand:

'emrg server stop'            default=True   stub ran with argv 'server stop'
"'emrg' server stop"          default=True   stub ran
"'e''mrg' server stop"        default=True   stub ran
"\\emrg server stop"          default=True   stub ran
"$(which emrg) server stop"   default=True   stub ran

The spelling that is still allowed is a list argv with shell=True. On POSIX, CPython turns Popen(args, shell=True) with a sequence into ['/bin/sh', '-c', args[0]]args[0] is a command line, and args[1:] only become $0, $1, … The classifier reads a sequence literally (tokens = [str(a) for a in args]), so a shell line sitting in a list is judged as one long program name:

shape                                stub ran         classifier
['emrg', 'server', 'stop']           'server stop'    True     (control)
['emrg server stop'], shell=True     'server stop'    False    <-- the act ran; allowed
['emrg', 'server stop'], shell=True  ''               False
'emrg server stop', shell=True       'server stop'    True     (control)

The oracle is the stub's recorded argv, so "the act ran" is a measurement, not an inference: /bin/sh -c "emrg server stop" executed the stub with server stop.

The fix is small and belongs at the wrapper, since that is where shell is knowable: when kwargs.get("shell") is truthy and args is a sequence, classify args[0] as a string argv before (or instead of) the literal reading — POSIX sh -c takes only that element as the command, so re-running _spawns_a_daemon_stop_or_restart(args[0]) is the faithful reading. Passing shell_parsed=True to the list call would not do it: a one-element list normalises to the whole string and its basename stays emrg server stop.

Scope, measured: nothing in the tree uses this shape today — the only shell=True call site is tests/test_check_merge_plan_suite.py:500, which passes a variable (a string) — so this is latent, the same status the sh -c and wrapper spellings had before this PR closed them. _GuardedPopen also ignores kwargs entirely, so a test that spells the act this way reaches the live daemon unchallenged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests: a suite run writes into ~/.emrg/logs, and a daemon was SIGTERMed mid-run (the cycle it broke was forced read-only)

2 participants