morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2320] [CLOSED] fix(test): keep executable paths with spaces intact on Windows #5161

New issue

Closed

opened 2026-10-31 01:04:27 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:27 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2320
Author: @harshdazz
Created: 10/12/2026
Status: ❌ Closed

Base: main ← Head: fix/windows-run-exclusive-spaces

📝 Commits (1)

8b0e311 fix(test): keep executable paths with spaces intact on Windows

📊 Changes

2 files changed (+15 additions, -1 deletions)

View changed files

📝 scripts/run-exclusive-command.mjs (+8 -1)
📝 tests/process-global/runnerCancellation.test.ts (+7 -0)

📄 Description

Summary

Fixes #2319. On Windows, npm run test:focused failed with 'C:\Program' is not recognized for any test that needs a build, because the exclusive runner sent node.exe's path to cmd.exe without quotes. The runner now starts .exe and .com paths directly, so the path and arguments arrive unchanged.

Problem and expected behavior

Smallest trigger: node scripts/run-exclusive.mjs probe "C:\Program Files\nodejs\node.exe" -e "console.log('child ran')" on Windows. runCommand() used shell: true on Windows, and Node joins the command and arguments into one cmd.exe /d /s /c "..." line without quoting, so the path is split at the space.

Expected: the runner starts the selected command with its path and arguments unchanged.

Change and scope

needsWindowsShell(executable) keeps the shell only on Windows and only when the command isn't an .exe or .com path. Bare npm / turbo still go through cmd.exe, so .cmd shims resolve the same way as before. On other platforms nothing changes.
New test in tests/process-global/runnerCancellation.test.ts: an executable path plus an argument with a space and quotes come through unchanged.
Not included: arguments with spaces passed to a bare command still go through cmd.exe unquoted. No current caller does that (the package scripts pass plain words), and doing it properly needs cmd.exe escaping, so I left it out to keep this small.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the final review checklist
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

Semantic owner and earliest changed stage: runCommand() in scripts/run-exclusive-command.mjs.
Contribution scope fit and linked maintainer decision: development script fix, not applicable.
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: Windows development scripts only. On other platforms needsWindowsShell is false, same as before.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. The lock and SIGINT/SIGTERM code isn't touched.
Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none. The script isn't in the npm package (npm pack --dry-run).

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: the new runner test above. The existing "does not treat an old database timestamp as permission to replace a live owner" test also failed on Windows for this reason and passes now.
Base reproduction or other evidence: on main (a97b713f), Windows:

$ npx vitest run --project process-global tests/process-global/runnerCancellation.test.ts
   × passes an executable path and arguments with spaces through unchanged 204ms
   × does not treat an old database timestamp as permission to replace a live owner 340ms
      Tests  2 failed | 3 skipped (5)

The new test fails with code: 1 and empty stdout. The existing one gets 'C:\\Program' is not recognized ... in stderr instead of "already running".

User-visible CLI/MCP output (if applicable): no CLI/MCP change. The developer command, before:

$ npm run test:focused -- tests/process-global/runnerCancellation.test.ts
Focused tests: tests/process-global/runnerCancellation.test.ts
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.

After:

 ✓ |process-global| tests/process-global/runnerCancellation.test.ts (5 tests | 3 skipped) 1922ms
 Test Files  1 passed (1)
      Tests  2 passed | 3 skipped (5)

Remaining proof gaps: run on Windows 11 with Node 22.19.0 only, not the pinned 24.18. Linux and macOS behavior is unchanged by construction, and CI covers it.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Not an evidence-bearing change, so these are left unticked.

Validation performed

All on Windows 11 x64, Node 22.19.0, base a97b713f.

