fix(mcp): bound pendingClaudePermissions / pendingApprovals via TTL sweeper + close clear by Feelw00 · Pull Request #71648 · openclaw/openclaw

Feelw00 · 2026-04-25T16:04:01Z

Summary

Problem: OpenClawChannelBridge (src/mcp/channel-bridge.ts) holds two instance-bound pending Maps — pendingClaudePermissions (L50) and pendingApprovals (L51) — that lack TTL/sweeper, close-clear, and cap. Sibling collections in the same class (queue cap=1000 at L355, pendingWaiters close-clear at L148 + per-waiter setTimeout fallback at L265) are all bounded; only these two are not.
Why it matters: long-running openclaw mcp serve accumulates entries on (a) missed Claude permission replies (operator typos / cross-channel responses / --dangerously-skip-permissions) and (b) gateway WebSocket drops that silence *.approval.resolved frames (onClose at L122-124 is reject-only, no missed-event catchup).
What changed: lazy-start a 5-min sweepPendingExpired() interval, .unref()'d so it never keeps the process alive. Per-Map TTL — 1h for Claude permissions (anchored at createdAtMs), respects payload.expiresAtMs for approvals (falls back to trackedAtMs + 30min when absent). close() now clears both Maps and stops the sweeper. Handler entry early-returns when this.closed to prevent post-close ghost writes.
What did NOT change: public listPendingApprovals shape, message protocols, or any other class collection. cap/FIFO is intentionally split to a follow-up PR (one-thing-per-PR; sweeper covers the slow leak, cap addresses adversarial bursts).

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #71646.

This PR fixes a bug or regression

Root Cause

Upstream commit history shows channel-bridge.ts evolving across refactor/seam-split commits over the last 6 weeks, but no commit added a TTL/sweep/cap guard for these two pending Maps. The asymmetry is structural:

Collection	Cap	Close-clear	TTL / sweeper	Status
`queue` (Array, L48)	`QUEUE_LIMIT=1000` while-shift	n/a	n/a	bounded
`pendingWaiters` (Set, L49)	n/a	`close()` clears (L148)	per-waiter `setTimeout` (L265)	bounded
`pendingClaudePermissions` (Map, L50)	none	none	none	leaks
`pendingApprovals` (Map, L51)	none	none	`expiresAtMs` stored at L382 but never drives a timer	leaks

pendingClaudePermissions only deletes when the operator reply matches /^(yes|no)\s+([a-km-z]{5})$/i AND has(requestId) is true (L455-459) — a single conditional-edge cleanup. pendingApprovals only deletes on the matching *.resolved frame (L389) — another single conditional-edge. No primary unconditional cleanup path exists.

Regression Test Plan

New file src/mcp/channel-bridge.test.ts (7 it blocks, fake timers, no production-branch divergence):

pendingClaudePermissions evicted by sweeper at TTL boundary
pendingApprovals evicted at expiresAtMs
pendingApprovals evicted at default TTL fallback when expiresAtMs is absent
pendingApprovals evicted even when both createdAtMs and expiresAtMs are absent (regression test for the now-anchored fallback bug caught in pre-PR review)
close() clears both Maps, stops the sweeper interval, and vi.getTimerCount() === 0
handleClaudePermissionRequest is a no-op after close() — prevents post-close ghost writes
Sweeper interval is not started before any pending entry is added (lazy-init)

channel-server.test.ts:366 fixture updated to the new {request, createdAtMs} wrapper shape.

User-visible / Behavior Changes

Long-running openclaw mcp serve sees pending Map sizes converge to a steady state instead of growing monotonically.
Operators who never reply to a Claude permission_request no longer keep the entry forever; it expires after 1h and the next permission_request for the same tool starts fresh.
Approvals announced over a dropped gateway WebSocket are auto-evicted at expiresAtMs (or trackedAtMs + 30min if the gateway omitted expiresAtMs).
No protocol surface changes; client tools see no difference.

