Skip to content

Treat CloudBase MCP as a keep-alive stdio server#193

Open
sevzq wants to merge 1 commit into
openclaw:mainfrom
sevzq:fix/cloudbase-keepalive-lifecycle
Open

Treat CloudBase MCP as a keep-alive stdio server#193
sevzq wants to merge 1 commit into
openclaw:mainfrom
sevzq:fix/cloudbase-keepalive-lifecycle

Conversation

@sevzq
Copy link
Copy Markdown

@sevzq sevzq commented May 31, 2026

Summary

  • mark CloudBase MCP as a default keep-alive stdio server
  • recognize both @cloudbase/cloudbase-mcp package invocations and cloudbase-mcp binary invocations
  • keep explicit lifecycle: "ephemeral" overrides respected

Context

CloudBase MCP device-code auth returns AUTH_PENDING after emitting the device challenge, then continues polling in the background so credentials can be persisted after the browser authorization succeeds. When CloudBase MCP is invoked as an ephemeral stdio server, that process exits immediately after returning AUTH_PENDING, so the polling loop is cut off before credentials can be written.

Treating CloudBase MCP as keep-alive matches that stateful auth flow and lets the daemon keep the MCP process alive until device-code polling finishes.

Tests

  • pnpm exec vitest run tests/lifecycle.test.ts
  • pnpm format:check
  • pnpm typecheck
  • pnpm lint:oxlint
  • smoke: resolveLifecycle(...) for @cloudbase/cloudbase-mcp@latest returns { "mode": "keep-alive" }

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 31, 2026

Codex review: needs real behavior proof before merge. Reviewed May 31, 2026, 6:17 AM ET / 10:17 UTC.

Summary
The PR adds CloudBase to the default keep-alive allowlist and command-signature matcher, plus lifecycle tests for npx, binary, and explicit ephemeral override cases.

Reproducibility: yes. for the lifecycle-classifier symptom: current main has no CloudBase allowlist entry or command signature, so source inspection shows CloudBase stays ephemeral by default. No live CloudBase device-code auth reproduction was provided or run in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Diff size: 2 files, 31 additions, 1 deletion. The patch is small, but it touches the default lifecycle classifier used by config normalization and persisted ad-hoc servers.
  • Lifecycle default change: 1 allowlist label and 2 command fragments added. This is the compatibility-sensitive behavior change maintainers need to consciously accept.
  • CloudBase tests: 3 cases added. The added tests cover both intended command forms and the explicit ephemeral opt-out path.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup 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 redacted terminal output, logs, or a recording from a real MCPorter + CloudBase device-code auth run showing the process stays alive after AUTH_PENDING and credentials persist.
  • Include the exact command/config used and redact private data such as API keys, phone numbers, IP addresses, and non-public endpoints.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists tests and a helper smoke check, but external PRs need redacted real CloudBase/MCPorter terminal output, logs, or a recording; updating the PR body should trigger re-review, 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.

Risk before merge

  • [P1] The PR changes the default lifecycle for matching CloudBase servers from ephemeral to daemon keep-alive, so existing configs may keep a long-running process unless users opt out with lifecycle: "ephemeral" or MCPORTER_DISABLE_KEEPALIVE.
  • [P1] The PR has unit-level coverage but no after-fix evidence from a real CloudBase device-code auth run showing AUTH_PENDING polling continues and credentials persist.

Maintainer options:

  1. Require live CloudBase auth proof (recommended)
    Ask for redacted terminal logs or a recording showing MCPorter keeps CloudBase MCP alive through AUTH_PENDING until credentials are persisted.
  2. Accept the provider-specific default
    Maintainers may choose to accept the new default based on the existing opt-out path and the focused lifecycle tests.
  3. Keep CloudBase opt-in only
    If a provider-specific default is not desired, pause this PR and document lifecycle: "keep-alive" as the supported CloudBase workaround instead.

Next step before merge

  • [P1] The remaining blocker is contributor-supplied real behavior proof and maintainer acceptance of the default lifecycle change, not a narrow automated repair.

Security
Cleared: No concrete security or supply-chain issue was found; the diff changes only lifecycle matching and tests, with no dependencies, workflows, scripts, or secret-handling code added.

Review details

Best possible solution:

Land the narrow allowlist/test change after real CloudBase device-code proof and maintainer acceptance of the default lifecycle change; otherwise keep users on the explicit lifecycle configuration workaround.

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

Yes for the lifecycle-classifier symptom: current main has no CloudBase allowlist entry or command signature, so source inspection shows CloudBase stays ephemeral by default. No live CloudBase device-code auth reproduction was provided or run in this read-only review.

Is this the best way to solve the issue?

Unclear until live proof is added; the implementation is a narrow match for the existing allowlist pattern, but the best merge path needs real CloudBase auth evidence for the default behavior change.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal provider compatibility improvement with limited blast radius, not an urgent core runtime outage.
  • add merge-risk: 🚨 compatibility: Merging changes the default lifecycle for matching CloudBase configurations from ephemeral to daemon keep-alive.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists tests and a helper smoke check, but external PRs need redacted real CloudBase/MCPorter terminal output, logs, or a recording; updating the PR body should trigger re-review, 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:

  • P2: This is a normal provider compatibility improvement with limited blast radius, not an urgent core runtime outage.
  • merge-risk: 🚨 compatibility: Merging changes the default lifecycle for matching CloudBase configurations from ephemeral to daemon keep-alive.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists tests and a helper smoke check, but external PRs need redacted real CloudBase/MCPorter terminal output, logs, or a recording; updating the PR body should trigger re-review, 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.
Evidence reviewed

What I checked:

  • Current main lifecycle defaults: Current main's DEFAULT_KEEP_ALIVE set contains chrome-devtools, mobile-mcp, and playwright, but not CloudBase. (src/lifecycle.ts:3, 2bf7a5eab23f)
  • Current main command signatures: Current main recognizes chrome-devtools, mobile-mcp, and playwright command fragments, so CloudBase package and binary invocations are not currently canonicalized as keep-alive. (src/lifecycle.ts:18, 2bf7a5eab23f)
  • Explicit lifecycle precedence: The existing resolver returns a parsed raw lifecycle before checking the default allowlist, so the PR's explicit ephemeral override test matches the source path. (src/lifecycle.ts:47, 2bf7a5eab23f)
  • PR source change: The PR head adds cloudbase to DEFAULT_KEEP_ALIVE and adds @cloudbase/cloudbase-mcp plus cloudbase-mcp to KEEP_ALIVE_COMMANDS. (src/lifecycle.ts:3, c4e67606b942)
  • PR test coverage: The PR head adds three lifecycle tests covering the CloudBase npm package invocation, binary invocation, and explicit ephemeral override. (tests/lifecycle.test.ts:40, c4e67606b942)
  • Daemon allowlist policy: The daemon design docs describe a hardcoded default allowlist for stateful stdio servers with per-server opt-out, which is the mechanism this PR extends. (docs/daemon.md:34, 2bf7a5eab23f)

Likely related people:

  • steipete: GitHub commit history for src/lifecycle.ts shows steipete authored the daemon keep-alive mode and keep-alive override work; local blame on current lifecycle lines also points to the release commit by Peter Steinberger. (role: feature owner / recent area contributor; confidence: high; commits: 2b570ac46dc4, 676cc15cbb75, 94e65ba0572e; files: src/lifecycle.ts, tests/lifecycle.test.ts, docs/daemon.md)
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.

@clawsweeper 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels May 31, 2026
Copy link
Copy Markdown
Contributor

LDMB123 commented May 31, 2026

Maintainer-side local verification for current head c4e67606b9421dfd6e94a5e87ff8cb20335c3dd1, since the GitHub Actions run is still waiting for maintainer approval and no approve/run control is exposed to this account:

  • pnpm exec vitest run tests/lifecycle.test.ts -> 5 passed.
  • pnpm format:check -> passed.
  • pnpm typecheck -> passed.
  • pnpm lint:oxlint -> passed with 0 warnings/errors.

This confirms the unit-level lifecycle behavior on the PR head. It does not replace the requested real CloudBase device-code auth proof; that still requires a real CloudBase authorization flow showing the process remains alive after AUTH_PENDING and credentials persist. I could not approve the waiting workflow from the current browser/account context, and connector rerun/approval actions are not available with this integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants