fix(discord): make /acp action use autocomplete#32762
fix(discord): make /acp action use autocomplete#32762sxu75374 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Use autocomplete for /acp action in Discord so inline values like '/acp close' are preserved instead of forcing the fallback picker menu. Signed-off-by: sxu75374 <[email protected]>
openclaw-barnacle
bot
added
channel: discord
Greptile SummaryThis PR fixes issue #32753 by forcing the Changes:
Confidence Score: 4/5
Last reviewed commit: 110ad77 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| (typeof arg.choices === "function" || | ||
| resolvedChoices.length > 25 || | ||
| (command.key === "acp" && arg.name === "action")); |
There was a problem hiding this comment.
Hardcoded command/arg identity in generic builder
This third condition couples a generic Discord option-building function to a specific command name ("acp") and argument name ("action"). If more commands need the same forced-autocomplete behavior in the future, you'd have to keep extending this condition with additional || clauses.
A cleaner, extensible approach would be to add a forceAutocomplete?: boolean field to CommandArgDefinition and set it on the action arg in the acp definition in commands-registry.data.ts. The condition here would then simply read:
const shouldAutocomplete =
resolvedChoices.length > 0 &&
(typeof arg.choices === "function" ||
resolvedChoices.length > 25 ||
arg.forceAutocomplete === true);And in commands-registry.data.ts:
{
name: "action",
description: "Action to run",
type: "string",
forceAutocomplete: true,
choices: [ "spawn", "cancel", ... ],
},This keeps the builder generic and moves the "force autocomplete" intent to where the command is defined.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/discord/monitor/native-command.ts
Line: 121-123
Comment:
**Hardcoded command/arg identity in generic builder**
This third condition couples a generic Discord option-building function to a specific command name (`"acp"`) and argument name (`"action"`). If more commands need the same forced-autocomplete behavior in the future, you'd have to keep extending this condition with additional `||` clauses.
A cleaner, extensible approach would be to add a `forceAutocomplete?: boolean` field to `CommandArgDefinition` and set it on the `action` arg in the `acp` definition in `commands-registry.data.ts`. The condition here would then simply read:
```ts
const shouldAutocomplete =
resolvedChoices.length > 0 &&
(typeof arg.choices === "function" ||
resolvedChoices.length > 25 ||
arg.forceAutocomplete === true);
```
And in `commands-registry.data.ts`:
```ts
{
name: "action",
description: "Action to run",
type: "string",
forceAutocomplete: true,
choices: [ "spawn", "cancel", ... ],
},
```
This keeps the builder generic and moves the "force autocomplete" intent to where the command is defined.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| it("registers /acp action with autocomplete instead of static choices", () => { | ||
| const cfg = { | ||
| commands: { native: true }, | ||
| channels: { | ||
| discord: { dm: { enabled: true, policy: "open" } }, | ||
| }, | ||
| agents: { defaults: { workspace: "/tmp/openclaw" } }, | ||
| session: { store: "/tmp/openclaw-sessions.json" }, | ||
| } as ReturnType<typeof import("../config/config.js").loadConfig>; | ||
|
|
||
| const acpSpec = listNativeCommandSpecsForConfig(cfg, { provider: "discord" }).find( | ||
| (spec) => spec.name === "acp", | ||
| ); | ||
| expect(acpSpec).toBeTruthy(); | ||
|
|
||
| const command = createDiscordNativeCommand({ | ||
| command: acpSpec!, | ||
| cfg, | ||
| discordConfig: cfg.channels!.discord!, | ||
| accountId: "default", | ||
| sessionPrefix: "discord:slash", | ||
| ephemeralDefault: true, | ||
| threadBindings: createNoopThreadBindingManager("default"), | ||
| }); | ||
|
|
||
| const actionOption = (command.options ?? []).find((option) => option.name === "action"); | ||
| expect(actionOption).toBeTruthy(); | ||
| expect(actionOption?.autocomplete).toBeTypeOf("function"); | ||
| expect(actionOption?.choices).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Test placed in unrelated test file
This test verifies that /acp registers its action option with autocomplete rather than static choices — that is a command registration / option-building concern. The host file's name (monitor.tool-result.accepts-guild-messages-mentionpatterns-match.test.ts) describes a completely different behavior (tool-result dispatching and mention-pattern matching), and reading the surrounding tests confirms that mismatch.
The PR description even points to src/auto-reply/commands-registry.test.ts as the place where acp-related command tests live (there is already a "keeps ACP native action choices aligned with implemented handlers" test there). Placing this test alongside the existing ACP registry tests would improve discoverability and keep the file responsibilities well-scoped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/discord/monitor.tool-result.accepts-guild-messages-mentionpatterns-match.test.ts
Line: 350-379
Comment:
**Test placed in unrelated test file**
This test verifies that `/acp` registers its `action` option with autocomplete rather than static choices — that is a command registration / option-building concern. The host file's name (`monitor.tool-result.accepts-guild-messages-mentionpatterns-match.test.ts`) describes a completely different behavior (tool-result dispatching and mention-pattern matching), and reading the surrounding tests confirms that mismatch.
The PR description even points to `src/auto-reply/commands-registry.test.ts` as the place where `acp`-related command tests live (there is already a `"keeps ACP native action choices aligned with implemented handlers"` test there). Placing this test alongside the existing ACP registry tests would improve discoverability and keep the file responsibilities well-scoped.
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #33136, which consolidates the /acp inline action autocomplete fix plus the bound-thread bot system message guard. Closing in favor of that PR. |
2 participants
Summary\n- force arg to register with in Discord command options\n- keep static choices disabled for this arg so inline values (e.g. ) are preserved\n- add a Discord command registration test to verify uses autocomplete and no static choices\n\n## Testing\n- pnpm exec vitest run --config vitest.unit.config.ts src/discord/monitor.tool-result.accepts-guild-messages-mentionpatterns-match.test.ts\n- pnpm exec vitest run --config vitest.unit.config.ts src/auto-reply/commands-registry.test.ts\n\nFixes #32753