Repository navigation
test(harness): restore VOYAGER_BIN / VOYAGER_PYTHON after bridge mechanics tests (fixes #6) - #7
Conversation
fixes HarryHeYu#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.
|
CI 结论补充：run 失败项是 这与本 PR 的改动无关，是当前 我在纯净上游 本 PR 只改 本 PR 自身的验证仍然成立：本机装真实 Core 1.3.1 的全量 Verification level: |
|
补充一条可核验的证据，便于 Review 时省一步核查（不重复上一条的内容）： 这 1 个红在 对照本 PR 的 run 留给你的判断仍是那条口径问题：Core |
|
按我在上面两条评论里说的「需要就另开小 PR」，现在把它落地了：#8 test(readme): pin install examples to released Core v1.3.1（若编号有变以链接为准）。 它是独立分支、单 commit、单文件（ 之所以单独开：#7 的约束是 issue #6 的「test-harness only / 不扩大范围」，而这条是 验证（纯净 两条 PR 现在各自独立可审：你可以先合 #8 让 顺带说明一处更新：#7 正文里「不推进一步 commit 以免作废已跑完的 CI」这句仍然成立——我这里推的是另一个分支，不是 #7 的 |
HarryHeYu
left a comment
HarryHeYu
left a comment
There was a problem hiding this comment.
APPROVE — reviewed against a real Core 1.3.1, not just the diff
Thank you @SunnyRad for the careful diagnosis and for keeping the change to two files. I verified each of the three things that matter here independently, against the real Core 1.3.1 (voyager.cli, schema_version: 1, all ops present), not a mock:
- Env restoration is correct.
withEnvRestoredsnapshots all three mutated keys up front and uses restore-or-delete (value === undefined -> delete), matching therecent-provider.test.tsconvention.withoutVoyagerEnvcoversPATH,VOYAGER_BIN,VOYAGER_PYTHON— the exact keys the three mechanics tests delete. - Async work completes before
finallyrestores.return await fn()is inside thetry, sorestore()runs only afterresolveVoyager()has settled. I specifically checked the trap described in the PR body (restoring in the calling microtask while the probe is still in flight); the shipped shape avoids it. Also confirmedresetBridgeCache()is in thefinally, so a mutated env cannot leave a stale cached resolution behind. - The whole-file regression genuinely detects the old defect. I ran a file-level mutation — reverting the helper to save only
PATH(the pre-fix behaviour) — and the regression turned red with exactlyVOYAGER_BIN leaked out of a mechanics test. The child-half assertion reads values captured at module load, stripsNODE_TEST_CONTEXT, uses a non-empty non-existent PATH, and asserts the child really ran both the mechanics tests and the env assertion, so it cannot pass as a silent no-op or skip into green.
Joint verification with real Core 1.3.1 (isolated home, VOYAGER_NO_SYNC=1, VOYAGER_PYTHON pinned, PYTHONPATH to the Core source):
# tests 33 # pass 33 # fail 0 # skipped 0
ok - a whole-file run with VOYAGER_BIN configured and no PATH fallback stays green (issue #6)
ok - resolves the real core and reports its bridge schema
ok - runJson returns parsed JSON from a real subcommand
ok - search finds a seeded session and reports its excerpt
Zero skipped: the integration tests actually executed against the real core.
CI: the one red plugin tests on linux is not introduced by this PR. It is the base-carried readme-consistency.test.ts:104 assertion (expected '1.3.0' / actual '1.3.1') inherited from 0f1724a; it reproduces on pristine main (run 43063357502). The other 5 cold-start matrix jobs are green. That assertion has now been corrected on main in c4c658a (CI 43156366514 = success), so this PR introduces 0 new failures and the merged main will carry both fixes.
No product code changed; resolveVoyager() and the "configured binary is authoritative" rule are untouched, as the issue scope requires. Squash-merging with you retained as author.
|
Merged as I independently re-verified against real Core 1.3.1: The separate |
…ore #22 Correct stale states: Core v1.3.1 is RELEASED (tag 81d7e3b), issues #1 and #6 are closed (PR #7 f39be17). Link the evidence_verify item to the decided design in dsh#9 and the new Core issue #22. OpenCode / provenance / native resume / VSIX / macOS CI keep their existing not-verified wording; nothing is promoted to done.
What was broken
Three mechanics tests in
tests/bridge.test.ts(old lines 164 / 188 / 221) each setPATH=''and deletedVOYAGER_BINandVOYAGER_PYTHONto prove discovery fails — but theirfinallyrestored onlyPATH.node:testruns a file's tests sequentially in one process, so the two deletions leaked into every later test. WithVOYAGER_BINconfigured as the only way to reach a real core (no shim on PATH), the two ambient-env integration tests that callintegrationInfo()/runJson()with noBridgeOptionsthen failed withVOYAGER_NOT_FOUND. A normal PATH masks the leak, which is why CI stayed green. Tracked as #6,REPRODUCED_ON_MAIN.How I reproduced it (real core, not a mock)
Isolated venv with Core
1.3.1installed from source, a genuinevoyager.exe,VOYAGER_BINpointing at it,VOYAGER_PYTHONat the venv interpreter, PATH reduced to a non-existent directory (no fallback),VOYAGER_NO_SYNC=1. Whole-file run, exactly the command in the issue:mainresolves the real core and reports its bridge schemaandrunJson returns parsed JSON from a real subcommand, bothVOYAGER_NOT_FOUNDWhat I changed —
tests/bridge.test.tsonly, plus a CHANGELOG entrywithEnvRestored()/withoutVoyagerEnv()— saves all three mutated keys and restores each with restore-or-delete (the patternrecent-provider.test.tsalready uses forSESSIONFLOW_ECHO_FILE; an absent key must be deleted, not set to the string"undefined"). The three mechanics tests now go through it. The async test body is awaited inside thetry, so the environment is restored only afterresolveVoyager()has settled.This detail mattered: my first version restored in the calling microtask, which put the ambient values back while the probe was still running. With a real core on PATH, two "no core" tests then passed by mistake — a test that cannot fail, the exact trap CONTRIBUTING.md rule 1 warns about. Fixed, and caught by the full run with a real core installed.
A hermetic whole-file regression — a parent test re-runs this file with
VOYAGER_BIN/VOYAGER_PYTHONsentinels and a non-existent PATH, and a child-half assertion checks all three keys survived the mechanics tests. It:NODE_TEST_CONTEXT, otherwise the nestednode --testis treated as a recursive run and silently reports nothing (measured: empty stdout,node:test run() is being called recursively);execvpfalls back to a default search path when PATH is unset or empty — that would let a system python resolve and break hermeticity;CHANGELOG.md[Unreleased] / Fixedentry.Deliberately not touched:
src/bridge.ts,resolveVoyager(), and the "a configured binary is authoritative, do not silently fall back" rule — the issue's scope note explicitly excludes them. No product code changed. Two files total:tests/bridge.test.ts,CHANGELOG.md.Verification level
INTEGRATION_TESTED(real Core 1.3.1 called over the bridge) +UNIT_TESTED(the hermetic whole-file regression).npm ci/npm run buildengines: >=22.19)npm test, no core installed103 tests / 83 pass / 0 fail / 20 skippednpm test, real core installed (mirrors CIplugin-testson Linux)103 / 103 pass / 0 fail / 0 skippedmain30 pass / 2 fail(VOYAGER_NOT_FOUND)33 pass / 0 failVOYAGER_BIN leaked out of a mechanics test(1 fail); restored →0 failNotes
main(0f1724a), not my stale forkmain. The commit has one parent and the diff is exactly the two files above.github.comgit transport is blocked on this host, so the branch was pushed through the GitHub Git Data API; content is identical to the locally tested files (SHA-256 verified).cold-start.ymlworkflow ran automatically for this fork PR (no approval gate was needed here, unlike Core PR #19). I will not push further commits after CI, so the result cannot be invalidated by a re-run.