Skip to content

Commit cbb11b3

Browse files
lupuleticsteipete
lupuletic
authored and
steipete
committed
fix(plugins): address review feedback for Matrix recovery paths (#52899)
1 parent 489797c commit cbb11b3

3 files changed

Lines changed: 176 additions & 18 deletions

File tree

‎src/cli/plugins-cli-test-helpers.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type { OpenClawConfig } from "../config/config.js";
44
import { createCliRuntimeCapture } from "./test-runtime-capture.js";
55

66
export const loadConfig = vi.fn<() => OpenClawConfig>(() => ({}) as OpenClawConfig);
7+
export const readConfigFileSnapshot = vi.fn();
78
export const writeConfigFile = vi.fn<(config: OpenClawConfig) => Promise<void>>(
89
async () => undefined,
910
);
@@ -39,6 +40,7 @@ vi.mock("../runtime.js", () => ({
3940

4041
vi.mock("../config/config.js", () => ({
4142
loadConfig: () => loadConfig(),
43+
readConfigFileSnapshot: (...args: unknown[]) => readConfigFileSnapshot(...args),
4244
writeConfigFile: (config: OpenClawConfig) => writeConfigFile(config),
4345
}));
4446

@@ -138,6 +140,7 @@ export function runPluginsCommand(argv: string[]) {
138140
export function resetPluginsCliTestState() {
139141
resetRuntimeCapture();
140142
loadConfig.mockReset();
143+
readConfigFileSnapshot.mockReset();
141144
writeConfigFile.mockReset();
142145
resolveStateDir.mockReset();
143146
installPluginFromMarketplace.mockReset();
@@ -161,6 +164,19 @@ export function resetPluginsCliTestState() {
161164
recordHookInstall.mockReset();
162165

163166
loadConfig.mockReturnValue({} as OpenClawConfig);
167+
readConfigFileSnapshot.mockResolvedValue({
168+
path: "/tmp/openclaw-config.json5",
169+
exists: true,
170+
raw: "{}",
171+
parsed: {},
172+
resolved: {},
173+
valid: true,
174+
config: {} as OpenClawConfig,
175+
hash: "mock",
176+
issues: [],
177+
warnings: [],
178+
legacyIssues: [],
179+
});
164180
writeConfigFile.mockResolvedValue(undefined);
165181
resolveStateDir.mockReturnValue("/tmp/openclaw-state");
166182
resolveMarketplaceInstallShortcut.mockResolvedValue(null);

‎src/cli/plugins-install-command.ts‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import fs from "node:fs";
22
import { cleanStaleMatrixPluginConfig } from "../commands/doctor/providers/matrix.js";
33
import type { OpenClawConfig } from "../config/config.js";
4-
import { loadConfig, readBestEffortConfig } from "../config/config.js";
4+
import { loadConfig, readConfigFileSnapshot } from "../config/config.js";
55
import { installHooksFromNpmSpec, installHooksFromPath } from "../hooks/install.js";
66
import { resolveArchiveKind } from "../infra/archive.js";
77
import { parseClawHubPluginSpec } from "../infra/clawhub.js";
8+
import { extractErrorCode } from "../infra/errors.js";
89
import { type BundledPluginSource, findBundledPluginSource } from "../plugins/bundled-sources.js";
910
import { formatClawHubSpecifier, installPluginFromClawHub } from "../plugins/clawhub.js";
1011
import { installPluginFromNpmSpec, installPluginFromPath } from "../plugins/install.js";
@@ -168,33 +169,39 @@ async function tryInstallHookPackFromNpmSpec(params: {
168169
return { ok: true };
169170
}
170171

171-
// loadConfig() throws when config is invalid; fall back to best-effort so
172-
// repair-oriented installs (e.g. reinstalling a broken Matrix plugin) can
173-
// still proceed from a partially valid config snapshot.
172+
// loadConfig() throws when config is invalid; fall back to the raw config
173+
// snapshot so repair-oriented installs (e.g. reinstalling a broken Matrix
174+
// plugin) can still proceed.
174175
// Only catch config-validation errors — real failures (fs permission, OOM)
175176
// must surface so the user sees the actual problem.
176-
// After loading, clean any stale Matrix plugin references so that
177-
// persistPluginInstall() → writeConfigFile() does not fail validation
178-
// on paths that no longer exist (#52899 concern 4).
179-
async function loadConfigForInstall(): Promise<OpenClawConfig> {
180-
let cfg: OpenClawConfig;
177+
// Narrow guard: only proceed from the snapshot when the file was parsed
178+
// successfully (snapshot.parsed has content). For parse/read failures the
179+
// snapshot config is {} which would cause writeConfigFile() to overwrite
180+
// the user's real config with a minimal stub (#52899 concern 4).
181+
export async function loadConfigForInstall(): Promise<OpenClawConfig> {
181182
try {
182-
cfg = loadConfig();
183+
const cfg = loadConfig();
184+
const cleaned = await cleanStaleMatrixPluginConfig(cfg);
185+
return cleaned.config;
183186
} catch (err) {
184-
if (isConfigValidationError(err)) {
185-
cfg = await readBestEffortConfig();
186-
} else {
187+
if (extractErrorCode(err) !== "INVALID_CONFIG") {
187188
throw err;
188189
}
189190
}
190-
const cleaned = await cleanStaleMatrixPluginConfig(cfg);
191+
// Config validation failed — recover from the raw snapshot.
192+
const snapshot = await readConfigFileSnapshot();
193+
const parsed = (snapshot.parsed ?? {}) as Record<string, unknown>;
194+
if (!snapshot.exists || Object.keys(parsed).length === 0) {
195+
const configErr = new Error(
196+
"Config file could not be parsed; run `openclaw doctor` to repair it.",
197+
);
198+
(configErr as { code?: string }).code = "INVALID_CONFIG";
199+
throw configErr;
200+
}
201+
const cleaned = await cleanStaleMatrixPluginConfig(snapshot.config);
191202
return cleaned.config;
192203
}
193204

194-
function isConfigValidationError(err: unknown): boolean {
195-
return err instanceof Error && (err as { code?: string }).code === "INVALID_CONFIG";
196-
}
197-
198205
export async function runPluginInstallCommand(params: {
199206
raw: string;
200207
opts: { link?: boolean; pin?: boolean; marketplace?: string };

‎src/cli/plugins-install-config.test.ts‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
import type { OpenClawConfig } from "../config/config.js";
3+
import type { ConfigFileSnapshot } from "../config/types.openclaw.js";
4+
5+
const loadConfigMock = vi.fn<() => OpenClawConfig>();
6+
const readConfigFileSnapshotMock = vi.fn<() => Promise<ConfigFileSnapshot>>();
7+
const cleanStaleMatrixPluginConfigMock = vi.fn();
8+
9+
vi.mock("../config/config.js", () => ({
10+
loadConfig: () => loadConfigMock(),
11+
readConfigFileSnapshot: () => readConfigFileSnapshotMock(),
12+
}));
13+
14+
vi.mock("../commands/doctor/providers/matrix.js", () => ({
15+
cleanStaleMatrixPluginConfig: (cfg: OpenClawConfig) => cleanStaleMatrixPluginConfigMock(cfg),
16+
}));
17+
18+
const { loadConfigForInstall } = await import("./plugins-install-command.js");
19+
20+
function makeSnapshot(overrides: Partial<ConfigFileSnapshot> = {}): ConfigFileSnapshot {
21+
return {
22+
path: "/tmp/config.json5",
23+
exists: true,
24+
raw: '{ "plugins": {} }',
25+
parsed: { plugins: {} },
26+
resolved: { plugins: {} } as OpenClawConfig,
27+
valid: false,
28+
config: { plugins: {} } as OpenClawConfig,
29+
hash: "abc",
30+
issues: [{ path: "plugins.installs.matrix", message: "stale path" }],
31+
warnings: [],
32+
legacyIssues: [],
33+
...overrides,
34+
};
35+
}
36+
37+
describe("loadConfigForInstall", () => {
38+
beforeEach(() => {
39+
loadConfigMock.mockReset();
40+
readConfigFileSnapshotMock.mockReset();
41+
cleanStaleMatrixPluginConfigMock.mockReset();
42+
43+
cleanStaleMatrixPluginConfigMock.mockImplementation((cfg: OpenClawConfig) => ({
44+
config: cfg,
45+
changes: [],
46+
}));
47+
});
48+
49+
it("returns the config directly when loadConfig succeeds", async () => {
50+
const cfg = { plugins: { entries: { matrix: { enabled: true } } } } as OpenClawConfig;
51+
loadConfigMock.mockReturnValue(cfg);
52+
53+
const result = await loadConfigForInstall();
54+
expect(result).toBe(cfg);
55+
expect(readConfigFileSnapshotMock).not.toHaveBeenCalled();
56+
});
57+
58+
it("runs stale Matrix cleanup on the happy path", async () => {
59+
const cfg = { plugins: {} } as OpenClawConfig;
60+
const cleanedCfg = { plugins: { cleaned: true } } as unknown as OpenClawConfig;
61+
loadConfigMock.mockReturnValue(cfg);
62+
cleanStaleMatrixPluginConfigMock.mockReturnValue({ config: cleanedCfg, changes: ["cleaned"] });
63+
64+
const result = await loadConfigForInstall();
65+
expect(cleanStaleMatrixPluginConfigMock).toHaveBeenCalledWith(cfg);
66+
expect(result).toBe(cleanedCfg);
67+
});
68+
69+
it("falls back to snapshot config when loadConfig throws INVALID_CONFIG and snapshot was parsed", async () => {
70+
const invalidConfigErr = new Error("config invalid");
71+
(invalidConfigErr as { code?: string }).code = "INVALID_CONFIG";
72+
loadConfigMock.mockImplementation(() => {
73+
throw invalidConfigErr;
74+
});
75+
76+
const snapshotCfg = {
77+
plugins: { installs: { matrix: { source: "path", installPath: "/gone" } } },
78+
} as unknown as OpenClawConfig;
79+
readConfigFileSnapshotMock.mockResolvedValue(
80+
makeSnapshot({
81+
parsed: { plugins: { installs: { matrix: {} } } },
82+
config: snapshotCfg,
83+
}),
84+
);
85+
86+
const result = await loadConfigForInstall();
87+
expect(readConfigFileSnapshotMock).toHaveBeenCalled();
88+
expect(cleanStaleMatrixPluginConfigMock).toHaveBeenCalledWith(snapshotCfg);
89+
expect(result).toBe(snapshotCfg);
90+
});
91+
92+
it("throws when loadConfig fails with INVALID_CONFIG and snapshot parsed is empty (parse failure)", async () => {
93+
const invalidConfigErr = new Error("config invalid");
94+
(invalidConfigErr as { code?: string }).code = "INVALID_CONFIG";
95+
loadConfigMock.mockImplementation(() => {
96+
throw invalidConfigErr;
97+
});
98+
99+
readConfigFileSnapshotMock.mockResolvedValue(
100+
makeSnapshot({
101+
parsed: {},
102+
config: {} as OpenClawConfig,
103+
}),
104+
);
105+
106+
await expect(loadConfigForInstall()).rejects.toThrow(
107+
"Config file could not be parsed; run `openclaw doctor` to repair it.",
108+
);
109+
});
110+
111+
it("throws when loadConfig fails with INVALID_CONFIG and config file does not exist", async () => {
112+
const invalidConfigErr = new Error("config invalid");
113+
(invalidConfigErr as { code?: string }).code = "INVALID_CONFIG";
114+
loadConfigMock.mockImplementation(() => {
115+
throw invalidConfigErr;
116+
});
117+
118+
readConfigFileSnapshotMock.mockResolvedValue(makeSnapshot({ exists: false, parsed: {} }));
119+
120+
await expect(loadConfigForInstall()).rejects.toThrow(
121+
"Config file could not be parsed; run `openclaw doctor` to repair it.",
122+
);
123+
});
124+
125+
it("re-throws non-config errors from loadConfig", async () => {
126+
const fsErr = new Error("EACCES: permission denied");
127+
(fsErr as { code?: string }).code = "EACCES";
128+
loadConfigMock.mockImplementation(() => {
129+
throw fsErr;
130+
});
131+
132+
await expect(loadConfigForInstall()).rejects.toThrow("EACCES: permission denied");
133+
expect(readConfigFileSnapshotMock).not.toHaveBeenCalled();
134+
});
135+
});

0 commit comments

Comments
 (0)