fix: address doctor review followups

This commit is contained in:
Gustavo Madeira Santana
2026-04-21 22:14:43 -04:00
parent 0433cec4ce
commit 5b1ff1bfe5
10 changed files with 177 additions and 26 deletions

View File

@@ -34,12 +34,12 @@ describe("doctor note emission", () => {
emitDoctorNotes({
note,
changeNotes: ["change \u001B[31mred\u001B[0m"],
changeNotes: ["change \u001B[31mred\u001B[0m\nnext line"],
warningNotes: ["warning \u001B]8;;https://example.test\u001B\\link\u001B]8;;\u001B\\\r"],
});
expect(note.mock.calls).toEqual([
["change red", "Doctor changes"],
["change red\nnext line", "Doctor changes"],
["warning link", "Doctor warnings"],
]);
});

View File

@@ -1,14 +1,21 @@
import { sanitizeForLog } from "../../terminal/ansi.js";
function sanitizeDoctorNote(note: string): string {
return note
.split("\n")
.map((line) => sanitizeForLog(line))
.join("\n");
}
export function emitDoctorNotes(params: {
note: (message: string, title?: string) => void;
changeNotes?: string[];
warningNotes?: string[];
}): void {
for (const change of params.changeNotes ?? []) {
params.note(sanitizeForLog(change), "Doctor changes");
params.note(sanitizeDoctorNote(change), "Doctor changes");
}
for (const warning of params.warningNotes ?? []) {
params.note(sanitizeForLog(warning), "Doctor warnings");
params.note(sanitizeDoctorNote(warning), "Doctor warnings");
}
}

View File

