Skip to content

fix(admin): automod-bypass reports success when a Discord role change fails - #26

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/automod-bypass-role-errors
Open

fix(admin): automod-bypass reports success when a Discord role change fails#26
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/automod-bypass-role-errors

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where Discord admins running /admin automod-bypass-toggle would be told the automod bypass role was added or removed even when Discord rejected the role change (missing permissions, role hierarchy, or API error). The member's roles stayed the same while the command reported a new security state.

That false success is worse than a failed command: staff can believe someone is exempt from automod when they are not, or believe a bypass was revoked when it is still attached.

Why This Change Was Made

/admin automod-bypass-toggle now waits for addRole / removeRole to finish. If Discord rejects the change, the command replies with an ephemeral error and does not claim success. The previous success copy is sent only after the role update resolves. /admin inactivity-warn is unchanged.

User Impact

Admins get an accurate private reply about whether automod bypass was actually granted or removed. A Discord API failure no longer looks like a completed security change.

Evidence

Live bun run of /tmp/proof-hermit-automod.ts against this branch. The script calls the previous fire-and-forget handler and the patched AutomodBypassToggle.run with a Discord member whose addRole / removeRole throws Missing Permissions, then prints the command replies.

$ bun /tmp/proof-hermit-automod.ts
BEFORE addRole reject
[
  {
    "content": "Added automod bypass role to <@user-123>.",
    "ephemeral": true
  }
]
BEFORE removeRole reject
[
  {
    "content": "Removed automod bypass role from <@user-123>.",
    "ephemeral": true
  }
]
AFTER addRole reject
[
  {
    "content": "Failed to add automod bypass role.",
    "ephemeral": true
  }
]
AFTER removeRole reject
[
  {
    "content": "Failed to remove automod bypass role.",
    "ephemeral": true
  }
]
AFTER addRole ok
[
  {
    "content": "Added automod bypass role to <@user-123>.",
    "ephemeral": true
  }
]
AFTER removeRole ok
[
  {
    "content": "Removed automod bypass role from <@user-123>.",
    "ephemeral": true
  }
]

Before this patch the command always printed the success sentence. After the patch a rejected role update prints Failed to add/remove automod bypass role. and the success sentence is reserved for a resolved role update.

Real behavior proof

  • Behavior or issue addressed: /admin automod-bypass-toggle reported that automod bypass was added or removed even when Discord rejected the role change, so staff could act on a false security state.

  • Real environment tested: macOS 26.6.1 Darwin 25.6.0 arm64, Node v26.7.0, bun 1.3.14, patched checkout /tmp/oc-impl-hermit-automod on fix/automod-bypass-role-errors (base 97d3731).

  • Exact steps or command run after this patch:

    bun /tmp/proof-hermit-automod.ts
  • Evidence after fix: terminal output from the patched command class:

    $ bun /tmp/proof-hermit-automod.ts
    AFTER addRole reject
    [
      {
        "content": "Failed to add automod bypass role.",
        "ephemeral": true
      }
    ]
    AFTER removeRole reject
    [
      {
        "content": "Failed to remove automod bypass role.",
        "ephemeral": true
      }
    ]
    AFTER addRole ok
    [
      {
        "content": "Added automod bypass role to <@user-123>.",
        "ephemeral": true
      }
    ]
    AFTER removeRole ok
    [
      {
        "content": "Removed automod bypass role from <@user-123>.",
        "ephemeral": true
      }
    ]
  • Observed result after fix: A rejected addRole / removeRole now replies Failed to add automod bypass role. or Failed to remove automod bypass role. (ephemeral). The success sentence is printed only after the role update resolves.

  • What was not tested: A live Discord guild with a real role hierarchy or missing-permissions token. The inactivity-warn thread member path is unchanged.

Origin

This behavior landed in 3e71d04 (admin and team lead commands, 2026-04-07) and has been present for 130 days. user.addRole(...).catch(() => {}) / user.removeRole(...).catch(() => {}) discarded the Discord error, then the command still replied with the success sentence.

Sibling /role already awaits addRole / removeRole before replying. Beta Pings already replies privately when the Discord role update throws.

Await addRole/removeRole on /admin automod-bypass-toggle. Reply with an
ephemeral error when Discord rejects the role change, and only report
success after the role update resolves.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 21, 2026, 6:59 PM ET / 22:59 UTC.

ClawSweeper review

What this changes

The PR awaits Discord automod-bypass role changes, shows an ephemeral failure on rejection, and adds regression tests for both outcomes.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked until real behavior proof from a real setup is added - 4 items remain

Current main still reports success before a rejected Discord role update settles, so this PR remains necessary. The core fix is sound, but unchanged P2 policy violations and mock-only proof block merge readiness.

Priority: P2
Reviewed head: 8e708e2a6c251d05af2e161d3f4fb8c5c1280f1d

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The core repair is focused and regression-tested, but unresolved policy defects and mock-only proof prevent merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The terminal transcript exercises injected member-method failures, not a real Discord client and guild role mutation; add redacted after-fix live evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The terminal transcript exercises injected member-method failures, not a real Discord client and guild role mutation; add redacted after-fix live evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current main remains affected: Current main discards role-update failures and immediately sends success, so rejected Discord mutations can still produce a false acknowledgement.
Patch addresses the source defect: The PR awaits addRole/removeRole, returns after a rejected mutation, and sends success only after the awaited operation resolves.
Repository response policy: The root policy requires Carbon v2 components instead of raw content and keeps automod response templates in the JSON configuration; the branch adds raw hard-coded failure replies.
Findings 2 actionable findings [P2] Use Carbon v2 components for failure replies
[P2] Move automod failure copy into response configuration
Security None None.

Live Verification

Command: bun test tests/adminAutomodBypass.test.ts

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

Assertions:

  • FAIL expect_output: does not claim success when addRole is rejected

How this fits together

The Discord admin command changes a member’s automod-bypass role and then privately reports the outcome to the administrator. The role controls whether automod applies to that member, so the acknowledgement must match the completed mutation.

flowchart LR
A[Discord administrator] --> B[Admin command]
B --> C[Member role update]
C --> D{Discord accepted update?}
D -->|Yes| E[Private success reply]
D -->|No| F[Private failure reply]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The terminal transcript exercises injected member-method failures, not a real Discord client and guild role mutation; add redacted after-fix live evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Use Carbon v2 components for failure replies (P2) - The new catch branch replies with a raw content payload, contrary to the repository’s required Carbon v2 response format. Preserve the ephemeral failure behavior through a Container and TextDisplay instead; the same issue recurs in the add-role branch.
  • Move automod failure copy into response configuration (P2) - The new failure text is hard-coded in the command although repository policy makes src/config/automod-messages.json the automod response-template source. Centralize this string there and use it from the command; the add-role branch duplicates the same problem.
  • Complete next step (P2) - The two policy violations are narrow mechanical repairs on the PR branch; the contributor must separately supply real behavior proof.

Findings

  • [P2] Use Carbon v2 components for failure replies — src/commands/admin.ts:171-174
  • [P2] Move automod failure copy into response configuration — src/commands/admin.ts:172
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 files affected; production +18/-2, tests +109 The role-handling change is focused and has dedicated regression coverage.

Technical review

Best possible solution:

Preserve the awaited mutation flow, render both failure paths with Carbon v2 components backed by configured automod copy, and add redacted proof from a real Discord role operation.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main visibly discards the rejected role-update promise before sending success, and the PR’s focused tests encode that rejected-operation path.

Is this the best way to solve the issue?

No. Awaiting the Discord mutation is the right repair, but the new failure replies must use Carbon v2 components and configuration-driven automod copy.

Full review comments:

  • [P2] Use Carbon v2 components for failure replies — src/commands/admin.ts:171-174
    The new catch branch replies with a raw content payload, contrary to the repository’s required Carbon v2 response format. Preserve the ephemeral failure behavior through a Container and TextDisplay instead; the same issue recurs in the add-role branch.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.
  • [P2] Move automod failure copy into response configuration — src/commands/admin.ts:172
    The new failure text is hard-coded in the command although repository policy makes src/config/automod-messages.json the automod response-template source. Centralize this string there and use it from the command; the add-role branch duplicates the same problem.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 17a621b47dba.

Labels

Label justifications:

  • P2: This fixes inaccurate administrator feedback for an automod-exemption role change.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The terminal transcript exercises injected member-method failures, not a real Discord client and guild role mutation; add redacted after-fix live evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] bun test tests/adminAutomodBypass.test.ts.

What I checked:

  • Current main remains affected: Current main discards role-update failures and immediately sends success, so rejected Discord mutations can still produce a false acknowledgement. (src/commands/admin.ts:167, 17a621b47dba)
  • Patch addresses the source defect: The PR awaits addRole/removeRole, returns after a rejected mutation, and sends success only after the awaited operation resolves. (src/commands/admin.ts:168, 8e708e2a6c25)
  • Repository response policy: The root policy requires Carbon v2 components instead of raw content and keeps automod response templates in the JSON configuration; the branch adds raw hard-coded failure replies. (AGENTS.md:3, 17a621b47dba)
  • Existing component failure convention: The current Beta Pings role-update error path uses a configured message in a Carbon Container/TextDisplay reply, providing a local pattern for this repair. (src/components/betaPingsButton.ts:66, 17a621b47dba)
  • Feature-history lead: Reachable commit metadata identifies the original admin-command commit as 3e71d04, authored by Shadow; the PR body also traces this behavior to that commit. (src/commands/admin.ts:167, 3e71d04eaf85)
  • Prior findings remain applicable: The current PR head is the same SHA reviewed previously, so both policy findings target unchanged code. (src/commands/admin.ts:171, 8e708e2a6c25)

Likely related people:

  • Shadow: Reachable metadata identifies Shadow as author of the original admin-command commit cited for this behavior. (role: introduced behavior; confidence: medium; commits: 3e71d04eaf85; files: src/commands/admin.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Replace raw failure replies with Carbon v2 components and configured automod response copy.
  • Add redacted after-fix evidence from a real Discord guild role mutation or rejected mutation.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-15T21:16:51.794Z sha 8e708e2 :: needs real behavior proof before merge. :: [P2] Use Carbon v2 components for the failure replies
  • reviewed 2026-08-21T14:07:52.438Z sha 8e708e2 :: needs real behavior proof before merge. :: [P2] Use Carbon v2 components for the failure replies | [P2] Move automod failure copy into the response config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant