Skip to content

fix(admin): surface inactivity-warn thread addMember failures - #27

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/inactivity-warn-addmember-errors
Open

fix(admin): surface inactivity-warn thread addMember failures#27
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/inactivity-warn-addmember-errors

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where Discord admins running /admin inactivity-warn would be told the private warning thread was created as a complete success even when Discord rejected addMember (missing permissions, thread membership API error). The thread and warning message still exist, but the staffer who ran the command is not in the private thread.

That false success is worse than a failed command: staff think they can see and follow the warning thread when they cannot.

Why This Change Was Made

/admin inactivity-warn now waits for thread.addMember to finish. The private thread and warning message are still created when membership fails. The ephemeral reply then reports that partial result (Failed to add you to the thread) instead of the unqualified created-thread sentence. /admin automod-bypass-toggle is unchanged.

User Impact

Admins get an accurate private reply about whether they were added to the inactivity warning thread. A Discord API failure no longer looks like a completed staff follow-up.

Evidence

Live bun run of /tmp/proof-hermit-addmember.ts against this branch. The script calls the previous fire-and-forget handler and the patched InactivityWarn.run with a thread whose addMember throws Missing Permissions, then prints the command replies.

$ bun /tmp/proof-hermit-addmember.ts
BEFORE addMember reject
[
  {
    "content": "Created inactivity warning thread for <@user-123> in <#1477357508833185954>.",
    "ephemeral": true
  }
]
AFTER addMember reject
[
  {
    "content": "Created inactivity warning thread for <@user-123> in <#1477357508833185954>. Failed to add you to the thread.",
    "ephemeral": true
  }
]
AFTER addMember ok
[
  {
    "content": "Created inactivity warning thread for <@user-123> in <#1477357508833185954>.",
    "ephemeral": true
  }
]

Before this patch the command always printed the unqualified created-thread sentence. After the patch a rejected addMember still creates the thread, then prints Failed to add you to the thread. The unqualified sentence is reserved for a resolved membership add.

Real behavior proof

  • Behavior or issue addressed: /admin inactivity-warn reported that the inactivity warning thread was created even when Discord rejected adding the staffer to that private thread, so staff could think they were in the thread when they were not.

  • 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-addmember on fix/inactivity-warn-addmember-errors (base 97d3731).

  • Exact steps or command run after this patch:

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

    $ bun /tmp/proof-hermit-addmember.ts
    AFTER addMember reject
    [
      {
        "content": "Created inactivity warning thread for <@user-123> in <#1477357508833185954>. Failed to add you to the thread.",
        "ephemeral": true
      }
    ]
    AFTER addMember ok
    [
      {
        "content": "Created inactivity warning thread for <@user-123> in <#1477357508833185954>.",
        "ephemeral": true
      }
    ]
  • Observed result after fix: A rejected addMember now replies that the thread was created and Failed to add you to the thread (ephemeral). The unqualified created-thread sentence is printed only after membership add resolves.

  • What was not tested: A live Discord guild with a real private-thread permission error. The automod-bypass role path is unchanged.

Origin

The silent thread.addMember(actorId).catch(() => { }) landed in ae6ce064 (Remove legacy review locks and Shadow references, 2026-07-25) and has been present for 21 days. The discarded Discord error was followed by the unqualified created-thread reply.

Await thread.addMember for /admin inactivity-warn. When Discord rejects
the add, still create the thread and send the warning, but reply with
the addMember failure instead of claiming unqualified success.

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 28, 2026, 1:39 AM ET / 05:39 UTC.

ClawSweeper review

What this changes

The PR waits for private-thread membership in /admin inactivity-warn and reports a partial success when Discord rejects adding the invoking administrator.

Merge readiness

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

This PR remains necessary: current main still swallows a rejected private-thread membership request and reports unqualified success. It needs the repository-required Carbon v2 response and real Discord-boundary proof before merge.

Priority: P2
Reviewed head: 5eb678afe3f07dd1742ab7e5551d21ea2e1497ab

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The provided terminal transcript uses fake Discord objects and explicitly excludes a real Discord permission failure; add redacted after-fix proof through the real Discord boundary 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 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The provided terminal transcript uses fake Discord objects and explicitly excludes a real Discord permission failure; add redacted after-fix proof through the real Discord boundary 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 still has the defect: The fetched default-branch revision suppresses a rejected add-member request and always sends the unqualified created-thread reply.
Introduced fix is focused: The PR replaces the swallowed rejection with an awaited attempt and a partial-result branch, and adds two focused command tests.
Carbon policy applies to the new reply: Repository policy requires Carbon v2 components instead of interaction reply content; the newly introduced partial-result reply still uses content.
Findings 1 actionable finding [P3] Render the partial-result reply with Carbon v2 components
Security None None.

Live Verification

Command: bun test tests/adminInactivityWarn.test.ts

Result: FAIL (failed) — execution before step 1 expect_output: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz | [ERR_PNPM_NO_LOCKFILE] Cannot install with "frozen-lockfile" because pnpm-lock.yaml is absent Note that in CI environments this setting is true by default. If you still need to run install in such cases, use "pnpm install --no-frozen-lockfile"

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz | [ERR_PNPM_NO_LOCKFILE] Cannot install with "frozen-lockfile" because pnpm-lock.yaml is absent Note that in CI environments this setting is true by default. If you still need to run install in such cases, use "pnpm install --no-frozen-lockfile"

Assertions:

  • FAIL expect_output: 2 pass

How this fits together

The admin command creates a private Discord warning thread, tries to add the invoking staff member, posts the warning, and returns an ephemeral result. The result determines whether staff know they can follow the new private thread.

flowchart LR
A[Admin runs inactivity warning] --> B[Create private warning thread]
B --> C[Add invoking staff member]
C --> D[Post warning message]
D --> E[Send ephemeral result]
C --> F[Membership rejected]
F --> E
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The provided terminal transcript uses fake Discord objects and explicitly excludes a real Discord permission failure; add redacted after-fix proof through the real Discord boundary 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.
  • Render the partial-result reply with Carbon v2 components (P3) - The introduced branch sends a raw content reply, but repository policy requires TextDisplay/Container instead of content. Convert this reply to the established component response pattern and update its focused test.
  • Resolve merge risk (P1) - The supplied proof does not exercise a real Discord private-thread membership rejection, so Discord API behavior remains unverified before merge.
  • Complete next step (P2) - The Carbon v2 conversion is a narrow mechanical repair; real Discord-boundary proof remains a contributor-owned merge requirement.

Findings

  • [P3] Render the partial-result reply with Carbon v2 components — src/commands/admin.ts:136-139
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +14, -1; tests +95 The functional change is small and accompanied by focused mocked coverage, but the added tests do not establish real Discord behavior.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Convert the introduced partial-result response to Carbon v2 components and attach redacted after-fix proof from a real Discord private-thread membership failure while preserving thread creation and warning delivery.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Convert the introduced partial-result response to Carbon v2 components and attach redacted after-fix proof from a real Discord private-thread membership failure while preserving thread creation and warning delivery.

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

Yes in source: if Discord rejects addMember, current main swallows that rejection and then reports success. No high-confidence live Discord reproduction was supplied.

Is this the best way to solve the issue?

No; awaiting the membership call is the narrow fix, but the introduced result must use the repository’s Carbon v2 response convention and needs real Discord-boundary evidence.

Full review comments:

  • [P3] Render the partial-result reply with Carbon v2 components — src/commands/admin.ts:136-139
    The introduced branch sends a raw content reply, but repository policy requires TextDisplay/Container instead of content. Convert this reply to the established component response pattern and update its focused test.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add status: needs maintainer proof decision: A ClawSweeper-authored PR needs a maintainer proof capture or override decision. Needs real behavior proof before merge: The provided terminal transcript uses fake Discord objects and explicitly excludes a real Discord permission failure; add redacted after-fix proof through the real Discord boundary 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.
  • remove status: 📣 needs proof: Current PR status label is status: needs maintainer proof decision.

Label justifications:

  • P2: This corrects an administrator-facing command result with limited operational blast radius.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: needs maintainer proof decision: A ClawSweeper-authored PR needs a maintainer proof capture or override decision. Needs real behavior proof before merge: The provided terminal transcript uses fake Discord objects and explicitly excludes a real Discord permission failure; add redacted after-fix proof through the real Discord boundary 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/adminInactivityWarn.test.ts.

What I checked:

  • Current main still has the defect: The fetched default-branch revision suppresses a rejected add-member request and always sends the unqualified created-thread reply. (src/commands/admin.ts:111, e360e25a5a75)
  • Introduced fix is focused: The PR replaces the swallowed rejection with an awaited attempt and a partial-result branch, and adds two focused command tests. (src/commands/admin.ts:135, 5eb678afe3f0)
  • Carbon policy applies to the new reply: Repository policy requires Carbon v2 components instead of interaction reply content; the newly introduced partial-result reply still uses content. (AGENTS.md:3, 5eb678afe3f0)
  • Existing component convention: Current command and component paths use Container and TextDisplay in interaction replies, providing an established conversion pattern. (src/commands/maintainer.ts:30, 5eb678afe3f0)
  • Proof remains mock-only: The PR body’s terminal transcript drives fake thread and interaction objects and explicitly says a live Discord private-thread permission failure was not tested.
  • Feature-history routing: The available local history identifies Hannes Rudolph's ae6ce06 commit as the relevant earlier command-area change; the PR body also identifies that commit as introducing the swallowed rejection. (src/commands/admin.ts:111, ae6ce064361e)

Likely related people:

  • Hannes Rudolph: Available history and the PR's cited origin connect ae6ce06 to the earlier add-member handling in the admin command. (role: introduced related command behavior; confidence: medium; commits: ae6ce064361e; files: src/commands/admin.ts)

Rank-up moves

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

  • Add after-fix proof from a real setup, such as a short recording, terminal output, linked artifact, or redacted logs.

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 (5 earlier review cycles)
  • reviewed 2026-08-15T22:41:52.360Z sha 5eb678a :: needs real behavior proof before merge. :: [P3] Use Carbon components for the partial-success reply
  • reviewed 2026-08-21T20:00:03.254Z sha 5eb678a :: needs real behavior proof before merge. :: [P3] Render the partial result with Carbon v2 components
  • reviewed 2026-08-22T03:11:51.128Z sha 5eb678a :: needs real behavior proof before merge. :: [P3] Use Carbon v2 components for the partial-result reply
  • reviewed 2026-08-22T15:54:18.329Z sha 5eb678a :: needs real behavior proof before merge. :: [P3] Render the partial-result reply with Carbon v2 components
  • reviewed 2026-08-22T22:06:45.132Z sha 5eb678a :: needs real behavior proof before merge. :: [P3] Render the partial-result reply with Carbon v2 components

@clawsweeper clawsweeper Bot added status: needs maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 28, 2026
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 maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant