Description

SLACK_SOCKET_RECONNECT_POLICY.maxAttempts was changed from 12 to 0. The reconnect loop treats maxAttempts <= 0 as no limit, causing infinite reconnect attempts.

In monitor/provider.ts, the bail-out condition is guarded by maxAttempts > 0, so setting it to 0 disables the cap:

• Input/trigger: repeated network disconnects / Slack socket start failures (can be attacker-influenced in hostile networks, or due to misconfig/outage)
• Behavior: while (!abortSignal.aborted) loop retries forever with backoff (up to 30s) and logs each failure
• Impact: persistent background activity and log flooding; may consume CPU/memory over time and repeatedly hit Slack endpoints, potentially contributing to rate limiting or operational DoS of the gateway process

Vulnerable logic (cap disabled when maxAttempts is 0):

if (SLACK_SOCKET_RECONNECT_POLICY.maxAttempts > 0 &&
    reconnectAttempts >= SLACK_SOCKET_RECONNECT_POLICY.maxAttempts) {
  throw new Error("...max attempts reached...");
}

Recommendation

Reintroduce a finite reconnect cap, or implement an explicit circuit breaker/disable flag instead of using 0.

Option A (simple): restore a reasonable cap (e.g., 12):

export const SLACK_SOCKET_RECONNECT_POLICY = {
  initialMs: 2_000,
  maxMs: 30_000,
  factor: 1.8,
  jitter: 0.25,
  maxAttempts: 12,
} as const;

Option B (explicit unlimited + safety): if unlimited retries are desired, add safeguards such as:

• maximum total retry duration (e.g., stop after N minutes and require manual restart)
• log rate limiting
• configurable maxAttempts via env/config with a safe default

Example duration cap (pseudo):

const startedAt = Date.now();
const MAX_RETRY_WINDOW_MS = 10 * 60_000;
...
if (Date.now() - startedAt > MAX_RETRY_WINDOW_MS) throw new Error("reconnect window exceeded");

Best possible solution:

Land a narrow Slack plugin fix that removes or revises the recoverable Socket Mode cap, preserves non-recoverable auth fail-fast behavior, updates Slack operator docs, removes the release-owned changelog edit, and includes real reconnect proof for the chosen recovery policy.

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

Yes at source level: current main still has the 12-attempt Slack Socket Mode cap and throws when reconnect attempts reach it, and the linked issue includes matching logs. I did not run a live Slack disconnect sequence in this read-only review.

Is this the best way to solve the issue?

No as submitted: the one-line runtime change is the narrow owner-boundary fix for the finite-cap failure, but docs, release-owned changelog, real behavior proof, and maintainer recovery-policy acceptance remain before merge.

Full review comments:

• [P2] Update Slack docs for unlimited retries — extensions/slack/src/monitor/reconnect-policy.ts:13
  Changing maxAttempts to 0 makes recoverable Socket Mode retries unlimited, but docs/channels/slack.md:532 still tells operators those failures stop after 12 attempts. Please update that user-facing guidance so the merged runtime and docs do not disagree.
  Confidence: 0.92
• [P3] Remove the release-owned changelog entry — CHANGELOG.md:33
  Normal PRs should not edit release-owned CHANGELOG.md; the PR body already carries the release-note context. Please drop this added line and leave final release notes to the release generation flow.
  Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

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

Label changes:

• add merge-risk: 🚨 availability: Indefinite retry may improve transient recovery but can also mask provider states that need escalation or restart, which CI cannot prove.
• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Related issue logs show an equivalent local mitigation, but this external PR still needs redacted contributor-run branch proof in the PR body; redact tokens, IPs, phone numbers, private endpoints, and after updating the body a fresh ClawSweeper review should run or a maintainer can comment @clawsweeper re-review. 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.

Label justifications:

• P1: The PR targets a real Slack channel outage mode where messages can stop delivering until manual restart.
• merge-risk: 🚨 compatibility: Changing the hardcoded Slack retry default from finite to unlimited changes existing operator-visible recovery behavior without a config or migration path.
• merge-risk: 🚨 availability: Indefinite retry may improve transient recovery but can also mask provider states that need escalation or restart, which CI cannot prove.
• 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 stronger real behavior proof before merge: Related issue logs show an equivalent local mitigation, but this external PR still needs redacted contributor-run branch proof in the PR body; redact tokens, IPs, phone numbers, private endpoints, and after updating the body a fresh ClawSweeper review should run or a maintainer can comment @clawsweeper re-review. 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.

PR surface:

Source 0, Docs +1. Total +1 across 2 files.

What I checked:

• Current main still uses a finite Slack retry cap: SLACK_SOCKET_RECONNECT_POLICY.maxAttempts is still 12 on current main, so this PR changes the existing Slack plugin runtime default rather than duplicating already-implemented behavior. (extensions/slack/src/monitor/reconnect-policy.ts:13, a7f96847ce2d)
• Provider treats non-positive maxAttempts as unlimited but still fails auth fast: The Slack provider formats non-positive maxAttempts as infinity, skips the cap only when maxAttempts <= 0, and still throws immediately for non-recoverable Slack auth errors before retrying. (extensions/slack/src/monitor/provider.ts:123, a7f96847ce2d)
• User-facing Slack docs would be stale after merge: The Slack docs still tell operators that recoverable Socket Mode start/start-wait failures stop after 12 attempts, which would contradict the PR's unlimited retry behavior. Public docs: docs/channels/slack.md. (docs/channels/slack.md:532, a7f96847ce2d)
• PR diff changes runtime policy and release-owned changelog: The PR changes only maxAttempts: 12 to 0 in the Slack reconnect policy and adds a normal-PR CHANGELOG.md entry, while repo policy says release generation owns changelog edits. (extensions/slack/src/monitor/reconnect-policy.ts:13, 7de350a271aa)
• Pinned Slack dependency retries internally without an explicit cap: The pinned @slack/[email protected] client increments reconnection failures and delays reconnect attempts internally; OpenClaw's wrapper adds the OpenClaw-visible cap and status/log behavior being changed here.
• Related issue has source and field evidence but not branch proof: The linked Slack outage issue includes matching retry 1/12 logs and an equivalent installed maxAttempts: 0 mitigation showing retry 1/∞, but the PR itself does not include contributor-run proof for this branch after the patch.

Likely related people:

• pandego: Authored the merged Slack Socket Mode reconnect PR that added the bounded reconnect loop and 12-attempt policy this PR changes. (role: introduced behavior; confidence: high; commits: 949faff5ce7d; files: src/slack/monitor/provider.ts, extensions/slack/src/monitor/provider.ts, extensions/slack/src/monitor/reconnect-policy.ts)
• Takhoffman: Merged the original Slack reconnect PR via squash and recorded validation, so they are relevant to the current behavior's provenance. (role: merger; confidence: medium; commits: 949faff5ce7d; files: src/slack/monitor/provider.ts)
• steipete: Extracted the Slack socket reconnect policy helpers into the separate policy module that now owns the constant changed by this PR. (role: feature-history contributor; confidence: medium; commits: 1dd77e410658; files: src/slack/monitor/reconnect-policy.ts, src/slack/monitor/provider.ts, extensions/slack/src/monitor/reconnect-policy.ts)
• scoootscooob: Moved the Slack channel implementation into extensions/slack/src, preserving the reconnect policy and provider paths now affected. (role: plugin migration contributor; confidence: medium; commits: 8746362f5ebf; files: extensions/slack/src/monitor/provider.ts, extensions/slack/src/monitor/reconnect-policy.ts)

• 🦀 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.

• 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.