Security Impact

None. The fix is purely about bounded memory growth in an opt-in long-running daemon. No auth / scope / token / sandbox surface touched. CODEOWNERS protected paths (/src/cron/service/jobs.ts, /src/cron/stagger.ts, /src/agents/*auth*, /src/agents/sandbox*) untouched.

Repro + Verification

Environment

pnpm 10.33.0 / Node (project default) / vitest 4.1.5
Worktree: clean checkout of upstream/main + this branch
Platform: macOS (Darwin 25.4.0)

Steps

pnpm install
pnpm check                                                       # type + lint
pnpm build                                                       # build
pnpm exec vitest run src/mcp/channel-bridge.test.ts              # new tests
pnpm exec vitest run src/mcp/channel-server.test.ts              # existing mcp tests after fixture sync
pnpm exec vitest run --config test/vitest/vitest.unit.config.ts  # unit project regression

Expected

All green. New file 7/7. Existing channel-server 6/6. Unit project 1575/1575 (was 1573 pre-fix; +2 from new file's count vs prior file count).

Actual

src/mcp/channel-bridge.test.ts: Tests 7 passed (7), 640ms
src/mcp/channel-server.test.ts: Tests 6 passed (6)
unit project: Test Files 189 passed (189), Tests 1575 passed (1575), 6.48s
pnpm check: clean
pnpm build: clean

Evidence

src/mcp/channel-bridge.ts:50-51 — pre-fix declarations of the two leaking Maps
src/mcp/channel-bridge.ts:136-152 — pre-fix close() body (only pendingWaiters.clear())
src/mcp/channel-bridge.ts:355-356 — pre-existing queue cap pattern (sibling reference)
src/mcp/channel-bridge.ts:265-267 — pre-existing per-waiter setTimeout (sibling reference)
Diff: ~80 lines prod (channel-bridge.ts +89 -4) + 9 lines fixture sync (channel-server.test.ts) + 173 lines new test file

Human Verification

I confirm I have reviewed every diff hunk, ran the build/check/test suites locally, and the regression tests fail on upstream/main (no fix) and pass on this branch (with fix).

Review Conversations

This is a fresh PR. No prior review threads.

Compatibility / Migration

Fully backwards-compatible. No public API, no protocol field, no config surface changed. The internal Map value shape ({request, createdAtMs} wrapper, {approval, trackedAtMs} wrapper) is private — only the test that previously cast through Record<string, unknown> was updated to match.

Risks and Mitigations

Risk: TTL constants (PENDING_CLAUDE_PERMISSION_TTL_MS=1h, PENDING_APPROVAL_DEFAULT_TTL_MS=30min, PENDING_SWEEP_INTERVAL_MS=5min) are hardcoded.
Mitigation: chosen for low operator-disruption (1h is longer than typical permission-think time; 30min matches existing expiresAtMs field intent). Happy to migrate to a config surface in a follow-up if requested.
Risk: PR fix: bind Claude permission replies to session #56420 (sessionKey binding, OPEN) touches the same set() line in handleClaudePermissionRequest.
Mitigation: line-level rebase only; semantic axes are orthogonal (binding vs leak). Whichever PR merges first, the other rebases cleanly.
Risk: cap/FIFO is not in this PR — adversarial bursts within a single sweep tick (5min) could still grow large in pathological cases.
Mitigation: deferred to a follow-up PR (one-thing-per-PR). Sweeper closes the slow leak that motivated the report; cap is a separate concern.

🤖 AI-Assisted (openclaw-audit pipeline). 5-agent post-harness cross-review (proceed: 4 real + 1 fix-insufficient → cap deferred to follow-up). 3-agent pre-PR cross-review round 1 caught a fallback-recalculation bug in the original sweeper (createdAtMs ?? now re-anchored expiry on every tick) → fixed via trackedAtMs instance bookkeeping; round 2 cleared 3/3 real-problem-real-fix.

Real behavior proof

Behavior or issue addressed: Without this patch, OpenClawChannelBridge.pendingClaudePermissions grows unbounded: the class starts no setInterval, so nothing evicts entries left behind by Claude permission requests that never get a matching reply. With this patch, the lazy ensurePendingSweeper() starts a real (unref'd) setInterval that evicts entries past PENDING_CLAUDE_PERMISSION_TTL_MS. This measurement uses the real Node.js wall clock and the production 1-hour TTL — no vi.useFakeTimers().
Real environment tested: macOS (darwin arm64), Node 23.x, isolated OPENCLAW_HOME. Both builds run the same probe against worktree src via tsx (no bundling). Production constants unchanged: PENDING_CLAUDE_PERMISSION_TTL_MS = 1h, PENDING_SWEEP_INTERVAL_MS = 5min.
Exact steps or command run after this patch:

$ pnpm install --frozen-lockfile           (both worktrees)
$ node_modules/.bin/tsx <probe>.mts        (real setTimeout sleep, ~70.0 min)
  probe: new OpenClawChannelBridge(...), 100x handleClaudePermissionRequest(),
         real wall-clock sleep past the 1h TTL, then read pendingClaudePermissions.size

Evidence after fix:

Live Node.js measurement of bridge.pendingClaudePermissions.size with a real setTimeout sleep across the production 1-hour TTL boundary:

[Build A] without this patch (base sha, no sweeper code):
  before=0 afterSet=100 afterTtl=100 sweeperPresent=False sleptMin=77.4
  -> Map still full after the TTL window: no eviction happened.

[Build B] with this patch (head sha, TTL sweeper):
  before=0 afterSet=100 afterTtl=0 sweeperPresent=True sleptMin=70.0
  -> real setInterval sweeper drained the Map after the 1h TTL.

Observed result after fix: With the patch, pendingClaudePermissions.size after the 1h TTL window = 0 (down from 100 set). Without the patch it stays at 100. The sweeper is a real unref'd setInterval driven by the real wall clock - no fake timer.
What was not tested: pendingApprovals (the second Map) shares the same sweepPendingExpired() pass and is covered by the unit suite rather than this wall-clock probe. Cap-based eviction is a separate, intentional follow-up axis.

greptile-apps · 2026-04-25T16:07:41Z

Greptile Summary

This PR adds a TTL sweeper, close()-clear, and post-close guards to the two previously unbounded pending Maps (pendingClaudePermissions, pendingApprovals) in OpenClawChannelBridge. The implementation is correct and well-tested across 8 scenarios with fake timers. No issues were found.

Confidence Score: 5/5

Safe to merge — targeted bug fix with no public API changes, thorough test coverage, and no logic errors found.

No P0 or P1 findings. The sweeper correctly self-terminates when both maps drain (restoring lazy-init), close() clears both maps and nulls the interval, and post-close guards prevent ghost writes. Deletion-during-iteration is safe for JS Map. All eight test scenarios exercise the relevant edge cases including the fallback-recalculation regression.

No files require special attention.

_{Reviews (3): Last reviewed commit: "test(mcp): keep channel-bridge test view..." | Re-trigger Greptile}

Feelw00 · 2026-04-25T16:55:51Z

Thanks for the catch — applied the self-terminate path in 9f16dd4823. sweepPendingExpired() now clearInterval + nulls out pendingSweepInterval when both maps are empty post-sweep, restoring the lazy-init contract. A new regression test covers drain → no scheduled timers + subsequent set re-arming the interval.

@greptile review and provide confidence score

clawsweeper · 2026-04-27T00:04:54Z

Codex review: needs maintainer review before merge. Reviewed May 30, 2026, 4:36 PM ET / 20:36 UTC.

Summary
The branch adds a lazy TTL sweeper and close-time clearing for the MCP channel bridge pending Claude permission and approval maps, plus focused Vitest coverage.

PR surface: Source +57, Tests +232. Total +289 across 3 files.

Reproducibility: yes. Current main stores both pending maps without TTL or close-clear, and the PR body includes a wall-clock probe showing base retained 100 pending Claude permissions while the patched branch drained them after the production TTL.

Review metrics: 1 noteworthy metric.

Internal retention timing: 3 added: 1h permission TTL, 30m approval fallback TTL, 5m sweep interval. These constants define new automatic cleanup behavior, so maintainers should notice and accept the timing before merge.

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

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

Rank-up moves:

[P2] Record maintainer acceptance of the 1h/30m retention windows and deferred cap/FIFO scope before merge.
After any rebase with the session-binding PR, rerun the targeted MCP bridge and channel-server tests.

Risk before merge

[P1] Merging changes unresolved MCP retention from effectively indefinite to 1h for Claude permissions and 30m for approvals without expiresAtMs, so very late operator replies or permission listings can observe different behavior.
[P1] The branch intentionally leaves hard cap/FIFO burst protection to follow-up, so burst growth inside one sweep interval remains possible after merge.
[P1] The related open session-binding PR touches the same Claude permission storage path, so whichever branch lands second needs a focused rebase and proof around both behaviors.

Maintainer options:

Accept bounded retention (recommended)
Merge once maintainers explicitly accept that unanswered Claude permissions expire after 1h and approvals without expiresAtMs expire after 30m, with cap/FIFO tracked separately.
Adjust retention before merge
If those retention windows are not acceptable, change the constants or preserve the old indefinite behavior behind an explicit maintainer-approved path before merge.
Pause for session binding
If maintainers want the open session-binding work to land first, pause this PR and rebase the sweeper onto that final storage shape.

Next step before merge

[P2] The remaining action is maintainer judgment on TTL retention semantics, cap follow-up scope, and ordering with the open session-binding PR; there is no narrow automated repair to queue.

Security
Cleared: The diff only changes MCP in-memory cleanup logic and tests; it does not add dependencies, workflows, secret handling, install scripts, or package metadata.

Review details

Best possible solution:

Merge the bounded cleanup after maintainers explicitly accept the retention windows, with cap/FIFO and session-binding ordering tracked separately.

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

Yes. Current main stores both pending maps without TTL or close-clear, and the PR body includes a wall-clock probe showing base retained 100 pending Claude permissions while the patched branch drained them after the production TTL.

Is this the best way to solve the issue?

Mostly yes. A lazy sweeper plus close cleanup is the narrow repair for slow retention leaks, but the hardcoded retention windows and deferred cap/FIFO protection need maintainer acceptance rather than automated repair.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against c80ec4332580.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live wall-clock output showing the patched bridge drains pendingClaudePermissions after the production TTL, with approval cleanup covered by focused tests.

Label justifications:

P2: This is a normal-priority reliability fix for slow memory growth in an opt-in long-running MCP path.
merge-risk: 🚨 compatibility: The PR changes how long unresolved MCP permission and approval state remains visible or replyable compared with current indefinite retention.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes live wall-clock output showing the patched bridge drains pendingClaudePermissions after the production TTL, with approval cleanup covered by focused tests.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live wall-clock output showing the patched bridge drains pendingClaudePermissions after the production TTL, with approval cleanup covered by focused tests.

Evidence reviewed

PR surface:

Source +57, Tests +232. Total +289 across 3 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	76	19	+57
Tests	2	238	6	+232
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	3	314	25	+289

What I checked:

Current main source: Current main keeps pendingClaudePermissions and pendingApprovals as Maps with no TTL or sweep state, and close() only clears pendingWaiters. (src/mcp/channel-bridge.ts:48, c80ec4332580)
PR implementation: The PR head adds 1h Claude permission TTL, 30m approval fallback TTL, a 5m sweeper interval, close-time map clearing, and timestamped pending entries. (src/mcp/channel-bridge.ts:45, d8b1a415c45c)
Regression coverage: The new test file covers Claude permission TTL eviction, approval expiresAt/default TTL eviction, close cleanup, post-close no-op behavior, lazy start, list-time pruning, and sweeper self-termination. (src/mcp/channel-bridge.test.ts:47, d8b1a415c45c)
Real behavior proof: The PR body reports a real Node wall-clock probe where base retained 100 pending Claude permissions past the production 1h TTL and the patched branch drained them to 0; approvals are covered by the unit suite rather than that wall-clock probe. (d8b1a415c45c)
Whitespace sanity: The touched MCP files have no diff-check whitespace errors against current main. (d8b1a415c45c)
History provenance: Current-main blame for the central MCP channel bridge pending-map and cleanup paths points to commit c5af09e, while the PR series is committed by Peter Steinberger with original branch commits authored by Feelw00. (src/mcp/channel-bridge.ts:48, c5af09e378be)

Likely related people:

shakkernerd: Current-main blame for the pending maps, close path, permission request storage, and approval tracking all points to c5af09e, which is attributed to Shakker / shakkernerd in the shallow history available here. (role: introduced current-main MCP bridge behavior; confidence: medium; commits: c5af09e378be; files: src/mcp/channel-bridge.ts)
steipete: The current PR head commit d8b1a41 is authored and committed by Peter Steinberger and tightens the pending cleanup implementation after prior review. (role: recent PR branch updater; confidence: high; commits: d8b1a415c45c; files: src/mcp/channel-bridge.ts, src/mcp/channel-bridge.test.ts, src/mcp/channel-server.test.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.

Feelw00 · 2026-04-27T03:00:32Z

Pushed eef0be2a2e — types-only test helper fix.

The previous CI run on pull/71648/merge failed check + check-test-types with 50+ TS2339 does not exist on type 'never' errors in channel-bridge.test.ts. Root cause: tsgo collapses OpenClawChannelBridge & BridgeInternals to never because pendingClaudePermissions / pendingApprovals / pendingSweepInterval exist on both constituents and are private on the class.

Fix: makeBridge() now returns BridgeInternals alone (via as unknown as) so no public/private same-name intersection happens, and BridgeInternals additionally declares the public methods the tests exercise. No production changes; the 8 regression tests still pass against the merged channel-bridge.ts.

@greptile review and provide confidence score

…ile P2) Greptile P2 review on PR openclaw#71648: the sweep interval kept firing every 5min in perpetuity once both maps emptied — `.unref()` made it safe but silently abandoned the lazy-init invariant. ensurePendingSweeper() would no-op on later sets because pendingSweepInterval stayed non-null. sweepPendingExpired() now clearInterval()s and nulls out pendingSweepInterval when both pending maps are empty after a sweep, restoring the lazy-init contract: a fresh set after drain re-arms the interval via the same ensurePendingSweeper() path. New regression test covers (1) drain triggers self-terminate + vi.getTimerCount()===0, (2) subsequent set re-arms the interval.

Feelw00 · 2026-05-06T08:32:24Z

@clawsweeper review

…ile P2) Greptile P2 review on PR openclaw#71648: the sweep interval kept firing every 5min in perpetuity once both maps emptied — `.unref()` made it safe but silently abandoned the lazy-init invariant. ensurePendingSweeper() would no-op on later sets because pendingSweepInterval stayed non-null. sweepPendingExpired() now clearInterval()s and nulls out pendingSweepInterval when both pending maps are empty after a sweep, restoring the lazy-init contract: a fresh set after drain re-arms the interval via the same ensurePendingSweeper() path. New regression test covers (1) drain triggers self-terminate + vi.getTimerCount()===0, (2) subsequent set re-arms the interval.

Feelw00 · 2026-05-20T05:51:09Z

Replaced the ## Real behavior proof section with a real wall-clock measurement, addressing the earlier Codex note that the fake-timer harness did not show a real setup.

The probe builds both base (pre-fix, no sweeper code) and head (this PR) from source, instantiates a real OpenClawChannelBridge, registers 100 Claude permission requests via handleClaudePermissionRequest, then sleeps ~70 minutes of real setTimeout wall clock past the production 1-hour PENDING_CLAUDE_PERMISSION_TTL_MS - no vi.useFakeTimers().

base (no sweeper code): pendingClaudePermissions.size = 100 after the TTL window - nothing evicted.
head (this PR): the real unref'd setInterval sweeper drained the Map to 0.

Production TTL/interval constants are unchanged; the probe uses them as-is. No code change in this update - PR description only.

@clawsweeper review

clawsweeper · 2026-05-20T05:53:09Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26147795541
Updated: 2026-05-20T07:34:18.494Z

clawsweeper · 2026-05-20T06:01:11Z

ClawSweeper PR egg

✨ Hatched: 🥚 common Frosted Shellbean

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

Merged PRs are hatchable.
Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: watches the merge queue.
Image traits: location merge queue dock; accessory rollback rope; palette seafoam, black, and opal; mood mischievous; pose peeking out from the egg shell; shell matte ceramic shell; lighting gentle morning glow; background delicate sparkle particles.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Frosted Shellbean in ClawSweeper.

What is this egg doing here?

Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

RomneyDa · 2026-05-29T06:34:51Z

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

clawsweeper · 2026-05-29T07:33:02Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26624483838
Updated: 2026-05-29T07:42:22.344Z

Feelw00 · 2026-05-29T07:34:19Z

@RomneyDa
Done. Rebased onto current main (head 8457a44); the Dependency Guard check is green now. Thanks for the heads up!

…ile P2) Greptile P2 review on PR openclaw#71648: the sweep interval kept firing every 5min in perpetuity once both maps emptied — `.unref()` made it safe but silently abandoned the lazy-init invariant. ensurePendingSweeper() would no-op on later sets because pendingSweepInterval stayed non-null. sweepPendingExpired() now clearInterval()s and nulls out pendingSweepInterval when both pending maps are empty after a sweep, restoring the lazy-init contract: a fresh set after drain re-arms the interval via the same ensurePendingSweeper() path. New regression test covers (1) drain triggers self-terminate + vi.getTimerCount()===0, (2) subsequent set re-arms the interval.

Feelw00 · 2026-05-30T04:55:39Z

Rebased onto the current main tip to clear the stale-base CI cancellation (the branch had drifted ~500 commits behind, so auto-response and Real behavior proof were cancelled rather than run).

No diff changes: the same three commits replay cleanly (upstream did not touch src/mcp/channel-bridge.ts or the test files in the interim). Verified locally on the rebased head: pnpm build green, pnpm check green, and the targeted src/mcp/channel-bridge.test.ts + src/mcp/channel-server.test.ts suites pass 18/18.

@clawsweeper review

clawsweeper · 2026-05-30T04:55:42Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26676518760
Updated: 2026-05-30T06:16:53.497Z

Feelw00 · 2026-05-30T05:31:43Z

The previous automated review reported an execution failure ("Review failed before ClawSweeper could summarize") rather than a verdict on the change. It landed during a transient CI window where the repo automation was rate-limited (the auto-response/label jobs failed with API rate limit exceeded for installation ID 106147218, and the unrelated test/scripts/crabbox-wrapper.test.ts cloud-provider check flaked). None of those touch this PR's diff, which is limited to src/mcp/channel-bridge.ts and its tests.

Re-requesting a clean review now that the window has passed. Locally on the rebased head (2d6158ba16): pnpm build, pnpm check, and the targeted src/mcp/channel-bridge.test.ts + src/mcp/channel-server.test.ts suites (18/18) are all green.

@clawsweeper review

clawsweeper · 2026-05-30T05:31:45Z

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26675785252
Updated: 2026-05-30T05:35:55.038Z

…clear OpenClawChannelBridge held two instance-bound Maps (pendingClaudePermissions L50, pendingApprovals L51) that lacked TTL/sweeper, close-clear, and cap — while the sibling collections in the same class (queue cap=1000, pendingWaiters close-clear + per-waiter setTimeout) were all bounded. Long-running `openclaw mcp serve` processes accumulated entries on missed Claude permission replies and on gateway WebSocket drops that silenced *.approval.resolved frames. Fix scope (one-thing-per-PR): - Wrap entries in PendingClaudePermissionEntry / PendingApprovalEntry with internal createdAtMs / trackedAtMs (instance bookkeeping, decoupled from payload-supplied fields so sweeper fallback is stable). - Lazy-start a 5-min sweepPendingExpired() interval on first set, .unref()'d so it never keeps the process alive. - TTLs: 1h for pendingClaudePermissions (createdAtMs-anchored), respects payload.expiresAtMs for pendingApprovals (falls back to trackedAtMs+30min when the server omits it). - close() now clears both Maps and clearInterval()s the sweeper. - handleClaudePermissionRequest / trackApproval early-return when this.closed to prevent post-close ghost writes. Cap/FIFO is intentionally split to a follow-up PR (one-thing-per-PR; sweeper covers the slow leak; cap addresses adversarial bursts). Tests: - New src/mcp/channel-bridge.test.ts (7 it blocks): TTL sweep for both Maps, default-TTL fallback, both-payload-fields-undefined case, close clears + vi.getTimerCount()===0, no-op-after-close, lazy-init guard. - channel-server.test.ts:366 fixture synced to wrapper shape. AI-assisted (openclaw-audit pipeline). Closes openclaw#71646. cross-review: - 5-agent post-harness primary_decision=proceed (real=4 + fix-insufficient=1 scope-down → applied: cap deferred). - 3-agent pre-pr round 1: critical-devil flagged 4 issues → fixed in this commit. - 3-agent pre-pr round 2: 3/3 real-problem-real-fix → proceed_to_pr. Upstream-dup check clean: 6w channel-bridge.ts commits all refactor/seam axis; PR openclaw#56420 (sessionKey binding) orthogonal but touches the same set() lines — rebase order may matter.

…ile P2) Greptile P2 review on PR openclaw#71648: the sweep interval kept firing every 5min in perpetuity once both maps emptied — `.unref()` made it safe but silently abandoned the lazy-init invariant. ensurePendingSweeper() would no-op on later sets because pendingSweepInterval stayed non-null. sweepPendingExpired() now clearInterval()s and nulls out pendingSweepInterval when both pending maps are empty after a sweep, restoring the lazy-init contract: a fresh set after drain re-arms the interval via the same ensurePendingSweeper() path. New regression test covers (1) drain triggers self-terminate + vi.getTimerCount()===0, (2) subsequent set re-arms the interval.

tsgo (used by check + check-test-types in CI) was reducing `OpenClawChannelBridge & BridgeInternals` to `never` because pendingClaudePermissions, pendingApprovals, and pendingSweepInterval exist on both constituents — private on the class, public on the test shape — and TypeScript collapses such intersections. Result: every member access on `bridge` in channel-bridge.test.ts (50+ sites) errored with TS2339 "Property X does not exist on type 'never'", even though the test file was structurally fine. CI surfaced this on the pull/71648/merge ref but the same failure reproduces on the PR head alone. Fix: - makeBridge() now returns BridgeInternals only, via `as unknown as`, so no intersection of public/private same-name members occurs. - BridgeInternals additionally declares the public methods we exercise (handleClaudePermissionRequest, close) so callers stay typed. No production changes; types-only test-helper edit. The 8 regression tests still pass against the merged channel-bridge.ts.

steipete · 2026-05-30T20:36:29Z

Verification before merge:

Behavior addressed: bounded MCP channel bridge pending Claude permission and approval tracking; listPendingApprovals now sweeps expired approvals before reporting them.
Real environment tested: local macOS checkout plus GitHub Actions for PR head d8b1a41.
Exact steps or command run after this patch:

node scripts/run-vitest.mjs src/mcp/channel-bridge.test.ts src/mcp/channel-server.test.ts
../agent-scripts/skills/autoreview/scripts/autoreview --mode local
../agent-scripts/skills/autoreview/scripts/autoreview --mode branch --base origin/main
git diff --check origin/main...HEAD
env -u OPENCLAW_TESTBOX pnpm check:changed
gh pr checks 71648 --watch --interval 10 --fail-fast
Evidence after fix: targeted MCP tests passed 2 files / 19 tests; both autoreview runs reported no accepted/actionable findings; local changed gate passed; PR CI passed for head d8b1a41.
Observed result after fix: stale approvals are swept before permissions_list_open output, Claude permission tracking stores only the timestamp needed for expiry, and close/sweeper cleanup remains covered.
What was not tested: no live long-running MCP daemon repro beyond the PR's submitted real-time proof; no docs changes needed because no public protocol or config surface changed.

openclaw-barnacle Bot added the size: M label Apr 25, 2026

greptile-apps Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread src/mcp/channel-bridge.ts

clawsweeper Bot mentioned this pull request Apr 26, 2026

mcp/channel-bridge: pendingClaudePermissions / pendingApprovals leak — no TTL, no close-clear, no cap #71646

Closed

clawsweeper Bot mentioned this pull request Apr 30, 2026

fix: bind Claude permission replies to session #56420

Closed

Feelw00 force-pushed the fix/mcp-channel-bridge-pending-leak branch from eef0be2 to a9b70b7 Compare May 6, 2026 01:32

Feelw00 force-pushed the fix/mcp-channel-bridge-pending-leak branch from bc04d6a to 528782a Compare May 6, 2026 08:10

Feelw00 force-pushed the fix/mcp-channel-bridge-pending-leak branch from 526aec7 to eb69de7 Compare May 11, 2026 00:17

Feelw00 force-pushed the fix/mcp-channel-bridge-pending-leak branch from 8457a44 to 2d6158b Compare May 30, 2026 04:55

steipete self-assigned this May 30, 2026

Feelw00 and others added 4 commits May 30, 2026 21:24

fix(mcp): tighten channel bridge pending cleanup

d8b1a41

steipete force-pushed the fix/mcp-channel-bridge-pending-leak branch from 2d6158b to d8b1a41 Compare May 30, 2026 20:30

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 30, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 30, 2026

steipete merged commit c6b1fed into openclaw:main May 30, 2026
129 checks passed

clawsweeper Bot mentioned this pull request Jun 30, 2026

fix(mcp): require owner for Claude permission replies #98256

Merged

Uh oh!

Uh oh!

Conversation

Feelw00 commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause

Regression Test Plan

User-visible / Behavior Changes

Security Impact

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification

Review Conversations

Compatibility / Migration

Risks and Mitigations

Real behavior proof

Uh oh!

greptile-apps Bot commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Feelw00 commented Apr 25, 2026

Uh oh!

clawsweeper Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Feelw00 commented Apr 27, 2026

Uh oh!

Feelw00 commented May 6, 2026

Uh oh!

Feelw00 commented May 20, 2026

Uh oh!

clawsweeper Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clawsweeper Bot commented May 20, 2026

Hatch command

Uh oh!

RomneyDa commented May 29, 2026

Uh oh!

clawsweeper Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Feelw00 commented May 29, 2026

Uh oh!

Feelw00 commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Feelw00 commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented May 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Feelw00 commented Apr 25, 2026 •

edited

Loading

greptile-apps Bot commented Apr 25, 2026 •

edited

Loading

clawsweeper Bot commented Apr 27, 2026 •

edited

Loading

clawsweeper Bot commented May 20, 2026 •

edited

Loading

clawsweeper Bot commented May 29, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading