From 476b49cb984141830aa1ceaf5476fbc6a5df3d9a Mon Sep 17 00:00:00 2001 From: SkyFi Geek <45924209+mobileskyfi@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:59:17 -0700 Subject: [PATCH 1/3] test(provisioning): surface stderr/exit code from the SSH probe on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second, independent host-OpenSSH batch-login check in "SSH key installed for quickchr managed user" only ever decoded stdout, so a failure collapsed to Received: "" with no trace of why — the same diagnostic blindness #88 fixed in verifyBatchLogin() itself, just not applied to the test's own duplicate probe. Two windows-x86/stable CI runs post-#88 (29097379124, 29106310402) hit this exact assertion with an empty stdout ~500ms after account creation, while the internal verifyBatchLogin() succeeded silently both times — a different, not-yet-diagnosed failure from #87's NUL bug. This is instrumentation only, to capture what ssh actually says on the next occurrence. --- test/integration/provisioning.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/integration/provisioning.test.ts b/test/integration/provisioning.test.ts index c71720d..986a222 100644 --- a/test/integration/provisioning.test.ts +++ b/test/integration/provisioning.test.ts @@ -517,6 +517,16 @@ describe.skipIf(SKIP)("SSH key provisioning", () => { { stdout: "pipe", stderr: "pipe", timeout: 20_000 }, ); const loginOut = new TextDecoder().decode(login.stdout); + if (!loginOut.includes("managed-key-login-ok")) { + // Diagnostic-blind otherwise: this probe only asserted on stdout, so a + // fast connection-refused/reset or a Windows file-locking error on the + // shared empty_ssh_config path (see #87's verifyBatchLogin diagnostic) + // would surface as a bare "" with no trace of why. + const loginErr = new TextDecoder().decode(login.stderr); + console.error( + `[ssh probe] exit ${login.exitCode}: ${(loginOut + loginErr).trim() || ""}`, + ); + } expect(loginOut).toContain("managed-key-login-ok"); } finally { if (instance) { From 36d72c1423112686cc68888f2ff482f5411becc7 Mon Sep 17 00:00:00 2001 From: SkyFi Geek <45924209+mobileskyfi@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:25:23 -0700 Subject: [PATCH 2/3] test(provisioning): switch SSH probe from spawnSync to spawn (matches verifyBatchLogin) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bun.spawnSync's timeout option reproduced 3/3 on windows-x86 as an instant, undiagnosable exitCode: null with empty stdout+stderr (~400ms after account creation — nowhere near any real timeout), confirmed by the prior commit's stderr/exit-code instrumentation. verifyBatchLogin() (src/lib/provision.ts) uses async Bun.spawn() with a manual kill timer instead and has never failed that way in the same CI runs. Switching the test's own duplicate probe to the same shape to test that as the root cause directly. --- test/integration/provisioning.test.ts | 38 ++++++++++++++++++--------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/test/integration/provisioning.test.ts b/test/integration/provisioning.test.ts index 986a222..877ed62 100644 --- a/test/integration/provisioning.test.ts +++ b/test/integration/provisioning.test.ts @@ -499,8 +499,16 @@ describe.skipIf(SKIP)("SSH key provisioning", () => { // Independent grounding of that flag: a real host-OpenSSH batch login // (BatchMode=yes, PasswordAuthentication=no) with the managed key must work. + // + // Uses the same async Bun.spawn + manual-kill-timer shape as + // verifyBatchLogin() (src/lib/provision.ts) rather than Bun.spawnSync's + // `timeout` option. On windows-x86, Bun.spawnSync here reproduced 3/3 as an + // instant, undiagnosable `exitCode: null` with empty stdout+stderr (~400ms + // after account creation — nowhere near any real timeout), while the async + // form used by verifyBatchLogin() has never failed that way in the same CI + // runs. This is that hypothesis under direct test. const emptyConfig = await ensureEmptySshConfig(sshDir); - const login = Bun.spawnSync( + const loginProc = Bun.spawn( [ "ssh", "-F", emptyConfig, @@ -514,18 +522,24 @@ describe.skipIf(SKIP)("SSH key provisioning", () => { `quickchr@127.0.0.1`, "-p", String(instance.ports.ssh), ':put "managed-key-login-ok"', ], - { stdout: "pipe", stderr: "pipe", timeout: 20_000 }, + { stdout: "pipe", stderr: "pipe", stdin: "ignore" }, ); - const loginOut = new TextDecoder().decode(login.stdout); - if (!loginOut.includes("managed-key-login-ok")) { - // Diagnostic-blind otherwise: this probe only asserted on stdout, so a - // fast connection-refused/reset or a Windows file-locking error on the - // shared empty_ssh_config path (see #87's verifyBatchLogin diagnostic) - // would surface as a bare "" with no trace of why. - const loginErr = new TextDecoder().decode(login.stderr); - console.error( - `[ssh probe] exit ${login.exitCode}: ${(loginOut + loginErr).trim() || ""}`, - ); + const loginTimer = setTimeout(() => { + try { loginProc.kill(); } catch { /* already gone */ } + }, 20_000); + let loginOut: string; + try { + const [out, err] = await Promise.all([ + new Response(loginProc.stdout).text(), + new Response(loginProc.stderr).text(), + ]); + const exitCode = await loginProc.exited; + loginOut = out; + if (!out.includes("managed-key-login-ok")) { + console.error(`[ssh probe] exit ${exitCode}: ${(out + err).trim() || ""}`); + } + } finally { + clearTimeout(loginTimer); } expect(loginOut).toContain("managed-key-login-ok"); } finally { From 05be9cea1a25b2cf4a71c17bd5fffc865fbfe451 Mon Sep 17 00:00:00 2001 From: SkyFi Geek <45924209+mobileskyfi@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:15:25 -0700 Subject: [PATCH 3/3] review: address Copilot PR feedback on the SSH probe rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Initialize the output accumulator up front instead of a bare `let x: string` only assigned on the happy path. - Match success against combined stdout+stderr, like verifyBatchLogin() does, instead of stdout alone — avoids missing a success message ssh happened to emit on stderr, and keeps the two probes consistent. --- test/integration/provisioning.test.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/integration/provisioning.test.ts b/test/integration/provisioning.test.ts index 877ed62..07c70b6 100644 --- a/test/integration/provisioning.test.ts +++ b/test/integration/provisioning.test.ts @@ -527,21 +527,25 @@ describe.skipIf(SKIP)("SSH key provisioning", () => { const loginTimer = setTimeout(() => { try { loginProc.kill(); } catch { /* already gone */ } }, 20_000); - let loginOut: string; + // Initialized up front (not `let x: string` assigned only on the happy path) and + // matched against combined stdout+stderr like verifyBatchLogin() does, not stdout + // alone — makes the failure mode deterministic and avoids missing a success message + // ssh happened to emit on stderr (review: github.com/tikoci/quickchr/pull/93). + let combined = ""; try { const [out, err] = await Promise.all([ new Response(loginProc.stdout).text(), new Response(loginProc.stderr).text(), ]); const exitCode = await loginProc.exited; - loginOut = out; - if (!out.includes("managed-key-login-ok")) { - console.error(`[ssh probe] exit ${exitCode}: ${(out + err).trim() || ""}`); + combined = (out + err).trim(); + if (!combined.includes("managed-key-login-ok")) { + console.error(`[ssh probe] exit ${exitCode}: ${combined || ""}`); } } finally { clearTimeout(loginTimer); } - expect(loginOut).toContain("managed-key-login-ok"); + expect(combined).toContain("managed-key-login-ok"); } finally { if (instance) { try { await instance.stop(); } catch { /* ignore */ }