mirror of https://github.com/openclaw/openclaw.git
fix(mattermost): reachable callbackUrl guard, nativeSkills support, URL reconciliation
- Reject slash command registration when callbackUrl resolves to
loopback but Mattermost baseUrl is remote; log actionable error
directing user to set commands.callbackUrl explicitly
- Honor commands.nativeSkills: when enabled, dynamically list skill
commands and register them with oc_ prefix alongside built-in commands
- Reconcile existing commands on callback URL change: attempt PUT update,
fallback to delete+recreate for stale commands with mismatched URLs
- Add updateMattermostCommand() for PUT /api/v4/commands/{id}
Addresses Codex review round 4 (P1 + P2 items).
This commit is contained in:
parent
f0a0766d99
commit
d486f208a2
|
|
@ -235,6 +235,58 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
|
|||
gatewayHost: cfg.gateway?.customBindHost ?? undefined,
|
||||
});
|
||||
|
||||
const isLoopbackHost = (hostname: string) =>
|
||||
hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1";
|
||||
|
||||
try {
|
||||
const mmHost = new URL(baseUrl).hostname;
|
||||
const callbackHost = new URL(callbackUrl).hostname;
|
||||
if (isLoopbackHost(callbackHost) && !isLoopbackHost(mmHost)) {
|
||||
runtime.error?.(
|
||||
`mattermost: slash commands callbackUrl resolved to ${callbackUrl} (loopback). This is unreachable from Mattermost at ${baseUrl}. Set channels.mattermost.commands.callbackUrl to a reachable URL (e.g. your public reverse proxy URL). Skipping slash command registration.`,
|
||||
);
|
||||
throw new Error("unreachable callbackUrl (loopback)");
|
||||
}
|
||||
} catch (err) {
|
||||
// If URL parsing fails or callback is loopback/unreachable, skip registration.
|
||||
throw err;
|
||||
}
|
||||
|
||||
const commandsToRegister: import("./slash-commands.js").MattermostCommandSpec[] = [
|
||||
...DEFAULT_COMMAND_SPECS,
|
||||
];
|
||||
|
||||
if (slashConfig.nativeSkills === true) {
|
||||
try {
|
||||
const { listSkillCommandsForAgents } =
|
||||
await import("../../../../src/auto-reply/skill-commands.js");
|
||||
const skillCommands = listSkillCommandsForAgents({ cfg: cfg as any });
|
||||
for (const spec of skillCommands) {
|
||||
const name = typeof spec.name === "string" ? spec.name.trim() : "";
|
||||
if (!name) continue;
|
||||
const trigger = name.startsWith("oc_") ? name : `oc_${name}`;
|
||||
commandsToRegister.push({
|
||||
trigger,
|
||||
description: spec.description || `Run skill ${name}`,
|
||||
autoComplete: true,
|
||||
autoCompleteHint: "[args]",
|
||||
});
|
||||
}
|
||||
} catch (err) {
|
||||
runtime.error?.(`mattermost: failed to list skill commands: ${String(err)}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Deduplicate by trigger
|
||||
const seen = new Set<string>();
|
||||
const dedupedCommands = commandsToRegister.filter((cmd) => {
|
||||
const key = cmd.trigger.trim();
|
||||
if (!key) return false;
|
||||
if (seen.has(key)) return false;
|
||||
seen.add(key);
|
||||
return true;
|
||||
});
|
||||
|
||||
const allRegistered: import("./slash-commands.js").MattermostRegisteredCommand[] = [];
|
||||
|
||||
for (const team of teams) {
|
||||
|
|
@ -242,7 +294,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
|
|||
client,
|
||||
teamId: team.id,
|
||||
callbackUrl,
|
||||
commands: DEFAULT_COMMAND_SPECS,
|
||||
commands: dedupedCommands,
|
||||
log: (msg) => runtime.log?.(msg),
|
||||
});
|
||||
allRegistered.push(...registered);
|
||||
|
|
|
|||
|
|
@ -91,6 +91,18 @@ type MattermostCommandCreate = {
|
|||
creator_id?: string;
|
||||
};
|
||||
|
||||
type MattermostCommandUpdate = {
|
||||
id: string;
|
||||
team_id: string;
|
||||
trigger: string;
|
||||
method: "P" | "G";
|
||||
url: string;
|
||||
description?: string;
|
||||
auto_complete: boolean;
|
||||
auto_complete_desc?: string;
|
||||
auto_complete_hint?: string;
|
||||
};
|
||||
|
||||
type MattermostCommandResponse = {
|
||||
id: string;
|
||||
token: string;
|
||||
|
|
@ -194,6 +206,22 @@ export async function deleteMattermostCommand(
|
|||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Update an existing custom slash command.
|
||||
*/
|
||||
export async function updateMattermostCommand(
|
||||
client: MattermostClient,
|
||||
params: MattermostCommandUpdate,
|
||||
): Promise<MattermostCommandResponse> {
|
||||
return await client.request<MattermostCommandResponse>(
|
||||
`/commands/${encodeURIComponent(params.id)}`,
|
||||
{
|
||||
method: "PUT",
|
||||
body: JSON.stringify(params),
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Register all OpenClaw slash commands for a given team.
|
||||
* Skips commands that are already registered with the same trigger + callback URL.
|
||||
|
|
@ -216,16 +244,15 @@ export async function registerSlashCommands(params: {
|
|||
log?.(`mattermost: failed to list existing commands: ${String(err)}`);
|
||||
}
|
||||
|
||||
const existingByTrigger = new Map(
|
||||
existing.filter((cmd) => cmd.url === callbackUrl).map((cmd) => [cmd.trigger, cmd]),
|
||||
);
|
||||
const existingByTrigger = new Map(existing.map((cmd) => [cmd.trigger, cmd]));
|
||||
|
||||
const registered: MattermostRegisteredCommand[] = [];
|
||||
|
||||
for (const spec of commands) {
|
||||
// Skip if already registered with same callback URL
|
||||
const existingCmd = existingByTrigger.get(spec.trigger);
|
||||
if (existingCmd) {
|
||||
|
||||
// Already registered with the correct callback URL
|
||||
if (existingCmd && existingCmd.url === callbackUrl) {
|
||||
log?.(`mattermost: command /${spec.trigger} already registered (id=${existingCmd.id})`);
|
||||
registered.push({
|
||||
id: existingCmd.id,
|
||||
|
|
@ -237,6 +264,51 @@ export async function registerSlashCommands(params: {
|
|||
continue;
|
||||
}
|
||||
|
||||
// Exists but points to a different URL: attempt to reconcile by updating
|
||||
// (useful during callback URL migrations).
|
||||
if (existingCmd && existingCmd.url !== callbackUrl) {
|
||||
log?.(
|
||||
`mattermost: command /${spec.trigger} exists with different callback URL; updating (id=${existingCmd.id})`,
|
||||
);
|
||||
try {
|
||||
const updated = await updateMattermostCommand(client, {
|
||||
id: existingCmd.id,
|
||||
team_id: teamId,
|
||||
trigger: spec.trigger,
|
||||
method: "P",
|
||||
url: callbackUrl,
|
||||
description: spec.description,
|
||||
auto_complete: spec.autoComplete,
|
||||
auto_complete_desc: spec.description,
|
||||
auto_complete_hint: spec.autoCompleteHint,
|
||||
});
|
||||
registered.push({
|
||||
id: updated.id,
|
||||
trigger: spec.trigger,
|
||||
teamId,
|
||||
token: updated.token,
|
||||
managed: false,
|
||||
});
|
||||
continue;
|
||||
} catch (err) {
|
||||
log?.(
|
||||
`mattermost: failed to update command /${spec.trigger} (id=${existingCmd.id}): ${String(err)}`,
|
||||
);
|
||||
// Fallback: try delete+recreate (safe for the oc_* namespace).
|
||||
try {
|
||||
await deleteMattermostCommand(client, existingCmd.id);
|
||||
log?.(`mattermost: deleted stale command /${spec.trigger} (id=${existingCmd.id})`);
|
||||
} catch (deleteErr) {
|
||||
log?.(
|
||||
`mattermost: failed to delete stale command /${spec.trigger} (id=${existingCmd.id}): ${String(deleteErr)}`,
|
||||
);
|
||||
// Can't reconcile; skip this command.
|
||||
continue;
|
||||
}
|
||||
// Continue on to create below.
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const created = await createMattermostCommand(client, {
|
||||
team_id: teamId,
|
||||
|
|
|
|||
Loading…
Reference in New Issue