feat(mattermost): add native slash command support (refresh)#18657
feat(mattermost): add native slash command support (refresh)#18657echo931 wants to merge 25 commits intoopenclaw:mainfrom
Conversation
Register custom slash commands via Mattermost REST API at startup,
handle callbacks via HTTP endpoint on the gateway, and clean up
commands on shutdown.
- New modules: slash-commands.ts (API + registration), slash-http.ts
(callback handler), slash-state.ts (shared state bridge)
- Config schema extended with commands.{native,nativeSkills,callbackPath,callbackUrl}
- Uses oc_ prefix for triggers (oc_status, oc_model, etc.) to avoid
conflicts with Mattermost built-in commands
- Opt-in via channels.mattermost.commands.native: true
- Capability nativeCommands: true exposed for command registry
Closes openclaw#16515
Address Codex review findings: 1. slash-http.ts: Token validation now rejects when commandTokens set is empty (e.g. registration failure). Previously an empty set meant any token was accepted — fail-open vulnerability. 2. slash-state.ts: Replaced global singleton with per-account state Map keyed by accountId. Multi-account deployments no longer overwrite each other's tokens, registered commands, or handlers. The HTTP route dispatcher matches inbound tokens to the correct account. 3. monitor.ts: Updated getSlashCommandState/deactivateSlashCommands calls to pass accountId.
Address Codex follow-up feedback: - Enforce DM/group allowlist + control-command gating for native slash commands (no unconditional CommandAuthorized=true). - Register callback HTTP routes for all configured callbackPath values (top-level + per-account overrides). - Track whether slash commands were created by this process; only delete managed commands on shutdown, leaving pre-existing commands intact.
- parseSlashCommandPayload JSON branch now validates required fields (token, team_id, channel_id, user_id, command) like the form-encoded branch, preventing runtime exceptions on malformed JSON payloads - normalizeCallbackPath() ensures leading '/' to prevent malformed URLs like 'http://host:portapi/...' when callbackPath lacks a leading slash - Applied in resolveSlashCommandConfig and resolveCallbackUrl Addresses Codex review round 3 (P2 items).
…RL 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).
…ackUrl pathname - normalizeAllowList/isSenderAllowed in slash-http.ts now matches the websocket monitor: strips mattermost:/user:/@ prefixes and supports the '*' wildcard, so configs that work for WS also work for slash cmds - registerSlashCommandRoute extracts pathname from explicit callbackUrl and registers it alongside callbackPath, so callbacks hit a registered route even when callbackUrl uses a non-default pathname Addresses Codex review round 5 (P1 + P2).
…rMap for accurate command name resolution - Export listSkillCommandsForAgents and SkillCommandSpec from plugin-sdk/index.ts (removes deep relative import in monitor.ts) - Add originalName field to MattermostCommandSpec for preserving pre-prefix names - Build trigger→originalName map in monitor.ts, thread through slash-state → slash-http - resolveCommandText() now uses triggerMap for accurate name lookup (oc_report → /oc_report correctly, not /report)
…nt race Snapshot registered commands, then deactivate state immediately on abort. Prevents race where new monitor activates fresh state that gets wiped by the delayed .then() of the old monitor's cleanup promise.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31eff977c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void cleanupSlashCommands({ | ||
| client, | ||
| commands, | ||
| log: (msg) => runtime.log?.(msg), | ||
| }).catch((err) => runtime.error?.(`mattermost: slash cleanup failed: ${String(err)}`)); |
There was a problem hiding this comment.
Await slash cleanup before allowing account restart
Launching cleanupSlashCommands as fire-and-forget on abort can delete commands that a new monitor instance has already adopted. During a quick restart/reload, the new process can list the still-existing commands, mark them as reused, and activate tokens, then the old async cleanup finishes and deletes those same command IDs, leaving native slash commands silently unregistered until another restart.
Useful? React with 👍 / 👎.
| for (const [, state] of accountStates) { | ||
| if (state.commandTokens.has(token) && state.handler) { | ||
| matchedHandler = state.handler; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Disambiguate token routing across Mattermost accounts
The multi-account router picks the first account whose token set contains the inbound token, but tokens are not guaranteed to be unique across account states (for example when two account configs reuse the same team commands). In that case callbacks are always dispatched to whichever state was iterated first, so commands can run under the wrong account/agent context and the other account never receives its callbacks.
Useful? React with 👍 / 👎.
|
Addressed in latest push ( For the routing ambiguity called out in #18657:
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review the last refresh (merging main) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7be68ecfc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const envPortRaw = process.env.OPENCLAW_GATEWAY_PORT?.trim(); | ||
| const envPort = envPortRaw ? Number.parseInt(envPortRaw, 10) : NaN; | ||
| const gatewayPort = | ||
| Number.isFinite(envPort) && envPort > 0 ? envPort : (cfg.gateway?.port ?? 3015); |
There was a problem hiding this comment.
Derive callback URL with the current gateway default port
This fallback still hardcodes 3015, but the gateway default runtime port is now 18789; when OPENCLAW_GATEWAY_PORT is absent and gateway.port is unset, slash commands are registered against the wrong callback port and Mattermost callbacks will fail to reach the server. This breaks native slash commands in boot paths that don't inject the env var (for example direct monitor invocations or test harnesses).
Useful? React with 👍 / 👎.
| const isMattermostSlashCommandCallback = mattermostCallbackPaths.has(requestPath); | ||
| if (requestPath.startsWith("/api/channels/") && !isMattermostSlashCommandCallback) { |
There was a problem hiding this comment.
Scope channel auth bypass to actual Mattermost callback routes
Gateway auth is skipped for any /api/channels/* request whose path matches a value from Mattermost callback config, but this check does not verify that the matched path is actually handled by the Mattermost slash route. If channels.mattermost.commands.callbackPath is misconfigured to another channel endpoint, that endpoint will become publicly reachable without gateway auth.
Useful? React with 👍 / 👎.
|
Addressed the Codex feedback in commit f8e1f1a:
Re-running CI now on the updated head. @codex |
|
To use Codex here, create an environment for this repo. |
1 similar comment
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd7e0d28cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : channelInfo | ||
| ? channelType?.toUpperCase() === "G" | ||
| ? "group" | ||
| : "channel" | ||
| : "channel"; |
There was a problem hiding this comment.
Fail closed when slash channel lookup cannot determine chat type
When fetchMattermostChannel fails, this code falls back to treating the invocation as channel instead of rejecting the request, so DM callbacks no longer go through dmPolicy enforcement. In a DM where lookup fails (for example transient API failure or permission issues), settings like dmPolicy: "disabled" can be bypassed if group policy allows commands, and the inverse misclassification can also incorrectly block valid DMs; returning a deny/temporary error when channel type is unknown would avoid policy drift.
Useful? React with 👍 / 👎.
|
Addressed in latest push ().\n\n- slash callback auth now fails closed when channel lookup cannot determine the channel type\n- returns ephemeral temporary error instead of defaulting to channel policy\n- logs lookup failure for diagnostics\n\nThis prevents DM/group policy drift when the channel lookup fails transiently. |
|
Correction: commit hash is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6cc649dd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| channelInfo = await fetchMattermostChannel(client, channelId); | ||
| } catch (err) { | ||
| log?.(`mattermost: slash channel lookup failed for ${channelId}: ${String(err)}`); |
There was a problem hiding this comment.
Pass logger into slash authorization error path
In authorizeSlashInvocation, the channel lookup catch path calls log?.(...), but this function never defines or receives a log variable. When fetchMattermostChannel fails (for example during transient Mattermost API outages or permission errors), this throws a ReferenceError, causing the callback handler to fail with a 500 instead of returning the intended temporary-deny response; slash commands become unusable exactly when channel lookup is degraded.
Useful? React with 👍 / 👎.
| runtime.error?.(`mattermost: slash cleanup failed: ${String(err)}`); | ||
| }); | ||
| }; | ||
| opts.abortSignal?.addEventListener("abort", onAbortCleanup, { once: true }); |
There was a problem hiding this comment.
Run slash cleanup immediately when abort signal is already set
Shutdown cleanup is only wired via abort event subscription here, but there is no immediate signal.aborted check. If cancellation happens during startup registration (or the monitor starts with an already-aborted signal), the handler never runs, slashShutdownCleanup stays null, and managed commands created earlier in startup are left registered and pointing at a dead callback endpoint until manually reconciled.
Useful? React with 👍 / 👎.
|
Addressed in latest push (
Local targeted tests pass:
|
|
👋 Hey! AI-assisted review here (Claude Code) — we're tracking this PR as prospective adopters with a live Mattermost deployment. The architecture looks solid, especially the security posture (fail-closed token validation, HTTP 409 on ambiguous multi-account routing, channel lookup failure handling). Great work on the iterative hardening rounds too. One observation on test coverage — the two largest new files have no tests:
The HTTP handler in particular has a lot of security-sensitive authorization logic (DM/group/channel policy enforcement, allowlist checking, pairing flow). Some high-value test scenarios that could help:
No blockers from our side — just flagging the gap in case it's useful. Looking forward to trying this out when it ships! 🎉 |
|
Thanks for the detailed pass — super helpful.\n\nAgreed on the coverage gap in and . I’m going to add a focused test slice before merge, prioritizing:\n- token validation / fail-closed paths\n- DM/group/channel policy gating\n- body parsing ( + JSON)\n- command text normalization ( stripping + skill mapping)\n\nI’ll push that as a follow-up commit in this PR and ping here when it’s in. |
|
Follow-up (formatting fix): Thanks again for the detailed pass — super helpful. Agreed on the coverage gap in slash-http.ts and slash-commands.ts. I’m going to add a focused test slice before merge, prioritizing:
I’ll push that as a follow-up commit in this PR and ping here when it’s in. |
|
Addressed remaining Mattermost native slash review points with focused tests only. Added coverage:
Local test commands/results:
|
Summary
Refresh of stale PR #17302 onto current
main.Reintroduces native Mattermost slash command support, including follow-up hardening commits from the original branch.
Included
Validation
pnpm exec vitest run src/auto-reply/command-control.test.ts src/gateway/server-http.hooks-request-timeout.test.ts extensions/mattermost/src/channel.test.ts extensions/mattermost/src/mattermost/client.test.tsRefs #16515
Greptile Summary
Reintroduces native Mattermost slash command support with comprehensive security hardening:
/api/v4/commands) and handles callbacks through dedicated HTTP endpointscommands.allowFromoverride semanticschannels.mattermost.commands.native=true, supports callback URL overrides for reverse proxies, and skill-based command registration vianativeSkillsConfidence Score: 4/5
Last reviewed commit: 31eff97