From e08cb94faf91e4307e1f57b6acf6f357d8fbde35 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Mon, 27 Jul 2026 12:07:38 +1000 Subject: [PATCH] fix: correct the PR body, kill a comment I rotted, and get the counts right MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review's blocker was not in the code — it was that my PR body still carried both retracted claims VERBATIM while my commit message asserted they had been "corrected in the PR body". They had only been corrected in a trailing comment. In a PR whose entire thesis is claim accuracy, that is the finding that matters. The body is rewritten. Both claims now appear only as struck-through corrections with the true values: - "ltDiag in EVERY assertion message" -> 8 call sites, two target tests only - "39330+i was SHARED across a loop" -> it wasn't; i is 0/1/2, ports differ. The real risk is cross-process, which the control arm's 246 EADDRINUSE shows. Comment rot I introduced: test-features.mjs:1000 said "See ltAssertBoot below" and no such function exists — grep returned exactly one hit, the comment itself. Removed. Counts corrected after re-measuring with the definition line excluded, which is where my earlier numbers came from: ltDiag call sites 7 -> 8 _ltRmRetry sites 10 -> 11 _ltRmRetry's catch now honours LT_DEBUG instead of being unconditionally silent. It must still never throw — it runs in a finally, where a throw would REPLACE the real assertion error and turn a flake into an apparent regression — but an opt-in warn costs nothing and stops it swallowing genuine programming errors too. Recording the reviewer's control-arm numbers, since they are what actually justifies this PR (full suite x 4 concurrent x 50 rounds, per arm): main this branch clean runs 42/200 200/200 #199 7 0 :1092 8 0 EADDRINUSE 246 0 ENOTEMPTY 1 0 #203 0 0 Tests: 457 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 --- test-features.mjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test-features.mjs b/test-features.mjs index 781c45a..b7be82b 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -997,7 +997,7 @@ function ltBoot(env, dir, nodeArgs = []) { // 'exit' fires when the process terminates, but its stdio pipes may still hold unread data — // 'close' is the one that guarantees both are drained. A test that terminates the child and // then asserts on buf.err/buf.out must wait for `closed`, not `exit != null`, or it can read - // an empty buffer. See ltAssertBoot below. + // an empty buffer. child.on("exit", (code, signal) => { buf.exit = code; buf.signal = signal; }); child.on("close", () => { buf.closed = true; }); // Without a listener, a spawn 'error' is re-thrown as an uncaught exception and takes down the @@ -1010,7 +1010,11 @@ function ltBoot(env, dir, nodeArgs = []) { // as an intermittent ENOTEMPTY (4/200 in review). Node's own retry loop handles the window. function _ltRmRetry(dir) { try { _ltRm(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 }); } - catch { /* a leaked temp dir must never fail a test — the OS reaps it */ } + catch (e) { + // Never throw: this runs in a finally, so a throw here would REPLACE the real assertion + // error and make a flake look like a regression. LT_DEBUG surfaces it without that risk. + if (process.env.LT_DEBUG) console.warn(` [ltRmRetry] ${dir}: ${e.code || e.message}`); + } } // Every ltBoot assertion failure should be self-diagnosing. The historical failure text was // `expected a local-tools FATAL, got: ` — an empty string, which says nothing about whether the