Skip to content

fix(shared): search cwd for empty POSIX PATH components - #28106

Open
Motaghi wants to merge 4 commits into
pingdotgg:mainfrom
Motaghi:behmot/posix-empty-path-components-20261031
Open

Motaghi wants to merge 4 commits into
pingdotgg:mainfrom
Motaghi:behmot/posix-empty-path-components-20261031

Conversation

@Motaghi

@Motaghi Motaghi commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

With a nonempty POSIX PATH such as :/usr/bin, native process spawning searches the current directory first. Both T3 command resolvers discarded that empty component, so discovery could report a missing command or select a later executable.

Both resolver loops now treat raw empty components as the current directory, preserving PATH order. Absent or entirely empty PATH values, whitespace-only and quoted-empty components, and Windows/PATHEXT retain their existing handling.

The review follow-up also makes command-result and macOS Git preparation caches distinguish the working directory when POSIX PATH contains relative entries. Directory-listing keys identify the actual searched directory, including directories with equal timestamps. Absolute-only PATH and Windows cache keys, TTL, capacity and relative return paths retain their handling. No production caller that changes process cwd was found; this cache case is a reproduced library edge case.

The existing child-process regression covers both real resolvers, shared caches across completed cwd transitions, hit/miss changes and executable precedence. It also exercises macOS Git preparation through its existing service seams. The original empty-component failures and the expanded cache regression were reproduced before their repairs. This stays within shell.ts and shell.test.ts and is submitted as a focused correction under the obvious-bug exception. When composing with the related #24656 refactor, retain the entirely-empty-PATH guard.

Validation on macOS with Node 26.12.1, rebased onto 79f1fad and pushed as f371165d:

  • Focused shell and selected context-budget tests: 42 passed, 2 existing skips (opt-in Nushell and Windows-only npm shim). The only subsequent source change was test destructuring formatting.
  • Local runtime probes passed through both production resolver entry points and executed disposable commands to verify precedence and cwd cache transitions. The actual macOS Xcode Git locator and spawn preparation also switched correctly between installed Git and disposable local Git in both directions, for empty and explicit-dot PATH entries.
  • No-mistakes review, formatting and changed-file lint passed. Supplemental checks on the exact pushed head passed both configured Knip scans and all 33 affected-package typechecks, with concurrency one. All 29,044 tracked file hashes, HEAD, status and staged state were unchanged across those supplemental checks.

Real Linux and Windows hosts, ACP registry flows and live web/mobile/desktop clients were not exercised. Windows branch probes on macOS do not establish native Windows behavior. Full repository suites, build and release smoke were not run. Upstream CI workflows await maintainer approval; no upstream CI pass is claimed.

Models and harnesses: Claude Sonnet 5.5 through T3 Code's Claude Agent harness; GPT-6.1-Sol through T3 Code's Codex harness.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 31, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 656226a

Macroscope's review found this PR approvable — Executable discovery now honors current-directory entries already present as empty POSIX PATH components. Windows behavior and empty or missing PATH handling remain unchanged.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 656226a and f371165.

Files selected for processing: 2
  • packages/shared/src/shell.test.ts
  • packages/shared/src/shell.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

On POSIX systems, command lookup now treats empty entries in a nonempty PATH as the current directory. Relative search directories and relevant cache keys use the current working directory. Tests cover command resolution, cache behavior, and Git selection.

Changes: POSIX PATH resolution

Layer / File(s) Summary
PATH search and cache behavior
packages/shared/src/shell.ts
Empty PATH entries become ".". Relative search directories are resolved against the current working directory. Command and macOS Git cache keys account for the working directory when PATH contains relative or empty entries.
Command and Git resolution tests
packages/shared/src/shell.test.ts
A POSIX-only test checks empty, relative, whitespace-only, and quoted-empty PATH entries. It also checks cached and uncached resolution across working directories, both resolver paths, and Git selection.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: maria-rcks, t3dotgg

Merge Risk: Minimal · up to f3711

On POSIX, command lookup now treats empty PATH entries as the current directory, which matches native spawn behavior. No merge-blocking issue is evident. Linux, Windows, and macOS Git caller flows were not exercised end to end.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title "fix(shared): search cwd for empty POSIX PATH components" is specific and matches the main change. The shell.ts change treats empty PATH components as the current directory in POSIX command …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description provides the required problem, change, scope rationale, verification results, limitations, and agent disclosure. It includes detailed test evidence and explains the obvious-bug excepti…
Full details: Title check

Explanation

The title "fix(shared): search cwd for empty POSIX PATH components" is specific and matches the main change. The shell.ts change treats empty PATH components as the current directory in POSIX command lookup, and the test file covers that behavior. It is short, uses a conventional commit prefix, and names the scope and the fix.

Full details: Description check

Explanation

The description provides the required problem, change, scope rationale, verification results, limitations, and agent disclosure. It includes detailed test evidence and explains the obvious-bug exception. The content is not organized under the template headings, but the required information is substantially complete.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments: 1
packages/shared/src/shell.ts-644-645: 1

644-645: Minor · Functional Correctness · Quick win

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Include the working directory in cache keys for current-directory PATH entries.

If a process changes working directory, the same nonempty PATH with an empty component can resolve a different executable. resolveCommandPathForPlatform checks its cache before searching, but its key does not include the working directory. A cached miss or result can therefore remain wrong for up to 30 seconds. Include the working directory in the cache key when PATH searches it. (raw.githubusercontent.com)


Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 79f1fad and 656226a.

Files selected for processing: 2
  • packages/shared/src/shell.test.ts
  • packages/shared/src/shell.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

@Motaghi
Motaghi force-pushed the behmot/posix-empty-path-components-20261031 branch from 656226a to f371165 Compare October 31, 2026 12:12
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 31, 2026
@Motaghi

Motaghi commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed the cwd-cache finding with sequential completed lookups, then fixed it in f371165. Relative POSIX PATH searches now include cwd in command-result and macOS Git preparation cache keys, and directory listings distinguish the searched directories even when their timestamps match. Absolute-only PATH and Windows handling remain unchanged.

The expanded regression failed before the repair. Fresh macOS validation passed 42 tests with 2 existing skips, both Knip scans and all 33 affected-package typechecks, plus local runtime probes through the real resolvers and native macOS Git preparation. Formatting and lint passed through no-mistakes. Linux/Windows hosts and live clients remain untested; upstream CI awaits maintainer approval. The PR description now records the final scope and validation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant