From 4eb9c549f66ea6507b93a538a10b1cca9d60cc6a Mon Sep 17 00:00:00 2001 From: Ankit Ranjan Date: Mon, 10 Aug 2026 20:31:06 +0530 Subject: [PATCH 1/2] fix: stop the plugin update guard firing on npm install artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plugin install` runs `npm install` in the checkout, so a plugin repo without a .gitignore reports node_modules/ and package-lock.json as untracked. The dirty-checkout guard then refused every update — blaming the user for work webcmd itself created, and leaving such plugins permanently un-updatable short of --force. getDirtyFiles now drops those paths (at any depth, covering monorepo sub-plugin installs) before the guard decides. Anything else untracked is still real user work and still blocks. Fixes the plugin-management E2E. Co-Authored-By: Claude Opus 5 --- src/plugin.test.ts | 16 ++++++++++++++++ src/plugin.ts | 31 ++++++++++++++++++++++--------- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/plugin.test.ts b/src/plugin.test.ts index 46eae40a..85bba178 100644 --- a/src/plugin.test.ts +++ b/src/plugin.test.ts @@ -1914,6 +1914,22 @@ describe('getDirtyFiles', () => { expect(pluginModule.getDirtyFiles('/some/dir')).toEqual(['M foo.js', '?? untracked.js']); }); + it('ignores the node_modules/package-lock.json that install itself created', () => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return '?? node_modules/\n?? package-lock.json\n?? packages/alpha/node_modules/\n M packages/alpha/package-lock.json\n'; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual([]); + }); + + it('still reports user work that merely looks like an install artifact', () => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return '?? node_modules_notes.md\n M src/package-lock.json.bak\n'; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual(['?? node_modules_notes.md', 'M src/package-lock.json.bak']); + }); + it('does not pass --untracked-files=no, so untracked files are reported (git already omits gitignored paths)', () => { mockExecFileSync.mockImplementation((cmd, args) => { expect(args).not.toContain('--untracked-files=no'); diff --git a/src/plugin.ts b/src/plugin.ts index 4208ca9c..79d3baa9 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -533,11 +533,10 @@ function describeGitError(error: unknown): string { /** * Report tracked-file modifications and untracked files within `dir` in a git checkout. * - * Untracked files are included on purpose: `git status` already excludes - * gitignored paths (build output like node_modules/dist never shows up), so - * anything untracked that does show up is real, unsaved user work — e.g. a - * new command file that hasn't been `git add`ed yet — which updating would - * destroy just as surely as an uncommitted edit to a tracked file. + * Untracked files are included on purpose: anything untracked is real, unsaved + * user work — e.g. a new command file that hasn't been `git add`ed yet — which + * updating would destroy just as surely as an uncommitted edit to a tracked + * file. The exception is `installArtifacts`: those are ours, not the user's. * * The `-- .` pathspec on `git status` restricts the report to `dir` itself. * Without it, git reports the *entire enclosing repository* — e.g. a plugin @@ -572,7 +571,8 @@ export function getDirtyFiles(dir: string): string[] { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], }); - return out.split('\n').map((line) => line.trim()).filter(Boolean); + return out.split('\n').map((line) => line.trim()).filter(Boolean) + .filter((entry) => !installArtifacts.test(dirtyEntryPath(entry))); } catch (error) { throw new PluginError( `Could not determine whether "${dir}" has uncommitted changes: git failed with: ${describeGitError(error)}`, @@ -581,10 +581,23 @@ export function getDirtyFiles(dir: string): string[] { } } +/** + * Artifacts `installDependencies` creates by running `npm install` in the + * checkout, at the repo root and in every sub-plugin of a monorepo. A plugin + * repo without a .gitignore reports them as dirty, so without this the guard + * fires on webcmd's own output and every such plugin is permanently + * un-updatable — blaming the user for work they never did. + */ +const installArtifacts = /(?:^|\/)(?:node_modules(?:\/|$)|package-lock\.json$)/; + +/** Path portion of a `git status --porcelain` entry (already trimmed of its leading space). */ +function dirtyEntryPath(entry: string): string { + return entry.startsWith('??') ? entry.slice(2).trim() : entry.replace(/^[MADRCU!]{1,2}\s+/, ''); +} + function describeDirtyEntry(entry: string): string { - const isUntracked = entry.startsWith('??'); - const file = entry.replace(/^\?\?\s*/, '').replace(/^[MADRCU! ]+\s*/, ''); - return isUntracked ? `${file} (new, unstaged)` : `${file} (modified)`; + const file = dirtyEntryPath(entry); + return entry.startsWith('??') ? `${file} (new, unstaged)` : `${file} (modified)`; } function assertPluginNotDirty(name: string, dir: string, force: boolean): void { From 55464957d444a36706369da00ec7b8895af225e7 Mon Sep 17 00:00:00 2001 From: Ankit Ranjan Date: Wed, 12 Aug 2026 00:52:10 +0530 Subject: [PATCH 2/2] fix: exempt only untracked npm artifacts from the plugin update guard Read the two porcelain status columns before any path filtering, so only `??` entries at node_modules/package-lock.json paths are treated as install output. Tracked, staged, deleted, renamed and unmerged entries at those same paths are user work updatePlugin would destroy, and keep blocking the update. Co-Authored-By: Claude Opus 5 --- src/plugin.test.ts | 22 +++++++++++++++++++++- src/plugin.ts | 17 +++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/plugin.test.ts b/src/plugin.test.ts index 85bba178..aaa8ddce 100644 --- a/src/plugin.test.ts +++ b/src/plugin.test.ts @@ -1917,7 +1917,7 @@ describe('getDirtyFiles', () => { it('ignores the node_modules/package-lock.json that install itself created', () => { mockExecFileSync.mockImplementation((cmd, args) => { if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; - return '?? node_modules/\n?? package-lock.json\n?? packages/alpha/node_modules/\n M packages/alpha/package-lock.json\n'; + return '?? node_modules/\n?? package-lock.json\n?? packages/alpha/node_modules/\n?? packages/alpha/package-lock.json\n'; }); expect(pluginModule.getDirtyFiles('/some/dir')).toEqual([]); }); @@ -1930,6 +1930,26 @@ describe('getDirtyFiles', () => { expect(pluginModule.getDirtyFiles('/some/dir')).toEqual(['?? node_modules_notes.md', 'M src/package-lock.json.bak']); }); + // Only `??` is npm's own output. Every tracked status at the same path is + // user work that updatePlugin would destroy, so it must keep blocking. + it.each([ + [' M package-lock.json', 'M package-lock.json'], + ['M package-lock.json', 'M package-lock.json'], + [' D package-lock.json', 'D package-lock.json'], + ['D package-lock.json', 'D package-lock.json'], + ['A package-lock.json', 'A package-lock.json'], + [' M packages/alpha/package-lock.json', 'M packages/alpha/package-lock.json'], + [' M node_modules/vendored/patch.js', 'M node_modules/vendored/patch.js'], + ['R old-lock.json -> package-lock.json', 'R old-lock.json -> package-lock.json'], + ['UU package-lock.json', 'UU package-lock.json'], + ])('keeps tracked entry %j dirty', (porcelain, expected) => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return `${porcelain}\n`; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual([expected]); + }); + it('does not pass --untracked-files=no, so untracked files are reported (git already omits gitignored paths)', () => { mockExecFileSync.mockImplementation((cmd, args) => { expect(args).not.toContain('--untracked-files=no'); diff --git a/src/plugin.ts b/src/plugin.ts index 79d3baa9..746b2a26 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -571,8 +571,9 @@ export function getDirtyFiles(dir: string): string[] { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], }); - return out.split('\n').map((line) => line.trim()).filter(Boolean) - .filter((entry) => !installArtifacts.test(dirtyEntryPath(entry))); + return out.split('\n').filter((line) => line.trim()) + .filter((line) => !isInstallArtifact(line)) + .map((line) => line.trim()); } catch (error) { throw new PluginError( `Could not determine whether "${dir}" has uncommitted changes: git failed with: ${describeGitError(error)}`, @@ -590,6 +591,18 @@ export function getDirtyFiles(dir: string): string[] { */ const installArtifacts = /(?:^|\/)(?:node_modules(?:\/|$)|package-lock\.json$)/; +/** + * True only for an artifact npm itself created: a `??` (untracked) porcelain + * entry at an artifact path. The status columns are read from the raw line + * before any trimming, because every other status — ` M`, `M `, ` D`, `A `, + * `R `, `UU` — is tracked work the user could lose when `updatePlugin` + * replaces the directory, no matter what the path looks like. + */ +function isInstallArtifact(line: string): boolean { + if (line.slice(0, 2) !== '??') return false; + return installArtifacts.test(line.slice(2).trim()); +} + /** Path portion of a `git status --porcelain` entry (already trimmed of its leading space). */ function dirtyEntryPath(entry: string): string { return entry.startsWith('??') ? entry.slice(2).trim() : entry.replace(/^[MADRCU!]{1,2}\s+/, '');