Skip to content

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

Merged
steipete merged 4 commits into
openclaw:mainfrom
sevzq:fix/cloudbase-keepalive-lifecycle
Jun 10, 2026
Merged

Treat CloudBase MCP as a keep-alive stdio server#193
steipete merged 4 commits into
openclaw:mainfrom
sevzq:fix/cloudbase-keepalive-lifecycle

Conversation

@sevzq

@sevzq sevzq commented May 31, 2026

Copy link
Copy Markdown
Contributor

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

clawsweeper Bot commented May 31, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 9, 2026, 11:41 PM ET / 03:41 UTC.

Summary
The PR adds CloudBase to default keep-alive lifecycle detection, recognizes CloudBase package and binary command signatures, documents the default, and adds lifecycle plus CLI fast-path regression tests.

Reproducibility: no. high-confidence end-to-end reproduction is available from this review. Source inspection shows current main does not classify CloudBase as keep-alive by default, and maintainer comments describe partial live AUTH_PENDING behavior, but completed credential persistence has not been demonstrated.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
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 redacted real CloudBase device-code auth proof showing credential persistence after AUTH_PENDING.
  • [P1] Keep the PR body updated with the proof and exact command/version used so the next review can validate the live path.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Tests and maintainer-side partial live notes are not enough; before merge, add redacted terminal output, logs, or a recording showing CloudBase device auth completes and credentials persist after AUTH_PENDING, then update the PR body to trigger a fresh ClawSweeper review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] Changing CloudBase from ephemeral-by-default to keep-alive-by-default can leave a daemon-managed process running for existing users unless maintainers accept that provider-specific default and opt-out behavior.
  • [P1] The available live evidence is still incomplete: it reportedly showed the real CloudBase child surviving AUTH_PENDING, but did not show completed device authorization or persisted credentials.

Maintainer options:

  1. Require completed CloudBase auth proof (recommended)
    Before merge, require redacted terminal output, logs, or a recording from a real CloudBase device-code authorization showing the process remains alive after AUTH_PENDING and credentials are persisted.
  2. Accept the provider default deliberately
    Maintainers can merge with the current partial live evidence if they explicitly accept the new long-running default and the remaining credential-persistence uncertainty.
  3. Defer or make CloudBase opt-in
    If completed CloudBase proof is not available, pause this PR or switch the behavior to documented opt-in keep-alive configuration instead of a new default.

Next step before merge

  • [P1] The remaining blocker is human proof and maintainer acceptance of a provider-specific default, not a safe automated code repair.

Security
Cleared: The diff changes lifecycle heuristics, docs, and tests only; it does not add dependencies, CI execution, secret handling, or credential storage code.

Review details

Best possible solution:

Land the narrow allowlist change only after redacted real CloudBase device-code auth proof shows polling continues after AUTH_PENDING and credentials persist, while keeping explicit ephemeral config authoritative.

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

No high-confidence end-to-end reproduction is available from this review. Source inspection shows current main does not classify CloudBase as keep-alive by default, and maintainer comments describe partial live AUTH_PENDING behavior, but completed credential persistence has not been demonstrated.

Is this the best way to solve the issue?

Unclear until live CloudBase authorization is proven. If that proof is added, the current approach is a narrow maintainable fix because it extends the existing lifecycle allowlist and leaves explicit ephemeral config on the config-aware path.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal compatibility improvement for a specific MCP server/auth flow with limited blast radius but real merge-proof requirements.
  • merge-risk: 🚨 compatibility: The PR changes CloudBase's default lifecycle from ephemeral to daemon keep-alive, which can alter upgrade behavior for existing CloudBase users.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish 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: Tests and maintainer-side partial live notes are not enough; before merge, add redacted terminal output, logs, or a recording showing CloudBase device auth completes and credentials persist after AUTH_PENDING, then update the PR body to trigger a fresh ClawSweeper review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Current main behavior: Current main's default keep-alive set and command-signature list include chrome-devtools, mobile-mcp, and playwright, but not CloudBase, so the requested CloudBase default is not already implemented on main. (src/lifecycle.ts:3, 3e27b64021c9)
  • PR lifecycle change: The PR head adds cloudbase to DEFAULT_KEEP_ALIVE and recognizes both @cloudbase/cloudbase-mcp and cloudbase-mcp command fragments. (src/lifecycle.ts:3, b7c0b2ba475e)
  • Explicit override safety: On the PR head, raw lifecycle config is still coerced and returned before the default allowlist is consulted, so lifecycle: "ephemeral" remains authoritative absent env forcing. (src/lifecycle.ts:48, b7c0b2ba475e)
  • Fast-path blocker fixed: The PR head's DAEMON_FAST_PATH_SERVERS set does not include CloudBase, and the fast-path check returns false for servers outside that set, preserving the config-aware runtime path for direct CloudBase calls. (src/cli.ts:17, b7c0b2ba475e)
  • Regression coverage: The PR adds tests for CloudBase package commands, CloudBase binary commands, explicit ephemeral overrides, and direct CloudBase calls staying on the runtime path. (tests/lifecycle.test.ts:44, b7c0b2ba475e)
  • Maintainer verification context: The latest maintainer comment says unit, check, full test, and autoreview passes were clean on head b7c0b2b, but also says real CloudBase authorization could not complete because no Tencent Cloud session or credentials were available. (b7c0b2ba475e)

