Skip to content

Recover gracefully when pnpm blocks build scripts during extension generation - #8491

Open
amcaplan wants to merge 2 commits into
mainfrom
graceful-pnpm-blocked-builds-on-generate
Open

Recover gracefully when pnpm blocks build scripts during extension generation#8491
amcaplan wants to merge 2 commits into
mainfrom
graceful-pnpm-blocked-builds-on-generate

Conversation

@amcaplan

@amcaplan amcaplan commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

Fixes https://github.com/shop/issues-develop/issues/23837

Recent pnpm versions refuse to run the build scripts of newly installed dependencies until the user approves them, and they fail the install outright (ERR_PNPM_IGNORED_BUILDS) when they can't prompt for approval. Since shopify app generate extension installs dependencies from within the generation task UI (non-interactive stdio), the prompt can never be shown, so the install always fails on affected setups.

Verified against pnpm 12.3.4: the install completes resolution and linking (creating node_modules inside the new extension directory) and then exits 1. Two problems with how we handled that:

  1. The raw pnpm install failure was surfaced as-is, with no guidance on how to recover.
  2. The cleanup of the partially generated extension (removeFile(directory) in extensionInit's catch) was unguarded — if the removal failed for any reason, its error replaced the original install error and the partially removed extension directory (e.g. just a node_modules folder) was left behind, matching the state reported in the issue.

WHAT is this pull request doing?

In packages/app/src/cli/services/generate/extension.ts (plus a small @shopify/cli-kit addition):

  • Detects the pnpm blocked-builds failure (ERR_PNPM_IGNORED_BUILDS) and raises an AbortError with next steps: run pnpm approve-builds in the app directory, then generate the extension again. This works because the failed install records the ignored builds in the app root's node_modules/.modules.yaml, so pnpm approve-builds lists them even after the extension directory is removed (verified locally).
  • Makes the cleanup resilient and best-effort: cli-kit's removeFile now accepts optional maxRetries/retryDelay options passed straight through to Node's fs.rm, which retries transient errors (EBUSY/ENOTEMPTY/EPERM — e.g. antivirus or indexer locks on the freshly written node_modules) with linear backoff, retrying the specific failing entry rather than restarting the walk. The cleanup uses {maxRetries: 10, retryDelay: 100} (~5.5s worst case). If removal still fails, a warning names the leftover directory and tells the user to delete it, and the error that actually interrupted the generation is still thrown.

All generation flows (UI, function, theme; workspace and non-workspace installs) go through this catch, so they're all covered.

How to test your changes?

  1. Create an app that uses pnpm workspaces, with pnpm ≥ 12 (or pnpm ≥ 10 with strictDepBuilds: true).
  2. Ensure at least one extension-template dependency has a build script that isn't approved (no onlyBuiltDependencies entry).
  3. Run shopify app generate extension and pick a JS-flavored template.
  4. Before: raw Command failed with exit code 1: pnpm install dump. After: an error banner explaining pnpm blocked the build scripts, with pnpm approve-builds + regenerate as next steps, and no leftover extensions/<name> directory.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

🤖 Generated with Claude Code

…neration

When generating an extension in a pnpm app, recent pnpm versions fail the
non-interactive dependency install with ERR_PNPM_IGNORED_BUILDS because the
build-script approval prompt can't be answered from within the generation
tasks. The raw install error was surfaced as-is, and if the cleanup of the
partially generated extension failed, the cleanup error masked it and left
files behind.

Now the failure is mapped to an actionable error (run pnpm approve-builds,
then generate again), and the cleanup is best-effort: a cleanup failure warns
about the leftover directory instead of replacing the original error.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Assisted-By: devx/9ab0e8f6-64e1-4036-bcd3-470ba040dcdf
@github-actions github-actions Bot added the Area: @shopify/app @shopify/app package issues label Sep 7, 2026
@amcaplan
amcaplan force-pushed the graceful-pnpm-blocked-builds-on-generate branch from af395f8 to c32dda4 Compare September 7, 2026 21:13
@github-actions github-actions Bot added Area: @shopify/cli @shopify/cli package issues and removed Area: @shopify/app @shopify/app package issues labels Sep 7, 2026
@amcaplan
amcaplan marked this pull request as ready for review September 7, 2026 21:17
Copilot AI lite review requested due to automatic review settings September 7, 2026 21:17
@amcaplan
amcaplan requested a review from a team as a code owner September 7, 2026 21:17

Copilot AI 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.

🟢 Approval recommended

The changes are well-scoped and validated with tests, with only minor nits identified.

Pull request overview

Improves the extension generation flow to handle pnpm’s “blocked build scripts” behavior gracefully, while making cleanup of partially generated extensions more resilient to transient filesystem locks.

Changes:

  • Detects pnpm’s ERR_PNPM_IGNORED_BUILDS failure during shopify app generate extension and surfaces an AbortError with recovery steps.
  • Updates @shopify/cli-kit’s removeFile to support fs.rm retry options and uses them to best-effort clean up partially generated extensions.
  • Adds/updates tests and changesets to cover the new failure mode and cleanup behavior.
File summaries
File Description
packages/cli-kit/src/public/node/fs.ts Adds retry options to removeFile by delegating to fs.promises.rm.
packages/app/src/cli/services/generate/extension.ts Adds pnpm blocked-builds detection + resilient cleanup and user guidance.
packages/app/src/cli/services/generate/extension.test.ts Adds tests for pnpm blocked-builds messaging and cleanup-failure warning behavior.
.changeset/remove-file-retry-options.md Changeset for removeFile retry options addition.
.changeset/graceful-pnpm-blocked-builds-on-generate.md Changeset for improved pnpm blocked-builds recovery UX.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +131 to +132
if (isPnpmBlockedBuildsError(error)) {
throw new AbortError(
Comment thread packages/cli-kit/src/public/node/fs.ts Outdated
@amcaplan
amcaplan force-pushed the graceful-pnpm-blocked-builds-on-generate branch 2 times, most recently from ade3613 to d116228 Compare September 8, 2026 10:26
A failed install can leave transient locks on the extension's
node_modules (for example, from an antivirus scanning the freshly
written files). Let removeFile pass maxRetries/retryDelay through to
Node's fs.rm, which retries EBUSY/ENOTEMPTY/EPERM with linear backoff,
and use that (10 retries, 100ms) when removing the partially generated
extension before falling back to the leftover-directory warning.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Assisted-By: devx/9ab0e8f6-64e1-4036-bcd3-470ba040dcdf
@amcaplan
amcaplan force-pushed the graceful-pnpm-blocked-builds-on-generate branch from d116228 to 46dcc14 Compare September 8, 2026 10:30
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/fs.d.ts
@@ -114,12 +114,26 @@ export declare function mkdir(path: string): Promise<void>;
  * @param path - Path to the directory to be created.
  */
 export declare function mkdirSync(path: string): void;
+interface RemoveFileOptions {
+    /**
+     * Number of times Node retries the removal when it hits a transient error
+     * (EBUSY, EMFILE, ENFILE, ENOTEMPTY or EPERM), waiting `retryDelay` milliseconds
+     * longer on each try. Defaults to 0 (no retries).
+     */
+    maxRetries?: number;
+    /**
+     * Milliseconds to wait between retries. Defaults to 100.
+     */
+    retryDelay?: number;
+}
 /**
- * Removes a file at the given path.
+ * Removes a file or directory (recursively) at the given path.
  *
- * @param path - Path to the file to be removed.
+ * @param path - Path to the file or directory to be removed.
+ * @param options - Retry behavior, passed through to Node's `fs.rm`. Useful when the removal can
+ * race with transient locks, such as an antivirus scanning freshly written files.
  */
-export declare function removeFile(path: string): Promise<void>;
+export declare function removeFile(path: string, options?: RemoveFileOptions): Promise<void>;
 /**
  * Renames a file.
  * @param from - Path to the file to be renamed.

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

Labels

Area: @shopify/cli @shopify/cli package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants