diff --git a/src/config/channel-config-metadata.ts b/src/config/channel-config-metadata.ts index d9dbde0a2d73..d456ec232f45 100644 --- a/src/config/channel-config-metadata.ts +++ b/src/config/channel-config-metadata.ts @@ -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> = { // 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(); + + 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); +} diff --git a/src/config/validation.channel-metadata.test.ts b/src/config/validation.channel-metadata.test.ts index 3da64392e68f..aaca48b5df22 100644 --- a/src/config/validation.channel-metadata.test.ts +++ b/src/config/validation.channel-metadata.test.ts @@ -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 & Pick, ): 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 diff --git a/src/config/validation.ts b/src/config/validation.ts index 15863e77c341..346fa3f1fbdc 100644 --- a/src/config/validation.ts +++ b/src/config/validation.ts @@ -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, key: string): boolean { + return Object.hasOwn(record, key) && record[key] !== undefined; +} + +function hasConfiguredDmAllowFrom( + record: Record, + 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): boolean { + return record.enabled !== false; +} + +type ChannelDmPolicyDependencyWarningOptions = { + dmAllowFromModes?: ReadonlyMap; +}; + +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; overriddenPluginIds?: Set; normalizedPlugins?: ReturnType; + channelDmAllowFromModes?: Map; channelSchemas?: Map< string, { @@ -1228,6 +1289,8 @@ function validateConfigObjectWithPluginsBase( return registryInfo; }; + const ensureLoadedRegistryInfo = (): RegistryInfo => registryInfo ?? loadValidationRegistry(); + const ensureCompatPluginIds = (): ReadonlySet => { if (compatPluginIdsResolved) { return compatPluginIds ?? new Set(); @@ -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 => { + 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;