Likely related people:

  • steipete: Peter Steinberger introduced the daemon keep-alive mode, later carried the lifecycle/fast-path performance work, and authored the PR-head fix that preserved CloudBase lifecycle overrides. (role: feature owner and recent area contributor; confidence: high; commits: 2b570ac46dc4, 6012708bf3f5, b7c0b2ba475e; files: src/lifecycle.ts, src/cli.ts, tests/cli-daemon-fast-path.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.

@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

LDMB123 commented May 31, 2026

Copy link
Copy Markdown
Contributor

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.

@steipete steipete force-pushed the fix/cloudbase-keepalive-lifecycle branch from c4e6760 to 360f688 Compare June 8, 2026 23:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 360f688f52

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/cli.ts Outdated

const FORCE_EXIT_GRACE_MS = 50;
const DAEMON_FAST_PATH_SERVERS = new Set(['chrome-devtools', 'mobile-mcp', 'playwright']);
const DAEMON_FAST_PATH_SERVERS = new Set(['chrome-devtools', 'mobile-mcp', 'playwright', 'cloudbase']);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect CloudBase ephemeral overrides before fast-pathing

When a user names a server cloudbase but sets lifecycle: "ephemeral", this new fast-path entry routes mcporter call cloudbase.<tool> directly to the daemon before the config is loaded, so the explicit opt-out in resolveLifecycle is never consulted. This is specific to direct calls matching the server name in DAEMON_FAST_PATH_SERVERS; users would need an env disable to avoid the daemon even though the documented per-server override should be enough.

Useful? React with 👍 / 👎.

@steipete

Copy link
Copy Markdown
Collaborator

Maintainer fix pushed for the explicit lifecycle blocker.

Change:

  • Removed CloudBase from the pre-config daemon fast-path allowlist.
  • Direct cloudbase.<tool> calls now load configuration first, so explicit lifecycle: "ephemeral" remains authoritative.
  • Default CloudBase definitions still resolve to keep-alive and route through the normal config-aware daemon wrapper.

Verification on head b7c0b2b:

  • pnpm exec vitest run tests/lifecycle.test.ts tests/cli-daemon-fast-path.test.ts -> 2 files, 10 tests passed.
  • pnpm check -> formatting, lint, and typecheck passed.
  • pnpm test -> 125 files passed, 1 skipped; 777 tests passed, 3 skipped.
  • Local patch autoreview -> clean.
  • Full branch autoreview against origin/main -> clean.

The remaining blocker is unchanged: real CloudBase device authorization must complete so credential persistence can be demonstrated. Prior live testing proved the real @cloudbase/cloudbase-mcp@2.21.1 child remains alive after AUTH_PENDING, while an explicit ephemeral control leaves zero surviving new CloudBase processes; authorization could not complete because no Tencent Cloud session/credentials were available.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 10, 2026

Copy link
Copy Markdown

🦞🧹
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:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 10, 2026
@steipete

Copy link
Copy Markdown
Collaborator

Best-effort live validation and maintainer decision:

  • Ran the real @cloudbase/cloudbase-mcp@2.21.1 package with isolated HOME and XDG directories.
  • Device authentication returned AUTH_PENDING.
  • The same mcporter daemon and CloudBase child processes remained alive for 78 minutes after the response, preserving the background polling path this PR fixes.
  • The inverse control with explicit lifecycle: "ephemeral" left zero surviving CloudBase processes.
  • Tencent Cloud International accepted the device authorization route, but requires provisioning a paid CloudBase environment before authorization can finish. The minimum available environment was USD 1.9948/month, so credential persistence was not exercised.
  • Focused tests, pnpm check, the full test suite, and branch autoreview passed. CI is green on Ubuntu, macOS, and Windows.

Maintainer explicitly accepts the remaining credential-persistence uncertainty and the provider-specific keep-alive default. Explicit ephemeral configuration remains authoritative.

@steipete steipete merged commit 4813cdf into openclaw:main Jun 10, 2026
3 checks passed
@steipete

Copy link
Copy Markdown
Collaborator

Landed in 4813cdf.

Final proof:

  • Focused lifecycle and daemon-routing tests passed.
  • pnpm check passed.
  • Full suite passed: 125 files passed, 1 skipped; 777 tests passed, 3 skipped.
  • Branch autoreview reported no accepted/actionable findings.
  • CI passed on Ubuntu, macOS, and Windows.
  • Real @cloudbase/cloudbase-mcp@2.21.1 validation returned AUTH_PENDING, and the same daemon plus CloudBase child remained alive for 78 minutes. The explicit ephemeral inverse control left zero surviving CloudBase processes.
  • Completed credential persistence remained untested because Tencent requires a paid CloudBase environment. Maintainer explicitly accepted that bounded uncertainty.

Thanks @sevzq.

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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

3 participants