@@ -35,7 +35,10 @@ vi.mock("./shared/channel-doctor.js", () => ({
}
return [];
},
collectChannelDoctorEmptyAllowlistExtraWarnings: () => [],
createChannelDoctorEmptyAllowlistPolicyHooks: () => ({
extraWarningsForAccount: () => [],
shouldSkipDefaultEmptyGroupAllowlistWarning: () => false,
}),
}));
vi.mock("./shared/empty-allowlist-scan.js", () => ({

View File

@@ -2,7 +2,7 @@ import { sanitizeForLog } from "../../terminal/ansi.js";
import { maybeRepairAllowlistPolicyAllowFrom } from "./shared/allowlist-policy-repair.js";
import { maybeRepairBundledPluginLoadPaths } from "./shared/bundled-plugin-load-paths.js";
import {
collectChannelDoctorEmptyAllowlistExtraWarnings,
createChannelDoctorEmptyAllowlistPolicyHooks,
collectChannelDoctorRepairMutations,
} from "./shared/channel-doctor.js";
import {
@@ -63,7 +63,7 @@ export async function runDoctorRepairSequence(params: {
const emptyAllowlistWarnings = scanEmptyAllowlistPolicyWarnings(state.candidate, {
doctorFixCommand: params.doctorFixCommand,
env,
extraWarningsForAccount: collectChannelDoctorEmptyAllowlistExtraWarnings,
...createChannelDoctorEmptyAllowlistPolicyHooks({ cfg: state.candidate, env }),
});
if (emptyAllowlistWarnings.length > 0) {
warningNotes.push(sanitizeLines(emptyAllowlistWarnings));

View File

@@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
import {
collectChannelDoctorCompatibilityMutations,
collectChannelDoctorEmptyAllowlistExtraWarnings,
createChannelDoctorEmptyAllowlistPolicyHooks,
} from "./channel-doctor.js";
const mocks = vi.hoisted(() => ({
@@ -247,4 +248,71 @@ describe("channel doctor compatibility mutations", () => {
expect.objectContaining({ cfg }),
);
});
it("reuses empty allowlist doctor entries across per-account hooks", () => {
const collectEmptyAllowlistExtraWarnings = vi.fn(({ prefix }: { prefix: string }) => [
`${prefix} extra`,
]);
const shouldSkipDefaultEmptyGroupAllowlistWarning = vi.fn(() => true);
const cfg = {
channels: {
matrix: {
accounts: {
work: {},
personal: {},
},
},
},
};
const env = { OPENCLAW_HOME: "/tmp/openclaw-test-home" };
mocks.resolveReadOnlyChannelPluginsForConfig.mockReturnValue({
plugins: [
{
id: "matrix",
doctor: {
collectEmptyAllowlistExtraWarnings,
shouldSkipDefaultEmptyGroupAllowlistWarning,
},
},
],
});
const hooks = createChannelDoctorEmptyAllowlistPolicyHooks({ cfg: cfg as never, env });
expect(
hooks.extraWarningsForAccount({
account: {},
channelName: "matrix",
cfg: cfg as never,
env,
prefix: "channels.matrix.accounts.work",
}),
).toEqual(["channels.matrix.accounts.work extra"]);
expect(
hooks.shouldSkipDefaultEmptyGroupAllowlistWarning({
account: {},
channelName: "matrix",
cfg: cfg as never,
env,
prefix: "channels.matrix.accounts.work",
}),
).toBe(true);
expect(
hooks.extraWarningsForAccount({
account: {},
channelName: "matrix",
cfg: cfg as never,
env,
prefix: "channels.matrix.accounts.personal",
}),
).toEqual(["channels.matrix.accounts.personal extra"]);
expect(mocks.resolveReadOnlyChannelPluginsForConfig).toHaveBeenCalledTimes(1);
expect(mocks.resolveReadOnlyChannelPluginsForConfig).toHaveBeenCalledWith(cfg, {
env,
includePersistedAuthState: false,
});
expect(collectEmptyAllowlistExtraWarnings).toHaveBeenCalledTimes(2);
expect(shouldSkipDefaultEmptyGroupAllowlistWarning).toHaveBeenCalledTimes(1);
});
});

View File

@@ -21,6 +21,13 @@ type ChannelDoctorLookupContext = {
env?: NodeJS.ProcessEnv;
};
export type ChannelDoctorEmptyAllowlistPolicyHooks = {
extraWarningsForAccount: (params: ChannelDoctorEmptyAllowlistAccountContext) => string[];
shouldSkipDefaultEmptyGroupAllowlistWarning: (
params: ChannelDoctorEmptyAllowlistAccountContext,
) => boolean;
};
function collectConfiguredChannelIds(cfg: OpenClawConfig): string[] {
const channels =
cfg.channels && typeof cfg.channels === "object" && !Array.isArray(cfg.channels)
@@ -111,6 +118,53 @@ function listChannelDoctorEntries(
return [...byId.values()];
}
function collectEmptyAllowlistExtraWarningsForEntries(
entries: readonly ChannelDoctorEntry[],
params: ChannelDoctorEmptyAllowlistAccountContext,
): string[] {
const warnings: string[] = [];
for (const entry of entries) {
const lines = entry.doctor.collectEmptyAllowlistExtraWarnings?.(params);
if (lines?.length) {
warnings.push(...lines);
}
}
return warnings;
}
function shouldSkipDefaultEmptyGroupAllowlistWarningForEntries(
entries: readonly ChannelDoctorEntry[],
params: ChannelDoctorEmptyAllowlistAccountContext,
): boolean {
return entries.some(
(entry) => entry.doctor.shouldSkipDefaultEmptyGroupAllowlistWarning?.(params) === true,
);
}
export function createChannelDoctorEmptyAllowlistPolicyHooks(
context: ChannelDoctorLookupContext,
): ChannelDoctorEmptyAllowlistPolicyHooks {
const entriesByChannel = new Map<string, ChannelDoctorEntry[]>();
const entriesForChannel = (channelName: string) => {
const existing = entriesByChannel.get(channelName);
if (existing) {
return existing;
}
const entries = listChannelDoctorEntries([channelName], context);
entriesByChannel.set(channelName, entries);
return entries;
};
return {
extraWarningsForAccount: (params) =>
collectEmptyAllowlistExtraWarningsForEntries(entriesForChannel(params.channelName), params),
shouldSkipDefaultEmptyGroupAllowlistWarning: (params) =>
shouldSkipDefaultEmptyGroupAllowlistWarningForEntries(
entriesForChannel(params.channelName),
params,
),
};
}
export async function runChannelDoctorConfigSequences(params: {
cfg: OpenClawConfig;
env: NodeJS.ProcessEnv;
@@ -238,24 +292,23 @@ export async function collectChannelDoctorRepairMutations(params: {
export function collectChannelDoctorEmptyAllowlistExtraWarnings(
params: ChannelDoctorEmptyAllowlistAccountContext,
): string[] {
const warnings: string[] = [];
for (const entry of listChannelDoctorEntries([params.channelName], {
cfg: params.cfg ?? {},
env: params.env,
})) {
const lines = entry.doctor.collectEmptyAllowlistExtraWarnings?.(params);
if (lines?.length) {
warnings.push(...lines);
}
}
return warnings;
return collectEmptyAllowlistExtraWarningsForEntries(
listChannelDoctorEntries([params.channelName], {
cfg: params.cfg ?? {},
env: params.env,
}),
params,
);
}
export function shouldSkipChannelDoctorDefaultEmptyGroupAllowlistWarning(
params: ChannelDoctorEmptyAllowlistAccountContext,
): boolean {
return listChannelDoctorEntries([params.channelName], {
cfg: params.cfg ?? {},
env: params.env,
}).some((entry) => entry.doctor.shouldSkipDefaultEmptyGroupAllowlistWarning?.(params) === true);
return shouldSkipDefaultEmptyGroupAllowlistWarningForEntries(
listChannelDoctorEntries([params.channelName], {
cfg: params.cfg ?? {},
env: params.env,
}),
params,
);
}

View File

@@ -12,6 +12,7 @@ type CollectEmptyAllowlistPolicyWarningsParams = {
env?: NodeJS.ProcessEnv;
parent?: DoctorAccountRecord;
prefix: string;
shouldSkipDefaultEmptyGroupAllowlistWarning?: typeof shouldSkipChannelDoctorDefaultEmptyGroupAllowlistWarning;
};
function usesSenderBasedGroupAllowlist(channelName?: string): boolean {
@@ -67,7 +68,10 @@ export function collectEmptyAllowlistPolicyWarningsForAccount(
if (
params.channelName &&
shouldSkipChannelDoctorDefaultEmptyGroupAllowlistWarning({
(
params.shouldSkipDefaultEmptyGroupAllowlistWarning ??
shouldSkipChannelDoctorDefaultEmptyGroupAllowlistWarning
)({
account: params.account,
channelName: params.channelName,
cfg: params.cfg,

View File

@@ -1,3 +1,4 @@
import type { ChannelDoctorEmptyAllowlistAccountContext } from "../../../channels/plugins/types.adapters.js";
import type { OpenClawConfig } from "../../../config/types.openclaw.js";
import type { DoctorAccountRecord, DoctorAllowFromList } from "../types.js";
import { collectEmptyAllowlistPolicyWarningsForAccount } from "./empty-allowlist-policy.js";
@@ -17,7 +18,10 @@ export type EmptyAllowlistAccountScanParams = {
type ScanEmptyAllowlistPolicyWarningsParams = {
doctorFixCommand: string;
env?: NodeJS.ProcessEnv;
extraWarningsForAccount?: (params: EmptyAllowlistAccountScanParams) => string[];
extraWarningsForAccount?: (params: ChannelDoctorEmptyAllowlistAccountContext) => string[];
shouldSkipDefaultEmptyGroupAllowlistWarning?: (
params: ChannelDoctorEmptyAllowlistAccountContext,
) => boolean;
};
export function scanEmptyAllowlistPolicyWarnings(
@@ -61,6 +65,8 @@ export function scanEmptyAllowlistPolicyWarnings(
env: params.env,
parent,
prefix,
shouldSkipDefaultEmptyGroupAllowlistWarning:
params.shouldSkipDefaultEmptyGroupAllowlistWarning,
}),
);
if (params.extraWarningsForAccount) {

View File

@@ -50,6 +50,10 @@ vi.mock("./channel-doctor.js", () => ({
];
},
),
createChannelDoctorEmptyAllowlistPolicyHooks: vi.fn(() => ({
extraWarningsForAccount: () => [],
shouldSkipDefaultEmptyGroupAllowlistWarning: () => false,
})),
shouldSkipChannelDoctorDefaultEmptyGroupAllowlistWarning: vi.fn(() => false),
}));

View File

@@ -156,12 +156,18 @@ export async function collectDoctorPreviewWarnings(params: {
}
if (hasChannelConfig) {
const { collectChannelDoctorEmptyAllowlistExtraWarnings } = await loadChannelDoctorModule();
const { createChannelDoctorEmptyAllowlistPolicyHooks } = await loadChannelDoctorModule();
const { scanEmptyAllowlistPolicyWarnings } = await import("./empty-allowlist-scan.js");
const emptyAllowlistHooks = createChannelDoctorEmptyAllowlistPolicyHooks({
cfg: params.cfg,
env,
});
const emptyAllowlistWarnings = scanEmptyAllowlistPolicyWarnings(params.cfg, {
doctorFixCommand: params.doctorFixCommand,
env,
extraWarningsForAccount: collectChannelDoctorEmptyAllowlistExtraWarnings,
extraWarningsForAccount: emptyAllowlistHooks.extraWarningsForAccount,
shouldSkipDefaultEmptyGroupAllowlistWarning:
emptyAllowlistHooks.shouldSkipDefaultEmptyGroupAllowlistWarning,
}).filter(
(warning) =>
!(