Skip to content

Commit 94014ff

Browse files
steipetesteipete
committed
fix: rollback npm peer planning failures
1 parent 9716548 commit 94014ff

4 files changed

Lines changed: 126 additions & 17 deletions

File tree

‎src/infra/npm-managed-root.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,9 @@ describe("managed npm root", () => {
386386
"old-peer": "1.0.0",
387387
plugin: "1.0.0",
388388
},
389+
devDependencies: {
390+
"dev-plugin": "1.0.0",
391+
},
389392
openclaw: {
390393
managedPeerDependencies: ["old-peer"],
391394
},
@@ -421,6 +424,17 @@ describe("managed npm root", () => {
421424
"node_modules/existing-root": {
422425
version: "1.0.0",
423426
},
427+
"node_modules/dev-peer": {
428+
dev: true,
429+
version: "3.0.0",
430+
},
431+
"node_modules/dev-plugin": {
432+
dev: true,
433+
peerDependencies: {
434+
"dev-peer": "^3.0.0",
435+
},
436+
version: "1.0.0",
437+
},
424438
"node_modules/new-peer": {
425439
peer: true,
426440
version: "2.1.0",
@@ -475,6 +489,9 @@ describe("managed npm root", () => {
475489
"new-peer": "2.1.0",
476490
plugin: "1.0.0",
477491
},
492+
devDependencies: {
493+
"dev-plugin": "1.0.0",
494+
},
478495
openclaw: {
479496
managedPeerDependencies: ["new-peer"],
480497
},

‎src/infra/npm-managed-root.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,10 @@ function isOptionalPeerDependency(manifest: Record<string, unknown>, peerName: s
288288
return isRecord(peerMetadata) && peerMetadata.optional === true;
289289
}
290290

291+
function isDevOnlyLockPackage(value: unknown): boolean {
292+
return isRecord(value) && value.dev === true;
293+
}
294+
291295
function readLockPackageName(location: string, value: unknown): string | undefined {
292296
if (isRecord(value)) {
293297
const packageName = readOptionalString(value.name);
@@ -322,7 +326,7 @@ function findLockPackageVersion(params: {
322326
}
323327
const preferredLocation = `node_modules/${params.packageName}`;
324328
const preferredPackage = params.lockfile.packages[preferredLocation];
325-
if (isRecord(preferredPackage)) {
329+
if (isRecord(preferredPackage) && !isDevOnlyLockPackage(preferredPackage)) {
326330
const preferredVersion = readOptionalString(preferredPackage.version);
327331
if (preferredVersion) {
328332
return preferredVersion;
@@ -331,7 +335,11 @@ function findLockPackageVersion(params: {
331335
for (const [location, value] of Object.entries(params.lockfile.packages).toSorted(
332336
([left], [right]) => left.localeCompare(right),
333337
)) {
334-
if (readLockPackageName(location, value) !== params.packageName || !isRecord(value)) {
338+
if (
339+
readLockPackageName(location, value) !== params.packageName ||
340+
!isRecord(value) ||
341+
isDevOnlyLockPackage(value)
342+
) {
335343
continue;
336344
}
337345
const version = readOptionalString(value.version);
@@ -350,7 +358,7 @@ function collectNpmLockPeerDependencyPins(params: {
350358
for (const [location, value] of Object.entries(packages).toSorted(([left], [right]) =>
351359
left.localeCompare(right),
352360
)) {
353-
if (location === "" || !isRecord(value)) {
361+
if (location === "" || !isRecord(value) || isDevOnlyLockPackage(value)) {
354362
continue;
355363
}
356364
const packageName = readLockPackageName(location, value);

‎src/plugins/install.npm-spec.e2e.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,55 @@ describe("installPluginFromNpmSpec e2e", () => {
694694
).rejects.toHaveProperty("code", "ENOENT");
695695
});
696696

697+
it("rolls back the managed root when npm peer planning fails", async () => {
698+
const rootDir = await makeTempDir("npm-plugin-peer-plan-rollback-e2e");
699+
const npmRoot = path.join(rootDir, "managed-npm");
700+
const blockedPlugin = `missing-peer-plugin-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
701+
const missingPeer = `missing-peer-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
702+
const registry = await startStaticRegistry([
703+
{
704+
packageName: blockedPlugin,
705+
latest: "1.0.0",
706+
versions: [
707+
await packPlugin({
708+
packageName: blockedPlugin,
709+
peerDependencies: { [missingPeer]: "^1.0.0" },
710+
peerDependenciesMeta: {},
711+
pluginId: blockedPlugin,
712+
version: "1.0.0",
713+
rootDir,
714+
}),
715+
],
716+
},
717+
]);
718+
process.env.NPM_CONFIG_REGISTRY = registry;
719+
process.env.npm_config_registry = registry;
720+
721+
const result = await installPluginFromNpmSpec({
722+
spec: `${blockedPlugin}@1.0.0`,
723+
npmDir: npmRoot,
724+
logger: { info: () => {}, warn: () => {} },
725+
timeoutMs: 120_000,
726+
});
727+
728+
if (result.ok) {
729+
throw new Error("expected npm peer planning to fail");
730+
}
731+
expect(result.error).toContain("npm peer dependency planning failed");
732+
const rootManifest = JSON.parse(
733+
await fs.readFile(path.join(npmRoot, "package.json"), "utf8"),
734+
) as {
735+
dependencies?: Record<string, string>;
736+
openclaw?: { managedPeerDependencies?: string[] };
737+
};
738+
expect(rootManifest.dependencies?.[blockedPlugin]).toBeUndefined();
739+
expect(rootManifest.dependencies?.[missingPeer]).toBeUndefined();
740+
expect(rootManifest.openclaw?.managedPeerDependencies ?? []).not.toContain(missingPeer);
741+
await expect(
742+
fs.lstat(path.join(npmRoot, "node_modules", blockedPlugin, "package.json")),
743+
).rejects.toHaveProperty("code", "ENOENT");
744+
});
745+
697746
it("does not take ownership of an existing root dependency observed as a peer", async () => {
698747
const rootDir = await makeTempDir("npm-plugin-peer-existing-root-e2e");
699748
const npmRoot = path.join(rootDir, "managed-npm");

‎src/plugins/install.ts‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -635,13 +635,47 @@ async function installPluginFromManagedNpmRoot(
635635
const rollbackPeerDependencySnapshot = await readManagedNpmRootPeerDependencySnapshot({
636636
npmRoot,
637637
});
638+
const rollbackFailedManagedNpmInstall = async (error: string): Promise<InstallPluginResult> => {
639+
await rollbackManagedNpmPluginInstall({
640+
npmRoot,
641+
packageName: params.packageName,
642+
targetDir: installRoot,
643+
timeoutMs,
644+
logger,
645+
peerDependencySnapshot: rollbackPeerDependencySnapshot,
646+
});
647+
return { ok: false, error };
648+
};
649+
const syncManagedPeerDependenciesForInstall = async (options?: {
650+
omitUnsupportedManagedOverrides?: boolean;
651+
}): Promise<{ ok: true; changed: boolean } | { ok: false; error: string }> => {
652+
try {
653+
return {
654+
ok: true,
655+
changed: await syncManagedNpmRootPeerDependencies({
656+
npmRoot,
657+
managedOverrides,
658+
omitUnsupportedManagedOverrides: options?.omitUnsupportedManagedOverrides,
659+
timeoutMs,
660+
}),
661+
};
662+
} catch (error) {
663+
return {
664+
ok: false,
665+
error: `npm peer dependency planning failed: ${error instanceof Error ? error.message : String(error)}`,
666+
};
667+
}
668+
};
638669
await upsertManagedNpmRootDependency({
639670
npmRoot,
640671
packageName: params.packageName,
641672
dependencySpec: params.dependencySpec,
642673
managedOverrides,
643674
});
644-
await syncManagedNpmRootPeerDependencies({ npmRoot, managedOverrides, timeoutMs });
675+
const initialPeerSync = await syncManagedPeerDependenciesForInstall();
676+
if (!initialPeerSync.ok) {
677+
return await rollbackFailedManagedNpmInstall(initialPeerSync.error);
678+
}
645679
const npmInstallArgs = [
646680
"npm",
647681
...createSafeNpmInstallArgs({
@@ -676,12 +710,12 @@ async function installPluginFromManagedNpmRoot(
676710
managedOverrides,
677711
omitUnsupportedManagedOverrides: true,
678712
});
679-
await syncManagedNpmRootPeerDependencies({
680-
npmRoot,
681-
managedOverrides,
713+
const aliasRetryPeerSync = await syncManagedPeerDependenciesForInstall({
682714
omitUnsupportedManagedOverrides: true,
683-
timeoutMs,
684715
});
716+
if (!aliasRetryPeerSync.ok) {
717+
return await rollbackFailedManagedNpmInstall(aliasRetryPeerSync.error);
718+
}
685719
install = await runCommandWithTimeout(npmInstallArgs, npmInstallOptions);
686720
}
687721
if (install.code !== 0) {
@@ -700,12 +734,13 @@ async function installPluginFromManagedNpmRoot(
700734
}
701735
let settledManagedPeerDependencies = false;
702736
for (let peerSyncPass = 0; peerSyncPass < 10; peerSyncPass += 1) {
703-
const syncedPeerDependencies = await syncManagedNpmRootPeerDependencies({
704-
npmRoot,
705-
managedOverrides,
737+
const peerSync = await syncManagedPeerDependenciesForInstall({
706738
omitUnsupportedManagedOverrides,
707-
timeoutMs,
708739
});
740+
if (!peerSync.ok) {
741+
return await rollbackFailedManagedNpmInstall(peerSync.error);
742+
}
743+
const syncedPeerDependencies = peerSync.changed;
709744
if (!syncedPeerDependencies) {
710745
settledManagedPeerDependencies = true;
711746
break;
@@ -727,13 +762,13 @@ async function installPluginFromManagedNpmRoot(
727762
}
728763
}
729764
if (!settledManagedPeerDependencies) {
730-
const syncedPeerDependencies = await syncManagedNpmRootPeerDependencies({
731-
npmRoot,
732-
managedOverrides,
765+
const peerSync = await syncManagedPeerDependenciesForInstall({
733766
omitUnsupportedManagedOverrides,
734-
timeoutMs,
735767
});
736-
settledManagedPeerDependencies = !syncedPeerDependencies;
768+
if (!peerSync.ok) {
769+
return await rollbackFailedManagedNpmInstall(peerSync.error);
770+
}
771+
settledManagedPeerDependencies = !peerSync.changed;
737772
}
738773
if (!settledManagedPeerDependencies) {
739774
await rollbackManagedNpmPluginInstall({

0 commit comments

Comments
 (0)