fix: align windows path tests with runtime behavior#45870
fix: align windows path tests with runtime behavior#45870NickHood1984 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3c5e55c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return input; | ||
| } | ||
| return input.replace(/^~(?=$|[\\/])/, home); | ||
| return path.resolve(input.replace(/^~(?=$|[\\/])/, home)); |
There was a problem hiding this comment.
Do not resolve unmatched tilde prefixes
expandHomePrefix now wraps the replacement in path.resolve(...), but the regex only replaces ~ when followed by end-of-string or a path separator. Inputs like ~alice/bin/tool therefore remain unchanged by replace and then get coerced into a cwd-relative absolute path (for example /cwd/~alice/bin/tool), which silently changes a non-home token into a different filesystem target. This can mis-resolve command paths and allowlist patterns that start with ~ but are not home-prefix syntax.
Useful? React with 👍 / 👎.
| if (process.platform === "win32") { | ||
| return getWindowsExecutableExtensions(env).includes(path.extname(filePath).toLowerCase()); | ||
| } |
There was a problem hiding this comment.
Skip PATHEXT gating for explicit executable files
On Windows this new branch treats a file as executable only if its extension appears in PATHEXT, even for explicit absolute/relative paths. PATHEXT is only meant for extension lookup when no extension is provided; with this change, resolveExecutablePath can reject a real file like C:\...\node.exe whenever the provided environment narrows PATHEXT (for example to .CMD), causing false negatives in command resolution under sanitized/custom envs.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR isolates Windows-specific test fixes from PR #40791, aligning unit test assertions with actual Windows runtime behaviour across path handling, executable resolution, and scheduled task cleanup. The changes are correct and well-scoped. Key changes:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/stable-node-path.ts
Line: 33
Comment:
**Case-insensitive regex but case-sensitive formula guard**
The regex was made case-insensitive with the `i` flag, meaning `cellarMatch[2]` (the `formula` capture) preserves the original casing from the input path. The `formula === "node"` guard directly below is a strict equality check — if a path ever contained `NODE` or `Node` in the Cellar segment, the fallback to `<prefix>/bin/node` would be silently skipped.
Homebrew always uses lowercase formula names in practice, so this is unlikely to cause real problems today. But since the regex was intentionally made case-insensitive, a consistent fix would be:
```suggestion
if (formula.toLowerCase() === "node") {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/stable-node-path.ts
Line: 20-21
Comment:
**Custom path join bypasses normalisation**
`joinStablePath` concatenates parts with a raw separator character rather than using `path.join` (or `path.posix.join`/`path.win32.join`). If `prefix` happens to have a trailing separator (e.g. from `cellarMatch[1]` ending with `/`), the resulting path would contain a doubled slash like `/opt/homebrew//opt/node/bin/node`.
The new regex uses `[\\/]+` (greedy, one-or-more), so the lazy `(.+?)` stops before the first run of slashes, giving a clean prefix in well-formed paths. However, using `path.posix.join` / `path.win32.join` (selected by the same `nodePath.includes("\\")` heuristic) would be more robust:
```ts
const join = nodePath.includes("\\") ? path.win32.join : path.posix.join;
const optPath = join(prefix, "opt", formula, "bin", binaryName);
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: ce3c5e5 |
| } | ||
|
|
||
| // For the default "node" formula, also try the direct bin symlink. | ||
| if (formula === "node") { |
There was a problem hiding this comment.
Case-insensitive regex but case-sensitive formula guard
The regex was made case-insensitive with the i flag, meaning cellarMatch[2] (the formula capture) preserves the original casing from the input path. The formula === "node" guard directly below is a strict equality check — if a path ever contained NODE or Node in the Cellar segment, the fallback to <prefix>/bin/node would be silently skipped.
Homebrew always uses lowercase formula names in practice, so this is unlikely to cause real problems today. But since the regex was intentionally made case-insensitive, a consistent fix would be:
| if (formula === "node") { | |
| if (formula.toLowerCase() === "node") { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/stable-node-path.ts
Line: 33
Comment:
**Case-insensitive regex but case-sensitive formula guard**
The regex was made case-insensitive with the `i` flag, meaning `cellarMatch[2]` (the `formula` capture) preserves the original casing from the input path. The `formula === "node"` guard directly below is a strict equality check — if a path ever contained `NODE` or `Node` in the Cellar segment, the fallback to `<prefix>/bin/node` would be silently skipped.
Homebrew always uses lowercase formula names in practice, so this is unlikely to cause real problems today. But since the regex was intentionally made case-insensitive, a consistent fix would be:
```suggestion
if (formula.toLowerCase() === "node") {
```
How can I resolve this? If you propose a fix, please make it concise.| const separator = nodePath.includes("\\") ? "\\" : "/"; | ||
| const joinStablePath = (...parts: string[]) => parts.join(separator); |
There was a problem hiding this comment.
Custom path join bypasses normalisation
joinStablePath concatenates parts with a raw separator character rather than using path.join (or path.posix.join/path.win32.join). If prefix happens to have a trailing separator (e.g. from cellarMatch[1] ending with /), the resulting path would contain a doubled slash like /opt/homebrew//opt/node/bin/node.
The new regex uses [\\/]+ (greedy, one-or-more), so the lazy (.+?) stops before the first run of slashes, giving a clean prefix in well-formed paths. However, using path.posix.join / path.win32.join (selected by the same nodePath.includes("\\") heuristic) would be more robust:
const join = nodePath.includes("\\") ? path.win32.join : path.posix.join;
const optPath = join(prefix, "opt", formula, "bin", binaryName);Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/stable-node-path.ts
Line: 20-21
Comment:
**Custom path join bypasses normalisation**
`joinStablePath` concatenates parts with a raw separator character rather than using `path.join` (or `path.posix.join`/`path.win32.join`). If `prefix` happens to have a trailing separator (e.g. from `cellarMatch[1]` ending with `/`), the resulting path would contain a doubled slash like `/opt/homebrew//opt/node/bin/node`.
The new regex uses `[\\/]+` (greedy, one-or-more), so the lazy `(.+?)` stops before the first run of slashes, giving a clean prefix in well-formed paths. However, using `path.posix.join` / `path.win32.join` (selected by the same `nodePath.includes("\\")` heuristic) would be more robust:
```ts
const join = nodePath.includes("\\") ? path.win32.join : path.posix.join;
const optPath = join(prefix, "opt", formula, "bin", binaryName);
```
How can I resolve this? If you propose a fix, please make it concise.…ath-tests # Conflicts: # src/daemon/schtasks.startup-fallback.test.ts # src/daemon/schtasks.stop.test.ts # src/infra/exec-allowlist-pattern.test.ts # src/infra/executable-path.test.ts # src/infra/executable-path.ts # src/infra/hardlink-guards.test.ts # src/infra/json-file.test.ts # src/infra/stable-node-path.ts # src/shared/config-eval.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7b069bace
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return input; | ||
| } | ||
| return input.replace(/^~(?=$|[\\/])/, home); | ||
| return path.resolve(input.replace(/^~(?=$|[\\/])/, home)); |
There was a problem hiding this comment.
Preserve unmatched tilde tokens in home prefix expansion
Wrapping the replacement in path.resolve(...) changes non-home tokens like ~alice/bin into cwd-anchored absolute paths, because the regex only replaces bare ~ or ~/.../~\\... and leaves ~alice untouched. Fresh evidence: callers like resolveAllowlistCandidatePath and resolveExecutablePath then treat that rewritten value as a concrete filesystem path, so an unresolved token can silently target <cwd>/~alice/... instead of remaining unresolved.
Useful? React with 👍 / 👎.
| if (process.platform === "win32") { | ||
| const ext = path.extname(filePath).toLowerCase(); | ||
| if (!ext) { | ||
| return true; | ||
| } | ||
| return resolveWindowsExecutableExtSet(undefined).has(ext); | ||
| return getWindowsExecutableExtensions(env).includes(path.extname(filePath).toLowerCase()); | ||
| } |
There was a problem hiding this comment.
Bypass PATHEXT checks for already-qualified executables
This Windows branch now requires the file extension to be present in PATHEXT even after a concrete file path has already been found, which can reject real binaries such as node.exe under restricted envs (for example PATHEXT=.CMD). Fresh evidence: resolveExecutableFromPathEnv now forwards the caller env into isExecutableFile, so PATH lookups of commands that already include an extension can incorrectly return undefined despite the file existing.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Source inspection of the PR diff gives helper-level reproductions: Next step before merge Security Review findings
Review detailsBest possible solution: Keep the Windows and schtasks test normalization, preserve current home and executable resolution semantics, and add focused regressions for unmatched tilde tokens, restricted PATHEXT, and stable-node casing. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of the PR diff gives helper-level reproductions: Is this the best way to solve the issue? No. The schtasks assertion update is aligned with current runtime, but the production helper changes should be narrowed or covered by regressions instead of changing approval and executable semantics as a side effect. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against ecd562b2b5f0. |
Summary
schtaskstests to account for Windows cleanup usingtaskkill.exeinstead of the non-WindowskillProcessTreehelperWhy
The Windows shard failures inherited by #40791 are unrelated to the watchdog feature itself. This PR isolates the Windows path and runtime test fixes into a reviewable slice.
Validation
pnpm exec vitest run --config vitest.unit.config.ts src/daemon/schtasks.startup-fallback.test.ts src/daemon/schtasks.stop.test.ts src/infra/home-dir.test.ts src/infra/update-global.test.ts src/infra/exec-approvals-store.test.ts src/infra/executable-path.test.ts src/infra/exec-allowlist-pattern.test.ts src/shared/config-eval.test.ts src/infra/run-node.test.ts src/infra/json-file.test.ts src/infra/stable-node-path.test.ts src/infra/hardlink-guards.test.ts