Skip to content

fix(discord): release PluralKit 404 bodies#109960

Merged
steipete merged 1 commit into
openclaw:mainfrom
RileyJJY:jjy/fix/pluralkit-404-body
Jul 21, 2026
Merged

fix(discord): release PluralKit 404 bodies#109960
steipete merged 1 commit into
openclaw:mainfrom
RileyJJY:jjy/fix/pluralkit-404-body

Conversation

@RileyJJY

@RileyJJY RileyJJY commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where Discord PluralKit lookups could leave a 404 response body open when a webhook message had no PluralKit record.

Why This Change Was Made

The missing-message path still returns null, but it now cancels any response body before leaving the PluralKit lookup. The existing bounded error-body path is unchanged.

User Impact

Discord users with PluralKit enabled keep the same missing-message behavior while the plugin releases upstream response streams promptly.

Evidence

  • Production module boundary: ./node_modules/.bin/tsx proof-pluralkit-404.ts imported the real PluralKit lookup module, exercised a 404 response with a stream body, and used a 200 JSON negative-control response to verify normal parsing still succeeds.
  • node scripts/run-vitest.mjs extensions/discord/src/pluralkit.test.ts -- -t "returns null on 404|bounds PluralKit API error bodies"
  • node scripts/run-vitest.mjs extensions/discord/src/pluralkit.test.ts
$ ./node_modules/.bin/tsx proof-pluralkit-404.ts
missing-result=null
missing-body-canceled=true
found-member=member-1
proof-pluralkit-404.ts 
import { fetchPluralKitMessageInfo } from "./extensions/discord/src/pluralkit.ts";

function cancelTrackedResponse(text: string, init: ResponseInit) {
  let canceled = false;
  const stream = new ReadableStream<Uint8Array>({
    start(controller) {
      controller.enqueue(new TextEncoder().encode(text));
    },
    cancel() {
      canceled = true;
    },
  });
  return {
    response: new Response(stream, init),
    wasCanceled: () => canceled,
  };
}

const missing = cancelTrackedResponse("missing", { status: 404 });
const missingResult = await fetchPluralKitMessageInfo({
  messageId: "missing-message",
  config: { enabled: true },
  fetcher: async () => missing.response,
});
console.log(`missing-result=${missingResult === null ? "null" : "unexpected"}`);
console.log(`missing-body-canceled=${missing.wasCanceled()}`);

const found = await fetchPluralKitMessageInfo({
  messageId: "found-message",
  config: { enabled: true },
  fetcher: async () =>
    new Response(
      JSON.stringify({
        id: "found-message",
        member: { id: "member-1", name: "Member" },
        system: { id: "system-1", name: "System" },
      }),
      {
        status: 200,
        headers: { "content-type": "application/json" },
      },
    ),
});
console.log(`found-member=${found?.member?.id ?? "missing"}`);

AI-assisted: built with Codex

@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: XS labels Jul 17, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 17, 2026
@clawsweeper

clawsweeper Bot commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 2:54 AM ET / 06:54 UTC.

Summary
The PR cancels a Discord PluralKit HTTP response body before returning null for a 404 lookup and extends the existing 404 regression test to assert cancellation.

PR surface: Source +1, Tests +2. Total +3 across 2 files.

Reproducibility: no. failing current-main run was executed in this review, but the path is source-reproducible: the 404 branch returns before consuming or cancelling Response.body, and the PR’s production-module terminal proof verifies the corrected streamed-response behavior.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] The branch is behind current main and several exact-head CI jobs were still in progress; refresh and review the final mergeable head before landing.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the existing missing-message contract, land the guarded 404-body cancellation with its focused regression test after exact-head CI completes, and avoid expanding the change into broader Discord or PluralKit policy work.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No mechanical repair is needed; wait for exact-head CI, refresh if required by the base update, and complete normal maintainer review of this already-focused patch.

Security
Cleared: The two-file diff only disposes an already-received HTTP response stream and adds a test; it adds no dependencies, permissions, credential handling, workflow changes, or new code-execution path.

Review details

Best possible solution:

Keep the existing missing-message contract, land the guarded 404-body cancellation with its focused regression test after exact-head CI completes, and avoid expanding the change into broader Discord or PluralKit policy work.

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

No failing current-main run was executed in this review, but the path is source-reproducible: the 404 branch returns before consuming or cancelling Response.body, and the PR’s production-module terminal proof verifies the corrected streamed-response behavior.

Is this the best way to solve the issue?

Yes. Cancelling only the 404 response body at the existing early return is the narrowest maintainable repair; it preserves the established null result and leaves the bounded non-404 error-body handling unchanged.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Discord resource-lifecycle bug fix with limited blast radius and no demonstrated user-facing delivery regression.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix terminal output from the production PluralKit lookup module showing a streamed 404 is cancelled while a 200 JSON response still parses correctly; no private data is shown.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal output from the production PluralKit lookup module showing a streamed 404 is cancelled while a 200 JSON response still parses correctly; no private data is shown.
Evidence reviewed

PR surface:

Source +1, Tests +2. Total +3 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 0 +1
Tests 1 3 1 +2
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 4 1 +3

What I checked:

  • Current-main gap and proposed fix: The supplied three-way PR context shows current main at e2e394e1ab80ca0de58329b97aab106eb7999c59 returns early for a 404 response, while PR head c6a485e6db2cbe01434e51d8aa5227809c3e479a adds guarded body cancellation immediately before the same null return. (extensions/discord/src/pluralkit.ts:65, c6a485e6db2c)
  • Focused regression coverage: The updated 404 test uses a tracked stream response and asserts that the stream was cancelled after fetchPluralKitMessageInfo returns null, covering the resource-release behavior without changing the successful lookup path. (extensions/discord/src/pluralkit.test.ts:70, c6a485e6db2c)
  • Feature provenance: The PluralKit lookup behavior appears to date to the original Discord PluralKit implementation in commit 8e2b17e0c5; the PR is a narrow lifecycle repair to that established lookup path rather than a competing implementation. (extensions/discord/src/pluralkit.ts:65, 8e2b17e0c5)
  • Real behavior proof: The PR body records a terminal run importing the production lookup module: a streamed 404 returns null and reports missing-body-canceled=true; a 200 JSON negative control still parses member member-1. Focused and full module Vitest commands are also listed. (extensions/discord/src/pluralkit.ts:65, c6a485e6db2c)
  • Merge state: GitHub reports the branch as cleanly mergeable but behind main; numerous exact-head validation jobs were still in progress in the supplied review context, so merge should wait for those results and any necessary refresh. (c6a485e6db2c)

Likely related people:

  • thewilloftheshadow: The history trail identifies this person with the original Discord PluralKit lookup implementation, making them the closest routing contact for lifecycle behavior in this module. (role: feature introducer; confidence: medium; commits: 8e2b17e0c5; files: extensions/discord/src/pluralkit.ts, extensions/discord/src/pluralkit.test.ts)
  • cxbAsDev: This person was explicitly mentioned on the PR when it entered maintainer review, indicating relevant connection to the Discord/PluralKit surface, though ownership is less direct than the original implementation history. (role: adjacent reviewer; confidence: low; files: extensions/discord/src/pluralkit.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (2 earlier review cycles)
  • reviewed 2026-07-17T12:00:54.016Z sha ee652cc :: needs maintainer review before merge. :: none
  • reviewed 2026-07-17T23:41:42.590Z sha d2a4574 :: needs maintainer review before merge. :: none

@RileyJJY
RileyJJY force-pushed the jjy/fix/pluralkit-404-body branch from ee652cc to d2a4574 Compare July 17, 2026 23:38
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 17, 2026
@RileyJJY
RileyJJY force-pushed the jjy/fix/pluralkit-404-body branch from d2a4574 to c6a485e Compare July 20, 2026 06:49
@steipete steipete self-assigned this Jul 21, 2026
@steipete
steipete force-pushed the jjy/fix/pluralkit-404-body branch from c6a485e to 67670cb Compare July 21, 2026 01:30
@steipete
steipete merged commit 9713006 into openclaw:main Jul 21, 2026
86 checks passed
@steipete

steipete commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jul 21, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 21, 2026
@RileyJJY
fix(discord): release PluralKit 404 bodies (openclaw#109960)
96ffc62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

channel: discord Channel integration: discord P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RileyJJY @steipete @Patrick-Erichsen