Skip to content

Fix bundled mode detection for bun-compiled sidecars - #440

Closed
LeoLB-Wang wants to merge 1 commit into
NanmiCoder:mainfrom
LeoLB-Wang:pr/bundled-mode-detection-upstream
Closed

Fix bundled mode detection for bun-compiled sidecars#440
LeoLB-Wang wants to merge 1 commit into
NanmiCoder:mainfrom
LeoLB-Wang:pr/bundled-mode-detection-upstream

Conversation

@LeoLB-Wang

Copy link
Copy Markdown

Summary

  • 修复 isInBundledMode() 对 Bun 编译产物的误判,不再只依赖 Bun.embeddedFiles
  • 通过 Bun 虚拟入口路径识别 bundled mode,确保桌面端 claude-sidecar 继续走内嵌 rg
  • 为 bundled mode 判定补充同区域测试,并复用共享的 Bun 虚拟路径判断逻辑

Feature Quality Contract

  • Changed surface: cli-core / provider-runtime
  • Tests added or updated:
    • bun test ./src/utils/__tests__/bundledMode.test.ts
    • bun test ./src/utils/__tests__/ripgrep.test.ts
  • Coverage evidence:
    • 上述定向测试均已通过
    • bun run verify 已执行
    • server-checks 通过,见 artifacts/quality-runs/2026-05-14T05-49-29-566Z/logs/server-checks.log,结果为 668 pass / 0 fail
    • 全量 verify 当前被 persistence-upgrade lane 的仓库既有失败阻断,详见下文
  • E2E / live-model evidence:
    • 未执行 live provider / live model
    • 本次修复使用运行时路径单元测试和本地 Bun compile 复现实验作为证据
  • Known risk / rollback:
    • 该改动属于 cli-core,需要维护者审核并添加 allow-cli-core-change
    • 如需回滚,仅需回退本 PR 的 bundled mode 判定改动

Verification

  • I ran the relevant local checks, or explained why they do not apply.
  • I added or updated same-area tests for every production behavior change.
  • I ran bun run verify for code changes, including the coverage gate.
  • New or changed executable production lines meet the changed-line coverage threshold, or the blocker/maintainer override is documented.
  • I attached or summarized the quality report path, JUnit/log artifact path, and pass/fail/skip counts.
  • I ran E2E/live smoke for cross-boundary, provider/runtime, desktop chat, agent-loop, native, or release changes, or documented the blocker.

实际执行命令

bun test ./src/utils/__tests__/bundledMode.test.ts
bun test ./src/utils/__tests__/ripgrep.test.ts
bun run verify

额外验证证据

  • 本地 Bun compile 复现实验(Bun 1.3.14):
    • bun probe.ts 时,process.argv[1] 是真实文件系统脚本路径,且 Bun.embeddedFiles.length === 0
    • bun build --compile probe.ts && ./probe-bin 时,process.argv[1] 变为 /$bunfs/root/probe-bin,但 Bun.embeddedFiles.length === 0
  • 这说明 Bun.embeddedFiles.length > 0 不是可靠的 bundled mode 判定条件

Quality report / blocker

  • bun run verify 报告目录:
    • artifacts/quality-runs/2026-05-14T05-49-29-566Z/
  • 已通过的关键 lane:
    • artifacts/quality-runs/2026-05-14T05-49-29-566Z/logs/server-checks.log
  • 当前 blocker:
    • artifacts/quality-runs/2026-05-14T05-49-29-566Z/logs/persistence-upgrade.log
    • desktop/src/lib/persistenceMigrations.test.ts 失败,报错为 TypeError: window.localStorage.clear is not a function
  • 我已在干净的 origin/main worktree 上单独执行同一条 desktop 测试,并复现完全相同的失败,说明这是仓库当前基线问题,不是本 PR 引入

Risk

  • This PR does not touch CLI core paths, or it has maintainer approval for allow-cli-core-change.
  • Production code changes include matching tests, or have maintainer approval for allow-missing-tests.
  • Coverage baseline/threshold changes have maintainer approval for allow-coverage-baseline-change.
  • Quarantined tests still have owners, exit criteria, and unexpired review windows.
  • Provider/runtime changes were covered by mock contract tests, and live smoke was run or explicitly deferred.

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 14, 2026
@github-actions

Copy link
Copy Markdown

PR quality triage

Changed areas: area:cli-core

CLI core policy: Blocked by policy until a maintainer applies allow-cli-core-change and approves the PR.

Missing-test policy: No missing-test policy block detected.

Coverage baseline policy: No coverage-baseline policy block detected.

CLI core files:

  • src/utils/__tests__/bundledMode.test.ts
  • src/utils/bundledMode.ts
  • src/utils/ripgrep.ts

Coverage policy files:

  • none

Expected checks:

  • change-policy
  • server-checks
  • coverage-checks

Test coverage signals:

  • Agent/model runtime path changed: use mock/request-shape tests in PR and maintainer live-model smoke before release.

Risk notes:

  • No special risk notes from changed paths.

Hard merge gates still come from GitHub Actions, not AI review.

Dosu handoff: Dosu can be used as the AI reviewer for risk explanation, missing-test prompts, and maintainer Q&A. If it does not comment automatically from the PR template, ask:

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

@dosubot

dosubot Bot commented May 14, 2026

Copy link
Copy Markdown

Here's my review across the requested dimensions:

Changed-Area Risk: 🟡 Medium

The fix is additive — it adds an OR condition (isBunVirtualPath(process.argv[1])) rather than replacing the existing Bun.embeddedFiles check, which remains as a fallback [1]. isInBundledMode() is called by ~12+ callers (ripgrep, MCP server paths, image processor, installation type detection), so the detection logic is high-leverage. However, the OR approach means existing paths that worked before will continue to work.

The consolidation of isBunVirtualPath() from ripgrep.ts into bundledMode.ts is a clean refactor that eliminates duplication [1]. One subtle difference: the old ripgrep version's markers were ['$bunfs', '~BUN'] (no leading slash), while the new shared version uses ['/$bunfs/', '/~bun/'] with leading slashes and lowercase. This is intentional (path is lowercased before matching), but worth confirming the Windows ~BUN case still matches after lowercasing → ~bun ✅.

Test Coverage: 🟢 Good

New bundledMode.test.ts covers the critical edge case (empty Bun.embeddedFiles + virtual entrypoint) and cross-platform path detection [1]. Existing ripgrep.test.ts provides indirect coverage via isUsableBuiltinRipgrepPath() .

Minor gaps:

  • No explicit test for process.argv[1] being undefined (though isBunVirtualPath handles null | undefinedfalse)
  • No test for the non-Bun runtime path (typeof Bun === 'undefined') — trivial but would be nice for completeness

Documentation Impact: 🟢 Low

Internal implementation detail only. The inline JSDoc comment explaining why Bun.embeddedFiles is insufficient is helpful for future maintainers. No user-facing docs need updating.

Desktop Startup Risk: 🟡 Medium

This is the most important dimension. The ripgrep fallback chain [2] uses isInBundledMode() to decide whether to dispatch via argv0='rg' (embedded mode). A false negative here means the desktop sidecar falls back to system/builtin ripgrep — which may not exist on end-user machines.

Before this PR: Bun-compiled sidecars with empty embeddedFiles → false negative → search broken.
After this PR: Virtual path check catches these correctly → embedded ripgrep dispatches properly.

The risk direction is toward correctness, not away from it. The remaining concern is a false positive (regular Bun dev scripts whose argv[1] somehow contains /$bunfs/ or /~bun/), which is extremely unlikely in practice.

CLI Core Impact: 🟡 Medium

The PR touches src/utils/bundledMode.ts which is in the cli-core surface (as noted in the PR description). The resolveBundledCliPathFromExecPath() function for CLI subprocess invocation checks process.execPath basename, not isInBundledMode() directly [3], so CLI subprocess spawning is not directly affected. Ripgrep availability in CLI mode is affected, but in the correct direction (fixing false negatives).


Overall assessment: This is a well-scoped, low-risk fix that addresses a real correctness issue. The additive OR logic ensures no regressions for existing working paths, and the new tests validate the critical edge case. The allow-cli-core-change label is needed per policy.

Recommend: ✅ Approve after manual desktop startup verification on macOS/Windows confirms embedded ripgrep dispatches correctly with the updated detection.

To reply, just mention @dosu.


Share context across your team and agents. Try Dosu.

Leave Feedback Learn about cc-haha with Dosu Add Dosu to your team

@LeoLB-Wang
LeoLB-Wang force-pushed the pr/bundled-mode-detection-upstream branch from db6f900 to 787a4fc Compare May 14, 2026 06:25
@LeoLB-Wang LeoLB-Wang closed this May 14, 2026
@LeoLB-Wang
LeoLB-Wang deleted the pr/bundled-mode-detection-upstream branch May 14, 2026 06:29
@dosubot dosubot Bot mentioned this pull request May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli-core needs-maintainer-approval size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant