PR #84244: fix(twitch): export clearRegistryForTest for cross-test isolation (#83887)

• Repository: openclaw/openclaw
• Author: hclsys
• State: closed | merged: False
• Link: https://github.com/openclaw/openclaw/pull/84244

Description (problem / solution / changelog)

Fixes #83887.

extensions/twitch/src/client-manager-registry.ts holds const registry = new Map<string, RegistryEntry>() at module scope. The companion class TwitchClientManager (twitch-client.ts:272) already exports clearForTest() — an explicit acknowledgement that module-level state needs to be reset between tests. The registry has no such escape hatch.

Under vitest --isolate=false (or any harness that keeps the module graph hot across cases), a test that calls getOrCreateClientManager('default', loggerA) leaves its entry behind. The next test calling getOrCreateClientManager('default', loggerB) hits the cache branch (if (existing) return existing.manager) and silently receives test A's manager — bound to test A's logger, mocks, and assertion targets. The bug is latent today only because the included tests mock at the resolveTwitchAccountContext level and never exercise the registry factory directly. Any test that does will chase a phantom mock-assertion failure.

Changes

• extensions/twitch/src/client-manager-registry.ts: add an exported clearRegistryForTest(): void { registry.clear(); }. Mirrors the existing clearForTest escape hatch on TwitchClientManager (same module convention, same naming). Documented as test-only with a comment explaining the --isolate=false failure mode.

Diff stat: 1 file, +17 / -0. No production code is touched and no production caller is added.

Real behavior proof

• Behavior or issue addressed: Sanitized issue evidence — the registry Map is module-scoped and survives across vitest cases unless explicitly cleared. getOrCreateClientManager short-circuits on registry.get(accountId) without comparing logger identity or any other freshness signal, so a stale entry from a prior test wins. The new helper gives a test's afterEach a way to reset the registry without resorting to vi.resetModules().

• Real environment tested: Local Node 22.x. Probe at /tmp/probe_83887.mjs does both halves of the proof. (a) Parses the patched client-manager-registry.ts and verifies clearRegistryForTest is exported with the exact body registry.clear() and that it is declared AFTER the module-level registry definition. (b) Replays the registry's get-or-create semantics in pure JS for four scenarios — buggy shape (no clear between tests → test B receives test A's manager+logger, confirming the #83887 symptom), patched shape with clear-between-tests (test B gets a fresh manager with loggerB), production cache-hit within a single test (same accountId returns cached manager — no regression), and clear-on-empty (safe no-op).

• Exact steps or command run after this patch: node /tmp/probe_83887.mjs

• Evidence after fix:

PASS: clearRegistryForTest is exported and calls registry.clear()
PASS: clearRegistryForTest is declared AFTER the module-level registry
PASS: replay (buggy): without registry clear, test B receives test A's manager+logger — confirms #83887 cross-test pollution
PASS: replay (patched): clear between tests → test B gets fresh manager with loggerB
PASS: replay (production cache-hit within a single test): same accountId returns the cached manager — no regression
PASS: replay (patched, empty registry): clear() on empty Map is a safe no-op

ALL CASES PASS

• Observed result after fix: Any test file exercising getOrCreateClientManager can now call clearRegistryForTest() in afterEach (mirroring the pattern twitch-client.test.ts:131 already uses for manager.clearForTest()). Production behavior is unchanged because production code does not import the new export.

• What was not tested: A live vitest --isolate=false run inside the repo CI — the harness setup for that is outside the scope of an additive test-helper export. The vitest case shape this enables is identical to the existing clearForTest pattern proven elsewhere in the same module.

Audit (per CLAUDE rules — all 5 steps)

