Skip to content

fix: use npx to invoke correct Yarn version instead of relying on Corepack PATH shims - #78

Merged
iskounen merged 3 commits into
mainfrom
iskounen/fix/setup-and-install-use-npx
Jun 25, 2026
Merged

fix: use npx to invoke correct Yarn version instead of relying on Corepack PATH shims#78
iskounen merged 3 commits into
mainfrom
iskounen/fix/setup-and-install-use-npx

Conversation

@iskounen

Copy link
Copy Markdown
Contributor

The approach in #77 of using corepack prepare --activate to route yarn to the correct version did not work. Each step in a composite action runs in a fresh shell, so PATH changes made by corepack enable do not carry over to subsequent steps. As a result, yarn --version still returned 4.10.3 despite the activation.

This PR replaces that approach with npx --yes yarn@<version> for Yarn 1 projects. npx fetches and runs the exact version specified in the packageManager field directly, without depending on PATH or Corepack shim setup.

For Berry projects, the system yarn is still used since Berry repos typically manage their own Yarn binary locally. For projects with no packageManager field, the existing version detection fallback is preserved.

🤖 Generated with Claude Code

…epack PATH shims

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot deleted a comment from claude Bot Jun 25, 2026
@iskounen
iskounen requested a review from amonkhouse June 25, 2026 20:31
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot deleted a comment from claude Bot Jun 25, 2026
Comment on lines +45 to 60
if [[ "$PKG_MANAGER" =~ ^yarn@1\. ]]; then
echo "Detected Yarn 1 (Classic)"
npx --yes "${PKG_MANAGER%%+*}" install --frozen-lockfile
elif [[ -n "$PKG_MANAGER" && "$PKG_MANAGER" =~ ^yarn@ ]]; then
echo "Detected Yarn (Berry)"
yarn install --no-immutable
else
# Yarn 1 (Classic)
echo "Detected Yarn $YARN_VERSION (Classic)"
yarn install --frozen-lockfile
YARN_VERSION=$(yarn --version)
if [[ "$YARN_VERSION" =~ ^[234] ]]; then
echo "Detected Yarn $YARN_VERSION (Berry)"
yarn install --no-immutable
else
echo "Detected Yarn $YARN_VERSION (Classic)"
yarn install --frozen-lockfile
fi
fi

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.

The Yarn 1 path was fixed to invoke the exact pinned version via npx, but the Berry path (line 48-50) and the fallback (line 51-60) still call the ambient global yarn. Since this PR removes corepack enable / corepack prepare --activate, a downstream repo with packageManager: yarn@3.x that calls this shared action would now run whatever yarn is on PATH (Yarn 1 on ubuntu-latest runners) instead of its pinned Berry version, unless their own workflow enables corepack.

This repo is unaffected (it's yarn@1.22.22, and danger.yml still runs corepack enable at the workflow level), so this isn't blocking. But for consistency with the npx approach, was leaving the Berry branch on ambient yarn intentional? It's an asymmetry worth a comment, or applying the same npx "${PKG_MANAGER%%+*}" treatment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, downstream repos that use Barry (Yarn 2+) are expected to have the correct yarn version on PATH.

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Code Review

Summary

This PR fixes Yarn version resolution in the shared setup-and-install composite action. Instead of relying on Corepack PATH shims (which proved unreliable), it removes the corepack enable step and invokes the exact pinned Yarn 1 version directly via npx, stripping any integrity hash from the packageManager field and tightening the Yarn 1 classifier so it doesn't accidentally match yarn@10+.

Issues Found

🟡 Important / Question — Berry path no longer pins its version (action.yml:48-60)
The Yarn 1 branch now invokes the exact version via npx "${PKG_MANAGER%%+*}", but the Berry branch and the yarn --version fallback still call the ambient global yarn. Because this PR also removes corepack prepare … --activate, a downstream consumer with packageManager: yarn@3.x calling this shared action would run whatever yarn is on PATH (Yarn 1 on ubuntu-latest) rather than its pinned Berry version — unless that consumer enables Corepack itself.

This repo is not affected (it pins yarn@1.22.22, and danger.yml:43-44 still runs corepack enable at the workflow level), so this is not blocking. But since duchamp is shared tooling that other repos check out into .tooling and consume from main (danger.yml:36-47), it's worth confirming the asymmetry is intentional. See the inline comment.

Areas Reviewed

  • Correctness of the fix ✅ Verified the three commits land correctly:
    • ${PKG_MANAGER%%+*} correctly strips a +sha512.… integrity suffix before passing to npx.
    • ^yarn@1\. correctly matches yarn@1.22.22 (and yarn@1.22.22+sha…) while not matching yarn@10.x — the trailing literal . is what makes this work.
    • --frozen-lockfile preserves the prior install semantics for Yarn 1.
  • Integrationnode -e extraction is unchanged and runs from $INSTALL_DIR, so it reads the correct package.json for both install-from-caller modes. npx is available without Corepack. The Yarn cache path (~/.cache/yarn/v6) is still correct when Yarn 1 runs via npx.
  • Bugs & edge cases ✅ No-packageManager repos fall through to the yarn --version fallback, which relies on a global yarn being present — true on GitHub-hosted runners. The nested Berry/Classic detection in that fallback is reasonable defensive code.
  • Testing — No automated tests exist for this action (it's a YAML composite action exercised by downstream CI); none expected.

Note: the untracked .tooling/ directory in the working tree is a stale local checkout of the action with the old Corepack logic; it's not part of this diff and is irrelevant to the change.

Nice, well-scoped fix with a clear commit history. The only thing worth a second look is the Berry-branch asymmetry above.

@iskounen
iskounen merged commit eaa9c2a into main Jun 25, 2026
6 checks passed
@iskounen
iskounen removed the request for review from amonkhouse June 25, 2026 21:01
@iskounen
iskounen deleted the iskounen/fix/setup-and-install-use-npx branch June 25, 2026 21:06
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.

2 participants