Skip browser cleanup when browser is disabled by poison · Pull Request #88303 · openclaw/openclaw

poison · 2026-05-30T10:58:34Z

Summary

skip browser tab cleanup during auto-reply rollover when root browser support or the default browser plugin is disabled
skip the same browser cleanup path for gateway sessions.reset using the shared effective browser-plugin config check
add coverage for root-disabled and plugins.entries.browser.enabled=false behavior in both reset paths

Real behavior proof

Behavior addressed:
Disabled-browser OpenClaw homes were still paying the browser maintenance cold path during daily session rollover / reset. In our real bot runtime this showed up as the first turn of the day taking roughly 2 minutes even though browser.enabled=false and no browser plugin was configured.
Real environment tested:
Local checkout of this PR head (a4731e704a7f3bebce97f1406df94f0804aa4fd2) with Node/tsx, replaying copied and redacted real OpenClaw home data from two Kubernetes bot PVCs: one production bot PVC and one staging bot PVC. Both copied homes have browser.enabled=false and no browser plugin entry.

Exact steps or command run after this patch:
Ran the real initSessionState rollover path from the PR-head source against each copied OpenClaw home, restoring the previous reset transcript into place so the next message triggers the same daily rollover path.

node --max-old-space-size=512 --import /tmp/openclaw-patch-32/node_modules/tsx/dist/loader.mjs /tmp/openclaw-pr-real-session-init-repro.mjs production-pvc /tmp/aidy-openclaw-pvc-dumps/ruthje 641c8e17-7614-4e92-a0e4-5ca450abfd36
node --max-old-space-size=512 --import /tmp/openclaw-patch-32/node_modules/tsx/dist/loader.mjs /tmp/openclaw-pr-real-session-init-repro.mjs staging-pvc /tmp/aidy-openclaw-pvc-dumps/oliver a363c9ad-6ec8-4fa5-8a7e-5db24add8ca6

Evidence after fix:
Terminal output from the PR-head checkout using copied/redacted real OpenClaw homes:

[session-init] store-load agent=main session=(no-session) elapsedMs=3 path=/var/folders/.../openclaw-real-init-production-pvc-.../.openclaw/agents/main/sessions/sessions.json
{
  "bot": "production-pvc",
  "repo": "/tmp/openclaw-patch-32",
  "source": "redacted real OpenClaw home copied from bot PVC",
  "browserEnabled": false,
  "elapsedMs": 193.403,
  "isNewSession": true,
  "previousSessionMatched": true,
  "resetArtifacts": 2,
  "nextStoreBytes": 700722,
  "nextMainSessionChanged": true,
  "eventLoopDelayMaxMs": 11.977,
  "eventLoopDelayMeanMs": 10.209,
  "rssDeltaMb": 33.484,
  "heapDeltaMb": -25.816,
  "rssMb": 389.391,
  "heapUsedMb": 135.433
}

[session-init] store-load agent=main session=(no-session) elapsedMs=7 path=/var/folders/.../openclaw-real-init-staging-pvc-.../.openclaw/agents/main/sessions/sessions.json
{
  "bot": "staging-pvc",
  "repo": "/tmp/openclaw-patch-32",
  "source": "redacted real OpenClaw home copied from bot PVC",
  "browserEnabled": false,
  "elapsedMs": 190.772,
  "isNewSession": true,
  "previousSessionMatched": true,
  "resetArtifacts": 2,
  "nextStoreBytes": 2862324,
  "nextMainSessionChanged": true,
  "eventLoopDelayMaxMs": 11.911,
  "eventLoopDelayMeanMs": 10.255,
  "rssDeltaMb": 50.094,
  "heapDeltaMb": -10.56,
  "rssMb": 407.156,
  "heapUsedMb": 159.851
}

Observed result after fix:
Both real copied homes completed the session rollover as new sessions with the expected previous session matched, the main session changed, and event-loop max delay stayed around 12ms. The disabled-browser path no longer loads browser cleanup and no longer reproduces the minute-plus first-turn stall. Before this change, the same investigation measured the browser cleanup stage alone at about 43.9s locally, and the live production/staging bot logs showed dispatch-to-turn gaps of about 112-122s during the daily rollover.
What was not tested:
I did not run a full packaged OpenClaw release build or a live browser-enabled cleanup session. This proof is scoped to the changed disabled-browser behavior using real copied OpenClaw home data, plus the focused regression tests listed below.
Proof limitations or environment constraints:
The PVC data and paths are redacted to avoid exposing private bot identifiers, user content, endpoints, or secrets. The copied configs were sanitized before local replay.
Before evidence:
During the original incident investigation, the copied production and staging homes both had browser.enabled=false; the unpatched rollover still invoked closeTrackedBrowserTabsForSessions, returned 0 closed tabs, and spent about 43.9s in the browser maintenance stage locally. Live bot logs around the same daily rollover showed event-loop stalls and first-turn dispatch-to-turn gaps of roughly 112-122s.

Why

When browser support is disabled, session reset can still call the browser maintenance facade. That facade may cold-load the bundled browser surface even though there are no browser tabs to clean up, adding avoidable latency and memory pressure to non-browser runtimes.

Validation

node scripts/run-vitest.mjs src/auto-reply/reply/session.test.ts src/gateway/server.sessions.reset-cleanup.test.ts
git diff --check

clawsweeper · 2026-05-30T11:00:41Z

Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 4:15 PM ET / 20:15 UTC.

Summary
The PR routes auto-reply rollover and gateway sessions.reset browser cleanup through a config-aware lifecycle helper, makes browser maintenance skip inactive browser-plugin loads, and adds disabled-browser regression coverage plus one MCP test wait fix.

PR surface: Source +23, Tests +166. Total +189 across 9 files.

Reproducibility: yes. Current main visibly calls browser cleanup from rollover and sessions.reset without checking disabled-browser config, and the PR body provides real copied-home evidence of the pre-fix stall path.

Review metrics: 1 noteworthy metric.

Disabled-browser config behavior: 2 existing disable paths changed. browser.enabled=false and plugins.entries.browser.enabled=false now suppress lifecycle tab cleanup, so maintainers should consciously accept the upgrade behavior before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

[P1] Add or record current-head terminal proof for disabled-browser rollover/reset behavior.
Confirm the focused tests on the current head after the latest force-push.

Proof guidance:

[P1] Needs stronger real behavior proof before merge: The PR body has strong terminal proof from copied/redacted real homes, but it targets an earlier head and does not fully cover the latest activation-aware browser-maintenance implementation. 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.

Risk before merge

[P1] Existing homes with browser.enabled=false or plugins.entries.browser.enabled=false will no longer run browser tab cleanup during rollover/reset; that appears intentional, but it is an upgrade-sensitive behavior change if any setup relied on cleanup despite disabling browser support.
[P1] The real terminal proof in the PR body targets an earlier head, while the current head includes later browser-maintenance activation logic; maintainers should require current-head proof or record their own verification before merge.

Maintainer options:

Require latest-head disabled-browser proof (recommended)
Ask for or record terminal/log proof against the current head showing disabled-browser rollover/reset avoids the browser maintenance cold path, then verify the focused tests.
Accept the cleanup semantic change
If maintainers agree that disabling browser support must also disable lifecycle tab cleanup, merge with that compatibility choice called out in the PR body or squash message.
Preserve cleanup unless explicitly strict
If stale tracked-tab cleanup is still required for disabled-browser homes, pause this PR and change the design to preserve default cleanup with a separate opt-out or migration story.

Next step before merge

[P1] Human review should confirm the compatibility-sensitive disabled-browser cleanup semantics and require current-head proof/checks; there is no narrow automated code repair to queue.

Security
Cleared: No concrete security or supply-chain regression was found; the diff does not change dependencies, workflows, lockfiles, secrets handling, or external artifact execution.

Review details

Best possible solution:

Land a final head that treats explicit browser disable switches as authoritative for lifecycle cleanup while preserving enabled-browser cleanup and recording current-head proof and focused validation.

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

Yes. Current main visibly calls browser cleanup from rollover and sessions.reset without checking disabled-browser config, and the PR body provides real copied-home evidence of the pre-fix stall path.

