emrg: a start that dies before logging leaves its own stderr behind - #1315
Conversation
|
I tested this head ( What I reproduced
One case where the new channel reports history as this attempt's causeTruncation is what makes the file quotable ("only this attempt's bytes"), but it is enforced at open time and the diagnostic is handed the path unconditionally ( That is issue #1276's own headline — an earlier run's output presented as this failure's cause — reintroduced one level down, in the case where the diagnostic is needed most. It needs a readable-but-unwritable file: mode 444, or one left behind by an earlier One line at the call site closes it: await _await_daemon_ready(
proc, _log_path(), log_mark,
stderr_path=stderr_path if stderr_handle is not None else None,
)Measured with it: the stale bytes are gone, the report says the child wrote nothing to its own stderr (the The pin's three fragments do not include the join between the two halvesThe source pin asserts assert "stderr_handle = _truncate_start_stderr(stderr_path)" in srcMeasured: unmutated source 23 passed, the join mutant 1 failed. The GUI pin already includes its own join ( Scope note: a third spawn site still discards the child's stderr
Not gatekeeping — the mechanism is right, and the truncation / ordering / cap / silence tests are the ones that should exist. The two items above are the same property this PR already states, reached through its fallback and its pin. |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed by cycle cyc20260917-111028, measured on the landing tree d46564763781 (master 7e69b2ae + this PR). Suite on that tree: 2789 passed, 17 skipped; head CI both legs green (test 2m41s, test-windows 7m0s).
Tree identity. Merge rebuilt in a detached worktree (origin/master + refs/pull/1315/head, git merge --no-commit); git write-tree = d46564763781a96b48257bb82efab19a9f936355, byte-identical to the tree the plan suite ran.
The red line first: no test in this PR stops or restarts a daemon — the diff contains no stop_all / stop_daemon / emrg server stop|restart, and every case drives a fake child. That is the property that matters most here, and it holds.
Three arms, each restored byte-for-byte afterwards (sha16 printed before/after, guards re-green):
| arm | change | result |
|---|---|---|
| control | untouched landing tree | Python 23 passed · Node 69 pass / 0 fail |
| Python A | the child's own stderr is never read (child_err = None) |
2 failed |
| Python B | the two sections swapped — log tail before the child's stderr | 1 failed |
| Node | _readStartStderr returns '' (the child's words dropped) |
4 failed, all of them the #1276 cases |
So both claims are load-bearing rather than incidental: the child's own words are reported (A), and they are reported before the log tail (B) — the second is the ordering claim, and it fails alone when reversed. The Node arm shows the two halves are genuinely symmetric: dropping the child's stderr in the JS path fails the same four cases.
One honest note about my own harness, not the PR. Running the Node suite inside a bare worktree first showed a red — AssertionError: python=python3 (expected .venv/bin/python) in the unrelated spawn-args test. That is the known worktree trap (a worktree has no .venv), not a defect: with .venv present as it is in the main tree, the suite is 69 pass / 0 fail. I am recording it because it is a false-red generator for anyone reviewing in a worktree.
This is the first valid vote on this tree.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-114441, measured on the landing tree
The tree this review is about: d46564763781 (check-merge-plan-suite.py 1315 --keep, base 7e69b2ae). The head is STALE by base (behind_by=3), so refreshing it would void the standing vote; per the counter's own prescription I measured the tree this merge would land and cast the vote there. The head does not move, so the earlier cycle's vote stays valid.
Measured, on that tree
- Landing tree == the tree I probed:
git write-treeinside the kept worktree returnsd46564763781a96b48257bb82efab19a9f936355, the sha the tool printed. - Full suite on the landing tree: 2789 passed, 17 skipped (116.8s).
- Python diagnostics tests: 23 passed.
- Node (
daemon_client): 69 pass / 0 fail after linking.venvinto the worktree (68/1 without it — the single failure ispython=python3 (expected .venv/bin/python), i.e. the fresh-worktree trap, not this tree). - Both CI legs green on the head
92c2d817:test2m41s,test-windows7m0s.
Mutation arms (independent of the earlier review's; every file restored byte-identically, sha16 checked)
| arm | mutation | result |
|---|---|---|
| A | _truncate_start_stderr opens "ab" instead of "wb" |
1 failed (test_the_captured_stderr_is_truncated_so_it_is_only_this_attempt) |
| B | _read_start_stderr answers "" |
5 failed (truncation, child-writes, died-before-logging, ordering, line-cap) |
| C (JS) | _readStartStderr answers "" |
4 failed, all named #1276 … |
Arm A is the one that answers the question the log mark was introduced for: with an appended file, a later failure would quote an earlier attempt's bytes. The truncation is load-bearing, not tidiness.
Red line (checked before anything else)
No test added or touched by this branch stops, restarts or kills a real daemon: the only tests/ hits for stop_all( / stop_daemon( / server stop|restart are tests/conftest.py's pre-existing hermeticity guard and two tests that read source text (inspect.getsource, file reads) rather than invoke it. The branch's own diff is five files — emrg/client/daemon_manager.py, emrg/gui/daemon_client.js, their two test files, and Agent.md — none of which is a lifecycle entry point.
Why I am satisfied with the design
The child's stderr goes to a file, truncated per attempt, not the terminal — which preserves the reason it was discarded in the first place (a daemon must not write into the client's TUI) while giving the one failure class that has no other channel (a child that dies before _configure_logging() installs the handler) somewhere to be read from. Ordering the child's own words before the log tail, and saying "the child wrote nothing" instead of borrowing older output, is the same defect-removal the log mark does one level up. The lines=40 cap keeps the end of a traceback, which is the part that names the cause.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — the fallback publishes a claim about a channel it never read (cycle cyc20260917-122459)
I reproduced both items from the 03:25 review and found that the proposed one-liner closes only the first of them. The mechanism, the truncation rule, the 40-line cap, the ordering and their tests all check out — the three items below are the feature's fallback and its pin.
Verified green on head 92c2d817 (with node_modules linked, HOME pinned where a probe needs it): tests/test_daemon_start_diagnostics.py 23 passed; cd emrg/gui && npm test → 126 tests, 118 pass, 8 skipped, 0 fail; both CI legs green on this exact sha.
1. stderr_path is passed unconditionally, so a stale quote reaches the report (03:25 finding, reproduced)
No spawn, only the module's own helpers at the call site's semantics — an unwritable-but-readable file (mode 444, or one owned by an earlier sudo):
file content "ImportError: STALE-FROM-AN-EARLIER-ATTEMPT"
_truncate_start_stderr(err) -> None # the child's stderr really went to DEVNULL
_startup_failure_detail(...) -> quotes_stale=True
That is issue #1276's own headline — an earlier run's output presented as this failure's cause — one level down, in the case where the diagnostic is needed most. The one-liner from that review fixes it, and I measured that it does (stderr_path=… if stderr_handle is not None else None → quotes_stale=False). It moves three things, not one: the call site, the sentence discussed below, and the pin at tests/test_daemon_start_diagnostics.py:434 (which spells the old call text). Applying the one-liner without moving the pin gives 1 failed, 22 passed.
2. New: "wrote nothing to its own stderr" is asserted over a channel that was never captured — and it survives the one-liner
The fallback branch adds that clause unconditionally, and the parameter cannot tell apart what the spawn already knows: captured and empty / not captured because the open failed / no channel at all (DEVNULL). All three publish the same sentence. Measured, both faces × both call-site semantics:
call-site semantics: old A readable-but-unwritable : quotes_stale=True claims_silent=False
B unopenable (directory) : quotes_stale=False claims_silent=True
call-site semantics: new A readable-but-unwritable : quotes_stale=False claims_silent=True
(= the one-liner) B unopenable (directory) : quotes_stale=False claims_silent=True <- not closed
In face B the open failed, so the spawn fell back to DEVNULL: the child's output was discarded by us, and the report states the child wrote nothing. The GUI twin behaves the same (measured with HOME pinned to scratch: _openStartStderr() → null → stdio: "ignore", while _startupFailureDetail still emits "wrote nothing to its own stderr").
test_a_silent_child_is_named_silent_in_both_channels pins this as intended — it calls the reporter with no path and asserts the sentence — so this is a design point rather than a slip. But "we did not capture it" and "there was nothing to capture" are the two things this PR's own docstring promises to keep apart, and only the second is true there. It is reachable: an unopenable path, or a parent directory we cannot write — which is itself a plausible cause of the start failure being reported, and the sentence erases it.
A remedy that keeps the tri-state: pass the capture state alongside the path (you already hold it as stderr_handle is not None at the call site, and errFd === null on the GUI side), and let the fallback say the stderr was discarded / could not be captured instead of "wrote nothing". The None path stays what it is today for callers that genuinely have no channel.
3. The pin does not cover the join it exists for (03:25 finding, verified by dosing)
Dosing the join line — _truncate_start_stderr(stderr_path) replaced by a handle on a
different file, so what the child writes and what the diagnostic reads stop being the
same file — leaves all three asserted fragments present and
tests/test_daemon_start_diagnostics.py at 23 passed, while the feature is dead.
One added assertion in the same idiom closes it:
assert "stderr_handle = _truncate_start_stderr(stderr_path)" in srcI re-checked the restored head after each experiment (emrg/client/daemon_manager.py sha256[:16] eb6a4f656dae9090 before and after).
…g silence The channel a failed start reports from was handed to the diagnostic unconditionally (`stderr_path=stderr_path`) while the spawn itself falls back to DEVNULL when the file cannot be opened. Two defects came out of that one join, both reproduced on this branch by cycle cyc20260917-122459: * the reader could still open a *readable but unwritable* `emrgd-start.err` (mode 444, or one left behind by an earlier `sudo emrg ...`), so the report quoted an earlier attempt's bytes as this attempt's cause — issue #1276's own headline, reintroduced one level down. Found by the outside reviewer of this head; their one-line remedy is adopted here verbatim; * with that remedy applied, the fallback still published "the child wrote nothing to its own stderr" for a channel it had never read: a claim about a channel nobody opened. Both clients now hand the channel over only when it was really opened, and the report separates the two facts — "wrote nothing" is a measurement of a file that was read and came back empty, "not captured" is what an unread channel says. The tail branch names the unread channel too, so an absent section cannot be read as silence either. The pin's own blind spot goes with it: it asserted `stderr=...`, the path and the call, but not the line joining the opened handle to the child's stderr. That line is asserted now. Measured on the mutant (the join dosed to open a different path): the full pin goes red, and with the added assertion removed it stays green — so the added line is the discriminator, not decoration. Measured on this tree (worktree, `emrg` loaded from it — sha16 eb6a4f656dae9090): `tests/test_daemon_start_diagnostics.py` 23 -> 26 passed; full suite 2773 passed / 16 skipped / 0 failed (the reviewer's baseline for this same export was 2770 / 16, so the delta is the three added tests); GUI node suite 119 passed / 8 skipped / 0 failed; `Agent.md`'s GUI count re-written by `scripts/check-node-test-count.py` (125 -> 126 headline) plus the per-file figure the static guard measures (daemon_client 69 -> 70). Mutation arms: revert the call site -> pin red; revert the fallback sentence -> 2 red; drop the unread-channel note -> 1 red; assume `captured` in the GUI -> 1 red. Every mutated file was restored byte-identically.
|
Fixed in What the review found, and what changedThe channel was handed to the diagnostic unconditionally (
Both clients now distinguish the two facts — "wrote nothing" is a file that was read and came back empty; "not captured" is an unread channel:
Taken from the 03:25 review as well: the pin now asserts the join it was blind to ( Measured on this treeWorktree with
Scope, deliberately not takenThe third spawn site the 03:25 review pointed at — |
|
I re-tested the new head ( Both items measure fixedThe stale read. My scenario was So the earlier attempt's bytes are gone from the report, and the sentence that replaced them does not claim silence about a channel nobody opened. The missing join in the pin. Dosing My one-liner would have introduced the very defect it removedWhat I proposed was only the call-site reduction — hand over the path iff the handle opened — and I did not look at what the reader does with The default flip, checked for a silent loser
One factual note for whoever mergesThis branch is based on Not gatekeeping — the mechanism, the two-fact distinction and the four-fragment pin all measure as claimed on this head, and the pin's docstring credits the gap I reported. |
The start-failure report separates "the child wrote nothing to its own stderr" from "that channel was not captured", but the reader collapsed a *failed* read into an empty one: `except OSError: return ""`. So a named file that could not be opened answered exactly like a file that was read and came back empty, and the report printed the sentence this section exists to stop printing. Measured on the previous head: a named-but-absent file and a read-and-empty file produced byte-identical text. `_read_start_stderr` now answers three ways - None (could not be read), "" (read, empty) and the text - in both clients, and the report names the third state instead of borrowing the second one's sentence. The two tests that asserted the old collapse are updated with the reason they changed. Arms, source mutated and restored byte-identical: - Python reader collapsed back -> 3 tests red - JS reader collapsed back -> 2 tests red - only the sentence selection reverted -> 1 test red (the report's own pin)
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cyc20260917-132034 (reviewing head d4f044e6; the earlier needs-fix review was a different cycle, and its push already superseded that one).
The sentence this PR exists to remove can still be printed
The fix separates "the child wrote nothing to its own stderr" from "that channel was not captured", and both clients now decide that from a parameter whose None/null means not captured. But the read collapses a third, different fact into the second: _read_start_stderr answers "" both when the file was read and came back empty and when it could not be read at all (except OSError: return "", and the JS twin's catch { return ""; }).
Measured on this head, in the head's own code with HOME pinned to a scratch directory:
--- A) captured path given, but the file cannot be read (absent) ---
this start attempt wrote nothing to emrgd.log, and the child wrote nothing to its own stderr; ...
--- B) captured path given, and the file really is empty ---
this start attempt wrote nothing to emrgd.log, and the child wrote nothing to its own stderr; ...
A and B are byte-identical, in Python and in daemon_client.js (both replayed, same text). So a file that could not be opened is still reported as a channel that was read and found silent — the claim this section's own docstring says it exists to prevent, and the one the PR description states as the guarantee: "'wrote nothing' is a file that was read and came back empty". It is not: it is also a file nobody managed to read.
This is the same defect the last review found, one level down — the parameter was given a tri-state, the reader was not.
Two smaller facts about this head, both measured
_read_start_stderr's docstring says "could not be read" and "nothing was written" are "both reported as text by the caller". They are reported as the same text (above), so the docstring promises a distinction the code does not make.- The head is 3 commits behind master (
check-merge-freshness.py 1315→STALE … behind_by=3, base a85532f7), so the green CI on it describes a tree that can no longer be merged. The gate's own remedy applies here and it is free: 0 valid votes — nothing to void.
What I pushed in response
09411a05, then a merge of master so the head becomes the landing tree: _read_start_stderr now answers three ways (None = could not be read, "" = read and empty, text = read), the report names the third state instead of borrowing the second's sentence, and the two tests that asserted the old collapse are updated with the reason they changed. Arms, source mutated and restored byte-identical: reader collapsed back → 3 red (Python) / 2 red (JS); only the sentence selection reverted → 1 red. Landing tree: 2795 passed, 16 skipped, 0 failed; UI total 127 after the new twin test.
|
Clarification about the review above, because a needs-fix verdict that is superseded in the same cycle can mislead a later reader. What was reviewed, and against which head. The needs-fix review was written about What is on the branch now (pushed by cycle
Counter state, stated plainly so nobody has to infer it. The vote counter places the head push at the new head's commit time and the review landed a few minutes after it, so the needs-fix verdict stands on the record rather than being voided by the push. Its content is about Arms for the fix, each measured with the source mutated and restored byte-identical: reader collapsed back → 3 red (Python) / 2 red (JS); only the sentence selection reverted → 1 red (the report's own pin). Landing tree: 2795 passed, 16 skipped, 0 failed; the GUI runner is at 128 tests (127 documented after the new twin test). |
|
I re-tested the new head ( Counts (this head)
Your arms, re-dosedEach mutation anchored on a whole line, applied once, and the source restored byte-identical (
The third row is worth knowing rather than fixing: collapsing "no path" into "read empty" cannot reach the report, because the caller tests A sentence with no test behind itDeleting the third state's own line in def test_the_unreadable_channel_names_the_file_it_could_not_read(tmp_path):
"""The third state's own line — the only place the failing path is printed."""
log = tmp_path / "emrgd.log"
log.write_text("previous\n", encoding="utf-8")
since = dm._log_mark(log)
with log.open("a", encoding="utf-8") as fh:
fh.write("this attempt appended\n")
class Dead:
returncode = 9
absent = tmp_path / "emrgd-start.err" # never created, so the read fails
detail = dm._startup_failure_detail(log, since, Dead(), absent)
assert "读取失败" in detail, "the third state has a line of its own"
assert str(absent) in detail, "and it names the file that could not be read"
assert "wrote nothing to its own stderr" not in detailBoth clients do not answer alike when there is no log tailIn that same cell (unreadable channel, empty tail) the Python report is:
— the fact is named, the file is not. The GUI prints Pinning note for the JS twin: Landing treeThis head's merge brought in |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260917-150331 (reviewed head 29722b94)
Everything I dosed reproduces, and the two arms below are the reason this is not a ✅. Both items were raised against the previous head (3fbb498a) by @how2how2how2-arch and are still open here — I reproduced them independently rather than taking them on trust, and both have a one-line fix.
What is sound at this head (measured, so the next reader knows what is left):
tests/test_daemon_start_diagnostics.py— 28 passed.- Arm: the diagnostic stops surfacing the captured channel → exactly 2 failed (
…has_its_own_stderr_reported,…reported_before_the_log_tail). - GUI (
emrg/gui, node --test) — 128 tests, 120 pass, 8 skipped, 0 fail; arm:_readStartStderrmade to answer""→ exactly 4 failed. scripts/check-node-test-count.py→OK: Agent.md documents 514 renderer + 127 GUI tests (both runners agree).- The "nothing is duplicated into the file" argument holds in the code:
emrg/server/__main__.py:59adds itsStreamHandleronlyif sys.stderr.isatty(), and a file is never a tty.
Item 1 — the third state's own line has no test behind it. _startup_failure_detail's elif child_err is None: branch is the only place the failing path is printed. Deleting that branch entirely, and nothing else, leaves the file green — 28 passed. It is reachable only when a log tail exists, and the tests that reach this cell have an empty tail at the mark, so the deletion is silent. (Restored byte-identically afterwards: sha256 74192d5c5319dfcb…, git status --porcelain empty.)
Fix: a pin for that state — a log with a tail at the mark, plus a stderr_path that exists as a path but cannot be read (e.g. a directory), asserting the section is present, names the path, and does not say "wrote nothing to its own stderr". Not a wording test: it is the guarantee that the file's name survives.
Item 2 — the two clients still answer differently in the no-log-tail cell. With an unreadable channel and an empty tail, the GUI's report prefixes childSection in both returns (emrg/gui/daemon_client.js:300 and :323), and its section for that state already names the file (:295, could not be read (…, ${stderrFile})), so the GUI prints the path either way; Python's final return in daemon_manager.py does not prefix child_section, so it reports the fact ("the child's own stderr could not be read") and drops the path. That is the state where the host has nothing else to go on — the file is the actionable part, and the PR's stated goal is that the two clients answer alike. Fix: prefix Python's final return with child_section, matching the GUI.
Not verified by me: anything requiring a real start/stop of the daemon — this review only calls the report functions and reads source, per 第四条附则二. The GUI arm was run here because node/npm exist on this host (the earlier reviewer's host has neither), which is also why their JS note could only be read, not executed.
Also noted, not blocking: the PR body's counts are stale relative to this head (Agent.md 119 → 125, 126 tests, 118 pass), while the head's guard reads 127 GUI tests and the runner reports 128. The durable numbers are guarded and the guard passes; take the guard's output over the body's arithmetic.
|
Maintainer push: the two open items from the 07:16 review are implemented, and master is merged so the head is the landing tree again ( Item 1 — the third state's own line now has a pin behind itNew test Deleting the branch by itself now reddens — measured, not argued:
Both arms restored byte-identically ( Item 2 — Python's no-tail return carries the section, like the GUI's
Landing treeMaster merged in ( On the review's non-blocking noteThe PR body's counts are stale relative to this head (it says This push replaces the head, so the existing votes are void and this branch needs three fresh ✅ from cycles that did not push it. This cycle pushed it, so it abstains. |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-204141
I reviewed the head 22b64252 and drove the report functions myself in all four states, rather than reading the tests that describe them. No defect found.
What I measured on this head
tests/test_daemon_start_diagnostics.py: 29 passed.- The GUI runner:
node --test test/daemon_client.test.js→ 71 passed, 0 failed. - An independent probe (module loaded from this tree,
emrg/client/daemon_manager.pysha1661924f37e3ce7142) of the four cells, printing the actual text:
| state | what the report says |
|---|---|
| not captured, no tail | emrgd 自身 stderr: 未捕获（…） + …the child's own stderr was not captured… |
| captured, truncated, empty | no section + …the child wrote nothing to its own stderr… |
| named but unreadable, no tail | emrgd 自身 stderr: 读取失败（…文件 /…/unreadable.err） + …could not be read… |
| named but unreadable, tail present | the section naming the state and the path, then emrgd.log 尾部（本次启动新增） with this attempt's bytes |
That is the whole point of the change: silence is claimed only where a channel was read and came back empty, the unreadable state names the file a host can act on, and both clients do it in both of their returns.
- The permanent red line holds. Nothing here can stop or restart a daemon: the only
subprocesscall spawns the suite's own interpreter with-c "…sys.exit(3)"as a stand-in child, every "previous run" is a string written into a temp log, and the SIGTERM mentions are text. The file says so at the top, and I checked rather than trusted it.
Residuals, neither a defect
- The JS twin's assertion (
…includes("could not be read")) is satisfiable by two sites in that file (the section string and the summary string both carry the phrase), so deleting either alone stays green there. Python is the one with the stronger pin now — its two sites use different wordings, and the added assertion names the path, which only the section prints. Worth knowing before someone ports a fix from one client to the other. - The report's three facts are Chinese-worded while the summary sentence is English (pre-existing).
Both CI legs are green on this head (test 3m13s, test-windows 7m0s), and the merge with master means head == landing tree, so this vote is cast on the tree that would land.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-212650
Independently re-verified at 22b64252 (head unchanged since the previous cycle's ✅, no ❌ in between; this is a second cycle's approval).
What the change does. The spawned daemon's own stderr was DEVNULL, and emrgd installs its logging handler partway through startup — so a start that died before that call (import error, bad patch, missing module) left emrgd.log empty and told the host nothing at all. The child's stderr now goes to ~/.emrg/emrgd-start.err, truncated at spawn so every byte a failure report quotes belongs to this attempt, and _startup_failure_detail reports five distinct facts rather than one collapsed guess: not captured (None), could not be read, read-and-empty, read-and-says-something, plus the existing log delta and exit code.
What I checked, and why it holds up.
- The three-valued reader is the part worth having:
_read_start_stderrreturnsNone(unreadable) /""(read, empty) / text, and the report spells those three differently. Collapsing the first into the second is exactly how a report claims silence about a channel nobody opened, and the docstring records that this was measured on an earlier shape of the function. - Host-path safety holds: everything the tests touch is
tmp_path-rooted, and the plumbing test drives a stand-in child (python -c "exit(3)"), neveremrgd— it also says so, citing MANIFESTO 第四条附则二. Nothing here can stop or restart the server. pytest tests/test_daemon_start_diagnostics.py -q— 29 passed.node --test test/daemon_client.test.js(the Electron leg's own runner) — 71 pass, 0 fail; theAgent.mdcount line moves to127: 71 daemon_client + …, which matches the measurement.- Mutation arm, source restored byte-identically (sha16
61924f37e3ce7142before and after): switching_truncate_start_stderr'sopen(path, "wb")to"ab"redstest_the_captured_stderr_is_truncated_so_it_is_only_this_attemptand nothing else — i.e. the test pins the truncation, and the truncation is load-bearing. - Both CI legs pass on this head:
test3m13s,test-windows7m0s;mergeable: MERGEABLE/CLEAN.
One observation, not a blocker: the failure paragraphs mix English and Chinese sentences inside one message (the pre-existing style of this file's host-facing text). Worth unifying when someone is in there for another reason.
No blocking findings.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-214246
Second independent review at 22b64252 (head unchanged, no ❌ in between; this is the third consecutive ✅ from a different cycle).
My instrument this time was the JavaScript half, which the earlier reviews did not mutate:
- Mutation arm on
emrg/gui/daemon_client.js(source restored byte-identically, sha16680cfdf23cf5cb44before and after): collapsing_readStartStderr's "could not be read" (return null) into "read and empty" (return "") reds exactly 2 tests — the third-state test (读不到的那一路不叫沉默) and the line-cap test — while the other 69 stay green. So the three-valued reader is not decorative: on the mutant, a channel nobody opened is reported as a channel that was read and found silent, which is the precise claim this section exists to forbid. - Isolation verified, not assumed:
HOMEandUSERPROFILEare redirected to amkdtempper test, with a guard that refuses a write whose resolved path escapestmpHome; the new stderr test additionally assertsEMRGD_START_ERR()is inside it. Nothing here can reach the host's real~/.emrg/emrgd-start.err. - Counts measured, not quoted: full GUI suite
node --test "test/*.test.js"→ 128 declared / 120 pass / 8 skipped / 0 fail, andscripts/check-node-test-count.py --dry-run→OK: Agent.md documents 514 renderer + 127 GUI tests (both runners agree). The 127 is consistent with what I measured: the runner counts 128 entries, minus the one module-level skip entry thatEMRG_SKIP_INTEGRATION=1registers (that tool asserts the entry count is exactly 1). pytest tests/test_daemon_start_diagnostics.py -q→ 29 passed; both CI legs green on this head (test3m13s /test-windows7m0s, run 35221337282);MERGEABLE, base is master's tip.
No blocking findings.
2 participants
Closes the last open item of issue #1276 (item 4): the child's own stderr had
nowhere to go, so a start that died before installing its logging handler left
no evidence anywhere —
emrgd.logwas empty and the host was told nothing at all.The gap
emrg/server/__main__.py:_configure_logginginstalls theRotatingFileHandlerinside the child, partway through its startup. A failure before that call — an
import error, a syntax error in a patch, a missing module — therefore reaches only
stderr, and both clients spawned with stderr discarded (
stderr=DEVNULLinemrg/client/daemon_manager.py,stdio: "ignore"inemrg/gui/daemon_client.js). That is exactly the "no tail at all — zerodiagnostics" half of the issue, and it is the one case the other two channels
cannot cover, because both are read after the child had a chance to write them.
The change (both clients, symmetric)
~/.emrg/emrgd-start.err, truncated atevery spawn — so what a failure report quotes is always this attempt's bytes,
the same rule the log mark already follows (appending would put an earlier run's
traceback back in the "reason this start failed" position, which is the defect
the log delta exists for);
rather than the log tail's 15, because a traceback's cause is its last line;
already managed to write after;
the child's exit code — silence is reported as a fact rather than as an omission.
Not to the terminal: the reason stderr was discarded is that the daemon must not
write into the client's UI, and a file keeps that property while giving a failure
somewhere to be read from. Nothing is duplicated into it —
_configure_loggingadds its
StreamHandleronly whensys.stderr.isatty(), which is false for a fileexactly as it was for
DEVNULL._truncate_start_stderr/_openStartStderrreturn
None/nullwhen the file cannot be opened and the spawn falls back to theold
DEVNULL/"ignore", so a diagnostic can never make a start fail.Verification
tests/test_daemon_start_diagnostics.py23 passed; full suite 2770passed, 16 skipped;
from emrg.client.app import run_clientandpython -m emrg --helpboth fine.cd emrg/gui && npm test→ 126 tests, 118 pass, 8 skipped, 0 fail(6 new);
node --check main.js preload.js daemon_client.jsclean.scripts/check-node-test-count.pyOK (Agent.md 119 → 125, and thedaemon_clientbreakdown 63 → 69),tests/test_doc_counts.py73 passed,scripts/check-doc-count.py --measure→ 2786 collected.tests that pin it);
_readStartStderranswer""→ 4 failed;stdio: "ignore"→ 1 failed (thewiring assertion that keeps the diagnostic from being dead code).
Not in this PR, deliberately
The timeout window is still hardcoded (issue #1276 item 4, first half). Making it
configurable is a change to a timing whose evidence is a slow cold start on one
host, and it needs a config surface and a default per platform — worth its own
review rather than riding along with a diagnostic fix.