Discord: reconcile native commands without restart churn#46597
Conversation
openclaw-barnacle
Bot
added
channel: discord
Greptile SummaryThis PR adds an opt-in Discord native command reconcile mode (
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/discord/src/monitor/native-command-state.ts
Line: 228-251
Comment:
**`leftAlone` counter is never incremented**
The `leftAlone` field is declared in `DiscordNativeCommandReconcileSummary` and is printed in both summary log lines, but it is initialized to `0` and never incremented anywhere in the function. In the current design every live command is either validated (matched by name) or scheduled for deletion — there are no commands that get "left alone" — so the counter will always be `0`. This makes the field appear in logs (`leftAlone=0`) without carrying any signal, which could confuse future readers.
Consider either removing `leftAlone` from the type and the two log strings, or adding a comment that clearly explains its intended purpose (e.g., for a future mode where commands from other applications are deliberately preserved).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/discord/src/monitor/provider.test.ts
Line: 84-95
Comment:
**Mock is missing the `validated` field from `DiscordNativeCommandReconcileSummary`**
The `summary` object in the initial mock return (and again in the `mockResolvedValue` call around line 457) omits the `validated: number` field that is required by `DiscordNativeCommandReconcileSummary`. If TypeScript's strict return-type inference tightens, or if any downstream assertion ever checks `summary.validated`, the mock will silently return `undefined` instead of a number.
The same omission appears a second time in the `mockResolvedValue` call:
`extensions/discord/src/monitor/provider.test.ts:457`
```suggestion
reconcileDiscordNativeCommandsMock: vi.fn(async () => ({
mode: "reconcile",
liveCount: 0,
savedCount: 0,
summary: {
validated: 0,
unchanged: 0,
created: 0,
updated: 0,
deleted: 0,
leftAlone: 0,
},
})),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/discord/src/monitor/native-command-state.ts
Line: 37-42
Comment:
**`savedCount` is always hardcoded to `0`**
Both return sites in `reconcileDiscordNativeCommands` set `savedCount: 0`. The field is present in `DiscordNativeCommandReconcileResult` and is referenced in the test mock, but it is never meaningfully populated — unlike the `summary` object, which carries the actual counts. If `savedCount` is genuinely unused in reconcile mode (e.g., it was carried over from the bulk deploy result shape), removing it from this return type would prevent confusion about what it is supposed to represent.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 979f499 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const summary: DiscordNativeCommandReconcileSummary = { | ||
| validated: 0, | ||
| unchanged: 0, | ||
| created: 0, | ||
| updated: 0, | ||
| deleted: 0, | ||
| leftAlone: 0, | ||
| }; | ||
|
|
||
| for (const desired of desiredCommands) { | ||
| const live = liveByName.get(desired.name); | ||
| if (live) { | ||
| if (live.signatureHash === desired.signatureHash) { | ||
| summary.validated += 1; | ||
| summary.unchanged += 1; | ||
| continue; | ||
| } | ||
| summary.validated += 1; | ||
| summary.updated += 1; | ||
| continue; | ||
| } | ||
|
|
||
| summary.created += 1; | ||
| } |
There was a problem hiding this comment.
leftAlone counter is never incremented
The leftAlone field is declared in DiscordNativeCommandReconcileSummary and is printed in both summary log lines, but it is initialized to 0 and never incremented anywhere in the function. In the current design every live command is either validated (matched by name) or scheduled for deletion — there are no commands that get "left alone" — so the counter will always be 0. This makes the field appear in logs (leftAlone=0) without carrying any signal, which could confuse future readers.
Consider either removing leftAlone from the type and the two log strings, or adding a comment that clearly explains its intended purpose (e.g., for a future mode where commands from other applications are deliberately preserved).
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/discord/src/monitor/native-command-state.ts
Line: 228-251
Comment:
**`leftAlone` counter is never incremented**
The `leftAlone` field is declared in `DiscordNativeCommandReconcileSummary` and is printed in both summary log lines, but it is initialized to `0` and never incremented anywhere in the function. In the current design every live command is either validated (matched by name) or scheduled for deletion — there are no commands that get "left alone" — so the counter will always be `0`. This makes the field appear in logs (`leftAlone=0`) without carrying any signal, which could confuse future readers.
Consider either removing `leftAlone` from the type and the two log strings, or adding a comment that clearly explains its intended purpose (e.g., for a future mode where commands from other applications are deliberately preserved).
How can I resolve this? If you propose a fix, please make it concise.| reconcileDiscordNativeCommandsMock: vi.fn(async () => ({ | ||
| mode: "reconcile", | ||
| liveCount: 0, | ||
| savedCount: 0, | ||
| summary: { | ||
| unchanged: 0, | ||
| created: 0, | ||
| updated: 0, | ||
| deleted: 0, | ||
| leftAlone: 0, | ||
| }, | ||
| })), |
There was a problem hiding this comment.
Mock is missing the validated field from DiscordNativeCommandReconcileSummary
The summary object in the initial mock return (and again in the mockResolvedValue call around line 457) omits the validated: number field that is required by DiscordNativeCommandReconcileSummary. If TypeScript's strict return-type inference tightens, or if any downstream assertion ever checks summary.validated, the mock will silently return undefined instead of a number.
The same omission appears a second time in the mockResolvedValue call:
extensions/discord/src/monitor/provider.test.ts:457
| reconcileDiscordNativeCommandsMock: vi.fn(async () => ({ | |
| mode: "reconcile", | |
| liveCount: 0, | |
| savedCount: 0, | |
| summary: { | |
| unchanged: 0, | |
| created: 0, | |
| updated: 0, | |
| deleted: 0, | |
| leftAlone: 0, | |
| }, | |
| })), | |
| reconcileDiscordNativeCommandsMock: vi.fn(async () => ({ | |
| mode: "reconcile", | |
| liveCount: 0, | |
| savedCount: 0, | |
| summary: { | |
| validated: 0, | |
| unchanged: 0, | |
| created: 0, | |
| updated: 0, | |
| deleted: 0, | |
| leftAlone: 0, | |
| }, | |
| })), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/discord/src/monitor/provider.test.ts
Line: 84-95
Comment:
**Mock is missing the `validated` field from `DiscordNativeCommandReconcileSummary`**
The `summary` object in the initial mock return (and again in the `mockResolvedValue` call around line 457) omits the `validated: number` field that is required by `DiscordNativeCommandReconcileSummary`. If TypeScript's strict return-type inference tightens, or if any downstream assertion ever checks `summary.validated`, the mock will silently return `undefined` instead of a number.
The same omission appears a second time in the `mockResolvedValue` call:
`extensions/discord/src/monitor/provider.test.ts:457`
```suggestion
reconcileDiscordNativeCommandsMock: vi.fn(async () => ({
mode: "reconcile",
liveCount: 0,
savedCount: 0,
summary: {
validated: 0,
unchanged: 0,
created: 0,
updated: 0,
deleted: 0,
leftAlone: 0,
},
})),
```
How can I resolve this? If you propose a fix, please make it concise.| export type DiscordNativeCommandReconcileResult = { | ||
| mode: "reconcile"; | ||
| liveCount: number; | ||
| savedCount: number; | ||
| summary: DiscordNativeCommandReconcileSummary; | ||
| }; |
There was a problem hiding this comment.
savedCount is always hardcoded to 0
Both return sites in reconcileDiscordNativeCommands set savedCount: 0. The field is present in DiscordNativeCommandReconcileResult and is referenced in the test mock, but it is never meaningfully populated — unlike the summary object, which carries the actual counts. If savedCount is genuinely unused in reconcile mode (e.g., it was carried over from the bulk deploy result shape), removing it from this return type would prevent confusion about what it is supposed to represent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/discord/src/monitor/native-command-state.ts
Line: 37-42
Comment:
**`savedCount` is always hardcoded to `0`**
Both return sites in `reconcileDiscordNativeCommands` set `savedCount: 0`. The field is present in `DiscordNativeCommandReconcileResult` and is referenced in the test mock, but it is never meaningfully populated — unlike the `summary` object, which carries the actual counts. If `savedCount` is genuinely unused in reconcile mode (e.g., it was carried over from the bulk deploy result shape), removing it from this return type would prevent confusion about what it is supposed to represent.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 979f499917
ℹ️ 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".
| for (const live of deletedLiveCommands) { | ||
| await params.client.rest.delete(Routes.applicationCommand(params.applicationId, live.id)); | ||
| } |
There was a problem hiding this comment.
reconcileDiscordNativeCommands deletes stale commands first (extensions/discord/src/monitor/native-command-state.ts:294-296) and only creates missing commands later, so a Discord 30034 create-limit error during the create loop can leave the app with commands removed but not replaced. Because runDiscordNativeCommandSyncOperation treats that error as a skipped sync (extensions/discord/src/monitor/provider.ts:310-317) rather than rolling back, this can silently degrade command availability for the rest of the quota window whenever reconcile needs at least one create.
Useful? React with 👍 / 👎.
| const liveByName = new Map(liveCommands.map((command) => [command.name, command])); | ||
| const deletedLiveCommands = liveCommands.filter((command) => !desiredByName.has(command.name)); |
There was a problem hiding this comment.
The reconcile diff keys live/desired commands by lowercase name only (liveByName / deletedLiveCommands), but Routes.applicationCommands returns all command types for the application. In environments that also register non-chat-input commands (user/message context commands), this logic will classify them as extras and delete them, or match the wrong command when names collide across types. Filter to chat-input commands (or include type in the identity key) before diffing.
Useful? React with 👍 / 👎.
openclaw-barnacle
Bot
added
docs
31995c9 to
02d50ae
Compare
openclaw-barnacle
Bot
removed
docs
02d50ae to
62d7184
Compare
|
Talked to @steipete - He suggested to ship this without the config option - just make the default. Hopefully we can get this into Carbon soon and then remove our detailed functionality. @thewilloftheshadow - thoughts? |
|
Sounds good to me |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36226959a2
ℹ️ 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 liveByName = new Map(liveCommands.map((command) => [command.name, command])); | ||
| const deletedLiveCommands = liveCommands.filter((command) => !desiredByName.has(command.name)); | ||
| const updatedCommands = desiredCommands.flatMap((desired) => { |
There was a problem hiding this comment.
The reconcile diff currently keys both live and desired commands only by lowercase name, so any non-chat-input application command returned by Routes.applicationCommands(...) can be treated as an extra and deleted, or incorrectly matched when names collide across command types. This affects bots that also register user/message context commands (or other non-chat command types) under the same application, because the map/indexing logic at this point does not preserve type as part of identity.
Useful? React with 👍 / 👎.
| for (const live of deletedLiveCommands) { | ||
| await params.client.rest.delete(Routes.applicationCommand(params.applicationId, live.id)); | ||
| } |
There was a problem hiding this comment.
Reconcile deletes stale commands before it attempts any creates, so if a later create hits Discord's daily create limit (30034) or another create-time failure, command availability is reduced and not restored in that run. Fresh evidence in this commit is that the delete loop executes first here, while runDiscordNativeCommandSyncOperation in extensions/discord/src/monitor/provider.ts treats 30034 as a skipped sync instead of rolling back, which can leave previously working commands removed for the remainder of the quota window.
Useful? React with 👍 / 👎.
b4903d6 to
028eb10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 028eb10e9c
ℹ️ 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".
| @@ -615,7 +615,7 @@ | |||
| "dependencies": { | |||
| "@agentclientprotocol/sdk": "0.16.1", | |||
| "@aws-sdk/client-bedrock": "^3.1009.0", | |||
| "@buape/carbon": "0.0.0-beta-20260216184201", | |||
| "@buape/carbon": "0.0.0-beta-20260317045421", | |||
There was a problem hiding this comment.
This change upgrades @buape/carbon, but the repository guideline in AGENTS.md explicitly states "Never update the Carbon dependency." Because this project treats Carbon changes as a restricted surface, updating it here introduces externally owned Discord behavior changes without the required explicit approval path and should be reverted (or moved to an approved, owner-driven update).
Useful? React with 👍 / 👎.
028eb10 to
10cf07e
Compare
|
Merged via squash.
Thanks @huntharo! |
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
) Merged via squash. Prepared head SHA: 37090da Co-authored-by: huntharo <[email protected]> Reviewed-by: @huntharo
2 participants
Summary
Describe the problem and fix in 2–5 bullets:
@buape/carbonto the beta that adds built-in reconcile, removed the local OpenClaw reconcile implementation, and switched the Discord provider to configure Carbon withcommandDeploymentMode: "reconcile".Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
commandDeploymentMode: "reconcile".native-command-statereconcile module and its tests are gone.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
pnpm -w install @buape/carbon@betaExpected
Actual
reconcileGlobalCommands()implementation.commandDeploymentMode: "reconcile"on the Carbon client and uses the normal Carbon deploy path.Evidence
Discord’s docs and maintainer guidance still justify why reconcile is the right deployment mode:
Source: https://docs.discord.com/developers/interactions/application-commands#making-a-global-command
Source: discord/discord-api-docs#4868 (comment)
The important change in this revision is ownership, not the end behavior:
extensions/discord/src/monitor/native-command-state.tsHuman Verification (required)
What you personally verified (not just CI), and how:
@buape/carbon@beta.Client.reconcileGlobalCommands()andcommandDeploymentMode: "reconcile"in its built output./vccommand is still included in the constructed Discord command listReview Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) Nopnpm installFailure Recovery (if this breaks)
package.jsonpnpm-lock.yamlextensions/discord/src/monitor/provider.tsRisks and Mitigations