Is this the best way to solve the issue?

Yes, with a merge gate. Centralizing the config-aware lifecycle skip and making the facade activation-aware is the narrow maintainable shape, but the latest head still needs maintainer-accepted compatibility semantics and current-head proof.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body has strong terminal proof from copied/redacted real homes, but it targets an earlier head and does not fully cover the latest activation-aware browser-maintenance implementation. 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 rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

P1: The PR addresses a real first-turn stall during session rollover/reset in disabled-browser runtimes, affecting agent responsiveness for existing bot homes.
merge-risk: 🚨 compatibility: The patch changes what existing disabled-browser configurations do during rollover/reset by no longer invoking browser tab cleanup.
rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body has strong terminal proof from copied/redacted real homes, but it targets an earlier head and does not fully cover the latest activation-aware browser-maintenance implementation. 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

PR surface:

Source +23, Tests +166. Total +189 across 9 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	4	38	15	+23
Tests	5	174	8	+166
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	9	212	23	+189

What I checked:

Current main rollover behavior: Current main calls browser cleanup during session rollover directly, with no disabled-browser config check at the cleanup call site. (src/auto-reply/reply/session.ts:873, a2b2c4a76c79)
Current main gateway reset behavior: Current main sessions.reset cleanup calls closeTrackedBrowserTabsForSessions directly after collecting reset keys, without passing or checking browser/plugin activation config. (src/gateway/session-reset-service.ts:378, a2b2c4a76c79)
PR lifecycle gate: The PR head makes cleanupBrowserSessionsForLifecycleEnd return before loading browser cleanup when either browser.enabled=false or plugins.entries.browser.enabled=false. (src/browser-lifecycle-cleanup.ts:16, 1a7e7f0c8efc)
PR facade activation gate: The PR head changes browser maintenance to check activated bundled-plugin access before loading browser-maintenance.js and to no-op when the surface is inactive. (src/plugin-sdk/browser-maintenance.ts:23, 1a7e7f0c8efc)
Regression coverage: The PR head adds rollover and sessions.reset tests that assert cleanup is skipped for root-disabled browser config and disabled browser plugin entry, plus facade-level tests for inactive browser plugin cleanup. (src/auto-reply/reply/session.test.ts:1918, 1a7e7f0c8efc)
Real proof scope: The PR body supplies terminal proof from copied/redacted real OpenClaw homes on commit a4731e7, but the current PR head is 1a7e7f0 and later commits changed the browser-maintenance implementation path. (1a7e7f0c8efc)

Likely related people:

