Skip to content

Normalize paths in bundle-ui-step same-path guard#7408

Closed
isaacroldan wants to merge 1 commit intostable/3.94.0from
river-fix/bundle-ui-step-resolve-path
Closed

Normalize paths in bundle-ui-step same-path guard#7408
isaacroldan wants to merge 1 commit intostable/3.94.0from
river-fix/bundle-ui-step-resolve-path

Conversation

@isaacroldan
Copy link
Copy Markdown
Contributor

The same-path guard at bundle-ui-step.ts compared localOutputDir and bundleOutputDir as raw strings. In practice these two values are computed along different code paths (dirname(buildUIExtension()) vs dirname(extension.outputPath) or joinPath(..., bundleFolder)) and can resolve to the same filesystem directory while differing as strings — e.g. when one path has a . segment, a trailing slash, or otherwise non-canonical shape. When that happens, the guard slips through and fs-extra rejects the copy with "Source and destination must not be the same".

Normalize both sides via resolvePath before comparing so the guard catches any string variant that maps to the same directory.

WHY are these changes introduced?

Fixes #0000

WHAT is this pull request doing?

How to test your changes?

Post-release steps

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

The same-path guard at bundle-ui-step.ts compared `localOutputDir` and
`bundleOutputDir` as raw strings. In practice these two values are
computed along different code paths (`dirname(buildUIExtension())` vs
`dirname(extension.outputPath)` or `joinPath(..., bundleFolder)`) and
can resolve to the same filesystem directory while differing as strings
— e.g. when one path has a `.` segment, a trailing slash, or otherwise
non-canonical shape. When that happens, the guard slips through and
fs-extra rejects the copy with "Source and destination must not be the
same".

Normalize both sides via `resolvePath` before comparing so the guard
catches any string variant that maps to the same directory.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@isaacroldan isaacroldan marked this pull request as ready for review April 27, 2026 15:08
@isaacroldan isaacroldan requested review from a team as code owners April 27, 2026 15:08
@github-actions
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/common/version.d.ts
@@ -1 +1 @@
-export declare const CLI_KIT_VERSION = "3.94.0";
\ No newline at end of file
+export declare const CLI_KIT_VERSION = "3.94.1";
\ No newline at end of file
packages/cli-kit/dist/public/node/cli-launcher.d.ts
@@ -1,8 +1,6 @@
-import type { LazyCommandLoader } from './custom-oclif-loader.js';
 interface Options {
     moduleURL: string;
     argv?: string[];
-    lazyCommandLoader?: LazyCommandLoader;
 }
 /**
  * Launches the CLI.
packages/cli-kit/dist/public/node/cli.d.ts
@@ -1,4 +1,3 @@
-import type { LazyCommandLoader } from './custom-oclif-loader.js';
 /**
  * IMPORTANT NOTE: Imports in this module are dynamic to ensure that "setupEnvironmentVariables" can dynamically
  * set the DEBUG environment variable before the 'debug' package sets up its configuration when modules
@@ -8,8 +7,6 @@ interface RunCLIOptions {
     /** The value of import.meta.url of the CLI executable module */
     moduleURL: string;
     development: boolean;
-    /** Optional lazy command loader for on-demand command loading */
-    lazyCommandLoader?: LazyCommandLoader;
 }
 /**
  * A function that abstracts away setting up the environment and running
@@ -20,7 +17,6 @@ export declare function runCLI(options: RunCLIOptions & {
     runInCreateMode?: boolean;
 }, launchCLI?: (options: {
     moduleURL: string;
-    lazyCommandLoader?: LazyCommandLoader;
 }) => Promise<void>, argv?: string[], env?: NodeJS.ProcessEnv, versions?: NodeJS.ProcessVersions): Promise<void>;
 /**
  * A function for create-x CLIs that automatically runs the "init" command.
@@ -42,5 +38,5 @@ export declare const jsonFlag: {
 /**
  * Clear the CLI cache, used to store some API responses and handle notifications status
  */
-export declare function clearCache(): Promise<void>;
+export declare function clearCache(): void;
 export {};
\ No newline at end of file
packages/cli-kit/dist/public/node/error.d.ts
@@ -1,6 +1,6 @@
+import { AlertCustomSection } from './ui.js';
 import { OutputMessage } from './output.js';
 import { InlineToken, TokenItem } from '../../private/node/ui/components/TokenizedText.js';
-import type { AlertCustomSection } from './ui.js';
 export { ExtendableError } from 'ts-error';
 export declare enum FatalErrorType {
     Abort = 0,
packages/cli-kit/dist/public/node/is-global.d.ts
@@ -1,4 +1,4 @@
-import type { PackageManager } from './node-package-manager.js';
+import { PackageManager } from './node-package-manager.js';
 /**
  * Returns true if the current process is running in a global context.
  *
packages/cli-kit/dist/public/node/notifications-system.d.ts
@@ -24,7 +24,7 @@ declare const NotificationSchema: zod.ZodObject<{
     surface: zod.ZodOptional<zod.ZodString>;
 }, "strip", zod.ZodTypeAny, {
     id: string;
-    type: "info" | "error" | "warning";
+    type: "error" | "info" | "warning";
     message: string;
     frequency: "always" | "once" | "once_a_day" | "once_a_week";
     ownerChannel: string;
@@ -41,7 +41,7 @@ declare const NotificationSchema: zod.ZodObject<{
     surface?: string | undefined;
 }, {
     id: string;
-    type: "info" | "error" | "warning";
+    type: "error" | "info" | "warning";
     message: string;
     frequency: "always" | "once" | "once_a_day" | "once_a_week";
     ownerChannel: string;
@@ -84,7 +84,7 @@ declare const NotificationsSchema: zod.ZodObject<{
         surface: zod.ZodOptional<zod.ZodString>;
     }, "strip", zod.ZodTypeAny, {
         id: string;
-        type: "info" | "error" | "warning";
+        type: "error" | "info" | "warning";
         message: string;
         frequency: "always" | "once" | "once_a_day" | "once_a_week";
         ownerChannel: string;
@@ -101,7 +101,7 @@ declare const NotificationsSchema: zod.ZodObject<{
         surface?: string | undefined;
     }, {
         id: string;
-        type: "info" | "error" | "warning";
+        type: "error" | "info" | "warning";
         message: string;
         frequency: "always" | "once" | "once_a_day" | "once_a_week";
         ownerChannel: string;
@@ -120,7 +120,7 @@ declare const NotificationsSchema: zod.ZodObject<{
 }, "strip", zod.ZodTypeAny, {
     notifications: {
         id: string;
-        type: "info" | "error" | "warning";
+        type: "error" | "info" | "warning";
         message: string;
         frequency: "always" | "once" | "once_a_day" | "once_a_week";
         ownerChannel: string;
@@ -139,7 +139,7 @@ declare const NotificationsSchema: zod.ZodObject<{
 }, {
     notifications: {
         id: string;
-        type: "info" | "error" | "warning";
+        type: "error" | "info" | "warning";
         message: string;
         frequency: "always" | "once" | "once_a_day" | "once_a_week";
         ownerChannel: string;
packages/cli-kit/dist/public/node/hooks/postrun.d.ts
@@ -1,7 +1,3 @@
-/**
- * Postrun hook — uses dynamic imports to avoid loading heavy modules (base-command, analytics)
- * at module evaluation time. These are only needed after the command has already finished.
- */
 import { Hook } from '@oclif/core';
 /**
  * Check if post run hook has completed.
packages/cli-kit/dist/public/node/hooks/prerun.d.ts
@@ -14,4 +14,4 @@ export declare function parseCommandContent(cmdInfo: {
  * Triggers a background check for a newer CLI version (non-blocking).
  * The result is cached and consumed by the postrun hook for auto-upgrade.
  */
-export declare function checkForNewVersionInBackground(): Promise<void>;
\ No newline at end of file
+export declare function checkForNewVersionInBackground(): void;
\ No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does this merit a test?

Copy link
Copy Markdown
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

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

shopify app build ❌ still not working

Image

shopify-river Bot pushed a commit that referenced this pull request Apr 27, 2026
`shopify app dev` and `shopify app deploy` reassign each extension's
outputPath to a bundle directory (`.shopify/dev-bundle/<uid>` or
`.shopify/deploy-bundle/<uid>`) before the build steps run. `shopify
app build` doesn't — outputPath stays at its constructor default
`joinPath(extension.directory, outputRelativePath)`, so the
include_assets step's outputDir collapses onto extension.directory.
With no bundle to populate, the configKey/static/pattern copy
helpers were forwarding source files (e.g. tools.json, instructions.md)
onto themselves and fs-extra rejected with 'Source and destination
must not be the same'. We were also writing manifest.json into the
user's source tree.

Short-circuit the step when outputDir resolves to extension.directory.
`shopify app build` only needs the actual bundle output (handled by
bundle_ui), not the include_assets copy + manifest pipeline.

The two regression tests verify both the canonical-path and
non-canonical-path (`/test/./extension/...`) shapes.

Reported by Justin Henricks <justin.henricks@shopify.com> (3.94.1
`Bundle UI Extension` failure) and Trish Ta during repro of #7408
(`Include UI Extension Assets` failure on the same family of apps).

Co-Authored-By: Justin Henricks <justin.henricks@shopify.com>
Co-Authored-By: Trish Ta <trish.ta@shopify.com>
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.

4 participants