Skip to content

Commit 402b0df

Browse files
shakkernerdshakkernerd
committed
fix: preserve owned plugin dependencies during peer repair
1 parent f4cb203 commit 402b0df

3 files changed

Lines changed: 170 additions & 24 deletions

File tree

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,7 @@ export async function syncManagedNpmRootPeerDependencies(params: {
521521
const manifest = await readManagedNpmRootManifest(manifestPath);
522522
const dependencies = readDependencyRecord(manifest.dependencies);
523523
const previousManagedPeerDependencies = readManagedPeerDependencyKeys(manifest.openclaw);
524+
const previousManagedPeerDependencySet = new Set(previousManagedPeerDependencies);
524525
const peerPins = await collectManagedNpmRootPeerDependencyPins({ npmRoot: params.npmRoot });
525526
const nextDependencies = { ...dependencies };
526527
for (const packageName of previousManagedPeerDependencies) {
@@ -541,7 +542,13 @@ export async function syncManagedNpmRootPeerDependencies(params: {
541542
delete overrides[key];
542543
}
543544
Object.assign(overrides, managedOverrides);
544-
const managedPeerDependencyKeys = Object.keys(peerPins).toSorted();
545+
const managedPeerDependencyKeys = Object.keys(peerPins)
546+
.filter(
547+
(packageName) =>
548+
previousManagedPeerDependencySet.has(packageName) ||
549+
!Object.hasOwn(dependencies, packageName),
550+
)
551+
.toSorted();
545552
const openclawMetadata = buildManagedOpenClawMetadata({
546553
current: manifest.openclaw,
547554
managedOverrideKeys,
@@ -569,13 +576,6 @@ export async function syncManagedNpmRootPeerDependencies(params: {
569576
return changed;
570577
}
571578

572-
export async function readManagedNpmRootPeerDependencyNames(params: {
573-
npmRoot: string;
574-
}): Promise<Set<string>> {
575-
const manifest = await readManagedNpmRootManifest(path.join(params.npmRoot, "package.json"));
576-
return new Set(readManagedPeerDependencyKeys(manifest.openclaw));
577-
}
578-
579579
export async function repairManagedNpmRootOpenClawPeer(params: {
580580
npmRoot: string;
581581
timeoutMs?: number;

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

Lines changed: 131 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ describe("installPluginFromNpmSpec e2e", () => {
444444
).resolves.toBeTruthy();
445445
});
446446

447-
it("does not attribute repaired pre-existing peer dependencies to later installs", async () => {
447+
it("scans repaired pre-existing peer dependencies during later installs", async () => {
448448
const rootDir = await makeTempDir("npm-plugin-repaired-peer-scan-e2e");
449449
const npmRoot = path.join(rootDir, "managed-npm");
450450
const pluginWithRuntimePeer = `existing-peer-plugin-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
@@ -531,13 +531,27 @@ describe("installPluginFromNpmSpec e2e", () => {
531531
logger: { info: () => {}, warn: () => {} },
532532
timeoutMs: 120_000,
533533
});
534-
if (!later.ok) {
535-
throw new Error(later.error);
534+
expect(later.ok).toBe(false);
535+
if (later.ok) {
536+
throw new Error("expected repaired peer dependency scan to block installation");
536537
}
538+
expect(later.error).toContain("Dynamic code execution detected");
537539

540+
await expect(
541+
fs.lstat(path.join(npmRoot, "node_modules", laterPlugin, "package.json")),
542+
).rejects.toHaveProperty("code", "ENOENT");
538543
await expect(
539544
fs.lstat(path.join(npmRoot, "node_modules", runtimePeer, "package.json")),
540-
).resolves.toBeTruthy();
545+
).rejects.toHaveProperty("code", "ENOENT");
546+
const rootManifest = JSON.parse(
547+
await fs.readFile(path.join(npmRoot, "package.json"), "utf8"),
548+
) as {
549+
dependencies?: Record<string, string>;
550+
openclaw?: { managedPeerDependencies?: string[] };
551+
};
552+
expect(rootManifest.dependencies?.[laterPlugin]).toBeUndefined();
553+
expect(rootManifest.dependencies?.[runtimePeer]).toBeUndefined();
554+
expect(rootManifest.openclaw?.managedPeerDependencies ?? []).not.toContain(runtimePeer);
541555
});
542556

543557
it("bounds peer dependency discovery across repeated nested package realpaths", async () => {
@@ -680,6 +694,119 @@ describe("installPluginFromNpmSpec e2e", () => {
680694
).rejects.toHaveProperty("code", "ENOENT");
681695
});
682696

697+
it("does not take ownership of an existing root dependency observed as a peer", async () => {
698+
const rootDir = await makeTempDir("npm-plugin-peer-existing-root-e2e");
699+
const npmRoot = path.join(rootDir, "managed-npm");
700+
const existingRootDependency = `existing-root-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
701+
const blockedPlugin = `blocked-plugin-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
702+
const runtimePeer = `runtime-peer-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`;
703+
const registry = await startStaticRegistry([
704+
{
705+
packageName: existingRootDependency,
706+
latest: "1.0.0",
707+
versions: [
708+
await packPlugin({
709+
packageName: existingRootDependency,
710+
pluginId: existingRootDependency,
711+
version: "1.0.0",
712+
rootDir,
713+
}),
714+
],
715+
},
716+
{
717+
packageName: blockedPlugin,
718+
latest: "1.0.0",
719+
versions: [
720+
await packPlugin({
721+
packageName: blockedPlugin,
722+
peerDependencies: {
723+
[existingRootDependency]: "^1.0.0",
724+
[runtimePeer]: "^1.0.0",
725+
},
726+
peerDependenciesMeta: {},
727+
pluginId: blockedPlugin,
728+
version: "1.0.0",
729+
rootDir,
730+
}),
731+
],
732+
},
733+
{
734+
packageName: runtimePeer,
735+
latest: "1.0.0",
736+
versions: [
737+
await packPlugin({
738+
indexJs: "eval('1');\n",
739+
packageName: runtimePeer,
740+
pluginId: runtimePeer,
741+
version: "1.0.0",
742+
rootDir,
743+
}),
744+
],
745+
},
746+
]);
747+
process.env.NPM_CONFIG_REGISTRY = registry;
748+
process.env.npm_config_registry = registry;
749+
750+
await fs.mkdir(npmRoot, { recursive: true });
751+
await fs.writeFile(
752+
path.join(npmRoot, "package.json"),
753+
`${JSON.stringify(
754+
{
755+
private: true,
756+
dependencies: { [existingRootDependency]: "1.0.0" },
757+
},
758+
null,
759+
2,
760+
)}\n`,
761+
"utf8",
762+
);
763+
await execFileAsync(
764+
"npm",
765+
[
766+
"install",
767+
"--omit=dev",
768+
"--omit=peer",
769+
"--legacy-peer-deps",
770+
"--loglevel=error",
771+
"--ignore-scripts",
772+
"--no-audit",
773+
"--no-fund",
774+
],
775+
{ cwd: npmRoot },
776+
);
777+
778+
const result = await installPluginFromNpmSpec({
779+
spec: `${blockedPlugin}@1.0.0`,
780+
npmDir: npmRoot,
781+
logger: { info: () => {}, warn: () => {} },
782+
timeoutMs: 120_000,
783+
});
784+
785+
expect(result.ok).toBe(false);
786+
const rootManifest = JSON.parse(
787+
await fs.readFile(path.join(npmRoot, "package.json"), "utf8"),
788+
) as {
789+
dependencies?: Record<string, string>;
790+
openclaw?: { managedPeerDependencies?: string[] };
791+
};
792+
expect(rootManifest.dependencies?.[existingRootDependency]).toBe("1.0.0");
793+
expect(rootManifest.dependencies?.[blockedPlugin]).toBeUndefined();
794+
expect(rootManifest.dependencies?.[runtimePeer]).toBeUndefined();
795+
expect(rootManifest.openclaw?.managedPeerDependencies ?? []).not.toContain(
796+
existingRootDependency,
797+
);
798+
expect(rootManifest.openclaw?.managedPeerDependencies ?? []).not.toContain(runtimePeer);
799+
await expect(
800+
fs.lstat(path.join(npmRoot, "node_modules", existingRootDependency, "package.json")),
801+
).resolves.toBeTruthy();
802+
await expect(
803+
fs.lstat(path.join(npmRoot, "node_modules", blockedPlugin, "package.json")),
804+
).rejects.toHaveProperty("code", "ENOENT");
805+
await expect(
806+
fs.lstat(path.join(npmRoot, "node_modules", runtimePeer, "package.json")),
807+
).rejects.toHaveProperty("code", "ENOENT");
808+
});
809+
683810
it("scrubs host peers when installing beside an existing host-peer plugin", async () => {
684811
const rootDir = await makeTempDir("npm-plugin-sibling-peer-e2e");
685812
const npmRoot = path.join(rootDir, "managed-npm");

‎src/plugins/install.ts‎

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import { resolveNpmIntegrityDriftWithDefaultMessage } from "../infra/npm-integri
1313
import {
1414
type ManagedNpmRootPeerDependencySnapshot,
1515
readManagedNpmRootInstalledDependency,
16-
readManagedNpmRootPeerDependencyNames,
1716
readManagedNpmRootPeerDependencySnapshot,
1817
readOpenClawManagedNpmRootOverrides,
1918
repairManagedNpmRootOpenClawPeer,
@@ -380,11 +379,21 @@ async function rollbackManagedNpmPluginInstall(params: {
380379
}
381380
if (params.peerDependencySnapshot) {
382381
try {
382+
const preRestorePeerDependencySnapshot = await readManagedNpmRootPeerDependencySnapshot({
383+
npmRoot: params.npmRoot,
384+
});
385+
const restoredPeerDependencyNames = new Set(
386+
params.peerDependencySnapshot.managedPeerDependencies,
387+
);
388+
const addedPeerDependencyNames =
389+
preRestorePeerDependencySnapshot.managedPeerDependencies.filter(
390+
(packageName) => !restoredPeerDependencyNames.has(packageName),
391+
);
383392
await restoreManagedNpmRootPeerDependencySnapshot({
384393
npmRoot: params.npmRoot,
385394
snapshot: params.peerDependencySnapshot,
386395
});
387-
await runCommandWithTimeout(
396+
const cleanupResult = await runCommandWithTimeout(
388397
[
389398
"npm",
390399
"install",
@@ -406,6 +415,25 @@ async function rollbackManagedNpmPluginInstall(params: {
406415
}),
407416
},
408417
);
418+
if (cleanupResult.code !== 0) {
419+
params.logger.warn?.(
420+
`npm install cleanup after rollback for ${params.packageName} exited ${cleanupResult.code}: ${cleanupResult.stderr.trim() || cleanupResult.stdout.trim()}`,
421+
);
422+
await Promise.all(
423+
addedPeerDependencyNames.map(async (packageName) => {
424+
try {
425+
await fs.rm(resolveManagedNpmRootPackageDir(params.npmRoot, packageName), {
426+
recursive: true,
427+
force: true,
428+
});
429+
} catch (error) {
430+
params.logger.warn?.(
431+
`Failed to remove rolled-back managed peer dependency ${packageName}: ${String(error)}`,
432+
);
433+
}
434+
}),
435+
);
436+
}
409437
} catch (error) {
410438
params.logger.warn?.(
411439
`Failed to restore managed npm peer dependencies after rollback for ${params.packageName}: ${String(error)}`,
@@ -504,16 +532,11 @@ function resolveManagedNpmRootPackageDir(npmRoot: string, packageName: string):
504532

505533
async function listNewManagedNpmRootPackageDirs(params: {
506534
beforeInstallPackageNames: Set<string>;
507-
excludePackageNames?: Set<string>;
508535
npmRoot: string;
509536
}): Promise<string[]> {
510537
const afterInstallPackageNames = await listManagedNpmRootPackageNames(params.npmRoot);
511538
return [...afterInstallPackageNames]
512-
.filter(
513-
(packageName) =>
514-
!params.beforeInstallPackageNames.has(packageName) &&
515-
!params.excludePackageNames?.has(packageName),
516-
)
539+
.filter((packageName) => !params.beforeInstallPackageNames.has(packageName))
517540
.map((packageName) => resolveManagedNpmRootPackageDir(params.npmRoot, packageName))
518541
.toSorted((left, right) => left.localeCompare(right));
519542
}
@@ -619,9 +642,6 @@ async function installPluginFromManagedNpmRoot(
619642
managedOverrides,
620643
});
621644
await syncManagedNpmRootPeerDependencies({ npmRoot, managedOverrides });
622-
const preExistingManagedPeerDependencyNames = await readManagedNpmRootPeerDependencyNames({
623-
npmRoot,
624-
});
625645
const npmInstallArgs = [
626646
"npm",
627647
...createSafeNpmInstallArgs({
@@ -808,7 +828,6 @@ async function installPluginFromManagedNpmRoot(
808828

809829
const newRootPackageDirs = await listNewManagedNpmRootPackageDirs({
810830
beforeInstallPackageNames: preInstallRootPackageNames,
811-
excludePackageNames: preExistingManagedPeerDependencyNames,
812831
npmRoot,
813832
});
814833
const result = await installPluginFromInstalledPackageDir({

0 commit comments

Comments
 (0)