npx vitest run --project process-global tests/process-global/runnerCancellation.test.ts: 2 passed | 3 skipped (the 3 skipped are runIf(process.platform !== "win32")).
npm run test:focused -- tests/process-global/runnerCancellation.test.ts: exit 0, output above.
npm run build:cached: exit 0, so bare turbo still runs through the shell.
node scripts/run-exclusive.mjs probe "C:\Program Files\nodejs\node.exe" -e "console.log('child ran')": prints child ran, exit 0.
npm run check: passes, Lint: 2650 files, 0 errors, 937 warnings., none in the changed files.
npm run jscpd: 2233 files, 1315 clones, 2.81% duplicated lines.
npm run verify:test-discovery: Discovered all 964 test files exactly once.
npx oxfmt --check on the two files: passes.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: no provider involved. Windows 11 locally.
Package or release metadata impact: none, the script isn't packaged.
Security, privacy, process, or containment review: this removes shell use for direct executables and adds none.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2320 **Author:** [@harshdazz](https://github.com/harshdazz) **Created:** 10/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/windows-run-exclusive-spaces` --- ### 📝 Commits (1) - [`8b0e311`](https://github.com/morluto/rea/commit/8b0e3113f14966d7fde53a19fcfb4d14120534d3) fix(test): keep executable paths with spaces intact on Windows ### 📊 Changes **2 files changed** (+15 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `scripts/run-exclusive-command.mjs` (+8 -1) 📝 `tests/process-global/runnerCancellation.test.ts` (+7 -0) </details> ### 📄 Description ## Summary Fixes #2319. On Windows, `npm run test:focused` failed with `'C:\Program' is not recognized` for any test that needs a build, because the exclusive runner sent `node.exe`'s path to `cmd.exe` without quotes. The runner now starts `.exe` and `.com` paths directly, so the path and arguments arrive unchanged. ## Problem and expected behavior Smallest trigger: `node scripts/run-exclusive.mjs probe "C:\Program Files\nodejs\node.exe" -e "console.log('child ran')"` on Windows. `runCommand()` used `shell: true` on Windows, and Node joins the command and arguments into one `cmd.exe /d /s /c "..."` line without quoting, so the path is split at the space. Expected: the runner starts the selected command with its path and arguments unchanged. ## Change and scope - `needsWindowsShell(executable)` keeps the shell only on Windows and only when the command isn't an `.exe` or `.com` path. Bare `npm` / `turbo` still go through `cmd.exe`, so `.cmd` shims resolve the same way as before. On other platforms nothing changes. - New test in `tests/process-global/runnerCancellation.test.ts`: an executable path plus an argument with a space and quotes come through unchanged. - Not included: arguments with spaces passed to a bare command still go through `cmd.exe` unquoted. No current caller does that (the package scripts pass plain words), and doing it properly needs `cmd.exe` escaping, so I left it out to keep this small. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the final review checklist unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `runCommand()` in `scripts/run-exclusive-command.mjs`. - Contribution scope fit and linked maintainer decision: development script fix, not applicable. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: Windows development scripts only. On other platforms `needsWindowsShell` is false, same as before. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. The lock and SIGINT/SIGTERM code isn't touched. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. The script isn't in the npm package (`npm pack --dry-run`). Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [x] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: the new runner test above. The existing "does not treat an old database timestamp as permission to replace a live owner" test also failed on Windows for this reason and passes now. - Base reproduction or other evidence: on `main` (`a97b713f`), Windows: ``` $ npx vitest run --project process-global tests/process-global/runnerCancellation.test.ts × passes an executable path and arguments with spaces through unchanged 204ms × does not treat an old database timestamp as permission to replace a live owner 340ms Tests 2 failed | 3 skipped (5) ``` The new test fails with `code: 1` and empty stdout. The existing one gets `'C:\\Program' is not recognized ...` in stderr instead of "already running". - User-visible CLI/MCP output (if applicable): no CLI/MCP change. The developer command, before: ``` $ npm run test:focused -- tests/process-global/runnerCancellation.test.ts Focused tests: tests/process-global/runnerCancellation.test.ts 'C:\Program' is not recognized as an internal or external command, operable program or batch file. ``` After: ``` ✓ |process-global| tests/process-global/runnerCancellation.test.ts (5 tests | 3 skipped) 1922ms Test Files 1 passed (1) Tests 2 passed | 3 skipped (5) ``` - Remaining proof gaps: run on Windows 11 with Node 22.19.0 only, not the pinned 24.18. Linux and macOS behavior is unchanged by construction, and CI covers it. For evidence-bearing changes: - [ ] Observed, derived, and inferred claims remain distinguishable. - [ ] Artifact identity, source provenance, and failed attempts remain preserved. - [ ] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. Not an evidence-bearing change, so these are left unticked. ## Validation performed All on Windows 11 x64, Node 22.19.0, base `a97b713f`. - `npx vitest run --project process-global tests/process-global/runnerCancellation.test.ts`: `2 passed | 3 skipped` (the 3 skipped are `runIf(process.platform !== "win32")`). - `npm run test:focused -- tests/process-global/runnerCancellation.test.ts`: exit 0, output above. - `npm run build:cached`: exit 0, so bare `turbo` still runs through the shell. - `node scripts/run-exclusive.mjs probe "C:\Program Files\nodejs\node.exe" -e "console.log('child ran')"`: prints `child ran`, exit 0. - `npm run check`: passes, `Lint: 2650 files, 0 errors, 937 warnings.`, none in the changed files. - `npm run jscpd`: `2233 files, 1315 clones, 2.81% duplicated lines.` - `npm run verify:test-discovery`: `Discovered all 964 test files exactly once.` - `npx oxfmt --check` on the two files: passes. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: no provider involved. Windows 11 locally. - Package or release metadata impact: none, the script isn't packaged. - Security, privacy, process, or containment review: this removes shell use for direct executables and adds none. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:04:27 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:43:04 +00:00

[PR #5161] [CLOSED] fix(process): clear cleanup_incomplete after owner-confirmed cleanup retry #6790

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5161

No description provided.

Rows
Columns