From 0d9db58399e8d397d727b7e68da2715b378c2e32 Mon Sep 17 00:00:00 2001 From: SunnyRad Date: Thu, 12 Nov 2026 01:58:14 +0800 Subject: [PATCH] test(harness): restore VOYAGER_BIN/PYTHON after bridge mechanics tests (fixes #6) Three mechanics tests in tests/bridge.test.ts emptied PATH and deleted VOYAGER_BIN / VOYAGER_PYTHON to prove discovery fails, but their finally restored only PATH. node:test runs a file's tests sequentially in one process, so those deletions leaked into the later ambient-env integration tests: with a configured core as the only way to reach a real one (no shim on PATH), they failed with VOYAGER_NOT_FOUND. A normal PATH masked the leak. - Add withEnvRestored()/withoutVoyagerEnv(): save all mutated keys and restore-or-delete each (the pattern recent-provider.test.ts already uses). Await the async test body inside the try so the env is restored only after resolveVoyager() has settled -- restoring in the calling microtask would put the ambient values back mid-probe and let a real core on PATH make a "no core" test pass by mistake. - Add a hermetic whole-file regression: a parent test re-runs this file with VOYAGER_BIN / VOYAGER_PYTHON sentinels and a non-existent PATH (no core fallback); the child asserts all three keys survived the mechanics tests. NODE_TEST_CONTEXT is stripped so the nested node --test is not skipped as a recursive run. No installed core is required, so it cannot skip into a false green. - CHANGELOG [Unreleased]/Fixed entry. No product code changes: src/bridge.ts and the "configured binary is authoritative" rule are untouched. --- CHANGELOG.md | 3 + tests/bridge.test.ts | 230 ++++++++++++++++++++++++++++++------------- 2 files changed, 166 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51bdec8..0be78e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ Windows 冷启动四关 + 真实 bridge 调用通过，Linux CI 补完整 boot - The Core stdin defect is a **v1.3.1 release gate** item. Candidate artifacts built before that fix must not be published as the final release. +### Fixed +- **The bridge mechanics tests leaked `VOYAGER_BIN` / `VOYAGER_PYTHON` into later tests ([#6](https://github.com/HarryHeYu/dsh-sessionflow/issues/6)).** Three tests emptied `PATH` and deleted both keys to prove discovery fails, but their `finally` restored only `PATH`. `node:test` runs a file's tests sequentially in one process, so when a configured core was the *only* way to reach a real core (no shim on PATH), the two ambient-env integration tests that followed failed with `VOYAGER_NOT_FOUND`. A normal PATH masked the leak, which is why CI stayed green. The three tests now save/restore all mutated keys through one helper (restore-or-delete, matching `recent-provider.test.ts`); no product code changed, and `resolveVoyager()`'s "configured binary is authoritative" rule is untouched. A hermetic whole-file regression re-runs the suite with sentinel values and no PATH fallback, so the leak cannot return silently. + ### Added - **Two new DSH channels verified and admitted** (2026-10-31): `0.2.1-rc.5` (the `next` dist-tag, requires cordis `~4.0.5`) and `0.2.2-alpha.2` (the diff --git a/tests/bridge.test.ts b/tests/bridge.test.ts index 4c46474..f23ef33 100644 --- a/tests/bridge.test.ts +++ b/tests/bridge.test.ts @@ -23,7 +23,7 @@ import { REAL_HOME, TEST_HOME, appdataIsShared, isIsolated } from './isolate.ts' import { test, after } from 'node:test'; import assert from 'node:assert/strict'; -import { execFile } from 'node:child_process'; +import { execFile, spawnSync } from 'node:child_process'; import { mkdtempSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; @@ -41,6 +41,19 @@ const exec = promisify(execFile); // that was right there.) const PATH_AT_LOAD = process.env['PATH'] ?? ''; +// Captured at module load, before any test body can run. The issue-#6 +// regression re-runs this whole file in a child with VOYAGER_BIN / +// VOYAGER_PYTHON set to sentinels; the child asserts these two values +// survived every mechanics test. Reading them at load (rather than inside +// the assertion) means a leak cannot hide by being re-read after the fact. +const AMBIENT_VOYAGER_BIN_AT_LOAD = process.env['VOYAGER_BIN']; +const AMBIENT_VOYAGER_PYTHON_AT_LOAD = process.env['VOYAGER_PYTHON']; +/** Set only in the guarded child re-run of the issue-#6 regression below. */ +const ISSUE6_CHILD = process.env['SESSIONFLOW_ISSUE6_CHILD'] === '1'; +/** Sentinel values the parent injects, so a leaked `delete` is observable. */ +const ISSUE6_BIN_SENTINEL = 'dsh-sessionflow-issue6-voyager-bin'; +const ISSUE6_PYTHON_SENTINEL = 'dsh-sessionflow-issue6-voyager-python'; + /** * How long to wait for a *test-spawned* core. * @@ -68,6 +81,52 @@ function execHermetic(file: string, args: string[]) { return exec(file, args, { env: { ...process.env, PATH: PATH_AT_LOAD } }); } +/** + * Save/restore for the environment a test is about to mutate. + * + * Issue #6: the three mechanics tests below used to delete `VOYAGER_BIN` and + * `VOYAGER_PYTHON` but restore only `PATH`. `node:test` runs the tests in a + * file sequentially in one process, so those deletions leaked into the later + * ambient-env integration tests — when a configured core was the *only* way to + * reach a real core (no shim on PATH), they failed with `VOYAGER_NOT_FOUND`. + * A test must not mutate the process environment it inherited. + * + * The restore-or-delete pattern is the same one `recent-provider.test.ts` uses + * for `SESSIONFLOW_ECHO_FILE`: a key that was absent must be deleted again, + * not set to `undefined` (which Node would coerce to the string "undefined"). + */ +async function withEnvRestored(keys: string[], fn: () => T | Promise): Promise { + const saved = new Map(); + for (const key of keys) saved.set(key, process.env[key]); + const restore = () => { + for (const [key, value] of saved) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + }; + // `fn` is awaited *inside* the try so the environment is only restored after + // the async work settles. Restoring in the microtask that calls `fn()` would + // put the ambient values back while `resolveVoyager` is still probing, and a + // real core on PATH would make a "no core" test pass by mistake. + try { + return await fn(); + } finally { + restore(); + resetBridgeCache(); + } +} + +/** The three mechanics tests that must prove discovery fails with no core. */ +function withoutVoyagerEnv(fn: () => T | Promise): Promise { + return withEnvRestored(['PATH', 'VOYAGER_BIN', 'VOYAGER_PYTHON'], async () => { + process.env['PATH'] = ''; + delete process.env['VOYAGER_BIN']; + delete process.env['VOYAGER_PYTHON']; + resetBridgeCache(); + return await fn(); + }); +} + // Temporary indexes are created under the OS temp dir and removed on exit: // a test suite must not leave scratch behind, least of all on a drive it does // not own. Set SESSIONFLOW_TEST_TMPDIR to redirect them. @@ -162,84 +221,121 @@ function shown(e: VoyagerError): string { } test('a missing core points at the source install, never at pipx', async () => { - const saved = process.env['PATH']; - process.env['PATH'] = ''; - delete process.env['VOYAGER_BIN']; - delete process.env['VOYAGER_PYTHON']; - resetBridgeCache(); - try { - await assert.rejects( - () => resolveVoyager({ python: 'C:/nope/python.exe' }), - (e: unknown) => - e instanceof VoyagerError && - e.code === 'VOYAGER_NOT_FOUND' && - // What the user sees is message + detail, so the hint may sit in either. - // PyPI is an unrelated library, so a pipx hint would send the user to - // the wrong project. - /github\.com\/HarryHeYu\/sessionFlow/.test(shown(e)) && - !/pipx/.test(shown(e)), - ); - } finally { - if (saved !== undefined) process.env['PATH'] = saved; - resetBridgeCache(); - } + await withoutVoyagerEnv(() => assert.rejects( + () => resolveVoyager({ python: 'C:/nope/python.exe' }), + (e: unknown) => + e instanceof VoyagerError && + e.code === 'VOYAGER_NOT_FOUND' && + // What the user sees is message + detail, so the hint may sit in either. + // PyPI is an unrelated library, so a pipx hint would send the user to + // the wrong project. + /github\.com\/HarryHeYu\/sessionFlow/.test(shown(e)) && + !/pipx/.test(shown(e)), + )); }); test('a configured core that fails is named, and is not silently replaced', async () => { - const saved = process.env['PATH']; - process.env['PATH'] = ''; - delete process.env['VOYAGER_BIN']; - delete process.env['VOYAGER_PYTHON']; - resetBridgeCache(); const bin = process.execPath; const stub = join(FIXTURES, 'stub-nocore.mjs'); - try { - await assert.rejects( - () => resolveVoyager({ voyagerBin: bin, voyagerArgs: [stub] }), - (e: unknown) => - e instanceof VoyagerError && - e.code === 'VOYAGER_NOT_FOUND' && - // Naming the operator's own command is the whole point: the generic - // "not installed" text would send them looking for a core they already - // configured. - e.message.includes(bin) && - e.message.includes(stub) && - /does not work/.test(e.message) && - /exited 127/.test(e.message) && - /configured core does not work/.test(e.message) && - // ...and it must say that falling back is what it refused to do, so the - // operator knows the choice was deliberate and how to opt back in. - /Refusing to fall back/.test(e.detail ?? '') && - /voyagerBin/.test(e.detail ?? ''), - ); - } finally { - if (saved !== undefined) process.env['PATH'] = saved; - resetBridgeCache(); - } + await withoutVoyagerEnv(() => assert.rejects( + () => resolveVoyager({ voyagerBin: bin, voyagerArgs: [stub] }), + (e: unknown) => + e instanceof VoyagerError && + e.code === 'VOYAGER_NOT_FOUND' && + // Naming the operator's own command is the whole point: the generic + // "not installed" text would send them looking for a core they already + // configured. + e.message.includes(bin) && + e.message.includes(stub) && + /does not work/.test(e.message) && + /exited 127/.test(e.message) && + /configured core does not work/.test(e.message) && + // ...and it must say that falling back is what it refused to do, so the + // operator knows the choice was deliberate and how to opt back in. + /Refusing to fall back/.test(e.detail ?? '') && + /voyagerBin/.test(e.detail ?? ''), + )); }); test('a PATH core is still used when nothing is configured', async () => { // The same broken command, reached the other way: unconfigured, the plugin // is free to discover the core, and the discovery failure is the generic // one. This is the contrast that makes the test above meaningful. - const saved = process.env['PATH']; - process.env['PATH'] = ''; - delete process.env['VOYAGER_BIN']; - delete process.env['VOYAGER_PYTHON']; - resetBridgeCache(); - try { - await assert.rejects( - () => resolveVoyager({ python: 'C:/nope/python.exe' }), - (e: unknown) => - e instanceof VoyagerError && - e.code === 'VOYAGER_NOT_FOUND' && - !/Refusing to fall back/.test(e.detail ?? ''), + await withoutVoyagerEnv(() => assert.rejects( + () => resolveVoyager({ python: 'C:/nope/python.exe' }), + (e: unknown) => + e instanceof VoyagerError && + e.code === 'VOYAGER_NOT_FOUND' && + !/Refusing to fall back/.test(e.detail ?? ''), + )); +}); + +// Issue #6 regression, child half. The parent (below) re-runs this whole file +// with VOYAGER_BIN / VOYAGER_PYTHON set to sentinels and a PATH with no core +// fallback. The three mechanics tests above delete those keys to prove +// discovery fails; this checks they put them back. On the pre-fix code the +// deletions leaked and this assertion failed — which is the point. It needs +// no real core, so it cannot skip into a false green. +if (ISSUE6_CHILD) { + test('the mechanics tests restore VOYAGER_BIN / VOYAGER_PYTHON, not just PATH (issue #6)', () => { + assert.equal(process.env['VOYAGER_BIN'], AMBIENT_VOYAGER_BIN_AT_LOAD, + 'VOYAGER_BIN leaked out of a mechanics test'); + assert.equal(process.env['VOYAGER_PYTHON'], AMBIENT_VOYAGER_PYTHON_AT_LOAD, + 'VOYAGER_PYTHON leaked out of a mechanics test'); + assert.equal(process.env['PATH'], PATH_AT_LOAD, + 'PATH leaked out of a mechanics test'); + }); +} else { + // Issue #6 regression, parent half. This is the exact command shape the + // maintainer used (`node --test tests/bridge.test.ts`) with the leak's + // trigger: VOYAGER_BIN / VOYAGER_PYTHON configured and no core on PATH. + // A reduced PATH makes the child hermetic (integration tests skip because + // no interpreter can seed an index), so this needs no installed core and + // cannot skip into a false green. The child-half assertion above is what + // turns red when a mechanics test leaks the two keys. + test('a whole-file run with VOYAGER_BIN configured and no PATH fallback stays green (issue #6)', () => { + // `node --test` sets NODE_TEST_CONTEXT=child for its own file processes. + // A nested `node --test` inheriting it recursively skips every file and + // reports nothing ("node:test run() is being called recursively"), which + // would turn this regression into a silent no-op. Delete it so the child + // is a standalone top-level run. + const childEnv = { ...process.env } as Record; + delete childEnv['NODE_TEST_CONTEXT']; + const res = spawnSync( + process.execPath, + ['--experimental-strip-types', '--test', fileURLToPath(import.meta.url)], + { + encoding: 'utf8', + timeout: 180_000, + env: { + ...childEnv, + SESSIONFLOW_ISSUE6_CHILD: '1', + VOYAGER_BIN: ISSUE6_BIN_SENTINEL, + VOYAGER_PYTHON: ISSUE6_PYTHON_SENTINEL, + // A non-empty path that does not exist: no `voyager` shim and no + // python launcher to fall back on, so the child's integration tests + // skip. It must be non-empty because POSIX `execvp` falls back to a + // default search path when PATH is unset *or empty* — that would let + // a system python resolve and make this regression non-hermetic. + PATH: join(tmpdir(), 'dsh-sessionflow-issue6-empty-path'), + }, + }, ); - } finally { - if (saved !== undefined) process.env['PATH'] = saved; - resetBridgeCache(); - } -}); + assert.equal(res.error, undefined, `the child re-run failed to start: ${res.error?.message ?? ''}`); + assert.equal(res.status, 0, + `a whole-file run with VOYAGER_BIN configured and no PATH fallback had failures:\n` + + (res.stdout ?? '') + (res.stderr ?? '')); + // Guard against a child that was a no-op (e.g. the file failed to load and + // reported zero tests). Both the mechanics tests and the env assertion + // must actually have run and passed. + assert.match(res.stdout ?? '', /a missing core points at the source install/, + 'the child never ran the mechanics tests — the regression proved nothing'); + assert.match(res.stdout ?? '', /the mechanics tests restore VOYAGER_BIN/, + 'the child never ran the env-restoration assertion — the regression proved nothing'); + assert.match(res.stdout ?? '', /fail 0/, + 'the child reported a failure'); + }); +} test('non-JSON output becomes a BAD_JSON error, not a parse crash', async () => { resetBridgeCache();