shakkernerd: Current-main blame for the browser maintenance facade, lifecycle helper, auto-reply rollover call, and gateway reset cleanup maps to commit e89417d in this checkout. (role: current-main code path author; confidence: medium; commits: e89417d77b54; files: src/plugin-sdk/browser-maintenance.ts, src/browser-lifecycle-cleanup.ts, src/auto-reply/reply/session.ts)
steipete: The latest branch commits revise the disabled-browser cleanup implementation, remove the earlier config helper, add plugin-entry handling, and stabilize an adjacent agents test. (role: recent branch refiner; confidence: high; commits: 5ef8d6efc02b, 6852403782ba, 1a7e7f0c8efc; files: src/browser-lifecycle-cleanup.ts, src/plugin-sdk/browser-maintenance.ts, src/auto-reply/reply/session.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.

poison · 2026-05-30T14:29:49Z

@clawsweeper re-review

clawsweeper · 2026-05-30T14:29:52Z

🦞🧹
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/26686342555
Updated: 2026-05-30T14:37:55.400Z

Avoid loading the browser maintenance surface during session reset when root browser support or the default browser plugin is explicitly disabled. This keeps non-browser runtimes from paying the browser facade cold-load cost while preserving cleanup for enabled/default browser configs. Tests cover auto-reply rollover and gateway sessions.reset for both root-disabled and plugin-entry-disabled browser configs. 🤖 Generated with Codex Co-Authored-By: Codex <[email protected]>

steipete · 2026-05-30T20:16:33Z

Behavior addressed: Browser lifecycle cleanup now skips browser session cleanup when either root browser support is disabled or the bundled browser plugin entry is disabled. The browser maintenance facade also checks plugin activation before using a cached browser surface.

Real environment tested: Local macOS checkout plus GitHub Actions on head SHA 1a7e7f0.

Exact steps or command run after this patch:

pnpm test src/plugin-sdk/browser-maintenance.test.ts src/browser-lifecycle-cleanup.test.ts src/auto-reply/reply/session.test.ts src/gateway/server.sessions.reset-cleanup.test.ts src/agents/auth-profiles/usage.test.ts
pnpm test src/agents/agent-bundle-mcp-runtime.test.ts
git diff --check
pnpm build
.agents/skills/autoreview/scripts/autoreview --mode local --parallel-tests "pnpm test src/plugin-sdk/browser-maintenance.test.ts src/browser-lifecycle-cleanup.test.ts src/auto-reply/reply/session.test.ts src/gateway/server.sessions.reset-cleanup.test.ts src/agents/auth-profiles/usage.test.ts src/agents/agent-bundle-mcp-runtime.test.ts"
gh run cancel 26692970390 --repo openclaw/openclaw; gh run rerun 26692970390 --repo openclaw/openclaw --failed, after that CI run became stuck on two long shards and one known flaky agents-core test before the final test fix.

Evidence after fix: Local focused Vitest shards passed, local agents-core MCP runtime test passed, git diff check passed, build passed, and autoreview reported no accepted/actionable findings. GitHub Actions passed on final head: CI 26693713166, CodeQL 26693713159, CodeQL Critical Quality 26693713157, OpenGrep PR Diff 26693713125, Workflow Sanity 26693713149, Dependency Guard 26693712478, Real behavior proof 26693717405 / 26693858594.

Observed result after fix: Disabled browser configs no longer load the browser cleanup surface during auto-reply session rollover or gateway session reset, while enabled/default configs still call cleanup. The resource-only MCP server test now waits for the asynchronous rejection log instead of racing the server write.

What was not tested: No live browser tab was opened or closed manually; coverage is via unit/runtime tests and CI.

…penclaw#91736, openclaw#88303) Honor top-level and profile cdpUrl for existing-session profiles, allow profile creation with cdpUrl, and skip browser lifecycle cleanup when browser is disabled. Co-authored-by: Cursor <[email protected]>

openclaw-barnacle Bot added gateway Gateway runtime size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 30, 2026

clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 30, 2026

openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 30, 2026

poison marked this pull request as ready for review May 30, 2026 11:09

poison force-pushed the codex/skip-disabled-browser-cleanup branch from f7a3976 to a4731e7 Compare May 30, 2026 14:26

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

steipete self-assigned this May 30, 2026

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 force-pushed the codex/skip-disabled-browser-cleanup branch from 7936844 to ca9a7a1 Compare May 30, 2026 18:24

openclaw-barnacle Bot added size: M and removed size: S proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 30, 2026

steipete force-pushed the codex/skip-disabled-browser-cleanup branch from 3371bdd to 6cef1cb Compare May 30, 2026 19:07

openclaw-barnacle Bot removed the agents Agent runtime and tooling label May 30, 2026

poison and others added 3 commits May 30, 2026 20:32

fix: skip browser cleanup for disabled browser plugin

5ef8d6e

fix: honor disabled browser plugin cleanup

6852403

steipete force-pushed the codex/skip-disabled-browser-cleanup branch from 6cef1cb to 6852403 Compare May 30, 2026 19:33

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

test: wait for resource-only mcp rejection log

1a7e7f0

openclaw-barnacle Bot added agents Agent runtime and tooling and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 30, 2026

steipete merged commit 4ac90a5 into openclaw:main May 30, 2026
152 of 160 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Skip browser cleanup when browser is disabled#88303

Skip browser cleanup when browser is disabled#88303
steipete merged 4 commits into
openclaw:mainfrom
poison:codex/skip-disabled-browser-cleanup

poison commented May 30, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

poison commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

steipete commented May 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Summary

Real behavior proof

Why

Validation

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!

poison 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!

2 participants

poison commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading