Skip to content

fix(scripts): spawn bench runners under process.execPath (#83921)#84304

Closed
hclsys wants to merge 1 commit into
openclaw:mainfrom
hclsys:fix-bench-spawner-use-execpath-83921
Closed

fix(scripts): spawn bench runners under process.execPath (#83921)#84304
hclsys wants to merge 1 commit into
openclaw:mainfrom
hclsys:fix-bench-spawner-use-execpath-83921

Conversation

@hclsys
Copy link
Copy Markdown
Contributor

@hclsys hclsys commented May 19, 2026

Fixes #83921.

scripts/test-cli-startup-bench-budget.mjs:108 and scripts/test-update-cli-startup-bench.mjs:67 both invoked spawnSync("node", args, …). The first argument is resolved through PATH. In environments using version managers (nvm, fnm, volta, asdf) the PATH-resolved node is a shim that may resolve to a different Node version than the one currently executing the parent script. The bench runner can then run on Node X while the parent runs on Node Y — measurements stop being comparable to the baseline, and the fixture-updater writes a fixture that no other run can reproduce.

The sibling scripts scripts/bench-cli-startup.ts and scripts/check-cli-startup-memory.mjs already use process.execPath for exactly this reason. The inconsistency is what the issue calls out as subtle: the underlying bench harness uses process.execPath for the CLI-under-test, but the runner / updater that drives it doesn't.

Changes

  • scripts/test-cli-startup-bench-budget.mjs: change spawnSync("node", args, …) to spawnSync(process.execPath, args, …) in resolveCurrentReportPath's spawn block. Inline comment cites the sibling pattern in bench-cli-startup.ts / check-cli-startup-memory.mjs and the issue.
  • scripts/test-update-cli-startup-bench.mjs: identical change in the top-level spawn at line 67.

Diff stat: 2 files, +13 / -2. No package.json, dependency, or interface change.

Real behavior proof

  • Behavior or issue addressed: Sanitized issue evidence — process.execPath is always the absolute path of the currently-executing Node binary, regardless of PATH state. The OpenClaw scripts dir already uses this pattern at bench-gateway-startup.ts:842, bench-gateway-restart.ts:1285, write-cli-startup-metadata.ts:208,246, openclaw-npm-postpublish-verify.ts:581, and bench-cli-startup.ts.

  • Real environment tested: Local Node 22.x running under nvm (/home/<user>/.nvm/versions/node/v22.22.0/bin/node) — the exact polyglot environment the issue's reproduction step calls out. Probe at /tmp/probe_83921.mjs does both halves of the proof. (a) Parses both patched scripts and verifies (i) spawnSync(process.execPath, args appears in each, (ii) no spawnSync("node", args calls remain. (b) Replays the structural difference between PATH-resolved node and process.execPath on the test machine — confirms process.execPath is an absolute path (immune to PATH/shim drift), that running spawnSync(process.execPath, ["--version"]) returns a valid version string, and that the sibling script bench-cli-startup.ts already uses the same pattern.

  • Exact steps or command run after this patch: node /tmp/probe_83921.mjs

  • Evidence after fix:

PASS: test-cli-startup-bench-budget.mjs uses process.execPath as the spawn binary
PASS: no remaining bare-'node' spawnSync in test-cli-startup-bench-budget.mjs
PASS: test-update-cli-startup-bench.mjs uses process.execPath as the spawn binary
PASS: no remaining bare-'node' spawnSync in test-update-cli-startup-bench.mjs
PASS: process.execPath is an absolute path (/home/.../v22.22.0/bin/node) — immune to PATH/shim drift
PASS: spawnSync(process.execPath, ['--version']) returned v22.22.0 — verified runnable
PASS: sibling bench-cli-startup.ts uses process.execPath in 1 location(s) — pattern mirrored correctly
  • Observed result after fix: Both bench runners now spawn the child Node process under the same binary as the parent, eliminating the version-mismatch class of bug entirely. No behavior change on systems where node (PATH) and process.execPath already agree (e.g., system Node, single-version Docker images).

  • What was not tested: A live multi-version nvm switch demonstrating the buggy shape — the issue's repro requires nvm shell activation pointing to a different version than the parent script. The probe demonstrates the structural mechanism that makes the bug impossible (absolute-path-driven spawn rather than PATH lookup), which is the actual fix being made.

Audit (per CLAUDE rules — all 5 steps)

  • Existing-helper check: process.execPath is the codebase's canonical answer to this concern — already used in 5+ sibling scripts including bench-cli-startup.ts, bench-gateway-startup.ts, bench-gateway-restart.ts, check-cli-startup-memory.mjs, write-cli-startup-metadata.ts. The fix is "use the existing primitive" not "add a helper." PASS
  • Shared-helper caller check: These are top-level scripts not imported by anything. The spawn argument change is local to the script's own main flow. PASS
  • Broader-fix rival scan: gh pr list --search '83921 in:title,body' and gh pr list --search 'spawnSync node process.execPath' return no open PRs. Issue timeline shows zero cross-references. PASS
  • Recent-merge audit: git log --oneline -3 -- scripts/test-cli-startup-bench-budget.mjs scripts/test-update-cli-startup-bench.mjs shows e1061a8b46 test(live): tolerate provider drift in release checks — unrelated. PASS
  • Prototype-pollution scan: N/A — first-argument swap on spawnSync.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels May 19, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 19, 2026

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the functional benchmark-spawner fix already landed on current main through the merged related PR, and this PR’s latest diff now only adds explanatory comments around code that is already fixed and covered by tests.

Canonical path: Keep the merged code and test coverage from the superseding PR, and avoid merging a second comment-only branch unless maintainers explicitly want a separate comment-polish change.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Review details

Best possible solution:

Keep the merged code and test coverage from the superseding PR, and avoid merging a second comment-only branch unless maintainers explicitly want a separate comment-polish change.

Do we have a high-confidence way to reproduce the issue?

No against current main: both wrapper scripts already use process.execPath, and the merged regression test asserts no bare node spawn remains. The original report was source-reproducible before the superseding fix landed.

Is this the best way to solve the issue?

No, merging this PR is no longer the best way to solve the issue because the functional fix and test coverage already landed through the superseding PR. The current branch only adds explanatory comments.

Security review:

Security review cleared: The latest diff only adds source comments and introduces no dependencies, workflows, permissions, secret handling, package resolution, or code execution changes.

What I checked:

Likely related people:

  • giodl73-repo: Authored the merged superseding fix that changed both benchmark wrappers to process.execPath and added regression coverage. (role: recent area contributor; confidence: high; commits: 67c12e036802; files: scripts/test-cli-startup-bench-budget.mjs, scripts/test-update-cli-startup-bench.mjs, test/scripts/cli-startup-bench-spawner.test.ts)
  • Ayaan Zaidi: Local history shows the benchmark wrapper scripts and adjacent startup benchmark tooling were introduced in ac28341. (role: introduced behavior; confidence: medium; commits: ac28341ebfcc; files: scripts/bench-cli-startup.ts, scripts/check-cli-startup-memory.mjs, scripts/test-cli-startup-bench-budget.mjs)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 110042d840bb; fix evidence: commit 67c12e036802, main fix timestamp 2026-05-19T23:21:26-07:00.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels May 19, 2026
)

`scripts/test-cli-startup-bench-budget.mjs` and
`scripts/test-update-cli-startup-bench.mjs` both called
`spawnSync("node", args, …)`, resolving the binary through PATH. In
environments using nvm / fnm / volta / asdf the shim PATH-resolves to
a different Node version than the one currently executing the parent
script. The bench runner can then execute on Node X while the parent
runs on Node Y, producing measurements that are not comparable to the
baseline or to what CI uses; the fixture-update script writes a
fixture that no other run can reproduce.

`bench-cli-startup.ts` and `check-cli-startup-memory.mjs` already use
`process.execPath` for exactly this reason. Bring the two test runners
into line with the existing pattern.
@hclsys hclsys force-pushed the fix-bench-spawner-use-execpath-83921 branch from 3c961d3 to bb1cd4d Compare May 20, 2026 08:50
@hclsys
Copy link
Copy Markdown
Contributor Author

hclsys commented May 20, 2026

Rebased onto current origin/main and resolved the benchmark script conflicts by preserving the branch intent: both bench helper scripts continue spawning under process.execPath, now with comments documenting why PATH-resolved node is avoided.

Validation after rebase:

  • node scripts/test-cli-startup-bench-budget.mjs --report test/fixtures/cli-startup-bench.json --skip-baseline -> passed
  • pnpm lint -- scripts/test-cli-startup-bench-budget.mjs scripts/test-update-cli-startup-bench.mjs -> 0 warnings / 0 errors
  • git diff --check origin/main...HEAD -> clean

@clawsweeper re-review

@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 20, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 20, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. P2 Normal backlog priority with limited blast radius. labels May 20, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 20, 2026

ClawSweeper applied the proposed close for this PR.

@clawsweeper clawsweeper Bot closed this May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. scripts Repository scripts size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benchmark spawner scripts use bare "node" instead of process.execPath, risking version mismatch

1 participant