Skip to content

Fix Testbox scripts pnpm bootstrap#4072

Closed
carderne wants to merge 1 commit into
mainfrom
fix/testbox-pnpm-bootstrap
Closed

Fix Testbox scripts pnpm bootstrap#4072
carderne wants to merge 1 commit into
mainfrom
fix/testbox-pnpm-bootstrap

Conversation

@carderne

Copy link
Copy Markdown
Contributor

Summary

  • add Node/pnpm bootstrapping to the Linux Testbox PR check script because Testbox run shells do not inherit the GitHub Actions PATH from warmup
  • add the same bootstrap to the Windows Testbox PR check script

Testing

  • bash -n scripts/test-pr-check.sh
  • PowerShell syntax check skipped locally because pwsh is not installed

@changeset-bot

changeset-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: faf42a9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 53af826a-eb1b-47b5-9732-26eacd8bf51a

📥 Commits

Reviewing files that changed from the base of the PR and between 243d52b and faf42a9.

📒 Files selected for processing (2)
  • scripts/test-pr-check-windows.ps1
  • scripts/test-pr-check.sh

Walkthrough

Both CI scripts (test-pr-check.sh and test-pr-check-windows.ps1) gain new helper functions to ensure pnpm is available before running PR checks. The bash script adds ensure_pnpm, which checks PATH, falls back to corepack prepare --activate, then falls back to npm install -g under a user-scoped prefix. The PowerShell script adds Find-NodeBin (scans tool-cache roots for an x64 Node binary matching a version prefix) and Ensure-Pnpm (same three-path fallback). Both scripts then conditionally prepend a cached Node 20 binary directory to PATH, call the helper, and print pnpm --version before proceeding with existing install/generate/test steps.

✨ 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/testbox-pnpm-bootstrap

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution! We require all external PRs to be opened in draft status first so you can address CodeRabbit review comments and ensure CI passes before requesting a review. Please re-open this PR as a draft. See CONTRIBUTING.md for details.

@github-actions github-actions Bot closed this Jun 29, 2026

@devin-ai-integration devin-ai-integration 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.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +26 to +27
Sort-Object Name |
Select-Object -Last 1

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.

🟡 Windows Node version lookup may select an older version when multiple patch releases are cached

The Node version directory lookup uses lexicographic string sorting (Sort-Object Name at scripts/test-pr-check-windows.ps1:26) instead of version-aware sorting, so a version like "20.20.9" is ranked higher than "20.20.10".

Impact: On a Windows runner with multiple cached Node patch releases, an older Node binary may be selected instead of the latest.

Lexicographic vs version sort mismatch with the Linux equivalent

The bash equivalent at scripts/test-pr-check.sh:39 uses sort -V (version sort), which correctly orders 20.20.9 before 20.20.10. The PowerShell Sort-Object Name does lexicographic comparison, where "20.20.9" > "20.20.10" because the character '9' > '1'. When Select-Object -Last 1 picks the last entry, it gets the wrong version.

For example, with directories 20.20.1, 20.20.2, 20.20.9, 20.20.10:

  • Bash sort -V | tail -n 120.20.10
  • PowerShell Sort-Object Name | Select-Object -Last 120.20.9
Suggested change
Sort-Object Name |
Select-Object -Last 1
Sort-Object { [version]$_.Name } |
Select-Object -Last 1
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant