fix(test-ipc-server): allow tilde in shell-arg paths for Windows 8.3 short names by zkochan · Pull Request #11661 · pnpm/pnpm

zkochan · 2026-05-14T23:37:45Z

Summary

On GitHub's Windows runners, os.tmpdir() returns the 8.3 short-name form of the user profile dir — C:\Users\RUNNER~1\AppData\Local\Temp — because runneradmin is longer than 8 chars. The ~ character then trips the allowlist regex in __utils__/test-ipc-server/src/TestIpcServer.ts's quoteShellArg, and every test that calls sendLineScript or generateSendStdinScript throws Unsupported character in shell argument.
The fix is to add ~ to the allowlist:
- cmd.exe performs no tilde expansion at all.
- POSIX shells only expand ~ when it is unquoted at the start of a word; inside the double-quoted "${arg}" wrapper produced by quoteShellArg it is literal.
The CodeQL shell-injection sanitization argument is unchanged — the regex is still anchored and still rejects every metacharacter.
This bug existed since ci: address zizmor findings across workflows #11608 (introduced the allowlist) but was masked because the Windows test legs had been silently no-op'ing — see ci: pin Run tests step to bash so $TEST_SCRIPT expands on Windows #11659. With that fix in, the failure surfaces on every Windows CI run that hits building/commands' recursive.ts:rebuild multiple packages in correct order.

Test plan

Merge or stack on top of ci: pin Run tests step to bash so $TEST_SCRIPT expands on Windows #11659 so the Windows leg actually executes tests.
CI passes on Windows for building/commands' test/build/recursive.ts.
No regression on Linux/macOS legs.

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

Tests
- Enhanced test infrastructure to properly handle shell paths containing the ~ character, improving compatibility with Windows short-path notation and Unix home directory references in test scenarios.

…short names `os.tmpdir()` on GitHub's Windows runners returns the 8.3 short-name form of the user-profile directory (e.g. `C:\Users\RUNNER~1\AppData\Local\Temp`) because `runneradmin` is longer than 8 characters. The `~` then trips the `quoteShellArg` allowlist regex and every test that calls `sendLineScript` or `generateSendStdinScript` throws "Unsupported character in shell argument". The tilde is safe to allow: - cmd.exe performs no tilde expansion at all. - POSIX shells only expand `~` when it is unquoted at the start of a word; inside the double-quoted `"${arg}"` wrapper produced here it is literal. The matching CodeQL shell-injection sanitization argument is unchanged — the allowlist is still anchored and still rejects every metacharacter. The bug was masked until #11659 because the Windows test legs had been silently no-op'ing since #11608. --- Written by an agent (Claude Code, claude-opus-4-7).

coderabbitai · 2026-05-14T23:38:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 25b52af0-e856-4b1e-83d6-2ae8a76304d6

📥 Commits

Reviewing files that changed from the base of the PR and between e8fc343 and 12004c1.

📒 Files selected for processing (1)

__utils__/test-ipc-server/src/TestIpcServer.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.

Files:

__utils__/test-ipc-server/src/TestIpcServer.ts

🧠 Learnings (1)

📚 Learning: 2026-05-14T09:04:00.133Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.

Applied to files:

__utils__/test-ipc-server/src/TestIpcServer.ts

🔇 Additional comments (3)

__utils__/test-ipc-server/src/TestIpcServer.ts (3)

132-133: LGTM!

166-171: LGTM!

182-182: LGTM!

📝 Walkthrough

Walkthrough

TestIpcServer's shell argument quoting mechanism extends its quoteShellArg allowlist to include the tilde (~) character. Documentation comments on sendLineScript and generateSendStdinScript are updated to reflect this allowlist change, with explicit notes on Windows 8.3 short-name temp path handling. The validation regex is updated accordingly.

Changes

Shell Argument Allowlist Update

Layer / File(s)	Summary
Update shell argument allowlist to include tilde character `__utils__/test-ipc-server/src/TestIpcServer.ts`	`quoteShellArg`'s validation regex is updated to permit tilde (~) in shell-quoted arguments. Documentation on `sendLineScript` and `generateSendStdinScript` is updated to explain the allowlist change and Windows 8.3 short temp path rationale.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A tilde needed shelter in the quoting fold,
Through Windows paths where short names grew old,
Documentation bloomed with reasoning true,
One little friend, now safe in the regex zoo! 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically describes the main change: allowing tilde in shell-arg paths for Windows 8.3 short names, which is the core fix in the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix-test-ipc-tilde

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…short names (pnpm#11661) `os.tmpdir()` on GitHub's Windows runners returns the 8.3 short-name form of the user-profile directory (e.g. `C:\Users\RUNNER~1\AppData\Local\Temp`) because `runneradmin` is longer than 8 characters. The `~` then trips the `quoteShellArg` allowlist regex and every test that calls `sendLineScript` or `generateSendStdinScript` throws "Unsupported character in shell argument". The tilde is safe to allow: - cmd.exe performs no tilde expansion at all. - POSIX shells only expand `~` when it is unquoted at the start of a word; inside the double-quoted `"${arg}"` wrapper produced here it is literal. The matching CodeQL shell-injection sanitization argument is unchanged — the allowlist is still anchored and still rejects every metacharacter. The bug was masked until pnpm#11659 because the Windows test legs had been silently no-op'ing since pnpm#11608. --- Written by an agent (Claude Code, claude-opus-4-7).

Copilot AI review requested due to automatic review settings May 14, 2026 23:37

Copilot started reviewing on behalf of zkochan May 14, 2026 23:38 View session

Copilot AI reviewed May 14, 2026

coderabbitai Bot approved these changes May 14, 2026

View reviewed changes

zkochan enabled auto-merge (squash) May 15, 2026 00:20

zkochan disabled auto-merge May 15, 2026 00:21

zkochan merged commit ae703b1 into main May 15, 2026
18 of 19 checks passed

zkochan deleted the fix-test-ipc-tilde branch May 15, 2026 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(test-ipc-server): allow tilde in shell-arg paths for Windows 8.3 short names#11661

fix(test-ipc-server): allow tilde in shell-arg paths for Windows 8.3 short names#11661
zkochan merged 1 commit into
mainfrom
fix-test-ipc-tilde

zkochan commented May 14, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 14, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

zkochan commented May 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 14, 2026 •

edited

Loading