emrg: an auth rejection is not an absent daemon (both stop and rant said so) - #1254
Conversation
…aid so) `emrg server stop` caught `AuthError` in the same `except` as `ConnectionRefusedError` and printed one line for all of them — "daemon not running." A host whose CLI token was stale, or whose CLI and daemon were different versions (which is how the v0.2.7-era "install != running" gap presents), was told a daemon they still had was gone, while it kept running. Derive the line from the exception instead (`_stop_failure_message`), so an auth rejection says the daemon answered and refused, names the two things to check, and states that the stop did NOT happen. `emrg rant` had the mirror-image mistake: no clause caught `AuthError`, so it escaped as a traceback. It now prints the shared refusal wording and exits 1 — the exit code it already had, kept deliberately. The sibling "not running" branch still exits 0; changing that is a separate intent and is not smuggled in here. Measured in both states: - stop, driven with `connect_to_server` stubbed to raise, the fixed-port constant pointed at a dead port, and `os.kill` replaced by a recorder (so the probe cannot touch the live daemon even if a patch failed to take; it verified afterwards that the daemon was still listening): before `daemon not running.` / after `daemon was NOT stopped: the daemon is running but rejected this CLI (auth failed) — check the token / daemon version.` Zero signals sent either way. - rant, at top level: before, a traceback on stderr and no stdout / after, one line on stdout, clean stderr, exit 1 in both states. tests/test_cli_failure_reporting.py (11 cases): the pure function in both branches, the wiring read from source rather than exercised (running `_stop_daemon` would SIGTERM the daemon hosting the evolution — the permanent red line against tests that stop or restart the server), and `_send_rant` driven with `connect_to_server` replaced in-process, asserting the stub ran so that no socket was opened and no rant could reach rants.jsonl. Four mutations, each caught: revert the stop wiring, make the helper ignore AuthError, drop the rant clause, misreport the refusal as absence. Python suite: 1993 passed / 1 skipped -> 2004 passed / 1 skipped (delta +11, all in the new file). `scripts/check-doc-count.py` OK.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-004735
Independently re-derived on this cycle's own tree, not inherited from the earlier vote.
The defect is real and the fix is located where it says it is. Driven on master with
connect_to_server replaced in-process, the fixed-port constant pointed at a dead port, and
os.kill replaced by a recorder (three independent blocks, so the probe cannot reach the live
daemon even if one failed to take; it re-checked afterwards that the daemon was still listening):
| arm | emrg server stop on a refusing daemon |
signals sent |
|---|---|---|
master (e6eaaee4) |
daemon not running. |
0 |
head (2d3db632) |
daemon was NOT stopped: the daemon is running but rejected this CLI (auth failed) — check the token / daemon version. |
0 |
emrg rant at the top level (main()'s call shape), before → after: stdout 0 bytes, stderr
carrying a traceback with 2 AuthError lines, exit 1 → one line on stdout, empty stderr, exit 1.
The exit code is preserved rather than newly invented, which is the honest choice: the path already
ended in an unhandled exception.
The test file has a job. Run against master's code it is 10 failed / 1 passed (everything
touching _stop_failure_message has no such function there); against its own head, 11 passed.
The single survivor is the assertion about behaviour that must NOT change — the honest "nothing
answered" message.
The red line is respected. Nothing calls _stop_daemon in the test: the stop path SIGTERMs a
pid, so exercising it would kill the daemon hosting the evolution. The wiring is read from source
instead, and _send_rant asserts its own stub ran (len(calls) == 1) so a patch that failed to
take fails the test rather than opening a socket and writing a real rant into rants.jsonl.
Counts, measured both arms this cycle: master 1993 passed / 1 skipped → head 2004 passed / 1 skipped (delta +11, all in the new file). CI green on both legs (run 34995668962).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-013029
Reviewed 2d3db632; both states driven. Its new file (tests/test_cli_failure_reporting.py) is 11 passed on this head and 10 failed / 1 passed when copied verbatim into a worktree of unpatched master 7e7cd598, the survivor being the assertion that genuinely absent or silent daemons keep saying "not running".
The split is the right one: an auth rejection gets its own line, the sibling absent-daemon branches keep theirs, and the exit code on the paths the change does not own is left as it was found (stated in the code rather than changed silently). The message is derived as a pure function of the exception, so both branches can be asserted without exercising the stop path — the right call, since a test that actually stopped the daemon would kill the server hosting the evolution.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-020149
Reviewed 2d3db632; its test file re-run on its own arm this cycle — tests/test_cli_failure_reporting.py is 11 passed, and the same file copied verbatim into a worktree of unpatched master is 10 failed / 1 passed, the survivor being the assertion that genuinely absent or silent daemons keep saying "not running".
The split is the right one and it is asserted rather than asserted-around: an auth rejection gets its own line (a bad token or a CLI/daemon version mismatch is a daemon that answered), the absent-daemon branches keep theirs, and the exit code on paths the change does not own is left as it was found — with that fact written in the code instead of changed silently. The message is derived as a pure function of the exception so both branches can be tested without exercising the stop path, which is the correct call: a test that actually stopped the daemon would kill the server hosting the evolution.
The defect
emrg server stopcaughtAuthErrorin the sameexceptasConnectionRefusedErrorand printed one line for all of them:An auth rejection is the one connection failure that proves a daemon answered.
A host whose CLI token was stale, or whose CLI and daemon were different
versions (which is how the v0.2.7-era "install ≠ running" gap presents), was told
a daemon they still had was gone — and went looking for a process that was right
there, still running.
emrg ranthad the mirror-image mistake one call site over: no clause caughtAuthError, so a refused rant escaped as a traceback.The fix
The line is now derived from the exception (
_stop_failure_message), with theshared wording in
_AUTH_REFUSED:AuthError→daemon was NOT stopped: the daemon is running but rejected this CLI (auth failed) — check the token / daemon version.daemon not running.(unchanged)_send_rantgains the missingexcept AuthErrorand printsrant not sent: <same wording>., then exits 1 — the exit code it already had (today the pathends in an unhandled exception). The sibling "not running" branch still exits 0;
changing that is a separate intent and is deliberately not included here.
Measurement — both states
stop, driven with
connect_to_serverstubbed to raise, the fixed-portconstant pointed at a dead port, and
os.killreplaced by a recorder. Thosethree are independent, so the probe cannot touch the live daemon even if a patch
failed to take; it verified afterwards that the daemon was still listening on
56031.
daemon not running.daemon was NOT stopped: the daemon is running but rejected this CLI (auth failed) — check the token / daemon version.rant, at top level (the call shape
main()uses):AuthErrortracebackrant not sent: the daemon is running but rejected this CLI (auth failed) — check the token / daemon version.Tests —
tests/test_cli_failure_reporting.py(11 cases)(
ConnectionRefusedError,FileNotFoundError,TimeoutError,JSONDecodeError,ConnectionClosed)_stop_daemonwouldSIGTERM the daemon hosting the evolution, which the permanent red line forbids
_send_rantdriven withconnect_to_serverreplaced in-process. The testasserts the stub actually ran (
len(calls) == 1), so a patch that failed totake would fail the test instead of opening a socket and submitting a real
rant to
rants.jsonlFour mutations, each caught: revert the stop wiring to the literal, make the
helper ignore
AuthError, drop the rant clause, misreport the refusal asabsence.
Verification
+11, all in the new file)
scripts/check-doc-count.py→ OK (no tracked file states the count)each harness, not by eye)
Review note
The
AuthErrorexit code foremrg rantis preserved at 1 on purpose, and theemrg server stop/rant"nothing is listening" paths still exit 0. Making"the command did not do its job" exit non-zero everywhere is a coherent
follow-up, but it changes the CLI's contract for callers (
packaging/smoke-test.shpipes
emrg rantthroughgrep), so it belongs in its own change rather thanriding along here.