Skip to content

fix(init): harden identity edits and feature toggle persistence - #62

Open
Adi-ty wants to merge 3 commits into
mainfrom
fix/init-hardening
Open

Adi-ty wants to merge 3 commits into
mainfrom
fix/init-hardening

Conversation

@Adi-ty

@Adi-ty Adi-ty commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Harden the init engine against silent failures, unsafe inputs, and partial writes so setup and manage flows always leave the project in a recoverable state.

Changes

Validation & selection (validation.js, selection.js):

  • Validate feature flags, setup flags, paths, and cleanup targets upfront before any mutation
  • Shared capability selection logic for setup and manage

Async rollback & error propagation (features.js, transform.js):

  • Rollback journal is async with aggregated AggregateError on undo failures
  • Identity edits wrapped in withFileRollback — failed writes/renames restore files and preserve unrelated metadata
  • Failed or no-op feature toggles no longer rewrite persisted state

Path safety (transform.js, cleanup.js, examples.js):

  • resolveWithin resolves symlinks via realpathSync to block path traversal
  • planRenames detects destination collisions and case-only renames
  • Cleanup targets validated upfront; project root deletion refused

Structured errors (git.js, index.js):

  • Git operations throw instead of returning false
  • Git failures report "Project setup completed" without misleading backup advice
  • Partial failure warnings when setup stops mid-mutation

Type validation (features.js):

  • Dependency and script values must be strings

How I verified

npm run check

Acceptance criteria

Code quality

  • Lint — zero errors
  • Tests pass

Housekeeping

  • CHANGELOG.md entry under ## Unreleased

Identity edits now roll back failed writes/renames and keep unrelated metadata. Failed or no-op feature toggles no longer rewrite persisted
state. Dependency/script values must be strings. Git failures now report project setup completion without misleading backup advice.
@Adi-ty
Adi-ty marked this pull request as ready for review September 15, 2026 05:50
Copilot AI balanced review requested due to automatic review settings September 15, 2026 05:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved rollback, persistence, validation, and rename-collision issues block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens @rtcamp/wp-tooling init setup and manage flows against invalid inputs, unsafe paths, partial writes, and silent failures.

Changes:

  • Adds shared validation and capability selection.
  • Adds rollback and error propagation for identity and feature operations.
  • Improves path safety, cleanup validation, and rename collision handling.
  • Expands tests and documents the changes in the changelog.
File summaries
File Summary
node-packages/wp-tooling/tests/init/validation.test.js Covers validation behavior.
node-packages/wp-tooling/tests/init/transform.test.js Covers path safety, rollback, and rename planning.
node-packages/wp-tooling/tests/init/selection.test.js Covers capability selection.
node-packages/wp-tooling/tests/init/persist.test.js Covers persistence behavior.
node-packages/wp-tooling/tests/init/manage.test.js Covers manage-flow behavior.
node-packages/wp-tooling/tests/init/index.test.js Covers setup orchestration and errors.
node-packages/wp-tooling/tests/init/identity.test.js Covers identity validation and edits.
node-packages/wp-tooling/tests/init/git.test.js Covers Git error propagation.
node-packages/wp-tooling/tests/init/features.test.js Covers feature validation and transitions.
node-packages/wp-tooling/tests/init/cleanup.test.js Covers cleanup validation and safety.
node-packages/wp-tooling/tests/init/capabilities.test.js Covers capability discovery and selection.
node-packages/wp-tooling/src/init/validation.js Implements shared validation.
node-packages/wp-tooling/src/init/transform.js Implements path safety and rename planning; case-insensitive destination collisions remain moderate (2 votes).
node-packages/wp-tooling/src/init/selection.js Implements shared capability selection.
node-packages/wp-tooling/src/init/persist.js Implements persistence support.
node-packages/wp-tooling/src/init/manage.js Implements safer manage orchestration.
node-packages/wp-tooling/src/init/index.js Implements setup orchestration; identity edits lack full rollback protection (critical, 2 votes).
node-packages/wp-tooling/src/init/identity.js Implements identity validation and rollback-aware edits.
node-packages/wp-tooling/src/init/git.js Implements Git error propagation.
node-packages/wp-tooling/src/init/features.js Implements feature transitions and validation; persistence failures can leave changes unapplied to state (critical, 2 votes), and marker types are not validated (moderate, 1 vote).
node-packages/wp-tooling/src/init/examples.js Implements symlink-safe example handling.
node-packages/wp-tooling/src/init/cleanup.js Implements cleanup validation; falsy malformed targets can still be silently accepted (moderate, 1 vote).
node-packages/wp-tooling/CHANGELOG.md Documents the unreleased changes.
Review details

Suppressed comments (2)

node-packages/wp-tooling/src/init/cleanup.js:22

  • The --clean flow still passes (config.cleanup && config.cleanup.targets) || [] to runCleanup, so falsy malformed values such as null, false, or '' become an empty list before this validator runs. That makes standalone cleanup silently accept invalid configuration even though setup validates it upfront. Pass the raw value through (or validate it in cleanFlow) so every entry point enforces the array contract.
const resolveCleanupTargets = (root, targets = []) => {
	if (!Array.isArray(targets)) {
		throw new Error(
			`Expected cleanup.targets to be an array, received ${JSON.stringify(targets)}`
		);

node-packages/wp-tooling/src/init/features.js:123

  • The manifest validator does not validate examples.marker or group.marker as strings. A truthy non-string marker (for example marker: 1) passes all preflight checks, then applyExamples reaches markerLine() after identity changes and calls marker.replace, causing a late TypeError and a partially applied setup. Validate the marker types/non-empty values before mutation.
	const groups = config.examples?.groups || [];
	if (!Array.isArray(groups)) {
		throw new Error('Expected examples.groups to be an array.');
	}
	const groupKeys = new Set();
	for (const group of groups) {
		if (
  • Files reviewed: 23/23 changed files
  • Comments generated: 3
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.


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

Comment thread node-packages/wp-tooling/src/init/features.js Outdated
Comment thread node-packages/wp-tooling/src/init/index.js Outdated
Comment thread node-packages/wp-tooling/src/init/transform.js Outdated
Comment thread node-packages/wp-tooling/src/init/transform.js Outdated
Comment thread node-packages/wp-tooling/src/init/identity.js Outdated
Copilot AI review requested due to automatic review settings September 16, 2026 04:36
@Adi-ty
Adi-ty requested a review from 00kaku September 16, 2026 04:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved moderate and critical findings remain in feature toggling and manage-state refresh paths.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

node-packages/wp-tooling/src/init/features.js:42

  • The || [] fallback makes the new array-type guard ineffective for explicit falsey non-arrays: features: null, false, or '' is silently treated as an empty manifest and setup proceeds. Since this function is the pre-mutation manifest validator (and the tests reject truthy non-arrays), only an omitted property should default to []; otherwise malformed configs are accepted instead of failing early.

This issue also appears on line 128 of the same file.

node-packages/wp-tooling/src/init/features.js:131

  • The same || [] fallback bypasses the array validation for explicit falsey values such as examples: { groups: null } or groups: false, so malformed example manifests are accepted. Default only when groups is undefined so the Array.isArray check can reject an explicitly supplied non-array.
	const groups = config.examples?.groups || [];
	if (!Array.isArray(groups)) {
		throw new Error('Expected examples.groups to be an array.');
	}

node-packages/wp-tooling/src/init/features.js:903

  • The survivor set includes every feature in wantOn, even features that are currently disabled and have not been enabled yet. If a later enable then fails, this leaves that feature's shared dependency/script in package.json while finalMap records it as disabled, producing an orphaned dependency. Calculate survivors from enabledKeys (or roll back the earlier disable) so only features actually still enabled retain ownership.
				const survivors = (config.features || []).filter(
					(feature) =>
						feature.key !== r.key &&
						(enabledKeys.has(feature.key) ||
							wantOn.has(feature.key))
				);

node-packages/wp-tooling/src/init/features.js:933

  • This enable-side catch has the same rollback propagation problem: an AggregateError from the feature journal is swallowed into failed, so finalization may persist state after a rollback that did not complete. Rethrow aggregate rollback failures so the outer journal restores earlier transitions and the caller receives the recovery error.
			} catch (err) {
				spin.fail(`Failed to enable ${r.label}`);
				ui.error(err.message);
				failed.push(r.key);
			}

node-packages/wp-tooling/src/init/manage.js:241

  • The interactive manage loop reuses one FeatureApi, but after toggleFeatures persists result.finalMap this branch only checks failed and never refreshes api.identity.features. A later toggle in the same session therefore gives detect/onEnable/onDisable hooks the stale persisted map; update the in-memory identity after a changed result before continuing.
		if (result.failed.length) {
			return;
		}
  • Files reviewed: 23/23 changed files
  • Comments generated: 1
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment on lines +909 to +913
} catch (err) {
spin.fail(`Failed to disable ${r.label}`);
ui.error(err.message);
failed.push(r.key);
}
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.

3 participants