mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-25 20:05:46 -06:00
fix(config): tighten dm policy warnings
This commit is contained in:
@@ -15,6 +15,17 @@ type ChannelMetadataRecord = ChannelSchemaMetadataWithOwnership & {
|
||||
originRank: number;
|
||||
};
|
||||
|
||||
type ChannelDmAllowFromMode = "topOnly" | "topOrNested" | "nestedOnly";
|
||||
|
||||
export type ChannelDmPolicyMetadata = {
|
||||
id: string;
|
||||
dmAllowFromMode?: ChannelDmAllowFromMode;
|
||||
};
|
||||
|
||||
type ChannelDmPolicyMetadataRecord = ChannelDmPolicyMetadata & {
|
||||
originRank: number;
|
||||
};
|
||||
|
||||
const PLUGIN_ORIGIN_RANK: Readonly<Record<PluginOrigin, number>> = {
|
||||
// Lower ranks are closer to the operator and should override farther bundled/global metadata.
|
||||
config: 0,
|
||||
@@ -121,3 +132,47 @@ export function collectChannelSchemaMetadata(
|
||||
entry,
|
||||
);
|
||||
}
|
||||
|
||||
/** Collects channel DM policy metadata without importing doctor/runtime command modules. */
|
||||
export function collectChannelDmPolicyMetadata(
|
||||
registry: PluginManifestRegistry,
|
||||
): ChannelDmPolicyMetadata[] {
|
||||
const byChannelId = new Map<string, ChannelDmPolicyMetadataRecord>();
|
||||
|
||||
const put = (
|
||||
channelId: string | undefined,
|
||||
originRank: number,
|
||||
dmAllowFromMode?: ChannelDmAllowFromMode,
|
||||
): void => {
|
||||
const id = channelId?.trim();
|
||||
if (!id) {
|
||||
return;
|
||||
}
|
||||
const current = byChannelId.get(id);
|
||||
if (current && current.originRank < originRank) {
|
||||
return;
|
||||
}
|
||||
byChannelId.set(id, {
|
||||
id,
|
||||
...(dmAllowFromMode ? { dmAllowFromMode } : {}),
|
||||
originRank,
|
||||
});
|
||||
};
|
||||
|
||||
for (const record of registry.plugins) {
|
||||
const originRank = PLUGIN_ORIGIN_RANK[record.origin] ?? Number.MAX_SAFE_INTEGER;
|
||||
const packageChannelId = record.packageChannel?.id?.trim();
|
||||
const dmAllowFromMode = record.packageChannel?.doctorCapabilities?.dmAllowFromMode;
|
||||
for (const channelId of record.channels) {
|
||||
put(channelId, originRank, channelId === packageChannelId ? dmAllowFromMode : undefined);
|
||||
}
|
||||
put(packageChannelId, originRank, dmAllowFromMode);
|
||||
for (const channelId of Object.keys(record.channelConfigs ?? {})) {
|
||||
put(channelId, originRank, channelId === packageChannelId ? dmAllowFromMode : undefined);
|
||||
}
|
||||
}
|
||||
|
||||
return [...byChannelId.values()]
|
||||
.toSorted((left, right) => left.id.localeCompare(right.id))
|
||||
.map(({ originRank: _originRank, ...entry }) => entry);
|
||||
}
|
||||
|
||||
@@ -176,6 +176,27 @@ function createCompatPluginConfigSchemaRegistry(): PluginManifestRegistry {
|
||||
};
|
||||
}
|
||||
|
||||
function createDmPolicyRegistry(params: {
|
||||
channelId: string;
|
||||
dmAllowFromMode?: "topOnly" | "topOrNested" | "nestedOnly";
|
||||
}): PluginManifestRegistry {
|
||||
return {
|
||||
diagnostics: [],
|
||||
plugins: [
|
||||
createPluginManifestRecord({
|
||||
id: params.channelId,
|
||||
channels: [params.channelId],
|
||||
packageChannel: {
|
||||
id: params.channelId,
|
||||
...(params.dmAllowFromMode
|
||||
? { doctorCapabilities: { dmAllowFromMode: params.dmAllowFromMode } }
|
||||
: {}),
|
||||
},
|
||||
}),
|
||||
],
|
||||
};
|
||||
}
|
||||
|
||||
function createPluginManifestRecord(
|
||||
overrides: Partial<PluginManifestRecord> & Pick<PluginManifestRecord, "id">,
|
||||
): PluginManifestRecord {
|
||||
@@ -416,6 +437,118 @@ describe("validateConfigObjectWithPlugins channel metadata (applyDefaults: true)
|
||||
});
|
||||
});
|
||||
|
||||
describe("validateConfigObjectWithPlugins DM policy warnings", () => {
|
||||
it("uses manifest metadata to skip nested-only DM config shapes", () => {
|
||||
const result = validateConfigObjectWithPlugins(
|
||||
{
|
||||
channels: {
|
||||
matrix: {
|
||||
dm: {
|
||||
policy: "open",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
pluginMetadataSnapshot: {
|
||||
manifestRegistry: createDmPolicyRegistry({
|
||||
channelId: "matrix",
|
||||
dmAllowFromMode: "nestedOnly",
|
||||
}),
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
expect(
|
||||
result.warnings.filter((warning) => warning.path.startsWith("channels.matrix")),
|
||||
).toEqual([]);
|
||||
}
|
||||
});
|
||||
|
||||
it("does not warn for disabled channels or accounts", () => {
|
||||
const result = validateConfigObjectWithPlugins(
|
||||
{
|
||||
channels: {
|
||||
mattermost: {
|
||||
enabled: false,
|
||||
dmPolicy: "open",
|
||||
accounts: {
|
||||
team: {
|
||||
dmPolicy: "open",
|
||||
},
|
||||
},
|
||||
},
|
||||
slack: {
|
||||
accounts: {
|
||||
work: {
|
||||
enabled: false,
|
||||
dmPolicy: "open",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
pluginMetadataSnapshot: {
|
||||
manifestRegistry: {
|
||||
diagnostics: [],
|
||||
plugins: [
|
||||
...createDmPolicyRegistry({ channelId: "mattermost" }).plugins,
|
||||
...createDmPolicyRegistry({ channelId: "slack" }).plugins,
|
||||
],
|
||||
},
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
expect(
|
||||
result.warnings.filter((warning) => warning.path.startsWith("channels.mattermost")),
|
||||
).toEqual([]);
|
||||
expect(
|
||||
result.warnings.filter((warning) => warning.path.startsWith("channels.slack")),
|
||||
).toEqual([]);
|
||||
}
|
||||
});
|
||||
|
||||
it("does not suggest channel allowFrom as sufficient when account allowFrom overrides it", () => {
|
||||
const result = validateConfigObjectWithPlugins(
|
||||
{
|
||||
channels: {
|
||||
mattermost: {
|
||||
allowFrom: ["*"],
|
||||
accounts: {
|
||||
team: {
|
||||
dmPolicy: "open",
|
||||
allowFrom: [],
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
pluginMetadataSnapshot: {
|
||||
manifestRegistry: createDmPolicyRegistry({ channelId: "mattermost" }),
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
const warning = result.warnings.find(
|
||||
(entry) => entry.path === "channels.mattermost.accounts.team.allowFrom",
|
||||
);
|
||||
expect(warning?.message).toContain(
|
||||
"remove channels.mattermost.accounts.team.allowFrom to inherit channels.mattermost.allowFrom",
|
||||
);
|
||||
expect(warning?.message).not.toContain("(or channels.mattermost.allowFrom)");
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("validateConfigObjectRawWithPlugins channel metadata", () => {
|
||||
it("still injects channel AJV defaults even in raw mode — persistence safety is handled by io.ts", () => {
|
||||
// Channel and plugin AJV validation always runs with applyDefaults: true
|
||||
|
||||
+101
-16
@@ -6,10 +6,10 @@ import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/st
|
||||
import { sanitizeForLog } from "../../packages/terminal-core/src/ansi.js";
|
||||
import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../agents/agent-scope.js";
|
||||
import {
|
||||
type ChannelDmAllowFromMode,
|
||||
resolveChannelDmAllowFrom,
|
||||
resolveChannelDmPolicy,
|
||||
} from "../channels/plugins/dm-access.js";
|
||||
import { getDoctorChannelCapabilities } from "../commands/doctor/channel-capabilities.js";
|
||||
import { isPathInside } from "../infra/path-guards.js";
|
||||
import { planManifestModelCatalogSuppressions } from "../model-catalog/index.js";
|
||||
import {
|
||||
@@ -48,7 +48,10 @@ import { isRecord, resolveUserPath } from "../utils.js";
|
||||
import { findDuplicateAgentDirs, formatDuplicateAgentDirError } from "./agent-dirs.js";
|
||||
import { appendAllowedValuesHint, summarizeAllowedValues } from "./allowed-values.js";
|
||||
import { GENERATED_BUNDLED_CHANNEL_CONFIG_METADATA } from "./bundled-channel-config-metadata.generated.js";
|
||||
import { collectChannelSchemaMetadataWithOwnership } from "./channel-config-metadata.js";
|
||||
import {
|
||||
collectChannelDmPolicyMetadata,
|
||||
collectChannelSchemaMetadataWithOwnership,
|
||||
} from "./channel-config-metadata.js";
|
||||
import { shouldSuppressMissingCodexPluginDiagnostics } from "./codex-plugin-diagnostics.js";
|
||||
import { materializeRuntimeConfig } from "./materialize.js";
|
||||
import type { OpenClawConfig, ConfigValidationIssue } from "./types.js";
|
||||
@@ -324,25 +327,70 @@ function formatRawChannelConfigIssueMessage(message: string): string {
|
||||
function buildDmPolicyDependencyWarning(params: {
|
||||
channelId: string;
|
||||
accountId?: string;
|
||||
allowFromSource?: "explicit" | "inherited";
|
||||
violation: DmPolicyAllowFromViolation;
|
||||
}): ConfigValidationIssue {
|
||||
const channelBase = `channels.${params.channelId}`;
|
||||
const scope = params.accountId ? `${channelBase}.accounts.${params.accountId}` : channelBase;
|
||||
const allowFromPath = `${scope}.allowFrom`;
|
||||
// Account allowFrom inherits the channel default when unset, so name both targets.
|
||||
const allowFromHint = params.accountId
|
||||
? `${allowFromPath} (or ${channelBase}.allowFrom)`
|
||||
const inherited = params.accountId && params.allowFromSource === "inherited";
|
||||
const allowFromSubject = inherited
|
||||
? `${allowFromPath} is unset and ${channelBase}.allowFrom`
|
||||
: allowFromPath;
|
||||
const accountInheritedTarget = inherited ? ` or ${channelBase}.allowFrom` : "";
|
||||
const accountOverrideFix =
|
||||
params.accountId && !inherited
|
||||
? `, remove ${allowFromPath} to inherit ${channelBase}.allowFrom,`
|
||||
: "";
|
||||
const message =
|
||||
params.violation === "open_requires_wildcard"
|
||||
? `${scope}.dmPolicy="open" but ${allowFromHint} does not include "*"; all DMs will be dropped. Add "*" to ${allowFromHint} or set ${scope}.dmPolicy to "pairing"/"allowlist".`
|
||||
: `${scope}.dmPolicy="allowlist" but ${allowFromHint} is empty; all DMs will be dropped. Add at least one sender ID to ${allowFromHint} or change ${scope}.dmPolicy.`;
|
||||
? `${scope}.dmPolicy="open" but ${allowFromSubject} does not include "*"; all DMs will be dropped. Add "*" to ${allowFromPath}${accountInheritedTarget}${accountOverrideFix} or set ${scope}.dmPolicy to "pairing"/"allowlist".`
|
||||
: `${scope}.dmPolicy="allowlist" but ${allowFromSubject} is empty; all DMs will be dropped. Add at least one sender ID to ${allowFromPath}${accountInheritedTarget}${accountOverrideFix} or change ${scope}.dmPolicy.`;
|
||||
return { path: allowFromPath, message };
|
||||
}
|
||||
|
||||
// Channel map keys that are not channels and must be skipped while scanning DM policy.
|
||||
const DM_POLICY_PSEUDO_CHANNEL_KEYS = new Set(["defaults", "modelByChannel", "tools"]);
|
||||
|
||||
function hasDefinedConfigValue(record: Record<string, unknown>, key: string): boolean {
|
||||
return Object.hasOwn(record, key) && record[key] !== undefined;
|
||||
}
|
||||
|
||||
function hasConfiguredDmAllowFrom(
|
||||
record: Record<string, unknown>,
|
||||
mode: ChannelDmAllowFromMode,
|
||||
): boolean {
|
||||
const dm = isRecord(record.dm) ? record.dm : null;
|
||||
if (mode === "nestedOnly") {
|
||||
return Boolean(
|
||||
(dm && hasDefinedConfigValue(dm, "allowFrom")) || hasDefinedConfigValue(record, "allowFrom"),
|
||||
);
|
||||
}
|
||||
return Boolean(
|
||||
hasDefinedConfigValue(record, "allowFrom") || (dm && hasDefinedConfigValue(dm, "allowFrom")),
|
||||
);
|
||||
}
|
||||
|
||||
function isConfigRecordEnabled(record: Record<string, unknown>): boolean {
|
||||
return record.enabled !== false;
|
||||
}
|
||||
|
||||
type ChannelDmPolicyDependencyWarningOptions = {
|
||||
dmAllowFromModes?: ReadonlyMap<string, ChannelDmAllowFromMode>;
|
||||
};
|
||||
|
||||
function hasChannelDmPolicyDependencyWarningCandidates(config: OpenClawConfig): boolean {
|
||||
if (!config.channels || !isRecord(config.channels)) {
|
||||
return false;
|
||||
}
|
||||
return Object.entries(config.channels).some(
|
||||
([channelId, channelValue]) =>
|
||||
!DM_POLICY_PSEUDO_CHANNEL_KEYS.has(channelId) &&
|
||||
isRecord(channelValue) &&
|
||||
isConfigRecordEnabled(channelValue),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Surface dmPolicy/allowFrom dependency problems generically for every channel that
|
||||
* exposes DM policy via the canonical top-level `dmPolicy`/`allowFrom` fields. These
|
||||
@@ -355,16 +403,23 @@ const DM_POLICY_PSEUDO_CHANNEL_KEYS = new Set(["defaults", "modelByChannel", "to
|
||||
* account->channel inheritance. `nestedOnly` channels (canonical fields under `dm.*`)
|
||||
* are skipped because their config shape does not match this warning's top-level paths.
|
||||
*/
|
||||
function collectChannelDmPolicyDependencyWarnings(config: OpenClawConfig): ConfigValidationIssue[] {
|
||||
function collectChannelDmPolicyDependencyWarnings(
|
||||
config: OpenClawConfig,
|
||||
options: ChannelDmPolicyDependencyWarningOptions = {},
|
||||
): ConfigValidationIssue[] {
|
||||
if (!config.channels || !isRecord(config.channels)) {
|
||||
return [];
|
||||
}
|
||||
const warnings: ConfigValidationIssue[] = [];
|
||||
for (const [channelId, channelValue] of Object.entries(config.channels)) {
|
||||
if (DM_POLICY_PSEUDO_CHANNEL_KEYS.has(channelId) || !isRecord(channelValue)) {
|
||||
if (
|
||||
DM_POLICY_PSEUDO_CHANNEL_KEYS.has(channelId) ||
|
||||
!isRecord(channelValue) ||
|
||||
!isConfigRecordEnabled(channelValue)
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
const mode = getDoctorChannelCapabilities(channelId).dmAllowFromMode;
|
||||
const mode = options.dmAllowFromModes?.get(channelId) ?? "topOnly";
|
||||
if (mode === "nestedOnly") {
|
||||
continue;
|
||||
}
|
||||
@@ -379,16 +434,24 @@ function collectChannelDmPolicyDependencyWarnings(config: OpenClawConfig): Confi
|
||||
continue;
|
||||
}
|
||||
for (const [accountId, accountValue] of Object.entries(channelValue.accounts)) {
|
||||
if (!isRecord(accountValue)) {
|
||||
if (!isRecord(accountValue) || !isConfigRecordEnabled(accountValue)) {
|
||||
continue;
|
||||
}
|
||||
const allowFromSource = hasConfiguredDmAllowFrom(accountValue, mode)
|
||||
? "explicit"
|
||||
: "inherited";
|
||||
const accountViolation = evaluateDmPolicyAllowFromDependency({
|
||||
policy: resolveChannelDmPolicy({ account: accountValue, parent: channelValue, mode }),
|
||||
allowFrom: resolveChannelDmAllowFrom({ account: accountValue, parent: channelValue, mode }),
|
||||
});
|
||||
if (accountViolation) {
|
||||
warnings.push(
|
||||
buildDmPolicyDependencyWarning({ channelId, accountId, violation: accountViolation }),
|
||||
buildDmPolicyDependencyWarning({
|
||||
channelId,
|
||||
accountId,
|
||||
allowFromSource,
|
||||
violation: accountViolation,
|
||||
}),
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -1147,9 +1210,6 @@ function validateConfigObjectWithPluginsBase(
|
||||
|
||||
const issues: ConfigValidationIssue[] = [];
|
||||
const warnings: ConfigValidationIssue[] = [];
|
||||
// Generic DM-policy/allowFrom dependency check on the raw user config (pre-defaults)
|
||||
// so account inheritance matches the per-channel Zod refinements.
|
||||
warnings.push(...collectChannelDmPolicyDependencyWarnings(base.config));
|
||||
const hasExplicitPluginsConfig = isRecord(raw) && Object.hasOwn(raw, "plugins");
|
||||
const explicitPluginReferences = collectExplicitPluginReferences(raw);
|
||||
|
||||
@@ -1174,6 +1234,7 @@ function validateConfigObjectWithPluginsBase(
|
||||
knownIds?: Set<string>;
|
||||
overriddenPluginIds?: Set<string>;
|
||||
normalizedPlugins?: ReturnType<typeof normalizePluginsConfig>;
|
||||
channelDmAllowFromModes?: Map<string, ChannelDmAllowFromMode>;
|
||||
channelSchemas?: Map<
|
||||
string,
|
||||
{
|
||||
@@ -1228,6 +1289,8 @@ function validateConfigObjectWithPluginsBase(
|
||||
return registryInfo;
|
||||
};
|
||||
|
||||
const ensureLoadedRegistryInfo = (): RegistryInfo => registryInfo ?? loadValidationRegistry();
|
||||
|
||||
const ensureCompatPluginIds = (): ReadonlySet<string> => {
|
||||
if (compatPluginIdsResolved) {
|
||||
return compatPluginIds ?? new Set<string>();
|
||||
@@ -1268,7 +1331,7 @@ function validateConfigObjectWithPluginsBase(
|
||||
};
|
||||
|
||||
const ensureRegistry = (): RegistryInfo => {
|
||||
const info = registryInfo ?? loadValidationRegistry();
|
||||
const info = ensureLoadedRegistryInfo();
|
||||
ensureCompatConfig();
|
||||
pushRegistryDiagnostics(info.registry);
|
||||
return info;
|
||||
@@ -1336,6 +1399,28 @@ function validateConfigObjectWithPluginsBase(
|
||||
return info.channelSchemas;
|
||||
};
|
||||
|
||||
const ensureChannelDmAllowFromModes = (): ReadonlyMap<string, ChannelDmAllowFromMode> => {
|
||||
const info = ensureLoadedRegistryInfo();
|
||||
if (!info.channelDmAllowFromModes) {
|
||||
info.channelDmAllowFromModes = new Map(
|
||||
collectChannelDmPolicyMetadata(info.registry).flatMap((entry) =>
|
||||
entry.dmAllowFromMode ? [[entry.id, entry.dmAllowFromMode] as const] : [],
|
||||
),
|
||||
);
|
||||
}
|
||||
return info.channelDmAllowFromModes;
|
||||
};
|
||||
|
||||
// Generic DM-policy/allowFrom dependency check on the raw user config (pre-defaults)
|
||||
// so account inheritance matches the per-channel Zod refinements.
|
||||
warnings.push(
|
||||
...(hasChannelDmPolicyDependencyWarningCandidates(base.config)
|
||||
? collectChannelDmPolicyDependencyWarnings(base.config, {
|
||||
dmAllowFromModes: ensureChannelDmAllowFromModes(),
|
||||
})
|
||||
: collectChannelDmPolicyDependencyWarnings(base.config)),
|
||||
);
|
||||
|
||||
let mutatedConfig = config;
|
||||
let channelsCloned = false;
|
||||
let pluginsCloned = false;
|
||||
|
||||
Reference in New Issue
Block a user