fix(acp): bypass ACP dispatch for /acp text commands in bound threads#66407
fix(acp): bypass ACP dispatch for /acp text commands in bound threads#66407kindomLee wants to merge 5 commits intoopenclaw:mainfrom
Conversation
`/acp close` (and every other `/acp ...` text command) sent inside a Discord thread bound to an ACP session never reached `handleAcpCommand` because `shouldBypassAcpDispatchForCommand` only whitelisted `/new`, `/reset`, and `!`-prefix bang commands for the bypass. Every other slash command — including `/acp ...` — fell through and was handed off to the thread's ACP session as conversational input, which the agent then replied to with a hallucinated natural-language message (`done`/`好的`) while the session stayed open and the binding unchanged. Add `isAcpCommandCandidate` mirroring `isResetCommandCandidate`, and wire it into the bypass flow right after the reset check so `/acp` routes to `handleAcpCommand` when the surface is allowed to handle text commands. The existing unit test "returns false for ACP slash commands" actively locked in the buggy behavior; it is flipped and renamed to tag the regression against openclaw#66298. Two additional tests cover the `CommandSource: "native"` path (Discord slash-command autocomplete) and the unrecognized-slash-command path. Closes openclaw#66298
Greptile SummaryFixes the ACP command routing bug where Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-acp-command-bypass.ts
Line: 61-63
Comment:
**`isAcpCommandCandidate` is gated on `allowTextCommands`; `isResetCommandCandidate` is not**
`isResetCommandCandidate` returns `true` unconditionally regardless of `allowTextCommands`, meaning `/reset` always bypasses ACP dispatch. The new `/acp` check returns `allowTextCommands`, so when a surface has `commands.text: false`, `/acp close` still falls through to the ACP session — preserving exactly the hallucination bug described in the PR, just on that narrower config. If that's intentional (ACP management commands require text commands to be enabled), a test for the `allowTextCommands = false` path with an `/acp` body would make it explicit and lock the behavior in. If the intent is for `/acp` to bypass unconditionally like `/new`/`/reset`, the guard should be `return true` here.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(acp): bypass ACP dispatch for /acp t..." | Re-trigger Greptile |
…emantics Addresses Greptile P2 on openclaw#66407: the previous guard returned `allowTextCommands` which left /acp gated on `commands.text`. That preserved exactly the hallucination bug from openclaw#66298 on the narrow `commands.text: false` config — the user could still not close a runaway ACP session without hand-editing thread-bindings.json, which is precisely the workaround the original issue complains about. /acp is a session-management command family (close / cancel / status / new / spawn / …), mirroring /new and /reset, and must bypass the ACP dispatch for the same reason those do: the user has to be able to escape a runaway session even when text commands are otherwise disabled on the surface. Change the guard to `return true`, aligned with `isResetCommandCandidate`. Add a regression test that locks in the bypass when `commands.text: false`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecda9d508b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…eset Addresses Codex P1 on openclaw#66407: the unconditional `return true` added in ecda9d5 for shouldBypassAcpDispatchForCommand was still half-broken because handleAcpCommand itself has an early-return `if (!allowTextCommands) return null;` that fires before any other check. On surfaces with `commands.text: false`, /acp close still fell through to the ACP session as conversational input — the exact hallucination bug openclaw#66298 reports, just on a narrower config. The !allowTextCommands gate is vestigial. It was added in the original ACP feature PR (openclaw#23580). The real security boundary was added later in openclaw#46789: `requireGatewayClientScopeForInternalChannel` requiring `operator.admin` scope for every mutating action (commands-acp.ts:110-119). Every mutating /acp action is still gated by that scope check. Non-mutating actions (`help`, `doctor`, `install`, `sessions`) are all read-only text output — verified: `handleAcpInstallAction` in diagnostics.ts:114-132 returns stopWithText with install-hint lines, it is NOT an actual installer. `isAuthorizedSender` check at line 98 still fires, so unauthorized senders are still blocked. This mirrors `maybeHandleResetCommand` in commands-reset.ts:18-82, which has NO allowTextCommands gate and unconditionally mutates via `resetConfiguredBindingTargetInPlace` — gated only by isAuthorizedSender. /acp is strictly more gated than /reset (both isAuthorizedSender AND operator.admin for mutating actions), so the parity move is safe. Regression test added: calls handleAcpCommand with allowTextCommands=false and asserts the /acp close lifecycle path still runs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47036624d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… prefix forms Addresses Codex P2 on openclaw#66407: the tight regexes in isAcpCommandCandidate and isResetCommandCandidate (`(?:\s|$)`) only accepted `/cmd` followed by whitespace or end-of-string, but the actual handlers accept more variants. handleAcpCommand uses `normalized.startsWith("/acp")` which lets `/acp@otherbot close` through (multi-bot environments where normalizeCommandBody does NOT strip the @mention because the bot username doesn't match — see commands-registry.test.ts:299 for the canonical test of that case). maybeHandleResetCommand has its own tight regex that exhibits the same asymmetry between bypass and handler if the handler is loosened in the future. This commit broadens both bypass regexes (and the matching regex in maybeHandleResetCommand for symmetry) to also accept `:` and `@` as boundary characters after the command token. The forms `/cmd@bot` and `/cmd:tail` are produced when normalizeCommandBody can't fully canonicalize the input — for example when the user mentions a bot that doesn't match `options.botUsername`. Practical impact is narrow (multi-bot Discord/Telegram, user mentions wrong bot, types an escape-hatch command in an ACP-bound thread) but the fix is one character class change per regex and locks in the invariant. Regression tests added to dispatch-acp-command-bypass.test.ts (4 cases: /acp@otherbot, /acp:tail, /reset@otherbot, /reset:tail) and to commands-reset.test.ts (2 cases for /reset@otherbot, /reset:tail). Scope expansion note for reviewers: the original PR fixes /acp; this commit also touches /reset for the same reason since the asymmetry is bug-shaped and easier to fix once than to leave as a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f6f9819b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…t is intentionally ignored Reverts the regex broadening from 2f6f981 (round 4 of openclaw#66407). Codex's P1 on that commit pointed out that widening the matchers to accept `@` after the command token causes /reset@otherbot ... and /new@otherbot to execute a local reset, even though the user was targeting another bot. That contradicts the project-wide convention encoded in src/auto-reply/command-control.test.ts:901-912: it("ignores telegram commands addressed to other bots", () => { expect(hasControlCommand("/help@otherbot", undefined, { botUsername: "openclaw" })).toBe(false); expect(hasControlCommand("/help@openclaw", undefined, { botUsername: "openclaw" })).toBe(true); }); normalizeCommandBody already enforces this: it strips the @mention only when the suffix matches options.botUsername, otherwise the @Otherbot form is preserved and downstream code is supposed to ignore it. Round 3's Codex P2 was technically right that the bypass and the handler had asymmetric regex tightness, but the correct way to resolve the asymmetry per project convention is to keep BOTH tight, not loosen both. I overcorrected. handleAcpCommand's pre-existing `startsWith("/acp")` loose check still mis-handles /acp@otherbot in the harmless way (action resolution fails inside the handler so nothing executes), and tightening that is out of scope for this PR. Reverts: - isResetCommandCandidate: `(?:[\s:@]|$)` -> `(?:\s|$)` - isAcpCommandCandidate: `(?:[\s:@]|$)` -> `(?:\s|$)` - maybeHandleResetCommand: `(?:[\s:@]|$)` -> `(?:\s|$)` Tests: - dispatch-acp-command-bypass.test.ts: the two `:colon-syntax` tests are removed (colon form is normalized away by normalizeCommandBody before the bypass sees it, so those tests were exercising a code path that never fires in production). The two `@otherbot` tests are flipped to assert false and renamed to lock in the convention with a reference to command-control.test.ts:901. - commands-reset-hooks.test.ts: the two round-4 regression tests are removed (they were validating the buggy round-4 behavior).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62db18f04a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export const handleAcpCommand: CommandHandler = async (params, _allowTextCommands) => { | ||
| const normalized = params.command.commandBodyNormalized; | ||
| if (!normalized.startsWith(COMMAND)) { | ||
| return null; |
There was a problem hiding this comment.
Restore text-command gate for /acp on native surfaces
Removing the allowTextCommands early return makes /acp handling run even when commands.text is disabled on native-command surfaces. In that configuration, inputs like /acp@otherbot close now pass startsWith(COMMAND) and get handled by this bot (help text or action handling) instead of being ignored as “not for us,” which conflicts with the command-addressing convention covered in src/auto-reply/command-control.test.ts:901. This is a behavioral regression introduced by this change for text-disabled native surfaces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — verified the claim against the current tree. Tracing /acp@otherbot close through 62db18f end-to-end:
normalizeCommandBody(src/auto-reply/commands-registry-normalize.ts:70-77) only rewrites the@usernamesuffix when it equalsoptions.botUsername; for@otherbotthe mention is preserved verbatim, socommandBodyNormalized = "/acp@otherbot close".handleAcpCommandpasses thestartsWith(COMMAND)check atcommands-acp.ts:90, slices/acpoff, and hands["@otherbot", "close"]toresolveAcpAction(commands-acp/shared.ts:95-119).@otherbotisn't in the action whitelist, so the resolver returns"help"andstopWithText(resolveAcpHelpText())runs.
So yes: post-PR, /acp@otherbot close on a commands.text: false surface now emits /acp help text where pre-PR it would have early-returned null. Confirmed.
Why I'd rather not restore the gate as the fix:
The wrong-bot help-text response isn't actually new behavior — it's pre-existing on every commands.text: true surface. There, allowTextCommands is true, the pre-PR early-return never fired, and /acp@otherbot close has always reached the same startsWith → resolveAcpAction → "help" path and emitted the same help text. That's been the behavior since the original ACP feature PR (#23580). What 62db18f changes is that commands.text: false surfaces now match that behavior; the underlying issue — startsWith("/acp") accepting a wrong-bot mention — is unchanged on either side of this PR.
Restoring the gate would also create a new broken state on the commands.text: false ACP-bound row, which is the row this PR exists to fix. With the gate back, /acp close (no mention, the actual escape hatch from #66298):
handleAcpCommandearly-returnsnullon!allowTextCommands(commands-acp.ts:88, gate restored).runCommands(get-reply-inline-actions.ts:497) returnsshouldContinue: true; no other handler claims the message.- The ACP reply hook fires (
acp-runtime.ts:80);shouldBypassAcpDispatchForCommandmatchesisAcpCommandCandidateand returnstrue(dispatch-acp-command-bypass.ts:66-68). tryDispatchAcpReplyseesbypassForCommand: trueand returnsnull(dispatch-acp.ts:294), so the ACP session is skipped.- The hook returns
void(acp-runtime.ts:112), no reply is produced, the close action never executes, and the runaway session stays open.
So restoring the gate doesn't recover pre-PR behavior — it puts the bypass and the handler in disagreement and silently drops /acp close on commands.text: false surfaces. The escape hatch the PR is built around stops working.
The deterministic help-text response on a wrong-bot mention is the strictly-better failure mode of the two: bounded output, no session impact, and consistent with what commands.text: true surfaces have always done for the same input.
Where the proper fix belongs: at the normalizer. normalizeCommandBody should drop /cmd@otherbot for any otherbot != options.botUsername before any startsWith matcher sees it, mirroring the convention already encoded for hasControlCommand at command-control.test.ts:901-912. That fix lives in one place and covers every command handler with the same startsWith shape (handleHelpCommand, handleStatusCommand, handleModelsCommand, …) — substantially out of scope for a PR titled "bypass ACP dispatch for /acp text commands in bound threads". Happy to file a follow-up issue for it if a maintainer agrees that's the right layer.
|
Fixed on The landed fix keeps Thanks @kindomLee; the changelog credits your work here. Closing this as superseded by the main-branch fix. |
Summary
/acp close(and every other/acptext command) sent inside a Discord thread bound to an ACP session never reacheshandleAcpCommand. It is handed off to the thread's ACP session and consumed as conversational input by the ACP agent, which replies with a hallucinated natural-language message while the session stays open.thread-bindings.jsonand restarting the gateway.shouldBypassAcpDispatchForCommandnow recognizes/acp ...as a bypass candidate in the same way/newand/resetalready are. When the surface is allowed to handle text commands, the message is routed tohandleAcpCommandinstead of the ACP session./acpcommand grammar,handleAcpCommanditself, the ACP session lifecycle,thread-bindings.jsonschema, the!-prefix command path, and every non-/acp/ non-/resetslash command still fall through to the ACP session exactly as before.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause
shouldBypassAcpDispatchForCommandonly whitelisted two slash-command shapes for the ACP-dispatch bypass:/new//reset(viaisResetCommandCandidate) and!-prefix bang commands. Every other slash command — including/acp ...— fell throughif (!normalized.startsWith("!")) return false;and was then dispatched to the thread's active ACP session as plain user input. The ACP agent (any model) then hallucinated a polite natural-language reply such asdone/好的, making the command look like it worked even thoughhandleAcpCommandhad never been invoked."returns false for ACP slash commands"indispatch-acp-command-bypass.test.tsactively locked in the buggy behavior — it asserted that/acp cancelreturnsfalsefrom the bypass check. That test is flipped in this PR to assert the correct post-fix behavior./acpis a registered command (commands-registry.shared.ts:342registerstextAlias: "/acp",commands-acp/shared.ts:15exportsCOMMAND = "/acp"), so the grammar has been there for a while; only the bypass filter was missing the prefix.Regression Test Plan
src/auto-reply/reply/dispatch-acp-command-bypass.test.tsshouldBypassAcpDispatchForCommandmust returntruefor/acp ...when the surface is allowed to handle text commands, both forCommandSource: "text"(default) andCommandSource: "native"(Discord slash autocomplete).User-visible / Behavior Changes
/acp close,/acp cancel,/acp status,/acp new, and every other/acp ...text/slash command issued from inside a thread bound to an ACP session now runs throughhandleAcpCommandas expected, instead of being swallowed by the thread's ACP session.Diagram
Security Impact (required)
/acpsubcommands is unchanged; only the routing of an already-registered command is corrected.Yes, explain risk + mitigation: N/ARepro + Verification
Environment
openclaw.serviceminimax/MiniMax-M2.7as the bound ACP agent (not model-specific — any ACP agent would hallucinate a reply to/acp close)channels.discord.threadBindingswithspawnAcpSessions: truechannels.discord.threadBindings.enabled: true,channels.discord.threadBindings.spawnAcpSessions: trueSteps
/acp spawn --thread herewith agentclaude(or any ACP agent). Confirm/root/.openclaw/discord/thread-bindings.jsongains the new entry./acp close(plain text, Discord autocomplete, or slash-command dropdown)./root/.openclaw/discord/thread-bindings.json.Expected
handleAcpCloseActionruns.✅ Closed ACP session <key>. Removed 1 binding.Actual (before fix)
handleAcpCloseActionnever runs:grep -E "Closed ACP|Removed.*binding|unbind" /root/clawd/logs/openclaw.logreturns zero matches for the full session window.thread-bindings.jsonentry is unchanged.donefrom the ACP agent; the session stays open.Actual (after fix)
handleAcpCloseAction→acpManager.closeSession→getSessionBindingService().unbindruns.thread-bindings.jsonbinding count drops from 2 → 1 as expected.✅ Closed ACP session <key>. Removed 1 binding.string.Evidence
Failing test before, passing after
The existing
"returns false for ACP slash commands"test indispatch-acp-command-bypass.test.tsasserted the buggy behavior (/acp cancel→ bypassfalse). After this patch, that assertion is flipped to the correct post-fix expectation and renamed to tag the regression back to this issue. Two additional tests were added for the native-command-source path and for unrecognized slash commands.Unit tests (local, this PR):
Full pipeline gates (local, this PR):
Live-run evidence referenced from #66298
When I originally filed #66298 (same GitHub account, same day) I had already locally patched a similar bypass in the compiled
dist/build and reported the before/after trace in the issue body —grep -E "Closed ACP|Removed.*binding|unbind" /root/clawd/logs/openclaw.logreturning no matches before, andhandleAcpCloseAction→acpManager.closeSession→getSessionBindingService().unbindfiring after, withthread-bindings.jsondropping from 2 → 1 bindings. I have not re-run a live end-to-end smoke test with this exact PR branch built from source; the unit-test and type/lint/build gates above are what this PR carries as fresh evidence.Human Verification (required)
What I personally verified for this PR branch:
pnpm vitest run src/auto-reply/reply/dispatch-acp-command-bypass.test.ts— 10/10 green.pnpm tsgo— 0 errors, 0 warnings across 11,435 files.pnpm check— all lanes clean.pnpm build— see CI.isAcpCommandCandidate):^\/acp(?:\s|$)/iaccepts/acp,/acp close,/ACP close; rejects/acpfoo,/acp-close.What was previously verified on a live deployment, as reported in #66298:
/acp closeinside a bound Discord thread reacheshandleAcpCommand→handleAcpCloseAction→acpManager.closeSession→sessionBindingService.unbind.thread-bindings.jsonbinding count drops by 1.dist/bundle on my own 2026.4.11 install, not with a from-source build of this PR branch.What I did not verify for this PR:
shouldBypassAcpDispatchForCommandpath so the behavior should be identical, but I do not have those integrations running and have not re-exercised them. Reviewers with those surfaces should smoke-test/acp closebefore cutting a release.CommandSource: "native"on a surface that actually exposes/acpthrough a plugin-registry native command UI — the new test covers the routing decision, but I did not run a real Discord slash-command autocomplete invocation against the from-source build.Review Conversations
Compatibility / Migration
Risks and Mitigations
/acp ...reaching the ACP session as raw user input (for example, an agent that treats/acp ...as a prompt template literal)./acpprefix is that it is a command, not content. There is no in-tree consumer that depends on receiving/acp ...as conversational input, and nothing in the ACP session grammar assigns meaning to a leading/acp. Any external agent that does rely on this would already have been broken by the Discord slash-command autocomplete surfacing/acpas a registered command.AI-Assisted PR
dist/-level patch on 2026.4.11 and is reported in [Bug]: /acp text commands inside a bound Discord thread get swallowed by the ACP LLM session instead of reaching handleAcpCommand #66298; I did not re-run that exact end-to-end smoke test against the from-source build of this PR branch.isAcpCommandCandidate) to mirror the existingisResetCommandCandidatestyle rather than inlining the regex in the bypass flow. The existing test"returns false for ACP slash commands"explicitly locked in the buggy behavior; this PR flips it and tags it to [Bug]: /acp text commands inside a bound Discord thread get swallowed by the ACP LLM session instead of reaching handleAcpCommand #66298.