Skip to content

test: make resolveClientCommand codex/claude tests hermetic (#642) - #643

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-08_hermetic-codex-claude-tests
Sep 8, 2026
Merged

test: make resolveClientCommand codex/claude tests hermetic (#642)#643
ranxianglei merged 1 commit into
masterfrom
2026-09-08_hermetic-codex-claude-tests

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes #642

Problem

tests/launcher.test.ts:1064resolveClientCommand: codex/claude resolve to themselves hard-coded /usr/bin as a PATH that does NOT contain codex/claude. On any machine with those CLIs in /usr/bin the not-on-PATH assertion fails:

AssertionError: Expected values to be strictly deep-equal:
  {
+   command: '/usr/bin/codex',
-   command: 'codex',
  }

Reproduced in this sandbox (has /usr/bin/codex + /usr/bin/claude): 1 failure before the fix, 0 after.

Change

Split the test into two hermetic tests, following the existing pi / codebuddy (#641) pattern:

  • not on PATH: PATH points at an empty temp dir → bare-name fallback (codex / claude)
  • on PATH: fake codex / claude executables created in a temp bin dir → the absolute path is returned

Test-only change; src/launcher.ts untouched (resolveOnPath was already correct).

Pre-flight

  • npm run typecheck — clean
  • npm test — 1241/1241 pass (on this sandbox, where the old test failed)
  • npm run build — success

The old test hard-coded /usr/bin as a PATH that does not contain
codex/claude. On any machine with those CLIs installed in /usr/bin the
not-on-PATH assertion failed (resolveOnPath returns the absolute path).

Split into two hermetic tests, following the pi/codebuddy pattern:
- not-on-PATH: PATH points at an empty temp dir → bare-name fallback
- on-PATH: fake codex/claude executables in a temp bin dir → the
  absolute path is returned
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-08_hermetic-codex-claude-tests (f0a85ec)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-643

Each push to this PR publishes a new version under the pr-643 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr643.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review: ✅ approved — ready to merge

I did an independent pass rather than taking the pre-flight at face value.

1. Bug is real & reproduced. In this environment /usr/bin/codex and /usr/bin/claude are present, so the old test's PATH: "/usr/bin" assumption ("a PATH that does NOT contain codex/claude") is false. Feeding that assertion through the actual resolveClientCommand returns the full path, not the bare name → the old test fails here. Confirmed pre-existing on master, not PR-introduced.

2. Fix is correct. resolveClientCommand (src/launcher.ts:1639) does resolved ?? client for codex/claude, and resolveOnPath (src/launcher.ts:1620) returns the full path only when the file actually exists. So:

  • empty temp-dir PATH → not found → bare-name fallback ✓
  • temp bin dir with fake codex/claude → found → absolute path ✓

3. Hermetic & consistent. Both new tests use fs.mkdtempSync(os.tmpdir()/bili-path-*) + a fake executable + try/finally cleanup — a structural match to the existing pi on PATH resolves to full path test and the #641 codebuddy pattern. No reliance on the host's real PATH.

4. Test-only. src/launcher.ts untouched; resolveOnPath was already correct. No product behavior change.

What I ran:

  • resolveClientCommand tests: 5/5 pass
  • full tests/launcher.test.ts: 103/103 pass
  • npm run typecheck: clean
  • (build unaffected — test-only; dist bundles src/ only)

Minor nit (non-blocking): in the "on PATH" test the two fs.writeFileSync calls sit before the try, so a write failure would leak the temp dir. This is identical to the existing pi test and writeFileSync into os.tmpdir() is effectively infallible, so I'd leave it as-is for pattern consistency.

Fixes #642 linkage is correct — #642 is exactly this non-hermetic-test report.

LGTM. Merge is human-only per AGENTS.md — please merge when ready: #643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: resolveClientCommand codex/claude test is environment-dependent (fails when codex/claude live in /usr/bin)

1 participant