• Existing-helper check: Mirrors the already-exported TwitchClientManager.clearForTest() (twitch-client.ts:272-275) — same module convention. The issue's _clearRegistryForTest naming with underscore prefix would have diverged from the live codebase; I used clearRegistryForTest to match the sibling. PASS
• Shared-helper caller check: clearRegistryForTest has zero callers initially — it is a test-only export. Production code (monitor.ts, send.ts, plugin.ts) imports only getOrCreateClientManager / getClientManager / removeClientManager, none of which are affected by adding a new export. PASS
• Broader-fix rival scan: gh pr list --search '83887 in:title,body' and gh pr list --search 'clearRegistryForTest in:title,body' return no open PRs. Issue timeline shows zero cross-references. PASS
• Recent-merge audit: git log --oneline -5 -- extensions/twitch/src/client-manager-registry.ts shows no recent commits touching this file. PASS
• Prototype-pollution scan: N/A — single Map.clear() call.

Changed files

• extensions/twitch/src/client-manager-registry.ts (modified, +17/-0)

PR #84309: fix(twitch): export clearRegistryForTest for cross-test isolation (#83887)

• Repository: openclaw/openclaw
• Author: clawsweeper[bot]
• State: closed | merged: True
• Link: https://github.com/openclaw/openclaw/pull/84309

Description (problem / solution / changelog)

Makes https://github.com/openclaw/openclaw/pull/84244 merge-ready for the ClawSweeper automerge loop. The edit pass should inspect the live PR diff, review comments, and failing checks; rebase if needed; keep the contributor branch credited; and stop only when validation is green or an external blocker is proven.

ClawSweeper 🐠 replacement reef notes:

• Cluster: automerge-openclaw-openclaw-84244
• Source PRs: https://github.com/openclaw/openclaw/pull/84244
• Credit: Source PR: https://github.com/openclaw/openclaw/pull/84244
• Validation: pnpm check:changed
• Replacement reason: ClawSweeper could not update the source PR branch directly, so it opened a writable replacement PR instead.
• Automerge requested by: @Takhoffman

• Repair fallback: GitHub rejected the repair branch push because it updates workflow files and the ClawSweeper app token does not have workflows permission

Inherited issue-closing references from the source PR: Fixes #83887

Co-author credit kept:

• @hclsys: Co-authored-by: hcl [email protected]

fish notes: model gpt-5.5, reasoning high; reviewed against 0dbda6bd2bae.

Changed files

• CHANGELOG.md (modified, +1/-0)
• extensions/twitch/src/client-manager-registry.test.ts (added, +36/-0)
• extensions/twitch/src/client-manager-registry.ts (modified, +22/-0)

PR #84375: fix(twitch): evict registry entry even when disconnectAll throws (#83886)

• Repository: openclaw/openclaw
• Author: hclsys
• State: open | merged: False
• Link: https://github.com/openclaw/openclaw/pull/84375

Description (problem / solution / changelog)

Fixes #83886.

removeClientManager in extensions/twitch/src/client-manager-registry.ts:75 is the public teardown entry point — plugin.ts:208 calls it when an account is stopped or reconfigured. The body is:

const entry = registry.get(accountId);
if (!entry) return;
await entry.manager.disconnectAll();
registry.delete(accountId);

If disconnectAll() rejects (a quit() call on a broken socket, an underlying tmi.js exception, etc.), registry.delete(accountId) is never reached. The entry survives in the module-level Map. The next getOrCreateClientManager(accountId, …) short-circuits on the cache branch (if (existing) return existing.manager) and hands back the broken / partially-disconnected manager — silent stale-state hazard on top of the original disconnect failure.

Changes

• extensions/twitch/src/client-manager-registry.ts: wrap disconnectAll in try { … } finally { registry.delete; logger.info }. The finally block is unconditional, so the registry is cleaned up whether the disconnect succeeded or threw. The original rejection still propagates to the caller (finally doesn't suppress it).
• extensions/twitch/src/client-manager-registry.test.ts: NEW colocated test file. Mocks TwitchClientManager at the constructor boundary so disconnectAll can be driven to resolve or reject without standing up a real @twurple/chat client. Four cases — happy path eviction, no-op when no entry exists, regression: throwing disconnectAll still evicts, and end-to-end: after a failed remove, getOrCreateClientManager constructs a fresh manager (not the stale one). Uses vi.resetModules() to start each test with an empty module-level registry, so the test file is self-contained and doesn't depend on a separate test-only export.

Diff stat: 2 files, +105 / -6. Behavior change is strictly safer: any case that previously resolved still does identically; the previously-broken throwing-disconnect path now leaves the registry in a clean state.

Real behavior proof

• Behavior or issue addressed: Sanitized issue evidence — removeClientManager is the sole caller-facing teardown for registry, and registry is the cache layer that getOrCreateClientManager short-circuits through. The eviction must be atomic with the teardown intent, not gated on a successful disconnect.

• Real environment tested: Local Node 22.x. Probe at /tmp/probe_83886.mjs does both halves of the proof. (a) Parses the patched client-manager-registry.ts and verifies (i) removeClientManager wraps disconnectAll in try { … } finally { … }, (ii) registry.delete(accountId) is inside the finally block, (iii) no remaining unguarded delete-after-disconnectAll path. (b) Replays the registry semantics in pure JS for four scenarios — buggy shape (throwing disconnectAll → registry retains 'acc-1', confirming the #83886 symptom), patched shape (throwing disconnectAll → registry evicts 'acc-1' AND error still propagates to caller), patched happy path (clean disconnect still evicts), and patched no-op (missing key returns without throw).

• Exact steps or command run after this patch: node /tmp/probe_83886.mjs

• Evidence after fix:

PASS: removeClientManager now wraps disconnectAll in try { … } finally { … }
PASS: registry.delete(accountId) is called inside the finally block (always runs)
PASS: no remaining unguarded delete-after-disconnectAll path
PASS: replay (buggy): disconnectAll throws → registry retains 'acc-1' (confirms #83886)
PASS: replay (patched): disconnectAll throws → registry evicts 'acc-1' AND error still propagates
PASS: replay (patched, happy path): clean disconnectAll → entry evicted as expected
PASS: replay (patched, missing key): no-op, no throw

ALL CASES PASS

• Observed result after fix: A removeClientManager(accountId) call where the underlying client's disconnectAll throws still removes the entry from the module-level registry. The next getOrCreateClientManager(accountId, …) call gets a fresh manager bound to whatever logger the caller passed, instead of the stale failed-disconnect instance. Production cache-hit behavior within a healthy session (same accountId, no remove) is unchanged.

• What was not tested: A live @twurple/chat client with a real broken WebSocket — the included regression test mocks disconnectAll at the boundary, which is the same shape the broader test suite uses for this module (twitch-client.test.ts mocks @twurple/chat at the constructor level).

Audit (per CLAUDE rules — all 5 steps)

• Existing-helper check: No helper introduced — try { … } finally { … } is the canonical primitive for "always run cleanup regardless of failure." The registry.delete call is the same operation the buggy path was attempting; only the placement moves. PASS
• Shared-helper caller check: removeClientManager has 1 production caller (extensions/twitch/src/plugin.ts:208, account-stop / account-reconfigure path). The new behavior is strictly safer: any case that previously resolved still does so; the previously-broken throwing-disconnect path now cleans up correctly. PASS
• Broader-fix rival scan: gh pr list --search '83886 in:title,body' and gh pr list --search 'removeClientManager twitch' return no open PRs. The issue timeline cross-references issue #83887 (which is a sibling clawpatch finding that I shipped via #84244 — not a competing PR). PASS
• Recent-merge audit: git log --oneline -5 -- extensions/twitch/src/client-manager-registry.ts shows e1061a8b46 test(live): tolerate provider drift in release checks — unrelated. PASS
• Prototype-pollution scan: N/A — Map.delete and try/finally on locally-bound values.

Changed files

• extensions/twitch/src/client-manager-registry.test.ts (added, +97/-0)
• extensions/twitch/src/client-manager-registry.ts (modified, +12/-6)