fix: stop the plugin update guard firing on npm install artifacts

[ankitranjan7]

No issue filed — found while investigating a failing test on #266, and unrelated to that PR, so it is on its own branch.

The problem

webcmd plugin update refuses to run on a plugin that the user never touched.

plugin install runs npm install --omit=dev --ignore-scripts inside the cloned plugin (src/plugin.ts). If that plugin repo has no .gitignore, git then reports node_modules/ and package-lock.json as untracked changes, and the "don't destroy uncommitted work" guard blocks the update:

$ webcmd plugin install github:rishabhraj36/webcmd-plugin-bookmyshow
✅ Plugin "bookmyshow" installed successfully.
$ webcmd plugin update bookmyshow
Error: Plugin "bookmyshow" has uncommitted changes that updating would destroy:
  node_modules/ (new, unstaged)
  package-lock.json (new, unstaged)

The guard is firing on webcmd's own output and blaming the user for work they never did. Such a plugin is permanently un-updatable short of --force, which is documented as "discard uncommitted changes" — an alarming flag to need on a clean checkout.

This is also what fails tests/e2e/plugin-management.test.ts > plugin update succeeds on an installed plugin on main.

What I changed

1. src/plugin.ts — getDirtyFiles now ignores node_modules/ and package-lock.json at any depth. Depth matters because monorepo installs run npm install at the repo root and in each sub-plugin.
2. src/plugin.ts — pulled the porcelain path parsing into one dirtyEntryPath helper, now shared with describeDirtyEntry.
3. Anything else untracked is still treated as real user work and still blocks the update. A file merely named like an artifact (node_modules_notes.md) still blocks — there is a test for that.

Deliberately not filtered: the .js files transpilePluginTs emits next to .ts sources. Those are indistinguishable from hand-written files, so failing closed is the right default.

Proof

• npx vitest run → 405 files, 4854 passed, 1 skipped, 0 failed — the E2E test now passes.
• npm run typecheck clean.
• By hand: install → update against the real bookmyshow plugin now succeeds.

🤖 Generated with Claude Code

[github-actions]

🟢 No documentation gap found — medium confidence

The automated review found no documentation gap in the supplied changes.

This review is advisory and does not block merging.

[beubax]

Maintainer review: changes requested

The underlying concern is valid. plugin install runs npm install inside a checkout, and repositories without an appropriate .gitignore can then report node_modules/ and package-lock.json as untracked. Refusing every later update because of files Webcmd itself created is a real usability bug.

The current filter weakens the data-loss guard too far, though.

Blocking behavior

getDirtyFiles() filters artifact-looking paths regardless of their porcelain status:

webcmd/src/plugin.ts

Lines 571 to 591 in 4eb9c54

	encoding: 'utf-8',
	stdio: ['pipe', 'pipe', 'pipe'],
	});
	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)}`,
	'This can happen when git refuses to run here (e.g. "detected dubious ownership in repository"). Re-run with --force to update anyway — this accepts the risk of discarding uncommitted work, which is why it is not the default.',
	);
	}
	}
	/**
	* 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$)/;

That means all of these can be silently removed from the dirty set:

• a tracked and modified package-lock.json;
• a staged lockfile;
• a tracked deletion;
• a tracked file under node_modules/ in a repository that deliberately versions such content;
• rename/conflict statuses whose destination happens to match the artifact pattern.

updatePlugin later replaces the plugin directory wholesale. Treating tracked changes as clean converts a false-positive fix into a possible loss of user work.

The new test currently codifies the unsafe behavior: it supplies M packages/alpha/package-lock.json and expects an empty dirty list. That entry is not an npm-created untracked artifact; it is a tracked modification and must continue blocking the update.

Required behavior

Please ignore only proven install-created untracked entries:

• status must be ??;
• path must be exactly package-lock.json or beneath/exactly node_modules, including the same paths inside monorepo subdirectories;
• every other porcelain status must be returned unchanged.

In other words, decide from the two status columns before path filtering. Avoid trimming away the leading status column before interpreting it. Tracked/staged/deleted/unmerged/renamed entries must never be exempted merely because their path looks generated.

Required tests

Please retain the reproduction for:

?? node_modules/
?? package-lock.json
?? packages/alpha/node_modules/
?? packages/alpha/package-lock.json

and add separate assertions that each still blocks:

 M package-lock.json
M  package-lock.json
 D package-lock.json
D  package-lock.json
A  package-lock.json
 M packages/alpha/package-lock.json

At least one tracked path under node_modules/ should also remain dirty. If rename parsing is supported here, cover R old -> package-lock.json as tracked user work rather than filtering it.

Scope and deployment

This is local plugin-update behavior only. Hosted plugin installation/update routes through Cloud and does not call this local replacement guard, so no Cloud deployment or package-pin change is needed.

Once the exemption is limited to ?? entries and the tracked-status regression tests are present, the approach is otherwise appropriately small.

[ankitranjan7]

Thanks — you're right that the filter was too broad. Pushed 5546495.

Only ?? entries are exempt now. getDirtyFiles no longer trims the line before deciding: it reads the two porcelain status columns off the raw line, and a path-based exemption is applied only when those columns are exactly ??. Everything else ( M, M , D, D , A , R , UU, …) is returned unchanged regardless of what the path looks like, so a tracked/staged lockfile, a tracked deletion, and deliberately versioned content under node_modules/ all keep blocking the update. Path matching is unchanged and still handles monorepo subdirectories: (^|/)(node_modules(/|$)|package-lock\.json$).

Tests. The reproduction is retained, with the M packages/alpha/package-lock.json line corrected to ?? packages/alpha/package-lock.json — you're right that as written it codified the unsafe behavior:

?? node_modules/
?? package-lock.json
?? packages/alpha/node_modules/
?? packages/alpha/package-lock.json

Added a table of tracked-status cases that each must still block, covering everything you listed plus a tracked path under node_modules/, a rename, and an unmerged entry:

 M package-lock.json
M  package-lock.json
 D package-lock.json
D  package-lock.json
A  package-lock.json
 M packages/alpha/package-lock.json
 M node_modules/vendored/patch.js
R  old-lock.json -> package-lock.json
UU package-lock.json

vitest run --project unit --project plugin src/plugin.test.ts passes (108 tests) and tsc --noEmit is clean.

Agreed on scope — local plugin-update behavior only, no Cloud deployment or package pin involved.