Conversation
…stinstall breakage
npm v12 (RFC 0054) stops running dependency lifecycle scripts by default. That breaks the CLI in two independent ways, both fixed here. Bun runtime supply: the bun npm package is a stub whose postinstall downloads the real binary. With scripts denied, node_modules/bun/bin/bun never materializes. We now declare the 16 @oven/bun-<platform> packages as exact-pinned optionalDependencies; they carry the binary as tarball content rather than fetching it, so npm os/cpu filters install exactly one and no script has to run. The resolvers fall back through bundled bun, @oven variants, hoisted/ancestor locations, then PATH. Windows entry point: bin linking is NOT gated by v12, but our launcher packages/cli/bin/llxprt starts with a /bin/sh shebang, so npm cmd-shim generated a Windows .cmd invoking /bin/sh, which does not exist. Since a bin field admits only one target, the fix is to stop declaring bin on packages/cli and move it into two os-gated packages that each declare bin.llxprt: llxprt-cli-posix (the existing sh launcher, byte-identical) and llxprt-cli-win32 (a native batch launcher, no node dependency). A real npm install on win32 confirmed this yields exactly one working shim with arguments forwarded and no bin collision. Also fixed along the way: the batch launcher swallowed its exit code by reading RC without delayed expansion, and the posix launcher hardcoded packages/cli when deriving the workspace root, which disabled its workspace Bun fallback for any other package directory. .gitattributes now pins *.cmd to CRLF; an LF-only batch file is mis-parsed by cmd.exe and silently breaks the published Windows entry point. Release tooling publishes the two launcher packages in exact version lockstep with the parent, and publish-integrity asserts that lockstep so a skewed bump cannot ship a CLI with no llxprt command at all. fixes #2978
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded POSIX and Windows launcher packages with pinned optional Bun platform packages. Launchers resolve bundled or ChangesBun launcher packaging and fallback
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
The two launcher packages were listed in the root workspaces array. npm installs workspaces unconditionally and enforces their os field, so the package not matching the host aborted the whole install with EBADPLATFORM: win32 rejected llxprt-cli-posix and Linux/macOS would have rejected llxprt-cli-win32. Root npm install was broken on every platform. Remove both from workspaces so they resolve as ordinary os-filtered optionalDependencies, which npm skips tolerantly when they do not match or cannot yet be resolved. Regenerate package-lock.json and bun.lock, which previously lacked the packages entirely and would have failed npm ci.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/package.json (1)
13-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the top-level
llxprtbin.On POSIX, the published package has no
binentry, so global installation does not createllxprt. The platform dependency only exposes a dependency-local bin. Keep"bin": { "llxprt": "bin/llxprt" }inpackages/cli/package.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/package.json` at line 13, Restore the top-level bin declaration in packages/cli/package.json, adding the llxprt command mapped to bin/llxprt alongside the existing package metadata. Preserve the platform dependency’s local bin configuration and ensure the published package exposes the llxprt executable for global POSIX installations.Source: MCP tools
🧹 Nitpick comments (7)
scripts/lib/npm-command.cjs (1)
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueList all probe stages in the error text.
The chain now probes four sources:
npm_execpath, the runtime executable directory,npm.cmdonPATH, and the prefix candidates. The guidance text names only the node.exe directory,NPM_CONFIG_PREFIX, andAPPDATA. Add thePATHscan so the message matches the actual behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/npm-command.cjs` around lines 105 - 116, Update createNpmCliNotFoundError to mention every probe stage in its guidance text: npm_execpath, the runtime executable directory, npm.cmd on PATH, and prefix candidates including NPM_CONFIG_PREFIX and APPDATA. Keep the existing probed list and installation guidance intact while adding the missing PATH scan description.scripts/tests/issue-2978-windows-launcher.bun.test.ts (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the stub source path.
stubSrcpoints atnode_modules/bun/bin/bun.exe. Issue#2978states that this exact file is absent when npm does not run thebunpackage postinstall. If it is missing,copyFileSyncinplaceBundledBunandplaceOvenBunthrows an opaqueENOENT, and every test in the suite fails without naming the cause. The sibling suitescripts/tests/issue-2978-oven-fallback.bun.test.tsuses theensureBun()helper fromscripts/tests/launcher-test-helpers.tsfor this reason. ReuseensureBun()here, or check existence once inbeforeEachand fail with a message that names the missing binary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/issue-2978-windows-launcher.bun.test.ts` at line 52, Guard stubSrc before the launcher tests copy it: reuse ensureBun() from launcher-test-helpers.ts, or add a beforeEach existence check that fails with a clear message naming the missing bun.exe binary. Ensure placeBundledBun and placeOvenBun no longer surface an opaque ENOENT when the npm postinstall artifact is absent.scripts/tests/publish-integrity.test.ts (1)
885-899: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
semver.validfor the exact-pin check.A first-character digit test accepts
1.xand1.2.x, which are ranges, not exact pins.semver.valid(spec) !== nullrejects them precisely. The lockstep test at lines 901-915 already compares the pin tocli.version, so this check adds value only when it is stricter than a prefix test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/publish-integrity.test.ts` around lines 885 - 899, Update the exact-pin assertion in the `packages/cli pins both platform packages as exact optionalDependencies` test to use `semver.valid(spec) !== null` instead of checking the first character. Preserve the existing per-package iteration and diagnostic message while ensuring wildcard and other range specifications are rejected.scripts/tests/publish-dependency-helpers.ts (1)
431-438: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNarrow the internal-package exemption.
The early return skips every remaining check for any internal package name, not only the root-coverage check. An internal package pinned with an unparseable version, or with a broad range such as
^0.9.0, now returnsnulland passes validation. The comment states the intent is exemption from root-manifest coverage for exact registry pins.Keep the version parse check, and require an exact pin, so the exemption stays limited to the described case.
♻️ Proposed change
- if (internal.has(depName)) { - return null; - } + if (internal.has(depName)) { + if (semver.valid(workspaceVersion) === null) { + return { + workspace, + name: depName, + kind, + message: + `${workspace}: internal package ${depName} must be pinned to an ` + + `exact version (got "${workspaceVersion}")`, + }; + } + return null; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/publish-dependency-helpers.ts` around lines 431 - 438, Update the internal-package branch in the dependency validation logic so it exempts only internal dependencies with a successfully parsed exact registry version; preserve the existing version-parse validation and continue normal checks for unparseable or broad-range versions. Keep the root-manifest coverage exemption scoped to the exact-pin case rather than returning null for every name in internal.scripts/tests/issue-2978-oven-fallback.bun.test.ts (1)
681-691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fall-through case explicit.
If the host has only one
@ovenvariant,variants[1] ?? variants[0]rewrites the same package that was just emptied.writeOvenPackagethen recreates the binary, so the test passes without exercising fall-through to a second variant. Add a guard so the intent is verifiable.♻️ Proposed change
const variants = selectOvenVariants(host); - const fallback = variants[1] ?? variants[0]; + if (variants.length < 2) { + throw new Error( + `host has only ${variants.length} `@oven` variant; cannot exercise fall-through`, + ); + } + const fallback = variants[1];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/issue-2978-oven-fallback.bun.test.ts` around lines 681 - 691, In the test flow around selectOvenVariants and writeOvenPackage, explicitly require at least two variants before selecting the fallback; fail the test when only one `@oven` variant is available, then use the second variant so the fallback cannot recreate the package just emptied.packages/cli/src/launcher/bun-path-resolver.ts (1)
215-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe doc comment describes ordering that this code path does not apply.
Lines 217-218 state the candidates "sort after
bin-nativeand alongside the bundledbun/bin/bun.execandidate".windowsOvenCandidatesoutput is never passed toorderWindowsBunCandidates. Line 266 hands it straight tofirstUsableCandidate, which probes in array order. Thekindfield satisfies theWindowsBunCandidatetype but does not influence ordering here.Restate the comment to describe the actual precedence: nearest ancestor first, probed after local native candidates and before PATH candidates.
📝 Proposed comment correction
/** - * `@oven` candidate paths for Windows (issue `#2978`). Classified as - * `direct-native` so they sort after `bin-native` and alongside the bundled - * `bun/bin/bun.exe` candidate, but are probed only AFTER the bundled binary - * was not found. Detection runs lazily via {`@link` resolvedOvenVariants}. + * `@oven` candidate paths for Windows (issue `#2978`). Returned in probe order: + * nearest ancestor first, then variant preference order. The caller probes + * this list after local native candidates and before PATH candidates; it is + * not passed through `orderWindowsBunCandidates`. The `direct-native` kind + * satisfies the `WindowsBunCandidate` type. Detection runs lazily via + * {`@link` resolvedOvenVariants}. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/launcher/bun-path-resolver.ts` around lines 215 - 232, Update the doc comment for windowsOvenCandidates to remove the inaccurate sorting claims and describe its actual precedence: candidates are generated from nearest ancestor outward, then probed after local native candidates and before PATH candidates. Keep the lazy resolvedOvenVariants() detection note and do not change the implementation.packages/cli/scripts/install-native-launchers.cjs (1)
418-494: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a drift guard for the duplicated
@ovenplatform table.
OVEN_PLATFORM_TABLE,PLATFORM_TABLE, andpackages/cli/package.jsoncurrently contain the same 16 package names. Add a test that compares all three package-name sets.OVEN_PACKAGE_NAMESonly coversPLATFORM_TABLE, so it cannot detect drift in the CJS table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/scripts/install-native-launchers.cjs` around lines 418 - 494, Add a drift-guard test covering the package-name sets from OVEN_PLATFORM_TABLE, PLATFORM_TABLE, and packages/cli/package.json, ensuring all three contain the same 16 names. Define or derive a dedicated set from OVEN_PLATFORM_TABLE rather than relying on OVEN_PACKAGE_NAMES, which only validates PLATFORM_TABLE, and make the test fail when any table diverges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 421-427: Add both launcher package paths to the root workspace
configuration used by the release workflow, or update the Publish
`@vybestack/llxprt-cli-posix` and Publish `@vybestack/llxprt-cli-win32` steps to run
npm publish from their package directories instead of using --workspace. Ensure
both packages resolve and publish successfully.
In `@docs/getting-started.md`:
- Line 37: Update the Bun installation guidance around the macOS PATH preference
and the later global-Bun statement so they are consistent: explicitly document
the macOS exception to the global-installation rule, or narrow/remove the
blanket statement that global Bun installations are never used.
- Around line 41-43: Remove the duplicate bundled Bun runtime error message in
the getting-started documentation, keeping only the single recovery message that
remains later in the section. Preserve the separate npm v12 explanation about
optionalDependencies and default-deny install scripts.
- Around line 31-35: Update the documentation’s failure description to state
that an error occurs only after all accepted Bun candidates and fallback probes
at every resolution level fail, rather than when the package-local Bun is
missing.
In `@package.json`:
- Line 386: Replace the unavailable `@x70102/ink` dependency version in
package.json with a version published on registry.npmjs.org, then regenerate
package-lock.json so it contains the resolved `@x70102/ink` entry and matching
integrity metadata.
In `@packages/cli/bin/llxprt`:
- Line 414: Escape the inner kernel32.dll quotes in the PowerShell -Command AVX2
probe so PowerShell receives the DllImport argument correctly. Apply the
identical change at packages/cli/bin/llxprt:414-414 and
packages/llxprt-cli-posix/bin/llxprt:414-414 to keep both launchers
synchronized.
- Around line 500-505: Add an exact bun dependency pin to the package manifests
for both launcher packages, and set each package’s _llxprt_bun_pin to the
matching `@oven/bun-`* package version so _llxprt_bun_validates performs version
validation instead of accepting any candidate.
In `@packages/cli/scripts/install-native-launchers.cjs`:
- Around line 591-666: Memoize the result of host detection in
selectHostOvenVariants so repeated calls do not rerun subprocess-based checks.
Add a module-level ovenVariantsCache following the TypeScript resolver’s
pattern, return the cached variants when available, and cache both the computed
variant list and the empty result before returning; keep
resolveOvenFromNodeModules unchanged.
In `@packages/llxprt-cli-posix/bin/llxprt`:
- Around line 253-270: Update the POSIX launcher’s entry-resolution logic around
_llxprt_entry to resolve the installed `@vybestack/llxprt-code` package root,
mirroring the Windows launcher’s package-resolution behavior, before checking
for bundle/llxprt.js or index.ts. Ensure the launcher no longer searches
relative to `@vybestack/llxprt-cli-posix` and preserves the existing missing-entry
error handling.
In `@packages/llxprt-cli-win32/bin/llxprt.cmd`:
- Around line 27-30: Update all three launcher failure paths in the Windows
llxprt.cmd script to exit with code 43 instead of 1, including the
package-not-found path around the shown endlocal/exit block and the paths around
the referenced ranges. Preserve the existing failure messages and cleanup
behavior.
- Around line 66-80: Update the Pass 2 logic in the :oven_loop block to include
`@oven`\bun-windows-aarch64\bin\bun.exe for Windows arm64 hosts. For x64 hosts,
avoid selecting the AVX2 bun-windows-x64 executable on CPUs without AVX2 by
reordering selection to prefer bun-windows-x64-baseline or by applying the
existing processor-feature check used by the launcher variants; preserve
nearest-ancestor traversal and the :bun_found flow.
In `@packages/vscode-ide-companion/NOTICES.txt`:
- Line 5478: Restore the ignore@7.0.5 notice in NOTICES.txt by replacing the
modified attribution with the exact upstream text: Copyright (c) 2013 Kael Zhang
<i@kael.me>, contributors. Preserve the attribution verbatim and do not apply
the project’s new-file copyright-header convention to this existing third-party
notice.
In `@README.md`:
- Line 118: Qualify the existing global-Bun exclusion bullet in README.md and
README_CN.md as applying only to Linux and Windows, so it no longer contradicts
the macOS PATH preference statement; update the corresponding English and
Chinese wording without changing the macOS behavior.
In `@scripts/bun-test-manifest.ts`:
- Around line 754-762: Update the Bun test manifest by adding the missing
`scripts/tests/issue-2978-windows-launcher.bun.test.ts` entry, using the
existing `scripts-launcher-oven` manifest object as the placement and
configuration reference so the Windows launcher test runs under Bun’s native
runner.
In `@scripts/tests/issue-2978-windows-launcher.bun.test.ts`:
- Around line 185-192: Update the spawn result handling around spawnSync and
EXIT_TOKEN_RE so a null stdout does not reach match; preserve the expected spawn
failure assertion by handling or propagating result.error before parsing stdout,
while retaining normal exit-code parsing for successful launches.
---
Outside diff comments:
In `@packages/cli/package.json`:
- Line 13: Restore the top-level bin declaration in packages/cli/package.json,
adding the llxprt command mapped to bin/llxprt alongside the existing package
metadata. Preserve the platform dependency’s local bin configuration and ensure
the published package exposes the llxprt executable for global POSIX
installations.
---
Nitpick comments:
In `@packages/cli/scripts/install-native-launchers.cjs`:
- Around line 418-494: Add a drift-guard test covering the package-name sets
from OVEN_PLATFORM_TABLE, PLATFORM_TABLE, and packages/cli/package.json,
ensuring all three contain the same 16 names. Define or derive a dedicated set
from OVEN_PLATFORM_TABLE rather than relying on OVEN_PACKAGE_NAMES, which only
validates PLATFORM_TABLE, and make the test fail when any table diverges.
In `@packages/cli/src/launcher/bun-path-resolver.ts`:
- Around line 215-232: Update the doc comment for windowsOvenCandidates to
remove the inaccurate sorting claims and describe its actual precedence:
candidates are generated from nearest ancestor outward, then probed after local
native candidates and before PATH candidates. Keep the lazy
resolvedOvenVariants() detection note and do not change the implementation.
In `@scripts/lib/npm-command.cjs`:
- Around line 105-116: Update createNpmCliNotFoundError to mention every probe
stage in its guidance text: npm_execpath, the runtime executable directory,
npm.cmd on PATH, and prefix candidates including NPM_CONFIG_PREFIX and APPDATA.
Keep the existing probed list and installation guidance intact while adding the
missing PATH scan description.
In `@scripts/tests/issue-2978-oven-fallback.bun.test.ts`:
- Around line 681-691: In the test flow around selectOvenVariants and
writeOvenPackage, explicitly require at least two variants before selecting the
fallback; fail the test when only one `@oven` variant is available, then use the
second variant so the fallback cannot recreate the package just emptied.
In `@scripts/tests/issue-2978-windows-launcher.bun.test.ts`:
- Line 52: Guard stubSrc before the launcher tests copy it: reuse ensureBun()
from launcher-test-helpers.ts, or add a beforeEach existence check that fails
with a clear message naming the missing bun.exe binary. Ensure placeBundledBun
and placeOvenBun no longer surface an opaque ENOENT when the npm postinstall
artifact is absent.
In `@scripts/tests/publish-dependency-helpers.ts`:
- Around line 431-438: Update the internal-package branch in the dependency
validation logic so it exempts only internal dependencies with a successfully
parsed exact registry version; preserve the existing version-parse validation
and continue normal checks for unparseable or broad-range versions. Keep the
root-manifest coverage exemption scoped to the exact-pin case rather than
returning null for every name in internal.
In `@scripts/tests/publish-integrity.test.ts`:
- Around line 885-899: Update the exact-pin assertion in the `packages/cli pins
both platform packages as exact optionalDependencies` test to use
`semver.valid(spec) !== null` instead of checking the first character. Preserve
the existing per-package iteration and diagnostic message while ensuring
wildcard and other range specifications are rejected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d3c17f5-f8a4-465d-9296-226db1784155
⛔ Files ignored due to path filters (7)
bun.lockis excluded by!**/*.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonproject-plans/issue2978/EVIDENCE.mdis excluded by!project-plans/**project-plans/issue2978/PLAN.mdis excluded by!project-plans/**project-plans/issue2978/REMEDIATION-BRIEF.mdis excluded by!project-plans/**project-plans/issue2978/REVIEW-NOTES.mdis excluded by!project-plans/**project-plans/issue2978/WINDOWS-ENTRYPOINT-RESEARCH.mdis excluded by!project-plans/**
📒 Files selected for processing (29)
.gitattributes.github/workflows/release.ymlCONTRIBUTING.mdREADME.mdREADME_CN.mddocs/getting-started.mdpackage.jsonpackages/cli/bin/llxprtpackages/cli/package.jsonpackages/cli/scripts/install-native-launchers.cjspackages/cli/src/launcher/bun-path-resolver.tspackages/cli/src/launcher/oven-bun-variants.tspackages/llxprt-cli-posix/LICENSEpackages/llxprt-cli-posix/README.mdpackages/llxprt-cli-posix/bin/llxprtpackages/llxprt-cli-posix/package.jsonpackages/llxprt-cli-win32/LICENSEpackages/llxprt-cli-win32/README.mdpackages/llxprt-cli-win32/bin/llxprt.cmdpackages/llxprt-cli-win32/package.jsonpackages/vscode-ide-companion/NOTICES.txtscripts/bun-test-manifest.tsscripts/lib/npm-command.cjsscripts/tests/issue-2603-install.test.tsscripts/tests/issue-2978-oven-fallback.bun.test.tsscripts/tests/issue-2978-windows-launcher.bun.test.tsscripts/tests/publish-dependency-helpers.tsscripts/tests/publish-integrity.test.tsscripts/version.ts
| MIT License | ||
|
|
||
| Copyright (c) 2013-2026 kael | ||
| Copyright (c) 2026 kael |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore the upstream ignore@7.0.5 attribution.
Line 5478 replaces the upstream notice with Copyright (c) 2026 kael. The exact ignore@7.0.5/LICENSE-MIT text says Copyright (c) 2013 Kael Zhang <i@kael.me>, contributors. (github.com) Preserve the upstream attribution verbatim.
Proposed fix
-Copyright (c) 2026 kael
+Copyright (c) 2013 Kael Zhang <i@kael.me>, contributors
+http://kael.me/As per coding guidelines, **/*: New files containing a copyright header must use the current calendar year, such as Copyright 2026 Vybestack LLC; this existing third-party notice is not a new project header.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Copyright (c) 2026 kael | |
| Copyright (c) 2013 Kael Zhang <i@kael.me>, contributors | |
| http://kael.me/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vscode-ide-companion/NOTICES.txt` at line 5478, Restore the
ignore@7.0.5 notice in NOTICES.txt by replacing the modified attribution with
the exact upstream text: Copyright (c) 2013 Kael Zhang <i@kael.me>,
contributors. Preserve the attribution verbatim and do not apply the project’s
new-file copyright-header convention to this existing third-party notice.
Sources: Coding guidelines, MCP tools
Remediates the review threads on PR #3086, plus two defects the review surfaced indirectly. Launcher correctness: - Windows .cmd now selects @oven variants by architecture. arm64 hosts get bun-windows-aarch64, which was previously absent entirely, so those hosts found no runtime at all. - AVX2 selection now fails open. The non-baseline bun-windows-x64 build requires AVX2 and dies with SIGILL without it, so it is no longer probed first unconditionally. The probe shells out to PowerShell, which is unreachable when PATH is trimmed; an inconclusive result now falls back to baseline-first-then-AVX2 so a lone AVX2 install still works, while an explicit negative result never offers AVX2. - Launcher failures exit 43, matching the posix launcher. - The sibling-package walk honoured only index.ts. packages/cli ships both bundle/llxprt.js and index.ts, so a published install would have silently run from source and bypassed the prebuilt bundle - a regression in the exact npm-install path this work exists to fix. It now applies the same precedence as the primary path. - The Bun version pin is recovered from the resolved main package's package.json rather than duplicated into two more manifests that would have to stay in lockstep (CWE-1104). - selectHostOvenVariants() is memoized; it was forking sysctl/PowerShell once per ancestor directory during the walk. Test coverage gap: - issue-2978-windows-launcher.bun.test.ts was never registered in bun-test-manifest.ts. Vitest skips *.bun.test.ts, so all eight behavioural launcher tests had never executed. Registering them immediately caught the AVX2 ordering defect above. - Guard stdout/stderr before matching so a spawn failure surfaces the real error instead of a TypeError. Dependency integrity: - package.json pinned @x70102/ink at 6.4.0-hotfix-flicker-scroll-merge-merge, a merge artefact with a doubled suffix that does not exist on the registry. The package was therefore not installed at all and npm ci would fail. Reverted to the spec on main and regenerated both lockfiles. Docs and attribution: - Reconcile contradictory statements about when the no-runtime error is raised and about the macOS PATH preference versus the "global Bun is not used" note, in getting-started.md, README.md and README_CN.md. - Restore the upstream copyright range in NOTICES.txt that a year-stamping pass had rewritten.
WalkthroughThis PR changes 42 file(s).
Changes
Magnitude🎯 4 (XL) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # actionable error and exit 43 immediately rather than preserving the same | ||
| # symlink (which would spin MAX_SYMLINK_HOPS iterations). The hop bound | ||
| # above still guards against cycles where readlink succeeds. | ||
| if ! _llxprt_target=$(readlink -- "$_llxprt_self" 2>/dev/null); then |
There was a problem hiding this comment.
[bug/high] On stock macOS BSD,
readlinkdoes not support the--end-of-options marker and treats it as a literal pathname. When the launcher is invoked through an npm bin symlink (the normal install path),readlink -- "$_llxprt_self"attempts to read a symlink literally named--, fails with ENOENT, and the script exits 43 with the misleading "could not resolve symlink" message. This directly contradicts the header comment that the script works on "stock macOS BSD". Fix: pass the pathname without--, since the pathname is already controlled and cannot begin with a dash in normal use. If defensive option-splitting is desired, usereadlink "$_llxprt_self"followed by2>/dev/null(already present) rather than--.
| workspace: 'cli-bundle', | ||
| cwd: '.', | ||
| files: ['scripts/tests/issue-2999-cli-bundle.bun.test.ts'], |
There was a problem hiding this comment.
[bug/high] High:
workspace: 'cli-bundle'does not correspond to any defined workspace inpackage.json/workspace config. The Bun native test runner resolves entries by workspace name, so this entry will fail to run (or error) instead of executingissue-2999-cli-bundle.bun.test.ts. Fix by correcting the workspace name to an existing workspace that owns the bundle build, or by adding acli-bundleworkspace definition.
| workspace: 'cli-bundle', | ||
| cwd: '.', | ||
| files: ['scripts/tests/issue-2999-cli-bundle.bun.test.ts'], |
There was a problem hiding this comment.
[bug/high] High:
workspace: 'cli-bundle'does not correspond to any declared npm workspace inpackage.json, so manifest-driven runs will fail to resolve the test directory instead of executing it. Change this to a real workspace that owns the bundle (or, if the runner supports root-level entries without a workspace, remove the workspace field) and keep the direct CI invocation in the nightly workflow if intended.
| _llxprt_po_musl=0 | ||
| if [ "$_llxprt_po_os" = linux ] && [ -f /etc/alpine-release ]; then | ||
| _llxprt_po_musl=1 | ||
| fi |
There was a problem hiding this comment.
[bug/medium] Musl detection is hardcoded to Alpine (
/etc/alpine-release). On other musl-based distros (Void Linux, Gentoo musl profile, etc.)_llxprt_po_muslstays0, so the script probes glibc@oven/bun-*variants first. Those will fail at runtime because the glibc-linked binary cannot load on a musl system. Fix: detect musl via the ELF interpreter (e.g.ldd --version 2>/dev/null | grep -q musl) instead of checking only the Alpine release file.
| function createNpmCliNotFoundError(probed) { | ||
| return new NpmCliNotFoundError( | ||
| `npm-cli.js could not be resolved on Windows (probed: ${probed.join(', ')}). ` + | ||
| 'Ensure npm is installed and accessible. This code checks the node.exe ' + | ||
| 'directory (setup-node / official installers), NPM_CONFIG_PREFIX, and ' + | ||
| 'APPDATA locations (nvm-windows, Volta, global installs). If none apply, ' + | ||
| 'install Node via setup-node or an official installer that ships npm ' + | ||
| 'alongside node.exe, or verify that NPM_CONFIG_PREFIX / APPDATA point to ' + | ||
| 'a valid npm installation.', |
There was a problem hiding this comment.
[documentation/medium] The error message here no longer matches the implemented resolution logic.
createNpmCliNotFoundErrorsays npm-cli.js is looked for in the node.exe directory, NPM_CONFIG_PREFIX, and APPDATA, but the newresolveFromPathNpmCmdPATH-scanning fallback is omitted. This mismatch will mislead users debugging failures on Windows or under Bun into thinking a working candidate was skipped.Suggested minimal fix: update the message to mention the PATH/npm.cmd resolution, e.g. replace the sentence starting at
This code checkswith one that also saysPATH (via npm.cmd)so the message reflects the actual strategies attempted.
| * @param {string[]} probed | ||
| * @returns {string | undefined} | ||
| */ | ||
| function resolveFromPathNpmCmd(env, options, probed) { |
There was a problem hiding this comment.
[test/medium] There are no tests for the new
resolveFromPathNpmCmdstrategy innpm-command.test.ts. This is the resolver path added to handle non-Node runtimes like Bun, whereprocess.execPathis the runtime binary and the node-dir fallback is unreliable. A PATH-scanning function is naturally error-prone (PATH casing, path splitting, cross-platform delimiters, npm.cmd locations), and it has no coverage at all, not even a regression test for the previously broken Bun scenario.Suggested minimal test additions in
scripts/tests/npm-command.test.ts:
- Case: PATH contains an npm.cmd dir with a matching npm-cli.js → resolves from PATH before prefix fallbacks.
- Case: multiple PATH dirs, only one has npm.cmd/npm-cli.js → resolves from that dir and does not append unrelated PATH candidates to
probed.- Case: PATH contains npm.cmd but the sibling npm-cli.js is missing → that PATH dir is probed and the resolver continues to prefix fallbacks.
Without these, the new strategy is unguarded against future refactors.
| function updateCliLauncherPlatformPins( | ||
| packageJsonPath: string, | ||
| version: string, | ||
| ): void { | ||
| const packageJson = readJson(packageJsonPath); | ||
| const optionalDeps = packageJson.optionalDependencies; | ||
| if (!isDependencyRecord(optionalDeps)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[bug/medium]
updateCliLauncherPlatformPinssilently returns whenoptionalDependenciesis missing or not a plain object (lines 189–191). Given the comment explicitly warns that version skew causes npm to silently skip the platform package and break the CLI, a release script should loudly fail or at least warn when this critical precondition is violated. As written, accidental removal of theoptionalDependenciessection (or one of the platform packages from it) would pass version bump without fixing the pins, producing a broken release.
| function updateCliLauncherPackageVersions(version: string): void { | ||
| for (const dir of CLI_LAUNCHER_PLATFORM_PACKAGE_DIRS) { | ||
| const packageJsonPath = resolve(process.cwd(), dir, 'package.json'); | ||
| const packageJson = readJson(packageJsonPath); | ||
| if (packageJson.version !== version) { | ||
| packageJson.version = version; | ||
| writeJson(packageJsonPath, packageJson); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[bug/medium]
updateCliLauncherPackageVersionsperforms file I/O in a loop with no error handling (lines 220–227). If a platform package directory is missing, unreadable, or unwritable, the unhandled exception aborts the script after step 5b may have already succeeded, leavingpackages/clipins updated but the platform packageversionfields stale — exactly the skew this code is meant to prevent. Wrap the loop in try/catch (or validate preconditions up front) so partial failures don’t corrupt release state.
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
# Conflicts: # scripts/bun-test-manifest.ts
The POSIX launcher was committed from Windows as mode 100644. npm preserves tarball modes, so the published launcher could not execute on Linux/macOS at all - the exact failure this package exists to prevent. Set 100755 and add a publish-integrity guard so it cannot regress. Also: add default *) cases to every case block in both sh launchers (shellcheck SC2249, --enable=all); convert orphaned <a id> anchors in README.md/README_CN.md into real headings so the doc-links guard resolves them; exempt the deliberately-undeclared os-gated launcher packages in bun-workspaces (declaring them as workspaces makes npm enforce their os field and fail EBADPLATFORM everywhere) using an exact-match assertion that also catches stale exemptions; treat those same packages as first-party in publish-integrity S6.
packages/cli declares the os-gated launcher packages as optionalDependencies. They cannot be root workspaces (npm enforces a workspace's os field and fails EBADPLATFORM on every platform, verified locally), so before their first publish they are unresolvable and absent from the lockfile. npm ci treats that as a manifest/lock desync and fails EUSAGE; npm install correctly omits an unresolvable optional dependency, exits 0, and leaves the lockfile byte-identical. The job only needs node_modules to run the Bun harness.
# Conflicts: # scripts/bun-test-manifest.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/bin/llxprt`:
- Around line 468-471: The musl probe in the launcher is limited to Alpine
detection. In packages/cli/bin/llxprt lines 468-471 and
packages/llxprt-cli-posix/bin/llxprt lines 468-471, update the _llxprt_musl
detection to use an ldd version check for musl instead of /etc/alpine-release,
applying the identical change in both launchers.
In `@packages/llxprt-cli-posix/bin/llxprt`:
- Around line 1-52: Add an automated parity check for the POSIX launchers,
comparing packages/llxprt-cli-posix/bin/llxprt with its corresponding launcher
and failing CI when they differ. Integrate the check into the existing build or
test workflow, or generate the POSIX launcher from a single source during
packaging so both launchers remain byte-identical.
In `@packages/llxprt-cli-win32/bin/llxprt.cmd`:
- Around line 86-109: Update the Windows launcher’s Bun candidate selection in
the :oven_ready search and the preceding local-package pass to read the pinned
“bun” version from !MAIN_PKG!\package.json and validate each candidate’s
package.json version before setting BUN_EXE. Reject mismatched `@oven` Bun
packages and PATH candidates as appropriate, preserving the existing search
order and fallback behavior while matching the POSIX launcher’s validation.
- Around line 83-85: Clear or initialize OVEN_AVX2 before the PowerShell probe
loop so an inherited value cannot affect variant selection when the probe
produces no output. Update the setup immediately before the for /f command,
preserving the existing True/False handling and ensuring the unset-probe case
retains baseline-first fail-open behavior.
In `@scripts/tests/issue-2978-oven-fallback.bun.test.ts`:
- Around line 681-691: Update the fallthrough test around selectOvenVariants and
the fallback assignment to skip the test when variants.length is less than two,
before clearing or recreating any package. Only proceed with the existing
variants[1] fallback and writeOvenPackage flow when a genuine second variant
exists.
In `@scripts/tests/issue-2978-windows-launcher.bun.test.ts`:
- Around line 136-142: Update placeOvenBun to write the expected version into
the `@oven` variant package.json alongside bun.exe, then add Windows launcher
validation matching the POSIX launcher’s version-pin behavior before executing
the binary. Extend the issue-2978 Windows tests with a mismatched-version case
that confirms llxprt.cmd rejects the package.
In `@scripts/version.ts`:
- Around line 183-198: Update updateCliLauncherPlatformPins to fail loudly when
the pin invariant cannot be applied: throw if optionalDependencies is not a
valid dependency record, and throw if any package in
CLI_LAUNCHER_PLATFORM_PACKAGES has no entry. Continue updating present pins
whose versions differ, preserving the existing changed tracking for valid
inputs.
- Around line 239-247: Reorder the release updates so launcher manifest versions
are successfully updated before rewriting packages/cli’s optional dependency
pins: call updateCliLauncherPackageVersions(newVersion) before
updateCliLauncherPlatformPins(cliPackageJsonPath, newVersion). Preserve the
existing lockstep versioning behavior and comments for steps 5b and 5c.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e671973-c753-4eca-b5b0-4fad14f7be74
⛔ Files ignored due to path filters (7)
bun.lockis excluded by!**/*.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonproject-plans/issue2978/EVIDENCE.mdis excluded by!project-plans/**project-plans/issue2978/PLAN.mdis excluded by!project-plans/**project-plans/issue2978/REMEDIATION-BRIEF.mdis excluded by!project-plans/**project-plans/issue2978/REVIEW-NOTES.mdis excluded by!project-plans/**project-plans/issue2978/WINDOWS-ENTRYPOINT-RESEARCH.mdis excluded by!project-plans/**
📒 Files selected for processing (29)
.gitattributes.github/workflows/ci.yml.github/workflows/release.ymlCONTRIBUTING.mdREADME.mdREADME_CN.mddocs/getting-started.mdpackage.jsonpackages/cli/bin/llxprtpackages/cli/package.jsonpackages/cli/scripts/install-native-launchers.cjspackages/cli/src/launcher/bun-path-resolver.tspackages/cli/src/launcher/oven-bun-variants.tspackages/llxprt-cli-posix/LICENSEpackages/llxprt-cli-posix/README.mdpackages/llxprt-cli-posix/bin/llxprtpackages/llxprt-cli-posix/package.jsonpackages/llxprt-cli-win32/LICENSEpackages/llxprt-cli-win32/README.mdpackages/llxprt-cli-win32/bin/llxprt.cmdpackages/llxprt-cli-win32/package.jsonscripts/lib/npm-command.cjsscripts/tests/bun-workspaces.test.tsscripts/tests/issue-2603-install.test.tsscripts/tests/issue-2978-oven-fallback.bun.test.tsscripts/tests/issue-2978-windows-launcher.bun.test.tsscripts/tests/publish-dependency-helpers.tsscripts/tests/publish-integrity.test.tsscripts/version.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/llxprt-cli-posix/package.json
- .gitattributes
- packages/llxprt-cli-win32/README.md
- .github/workflows/release.yml
- docs/getting-started.md
- CONTRIBUTING.md
- packages/llxprt-cli-win32/package.json
- package.json
- packages/llxprt-cli-posix/LICENSE
- packages/cli/package.json
- packages/llxprt-cli-win32/LICENSE
- packages/cli/scripts/install-native-launchers.cjs
- scripts/tests/publish-dependency-helpers.ts
- README_CN.md
- packages/llxprt-cli-posix/README.md
- README.md
- scripts/tests/issue-2603-install.test.ts
- packages/cli/src/launcher/bun-path-resolver.ts
- scripts/lib/npm-command.cjs
| _llxprt_po_musl=0 | ||
| if [ "$_llxprt_po_os" = linux ] && [ -f /etc/alpine-release ]; then | ||
| _llxprt_po_musl=1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Musl detection covers only Alpine in both POSIX launchers. Both files set _llxprt_po_musl=1 only when /etc/alpine-release exists. On a non-Alpine musl host (Void musl, Gentoo musl profile), the probe orders glibc @oven/bun-linux-* variants first, and a glibc-linked binary cannot load on a musl system. The shared root cause is the Alpine-specific file test.
packages/cli/bin/llxprt#L468-L471: replace the/etc/alpine-releasetest with a loader check, for exampleldd --version 2>&1 | grep -qi musl.packages/llxprt-cli-posix/bin/llxprt#L468-L471: apply the identical change so the copied launcher stays in sync.
📍 Affects 2 files
packages/cli/bin/llxprt#L468-L471(this comment)packages/llxprt-cli-posix/bin/llxprt#L468-L471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/bin/llxprt` around lines 468 - 471, The musl probe in the
launcher is limited to Alpine detection. In packages/cli/bin/llxprt lines
468-471 and packages/llxprt-cli-posix/bin/llxprt lines 468-471, update the
_llxprt_musl detection to use an ldd version check for musl instead of
/etc/alpine-release, applying the identical change in both launchers.
| #!/bin/sh | ||
| # shellcheck disable=SC2250 | ||
| set -u | ||
|
|
||
| # Resolve the real location of this script, following symlinks using only | ||
| # portable POSIX constructs (readlink without -f, case-based join). This works | ||
| # on stock macOS BSD and Linux without GNU coreutils. | ||
| # | ||
| # A bounded iteration count guards against pathological symlink cycles. In | ||
| # practice npm .bin links are single-hop, but a crafted or corrupt chain is | ||
| # rejected after MAX_SYMLINK_HOPS rather than looping indefinitely. | ||
| _llxprt_self=$0 | ||
| _llxprt_hops=0 | ||
| MAX_SYMLINK_HOPS=40 | ||
| while [ -L "$_llxprt_self" ]; do | ||
| _llxprt_hops=$((_llxprt_hops + 1)) | ||
| if [ "$_llxprt_hops" -gt "$MAX_SYMLINK_HOPS" ]; then | ||
| printf '%s\n' 'LLxprt Code: symlink resolution exceeded maximum hops (possible cycle).' >&2 | ||
| printf '%s\n' "The symlink chain at $_llxprt_self may be cyclic or corrupt." >&2 | ||
| printf '%s\n' 'Reinstall the package with "npm install @vybestack/llxprt-code"' >&2 | ||
| exit 43 | ||
| fi | ||
| _llxprt_dir=$(dirname -- "$_llxprt_self") | ||
| # On readlink failure (permission denied, dangling link, I/O error), emit an | ||
| # actionable error and exit 43 immediately rather than preserving the same | ||
| # symlink (which would spin MAX_SYMLINK_HOPS iterations). The hop bound | ||
| # above still guards against cycles where readlink succeeds. | ||
| if ! _llxprt_target=$(readlink -- "$_llxprt_self" 2>/dev/null); then | ||
| printf '%s\n' 'LLxprt Code: could not resolve symlink (readlink failed).' >&2 | ||
| printf '%s\n' "The symlink at $_llxprt_self is broken, cyclic, or unreadable." >&2 | ||
| printf '%s\n' 'Reinstall the package with "npm install @vybestack/llxprt-code"' >&2 | ||
| exit 43 | ||
| fi | ||
| case "$_llxprt_target" in | ||
| /*) _llxprt_self=$_llxprt_target ;; | ||
| *) _llxprt_self=$_llxprt_dir/$_llxprt_target ;; | ||
| esac | ||
| done | ||
| _llxprt_script_dir=$(cd -- "$(dirname -- "$_llxprt_self")" 2>/dev/null && pwd) || \ | ||
| _llxprt_script_dir=$(dirname -- "$_llxprt_self") | ||
|
|
||
| # Read the pinned Bun dependency version and package name from this package's | ||
| # own package.json. When an exact pin is present, discovered Bun candidates are | ||
| # validated against this pin so an unrelated Bun (a different version) is | ||
| # rejected even if it lives inside an allowed boundary. A missing or unreadable | ||
| # candidate package.json/version is also rejected when a pin exists, so a | ||
| # partial install cannot silently fall through to an unrelated Bun. | ||
| _llxprt_pkg_json=$_llxprt_script_dir/../package.json | ||
| _llxprt_bun_pin="" | ||
| if [ -f "$_llxprt_pkg_json" ]; then | ||
| _llxprt_bun_pin=$(sed -n 's/^[[:space:]]*"bun"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' -- "$_llxprt_pkg_json" 2>/dev/null | head -n1) | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether the two POSIX launchers are byte-identical and
# whether any test or build step enforces that.
set -uo pipefail
a=packages/cli/bin/llxprt
b=packages/llxprt-cli-posix/bin/llxprt
for f in "$a" "$b"; do
[ -f "$f" ] || { echo "missing: $f"; exit 0; }
done
echo "--- sha256 ---"
sha256sum -- "$a" "$b" 2>/dev/null || shasum -a 256 -- "$a" "$b"
echo "--- diff (empty means identical) ---"
diff -u -- "$a" "$b" | head -60
echo "--- any test/script referencing both launcher paths ---"
rg -n -C3 'llxprt-cli-posix/bin/llxprt' \
--glob '!**/node_modules/**' \
--glob '*.ts' --glob '*.js' --glob '*.cjs' --glob '*.json' --glob '*.yml' .Repository: vybestack/llxprt-code
Length of output: 1105
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "--- launcher references and copy/generation rules ---"
rg -n -C3 \
'packages/(cli/bin/llxprt|llxprt-cli-posix/bin/llxprt)|cp .*llxprt|copy.*launcher|launcher.*copy|generate.*llxprt|llxprt.*generate' \
--glob '!**/node_modules/**' \
--glob '!**/.git/**' \
--glob '*.ts' --glob '*.js' --glob '*.cjs' --glob '*.json' \
--glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob 'Makefile' --glob 'Dockerfile' .
echo "--- package and integrity-test sections ---"
for f in package.json packages/cli/package.json packages/llxprt-cli-posix/package.json scripts/tests/publish-integrity.test.ts; do
if [ -f "$f" ]; then
echo "### $f"
case "$f" in
scripts/tests/publish-integrity.test.ts) sed -n '950,1030p' "$f" ;;
*) cat "$f" ;;
esac
fi
done
echo "--- exact file relationship ---"
python3 - <<'PY'
from pathlib import Path
a = Path("packages/cli/bin/llxprt").read_bytes()
b = Path("packages/llxprt-cli-posix/bin/llxprt").read_bytes()
print({"byte_identical": a == b, "bytes_a": len(a), "bytes_b": len(b)})
PYRepository: vybestack/llxprt-code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "--- tracked launcher metadata ---"
git ls-files -s -- packages/cli/bin/llxprt packages/llxprt-cli-posix/bin/llxprt
wc -l -- packages/cli/bin/llxprt packages/llxprt-cli-posix/bin/llxprt
sha256sum -- packages/cli/bin/llxprt packages/llxprt-cli-posix/bin/llxprt
echo "--- package scripts containing launcher or packaging references ---"
python3 - <<'PY'
import json
from pathlib import Path
for name in ("package.json", "packages/cli/package.json", "packages/llxprt-cli-posix/package.json"):
p = Path(name)
if not p.is_file():
continue
data = json.loads(p.read_text())
print(f"### {name}")
for k, v in data.get("scripts", {}).items():
text = v if isinstance(v, str) else json.dumps(v)
if any(term in text.lower() for term in ("llxprt", "package", "publish", "copy", "generate", "build")):
print(f"{k}: {text}")
PY
echo "--- relevant tracked references outside generated/coverage data ---"
git grep -n -E \
'packages/(cli/bin/llxprt|llxprt-cli-posix/bin/llxprt)|cli/bin/llxprt|llxprt-cli-posix/bin/llxprt' \
-- \
'*.ts' '*.tsx' '*.js' '*.cjs' '*.mjs' '*.json' '*.yml' '*.yaml' '*.sh' 'Makefile' \
':!**/coverage/**' ':!**/node_modules/**' \
| head -120
echo "--- integrity test names and launcher assertions ---"
rg -n -C4 \
'POSIX launcher|launcher|llxprt-cli-posix|cli/bin/llxprt|toEqual|toBe' \
scripts/tests/publish-integrity.test.ts | head -180Repository: vybestack/llxprt-code
Length of output: 14285
Add a CI check for launcher parity.
The two 704-line launchers are byte-identical, but no build or test step compares them. Add a parity test, or generate packages/llxprt-cli-posix/bin/llxprt during packaging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/llxprt-cli-posix/bin/llxprt` around lines 1 - 52, Add an automated
parity check for the POSIX launchers, comparing
packages/llxprt-cli-posix/bin/llxprt with its corresponding launcher and failing
CI when they differ. Integrate the check into the existing build or test
workflow, or generate the POSIX launcher from a single source during packaging
so both launchers remain byte-identical.
| for /f "delims=" %%A in ('powershell -NoProfile -Command "(Add-Type -MemberDefinition '[DllImport(\"kernel32.dll\")] public static extern bool IsProcessorFeaturePresent(int f);' -Name K -Namespace W -PassThru)::IsProcessorFeaturePresent(40)" 2^>nul') do set "OVEN_AVX2=%%A" | ||
| if /i "!OVEN_AVX2!"=="True" set "OVEN_VARIANTS=bun-windows-x64 bun-windows-x64-baseline" | ||
| if /i "!OVEN_AVX2!"=="False" set "OVEN_VARIANTS=bun-windows-x64-baseline" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear OVEN_AVX2 before the probe.
Line 83 assigns OVEN_AVX2 only when powershell produces output. setlocal copies the parent environment, so an inherited OVEN_AVX2=True survives into this scope. On a baseline x64 host without PowerShell, Line 84 then selects bun-windows-x64 first and the process dies with SIGILL. The documented fail-open behavior expects the baseline-first order in that case.
Initialize the variable before the for /f loop.
🐛 Proposed fix
+set "OVEN_AVX2="
for /f "delims=" %%A in ('powershell -NoProfile -Command "(Add-Type -MemberDefinition '[DllImport(\"kernel32.dll\")] public static extern bool IsProcessorFeaturePresent(int f);' -Name K -Namespace W -PassThru)::IsProcessorFeaturePresent(40)" 2^>nul') do set "OVEN_AVX2=%%A"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for /f "delims=" %%A in ('powershell -NoProfile -Command "(Add-Type -MemberDefinition '[DllImport(\"kernel32.dll\")] public static extern bool IsProcessorFeaturePresent(int f);' -Name K -Namespace W -PassThru)::IsProcessorFeaturePresent(40)" 2^>nul') do set "OVEN_AVX2=%%A" | |
| if /i "!OVEN_AVX2!"=="True" set "OVEN_VARIANTS=bun-windows-x64 bun-windows-x64-baseline" | |
| if /i "!OVEN_AVX2!"=="False" set "OVEN_VARIANTS=bun-windows-x64-baseline" | |
| set "OVEN_AVX2=" | |
| for /f "delims=" %%A in ('powershell -NoProfile -Command "(Add-Type -MemberDefinition '[DllImport(\"kernel32.dll\")] public static extern bool IsProcessorFeaturePresent(int f);' -Name K -Namespace W -PassThru)::IsProcessorFeaturePresent(40)" 2^>nul') do set "OVEN_AVX2=%%A" | |
| if /i "!OVEN_AVX2!"=="True" set "OVEN_VARIANTS=bun-windows-x64 bun-windows-x64-baseline" | |
| if /i "!OVEN_AVX2!"=="False" set "OVEN_VARIANTS=bun-windows-x64-baseline" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/llxprt-cli-win32/bin/llxprt.cmd` around lines 83 - 85, Clear or
initialize OVEN_AVX2 before the PowerShell probe loop so an inherited value
cannot affect variant selection when the probe produces no output. Update the
setup immediately before the for /f command, preserving the existing True/False
handling and ensuring the unset-probe case retains baseline-first fail-open
behavior.
| :oven_ready | ||
| set "WALK=!MAIN_PKG!" | ||
| :oven_loop | ||
| for %%V in (!OVEN_VARIANTS!) do ( | ||
| if exist "!WALK!\node_modules\@oven\%%V\bin\bun.exe" ( | ||
| set "BUN_EXE=!WALK!\node_modules\@oven\%%V\bin\bun.exe" | ||
| goto :bun_found | ||
| ) | ||
| ) | ||
| for %%I in ("!WALK!\..") do set "PARENT=%%~fI" | ||
| if "!PARENT!"=="!WALK!" goto :bun_pass3 | ||
| set "WALK=!PARENT!" | ||
| goto :oven_loop | ||
|
|
||
| :bun_pass3 | ||
| rem Pass 3: a Bun already present on PATH. | ||
| for /f "delims=" %%P in ('where bun.exe 2^>nul') do ( | ||
| set "BUN_EXE=%%P" | ||
| goto :bun_found | ||
| ) | ||
| for /f "delims=" %%P in ('where bun.cmd 2^>nul') do ( | ||
| set "BUN_EXE=%%P" | ||
| goto :bun_found | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This launcher validates no Bun version.
Passes 1, 2, and 3 accept the first bun.exe they find. The POSIX launcher applies _llxprt_bun_validates to every candidate, so a candidate whose package.json version does not equal the pin is rejected. On Windows, a stale @oven/bun-* package or an old PATH Bun is accepted, and the failure appears later as a runtime error from the CLI.
Read the "bun" pin from !MAIN_PKG!\package.json and compare it to the candidate package.json version for passes 1 and 2, to match the POSIX launcher behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/llxprt-cli-win32/bin/llxprt.cmd` around lines 86 - 109, Update the
Windows launcher’s Bun candidate selection in the :oven_ready search and the
preceding local-package pass to read the pinned “bun” version from
!MAIN_PKG!\package.json and validate each candidate’s package.json version
before setting BUN_EXE. Reject mismatched `@oven` Bun packages and PATH candidates
as appropriate, preserving the existing search order and fallback behavior while
matching the POSIX launcher’s validation.
| const host = detectHostPlatform(); | ||
| if (host === null) { | ||
| throw new Error('host detection failed'); | ||
| } | ||
| const variants = selectOvenVariants(host); | ||
| const fallback = variants[1] ?? variants[0]; | ||
| writeOvenPackage( | ||
| join(pkgRoot, 'node_modules'), | ||
| { packageName: fallback.packageName, exeName: fallback.exeNames[0] }, | ||
| realBunVersion(), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Guard the fallthrough test against single-variant hosts.
Line 686 uses variants[1] ?? variants[0]. If the host has only one candidate variant, the fallback is the variant that lines 674-676 just emptied. writeOvenPackage then re-creates the binary in that same package directory, so the test passes by restoring the first variant instead of falling through to a second one.
Single-variant hosts are real: darwin/arm64, linux/arm64 (glibc), and win32/arm64 each produce exactly one row from selectOvenVariants. On an Apple-silicon runner this test therefore asserts nothing about fallthrough.
Skip the test when a second variant does not exist.
🐛 Proposed fix
const variants = selectOvenVariants(host);
- const fallback = variants[1] ?? variants[0];
+ const fallback = variants[1];
+ if (fallback === undefined) {
+ // Single-variant host (e.g. darwin/arm64): there is no second variant
+ // to fall through to, so this scenario is not reachable here.
+ return;
+ }
writeOvenPackage(
join(pkgRoot, 'node_modules'),
{ packageName: fallback.packageName, exeName: fallback.exeNames[0] },
realBunVersion(),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const host = detectHostPlatform(); | |
| if (host === null) { | |
| throw new Error('host detection failed'); | |
| } | |
| const variants = selectOvenVariants(host); | |
| const fallback = variants[1] ?? variants[0]; | |
| writeOvenPackage( | |
| join(pkgRoot, 'node_modules'), | |
| { packageName: fallback.packageName, exeName: fallback.exeNames[0] }, | |
| realBunVersion(), | |
| ); | |
| const host = detectHostPlatform(); | |
| if (host === null) { | |
| throw new Error('host detection failed'); | |
| } | |
| const variants = selectOvenVariants(host); | |
| const fallback = variants[1]; | |
| if (fallback === undefined) { | |
| // Single-variant host (e.g. darwin/arm64): there is no second variant | |
| // to fall through to, so this scenario is not reachable here. | |
| return; | |
| } | |
| writeOvenPackage( | |
| join(pkgRoot, 'node_modules'), | |
| { packageName: fallback.packageName, exeName: fallback.exeNames[0] }, | |
| realBunVersion(), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/issue-2978-oven-fallback.bun.test.ts` around lines 681 - 691,
Update the fallthrough test around selectOvenVariants and the fallback
assignment to skip the test when variants.length is less than two, before
clearing or recreating any package. Only proceed with the existing variants[1]
fallback and writeOvenPackage flow when a genuine second variant exists.
| function placeOvenBun(nodeModules: string, variant: OvenVariant): string { | ||
| const dir = path.join(nodeModules, '@oven', variant, 'bin'); | ||
| mkdirSync(dir, { recursive: true }); | ||
| const exe = path.join(dir, 'bun.exe'); | ||
| copyFileSync(stubSrc, exe); | ||
| return exe; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the Windows .cmd launcher validates the `@oven` package.json version pin.
set -euo pipefail
fd -H -t f 'llxprt.cmd' packages | while IFS= read -r f; do
echo "===== $f ====="
cat -n "$f"
done
echo "===== POSIX launcher pin logic ====="
fd -H -t f 'llxprt' packages/llxprt-cli-posix/bin --exec rg -n -C4 'package\.json|version|pin|`@oven`' {}Repository: vybestack/llxprt-code
Length of output: 21424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Windows fixture and assertions ====="
cat -n scripts/tests/issue-2978-windows-launcher.bun.test.ts | sed -n '80,290p'
echo "===== POSIX mismatch test ====="
cat -n scripts/tests/issue-2978-oven-fallback.bun.test.ts | sed -n '620,680p'
echo "===== Bun dependency declarations ====="
rg -n -C3 '"bun"[[:space:]]*:' packages scripts | head -120Repository: vybestack/llxprt-code
Length of output: 19945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Relevant manifest files ====="
for f in packages/llxprt-code/package.json packages/llxprt-cli-win32/package.json package.json; do
if [ -f "$f" ]; then
echo "----- $f -----"
rg -n -C4 '"bun"|dependencies|optionalDependencies|peerDependencies' "$f"
fi
done
echo "===== Bun declarations across tracked manifests ====="
git ls-files '*package.json' | xargs -r rg -n -C2 '"bun"[[:space:]]*:[[:space:]]*"'
echo "===== Windows test helpers and version-related symbols ====="
rg -n -C3 'package\.json|version|pin|placeOvenBun|buildLayout' scripts/tests/issue-2978-windows-launcher.bun.test.tsRepository: vybestack/llxprt-code
Length of output: 246
Add Windows @oven pin validation.
llxprt.cmd executes @oven/<variant>/bin/bun.exe without checking its package.json version. This differs from the POSIX launcher, which rejects mismatched versions. Add equivalent Windows validation, write the matching manifest in placeOvenBun, and add a mismatched-version rejection test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/issue-2978-windows-launcher.bun.test.ts` around lines 136 -
142, Update placeOvenBun to write the expected version into the `@oven` variant
package.json alongside bun.exe, then add Windows launcher validation matching
the POSIX launcher’s version-pin behavior before executing the binary. Extend
the issue-2978 Windows tests with a mismatched-version case that confirms
llxprt.cmd rejects the package.
| function updateCliLauncherPlatformPins( | ||
| packageJsonPath: string, | ||
| version: string, | ||
| ): void { | ||
| const packageJson = readJson(packageJsonPath); | ||
| const optionalDeps = packageJson.optionalDependencies; | ||
| if (!isDependencyRecord(optionalDeps)) { | ||
| return; | ||
| } | ||
| let changed = false; | ||
| for (const pkg of CLI_LAUNCHER_PLATFORM_PACKAGES) { | ||
| if (optionalDeps[pkg] !== undefined && optionalDeps[pkg] !== version) { | ||
| optionalDeps[pkg] = version; | ||
| changed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail loudly when a launcher pin is absent.
updateCliLauncherPlatformPins returns at Line 190 when optionalDependencies is missing. It also skips any package whose key is absent at Line 194. The block comment states that a skew leaves the consumer with no llxprt command. A release script must not pass silently when this invariant cannot be applied.
Throw when optionalDependencies is missing, or when a package in CLI_LAUNCHER_PLATFORM_PACKAGES has no entry.
🐛 Proposed fix to enforce the pin invariant
const packageJson = readJson(packageJsonPath);
const optionalDeps = packageJson.optionalDependencies;
if (!isDependencyRecord(optionalDeps)) {
- return;
+ throw new Error(
+ `${packageJsonPath} has no "optionalDependencies"; the CLI launcher platform pins cannot be updated.`,
+ );
}
let changed = false;
for (const pkg of CLI_LAUNCHER_PLATFORM_PACKAGES) {
- if (optionalDeps[pkg] !== undefined && optionalDeps[pkg] !== version) {
+ if (optionalDeps[pkg] === undefined) {
+ throw new Error(
+ `${packageJsonPath} is missing the optionalDependencies entry "${pkg}"; the launcher pin would drift.`,
+ );
+ }
+ if (optionalDeps[pkg] !== version) {
optionalDeps[pkg] = version;
changed = true;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/version.ts` around lines 183 - 198, Update
updateCliLauncherPlatformPins to fail loudly when the pin invariant cannot be
applied: throw if optionalDependencies is not a valid dependency record, and
throw if any package in CLI_LAUNCHER_PLATFORM_PACKAGES has no entry. Continue
updating present pins whose versions differ, preserving the existing changed
tracking for valid inputs.
| // 5b. Re-pin the os-gated CLI launcher platform packages in lockstep with the | ||
| // release version so packages/cli's optionalDependencies can never reference a | ||
| // version that does not exist on the registry (issue #2978). | ||
| updateCliLauncherPlatformPins(cliPackageJsonPath, newVersion); | ||
|
|
||
| // 5c. Bump the os-gated launcher packages' own version field. They are not | ||
| // workspaces (issue #2978), so the workspace-driven bump does not reach them; | ||
| // their own version must stay in lockstep with packages/cli's pin. | ||
| updateCliLauncherPackageVersions(newVersion); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bump the launcher manifests before the pin rewrite.
Step 5b writes packages/cli/package.json first. Step 5c then writes the two launcher manifests. If step 5c fails (missing directory, unwritable file), the pin is already updated and the launcher manifest versions are stale. That is the exact skew this code prevents.
Validate that both launcher manifests exist and are writable before step 5b runs, or run updateCliLauncherPackageVersions first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/version.ts` around lines 239 - 247, Reorder the release updates so
launcher manifest versions are successfully updated before rewriting
packages/cli’s optional dependency pins: call
updateCliLauncherPackageVersions(newVersion) before
updateCliLauncherPlatformPins(cliPackageJsonPath, newVersion). Preserve the
existing lockstep versioning behavior and comments for steps 5b and 5c.
| function ovenDarwinHasAvx2() { | ||
| try { | ||
| const r = require('child_process').spawnSync( | ||
| 'sysctl', | ||
| ['-n', 'machdep.cpu'], | ||
| { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| }, | ||
| ); | ||
| return r.status === 0 && (r.stdout || '').includes('AVX2'); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
[bug/high] In
ovenDarwinHasAvx2(),sysctl -n machdep.cpureturns the CPU brand string (e.g., "Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz"), not CPU feature flags. The returned string never contains "AVX2", so this function always returnsfalseon Intel Macs.Effect: Intel Macs with AVX2 support are always classified as baseline, causing them to receive the non-AVX2
bun-darwin-x64-baselinevariant instead of the optimizedbun-darwin-x64. This is a silent performance regression on a large class of macOS hardware.Fix: Query the actual feature flags. On macOS,
machdep.cpu.featuresandmachdep.cpu.leaf7_featurescontain AVX2. Update the sysctl key and include both in the check. Example:sysctl -n machdep.cpu.features machdep.cpu.leaf7_featuresand search for "AVX2" in the combined output.There are no tests covering
ovenDarwinHasAvx2or macOS AVX2 detection in the new test files, so this regression has no test coverage protecting against reintroduction.
| if _llxprt_path_bun_version=$(bun --version 2>/dev/null) && \ | ||
| [ -n "$_llxprt_path_bun_version" ] && \ | ||
| _llxprt_version_ge "$_llxprt_path_bun_version" "$_llxprt_bun_pin"; then | ||
| # Issue #3021: an ad-hoc or cdhash-only signed PATH Bun cannot satisfy the | ||
| # identity-based macOS Keychain ACL, so every credential read degrades to a | ||
| # login-password prompt that "Always Allow" cannot persist (#3020 sealed | ||
| # change_acl with an empty application list). Inspect the designated | ||
| # requirement of the exact binary selected and warn once unless it carries | ||
| # Oven's exact team-identity clause (certificate leaf[subject.OU] = | ||
| # "7FRXF46ZSN"), which is the OU stored in the existing Keychain ACL. The | ||
| # warning is advisory: a Bun that is ad-hoc signed or has no team identity | ||
| # runs llxprt correctly except for Keychain access, and skipping it would | ||
| # silently restore the npm-unlink failure mode #2962 exists to prevent. | ||
| if _llxprt_path_bun_exe=$(command -v bun 2>/dev/null) && \ | ||
| [ -n "$_llxprt_path_bun_exe" ]; then | ||
| # Suppress the warning only when codesign succeeds AND its output carries | ||
| # Oven's exact team-identity clause. A failing codesign (unsigned binary, | ||
| # or an error whose diagnostic merely echoes the clause), or a successful | ||
| # inspection signed by any other team, must still warn — the Keychain ACL | ||
| # stores Oven's OU (7FRXF46ZSN) and only a binary signed by that exact | ||
| # team can satisfy it. | ||
| _llxprt_has_team_id=0 | ||
| if _llxprt_dr=$(codesign -d --requirements - "$_llxprt_path_bun_exe" 2>&1); then | ||
| case "$_llxprt_dr" in | ||
| *certificate\ leaf\[subject.OU]\ =\ \"7FRXF46ZSN\"*) _llxprt_has_team_id=1 ;; | ||
| *) ;; | ||
| esac | ||
| fi | ||
| if [ "$_llxprt_has_team_id" -eq 0 ]; then | ||
| printf '%s\n' 'LLxprt Code: the Bun on your PATH is ad-hoc signed or otherwise' >&2 | ||
| printf '%s\n' 'lacks a stable team identity, so it cannot hold a persistent macOS' >&2 | ||
| printf '%s\n' 'Keychain grant. You will be prompted for your login password on every' >&2 | ||
| printf '%s\n' 'credential read, and "Always Allow" will not persist (#3020).' >&2 | ||
| printf '%s\n' 'Install the official Bun release signed by Oven:' >&2 | ||
| printf '%s\n' ' brew uninstall bun && brew install oven-sh/bun/bun' >&2 | ||
| printf '%s\n' ' curl -fsSL https://bun.com/install | bash' >&2 | ||
| fi | ||
| fi | ||
| exec bun "$_llxprt_entry" "$@" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
[bug/medium] The macOS codesign advisory warning for PATH
bun(issue #3021) was removed, but the corresponding runtime behavior was not changed: this script stillexec bun "$_llxprt_entry" "@"on macOS when the version pin is satisfied. That means macOS users who fall back to an ad-hoc-signed PATH Bun still silently lose persistent Keychain grants. Removing the warning without removing or replacing the PATH fallback creates a behavioral regression: the user-visible failure mode still exists, but the launcher no longer warns about it. Restore the warning on this code path, or remove the PATHbunfallback on macOS if the project no longer wants to tolerate that Keychain limitation.
| if [ -x "$_llxprt_pkg_root/node_modules/bun/bin/bun.exe" ] && \ | ||
| _llxprt_bun_validates "$_llxprt_pkg_root/node_modules/bun/bin/bun.exe"; then | ||
| _llxprt_bun=$_llxprt_pkg_root/node_modules/bun/bin/bun.exe | ||
| fi | ||
|
|
||
| # 1b. @oven fallback (issue #2978): package-local @oven/bun-<platform> variant. | ||
| # Probed only when bun/bin/bun.exe was absent (npm v12 default-deny blocked | ||
| # bun's postinstall). Detection (uname, sysctl, /proc/cpuinfo) runs lazily | ||
| # inside _llxprt_probe_oven, never on a normal install. | ||
| if [ -z "$_llxprt_bun" ]; then | ||
| _llxprt_probe_oven "$_llxprt_pkg_root/node_modules" | ||
| fi | ||
|
|
||
| # 2. Hoisted Bun within the enclosing node_modules (installed packages only). | ||
| if [ -z "$_llxprt_bun" ] && _llxprt_find_enclosing_nm; then | ||
| if [ -x "$_llxprt_enclosing_nm/bun/bin/bun.exe" ] && \ | ||
| _llxprt_bun_validates "$_llxprt_enclosing_nm/bun/bin/bun.exe"; then | ||
| _llxprt_bun=$_llxprt_enclosing_nm/bun/bin/bun.exe | ||
| fi | ||
| # 2b. @oven fallback: hoisted @oven variant within enclosing node_modules. | ||
| if [ -z "$_llxprt_bun" ]; then | ||
| _llxprt_probe_oven "$_llxprt_enclosing_nm" | ||
| fi | ||
| fi | ||
|
|
||
| # 3. Workspace-root Bun (source workspace only). The package is NOT under a | ||
| # node_modules; verify the repository root two levels up | ||
| # (packages/cli -> packages -> repo-root) is a genuine workspace whose | ||
| # manifest references this package, then accept only its node_modules/bun. | ||
| if [ -z "$_llxprt_bun" ] && ! _llxprt_find_enclosing_nm; then | ||
| _llxprt_ws_candidate=$(cd -- "$_llxprt_pkg_root/../.." 2>/dev/null && pwd) || \ | ||
| _llxprt_ws_candidate="" | ||
| if [ -n "$_llxprt_ws_candidate" ] && \ | ||
| _llxprt_verify_workspace_root "$_llxprt_ws_candidate" && \ | ||
| [ -x "$_llxprt_ws_root/node_modules/bun/bin/bun.exe" ] && \ | ||
| _llxprt_bun_validates "$_llxprt_ws_root/node_modules/bun/bin/bun.exe"; then | ||
| _llxprt_bun=$_llxprt_ws_root/node_modules/bun/bin/bun.exe | ||
| fi | ||
| # 3b. @oven fallback: workspace-root @oven variant. | ||
| if [ -z "$_llxprt_bun" ] && [ -n "${_llxprt_ws_root:-}" ]; then | ||
| _llxprt_probe_oven "$_llxprt_ws_root/node_modules" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
[bug/medium] Steps 1, 2, and 3 hardcode
node_modules/bun/bin/bun.exefor the legacybunnpm package. On POSIX platforms thebunnpm package installs the runtime asbin/bun(no extension), while on win32 it isbin/bun.exe. Because this launcher only probesbun.exe, a POSIX install where thebunpostinstall actually ran (npm v11 or explicit script allowance) will never discover the valid local binary and will fall through to@ovenvariants unnecessarily. This contradicts the documented resolver order and wastes the bundled runtime when it is present. Probe bothbun.exeandbun(andbun.cmdon win32) for the legacybunpackage, matching the platform-aware candidate list indirectDependencyCandidatesForPlatform.
| itNeedsSymlinks( | ||
| 'accepts a hoisted @oven binary within enclosing node_modules', | ||
| () => { |
There was a problem hiding this comment.
[maintainability/medium] This test is gated with
itNeedsSymlinks, which skips it on Windows. However, the test body never creates or uses symlinks — it only creates directories, copies files, and spawns the launcher. The correct gate isitPosix, because the test spawns the POSIX shell launcher (#!/bin/sh) which does not exist on stock Windows. UsingitNeedsSymlinksis misleading about why the test is skipped and could confuse future maintainers into thinking symlink support is required when it isn't.
| describePosixOnly( | ||
| 'issue #2978 @oven fallback — resolveBunExe parity (.cjs)', | ||
| () => { |
There was a problem hiding this comment.
[maintainability/medium] This
describeblock is wrapped indescribePosixOnly, which skips the entire suite on Windows. However, the file-level comment at lines 78-79 explicitly states: "Pure-logic, manifest, detection, and resolveBunExe tests still run everywhere because they never spawn the launcher." The.cjsresolver is platform-agnostic filesystem logic and should be validated on Windows too, especially since this PR modifies Windows entry points. Skipping on Windows creates a coverage gap for Windows-specific path-handling bugs inresolveBunExe. ReplacedescribePosixOnlywith a plaindescribeso these tests run on all platforms.
| interface PlatformRow { | ||
| readonly os: string; | ||
| readonly arch: string; | ||
| readonly avx2?: boolean; | ||
| readonly abi?: string; | ||
| readonly bin: string; | ||
| readonly exe: string; | ||
| } |
There was a problem hiding this comment.
[bug/low] The
abifield onPlatformRowis typed asstring | undefined, but it is only ever assigned the literal values'musl','android', orundefinedinPLATFORM_TABLE. The file already definesHostAbi = 'musl' | 'android' | undefinedfor this exact purpose, but the interface erases that union to a broadstring. This weakens type safety: any string could be assigned without a TypeScript error, and downstream comparisons likerow.abi === hostAbilose the narrowing benefit of a discriminated union. Change the field toreadonly abi?: HostAbi;to keep the platform table and host detection types aligned.
…file The sibling-package entry walk added for #2978 could never terminate: POSIX permits pwd to report a leading '//' as a distinct pathname, so 'cd ..' from the root alternates / -> // -> / and the parent-equals-current guard never fires. The launcher spun until killed, so issue-2603-launcher-hardening never reached the exit-43 guard and saw status null instead of 43. Normalise a leading '//', stop explicitly at the root, and cap the walk depth. Reproduced under sh and confirmed the scenario now exits 43. Also split the issue-#2978 launcher invariants out of publish-integrity.test.ts, which had crossed ESLint's 800 non-blank/non-comment max-lines limit at 821.
The release-like pack/install smoke installed the CLI tarball but produced no llxprt bin at all: global and local bin links were missing and every --version invocation failed with ENOENT. The os-gated launcher packages (@vybestack/llxprt-cli-posix and -win32) are deliberately NOT root npm workspaces, because npm enforces a workspace's os field and would fail EBADPLATFORM on every platform. The smoke helper enumerated only rootPkg.workspaces, so the launchers were never packed, their optionalDependencies were never rewritten to file: tarballs, and npm silently skipped the unresolvable optional deps. Pack them explicitly, and pack every internal package by directory rather than via npm pack -w <name>, which fails No workspaces found for exactly these non-workspace packages. Packing inside the package directory works uniformly for workspace and non-workspace packages alike.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/issue-2978-launcher-exec-bit.test.ts (1)
61-65: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the redundant exact-pin loop.
The later lockstep assertion already requires each specifier to equal
cli.version, so it rejects npm ranges such as1and1.0.x.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/issue-2978-launcher-exec-bit.test.ts` around lines 61 - 65, Remove the redundant exact-pin validation loop around the first-character check in the test, while retaining the later lockstep assertion that compares each optional dependency specifier with cli.version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/tests/issue-2978-launcher-exec-bit.test.ts`:
- Around line 61-65: Remove the redundant exact-pin validation loop around the
first-character check in the test, while retaining the later lockstep assertion
that compares each optional dependency specifier with cli.version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b140d481-fa50-4e56-9ec9-10aa6c139ca5
📒 Files selected for processing (5)
packages/cli/bin/llxprtpackages/llxprt-cli-posix/bin/llxprtscripts/tests/issue-2603-release-pack.cjsscripts/tests/issue-2978-launcher-exec-bit.test.tsscripts/tests/publish-integrity.test.ts
💤 Files with no reviewable changes (1)
- scripts/tests/publish-integrity.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/llxprt-cli-posix/bin/llxprt
- packages/cli/bin/llxprt
npm links only the installed package's own bin entries on a global install, never a dependency's. Moving the llxprt bin into the os-gated platform packages therefore left 'npm i -g @vybestack/llxprt-code' with no llxprt command at all, which is why only the two global smoke checks failed in CI while local-install and npx passed. Reinstate bin.llxprt in packages/cli pointing at a self-sufficient #!/usr/bin/env node shim. A node shebang is what npm v12's cmd-shim can wrap correctly on Windows; the original #2978 bug was a /bin/sh shebang producing a .cmd that invokes a nonexistent /bin/sh. The shim resolves the bundled @oven/bun-<variant> binary itself so it stays correct regardless of whether the platform packages are retired, preserves the launcher entry precedence (LLXPRT_FORCE_SOURCE_ENTRY, then bundle, then source), forwards argv and signals, and exits 43 with a diagnostic when Bun or the entry is missing. Invariant tests that asserted packages/cli declares no bin are inverted to the new contract and strengthened with a shebang regression guard.
Remediates the actionable findings from the review of dec99ec: - eslint.config.js: drop the redundant process/console global overrides (both are already provided by globals.node), and drop the no-unused-vars exception block entirely -- the shim has no unused bindings, so the guardrail does not need weakening for it. - issue-2978-launcher-exec-bit.test.ts: trim the shebang line before comparing. split('\n', 1)[0] leaves a trailing CR on a CRLF checkout, which made the assertion fail for a file that was actually correct. - issue-2978-node-shim.bun.test.ts: use let rather than var in the generated fixture entry, and guard the bundled bun stand-in behind requireStubBun() so a missing or relocated bun package fails with an actionable message instead of an opaque ENOENT from copyFileSync. Two review findings about @oven variant selection (Android hosts report process.platform === 'linux', and musl detection only probes /etc/alpine-release) are deliberately NOT addressed here: the shim mirrors packages/cli/scripts/install-native-launchers.cjs exactly, so changing only one of the two resolvers would make them disagree about which Bun variant to select. Both are pre-existing and belong in a separate change that fixes them together. A third finding claimed the shim relies on the global package root to find a bundled Bun; it already walks its own node_modules ancestry first (resolveBun -> ancestorNodeModulesDirs(shimDir)), so there was nothing to change.
1 participant
fixes #2978
Why
npm v12 (RFC 0054) stops running dependency lifecycle scripts by default. Verified in the v12 arborist source, bin linking is explicitly not gated:
where
scriptsAllowed = dangerouslyAllowAllScripts || node.isLink || node.isWorkspace || isScriptAllowed(...) === true.Because
isLink/isWorkspacebypass the gate, monorepo andnpm linkinstalls never showed this bug — only registry consumers are affected, which is why it went unnoticed. Phase-1 advisory warnings already ship in npm 11.16.0.This breaks the CLI in two independent ways. Both are fixed here, in one PR.
Fix 1 — Bun runtime supply
The
bunnpm package is a stub whosepostinstall(node install.js) downloads the real binary. Denied ⇒node_modules/bun/bin/bun[.exe]never materializes ⇒ dangling bin link.We now declare the 16
@oven/bun-<platform>packages as exact-pinnedoptionalDependencies. These carry the binary as tarball content rather than fetching it (verified:scripts: {}completely empty,bin: undefined,os/cpufilters present), so npm installs exactly one and no script has to run.Resolver order, applied consistently across
bun-path-resolver.ts,oven-bun-variants.tsandinstall-native-launchers.cjs: bundled bun →@ovenvariants → hoisted/ancestor locations →PATH. Each candidate is existence-checked and falls through.Fix 2 — Windows entry point
packages/cli/bin/llxprtstarts with#!/bin/sh. With our own postinstall denied, npm falls back to its cmd-shim, which is generated from that shebang — producing a Windows.cmdthat invokes/bin/sh, which does not exist. Thellxprtcommand was simply broken on Windows.Measured cmd-shim behaviour on npm 11.16.0:
#!/bin/sh(what we shipped).cmdreferences/bin/sh— broken#!/usr/bin/env node#!/usr/bin/env bun.cmd, no shebangA
binfield admits only one target, so there is no per-OS mapping. The fix is to stop declaringbinonpackages/cliand move it into two os-gated packages that each declarebin.llxprt:@vybestack/llxprt-cli-posix—os: [darwin, linux, freebsd], ships the existing sh launcher byte-identical@vybestack/llxprt-cli-win32—os: [win32], ships a native batch launcher with no node dependencyBoth are exact-pinned
optionalDependenciesofpackages/cliin version lockstep.packages/cli/bin/llxprtis retained because the rootpackage.jsonisprivate: true(dev-only, never published) and still points itsbinthere.Proven, not assumed
A real
npm installon win32 confirmed the arrangement:npm exited 0 with no warnings, no bin collision, and the posix child correctly skipped by the
osfilter. No hand-written.ps1is needed — npm generates one that delegates to our.cmd.Bugs found and fixed along the way
!RC!without delayed expansion enabled, so every failure reported success.packages/cliwhen deriving the workspace root, disabling its workspace Bun fallback for any other package directory. Now derived dynamically viabasename. (Found by code review.).gitattributesnow pins*.cmdto CRLF. The repo sets* text=auto eol=lfglobally; an LF-only batch file is mis-parsed bycmd.exeand silently breaks the published Windows entry point. This actually happened mid-development and failed all 8 launcher tests.git check-attrconfirms.cmd → crlfand both sh launchers →lf.Release safety
release.ymlandscripts/version.tspublish the two launcher packages in exact version lockstep with the parent, and before it — the parent's exact pins must already exist on the registry or installs fail.publish-integrity.test.tsnow asserts that lockstep, so a skewed version bump cannot ship a CLI with nollxprtcommand at all.npm pack --dry-runconfirms each new package actually ships its bin payload (4 files each), not an empty tarball.Verification
npm run lintnpm run typechecknpm run formatnpm run buildNew behavioural tests (
issue-2978-windows-launcher.bun.test.ts, 8 cases) build real temp package trees and exercise a real bun binary — no mock theater, perdev-docs/RULES.md.Pre-existing failures, not caused by this PR:
packages/providers/.../e2e-credential-flow.test.ts"Scenario 7: Connection Loss" (30s timeout) was A/B'd against baseline8c8bf865din a separate worktree and fails identically there;git diff 8c8bf865dshows zero changes topackages/providers,core,storageorauth. Also pre-existing: 2 failures inpackages/storage/test-bun/credential-write-lock.bun.ts.An
ocrreview over 17 files returned 5 findings: 2 disproven as false positives, 1 real defect fixed (the hardcodedpackages/cliabove), 1 assertion-symmetry improvement accepted, 1 low-severity LICENSE boilerplate note matching the repo root's existing form.Design rationale and the full evidence log are recorded in
project-plans/issue2978/WINDOWS-ENTRYPOINT-RESEARCH.md.Summary by CodeRabbit
New Features
Documentation
Bug Fixes