Skip to content

improve: skip Gateway migration preflight without legacy state#96234

Closed
efpiva wants to merge 1 commit into
openclaw:mainfrom
efpiva:edpiva/gateway-startup-migration-preflight
Closed

improve: skip Gateway migration preflight without legacy state#96234
efpiva wants to merge 1 commit into
openclaw:mainfrom
efpiva:edpiva/gateway-startup-migration-preflight

Conversation

@efpiva

@efpiva efpiva commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

openclaw gateway run paid doctor startup preflight work even when no legacy state was present. Earlier versions of this PR skipped too aggressively; the current version keeps the skip only after checking the legacy inputs that the doctor migration path can still handle.

Why This Change Was Made

This change gates the Gateway foreground startup doctor migration preflight behind legacy-input detection, matching the existing read-only-command intent while preserving automatic migration for known legacy sources.

The detector now covers or delegates to:

  • core legacy state directories, sidecars, pending SQLite archives, delivery queues, debug-proxy, config-health/update/current-conversation state, cron legacy stores/run logs, and exec approvals
  • canonical and configured session stores, including JSON5 stores, non-default/retired agent stores, ACP metadata, stale migrated session-file repairs, and shared-store cross-agent orphan-key behavior
  • bundled channel legacy state sidecars, including Telegram sidecars without sessions.json, named-account Telegram sidecars, and configured/default account paths
  • shipped plugin doctor migration sources (Active Memory, Phone Control, ACPX, Device Pair, Matrix, Nostr, MSTeams, Voice Call, Memory Core, Memory Wiki, Workboard, Telegram)
  • generic installed plugin doctor-contract-api state migrations by invoking their public detectLegacyState contract after config load
  • conservative fallback when the shared-state SQLite probe cannot run

It also avoids persistent false positives from already-archived *.migrated plugin sources, normal current plugin-state rows, current canonical agent directories, current custom session stores, current shared debug-proxy SQLite DBs, and unmigratable Voice Call files under OPENCLAW_HOME when the Voice Call default is OS-home based.

User Impact

Gateway startup still runs doctor preflight when legacy/migratable state exists. On no-legacy startup, the reviewed patch keeps a meaningful Gateway-ready improvement, though smaller than the initial draft because the final version preserves generic plugin doctor migration detection.

Latest benchmark artifact:

  • .artifacts/gateway-startup-upstream/final-reviewed-gateway-startup.json
  • log: .artifacts/gateway-startup-upstream/final-reviewed-gateway-startup.log

P50 Gateway-ready deltas vs the original origin/main baseline artifact .artifacts/gateway-startup-upstream/20260623T215700Z-main-baseline/gateway-startup.json:

case baseline p50 ready ms reviewed candidate p50 ready ms delta
default 3380.7 3104.4 -276.3ms (-8.2%)
skipChannels 3087.7 2482.2 -605.5ms (-19.6%)
oneInternalHook 3060.8 2463.9 -596.9ms (-19.5%)
allInternalHooks 3056.5 2468.0 -588.5ms (-19.3%)
fiftyPlugins 3291.8 2692.4 -599.4ms (-18.2%)
fiftyStartupLazyPlugins 3170.3 2496.9 -673.4ms (-21.2%)

Evidence

Passed locally on the reviewed head 0149e9d9c1:

  • pnpm test src/cli/program/config-guard.test.ts
    • 88 tests passed
  • pnpm test extensions/msteams/doctor-contract-api.test.ts
    • 5 tests passed
  • OPENCLAW_TSGO_HEAVY_CHECK_LOCK_HELD=1 pnpm tsgo:core
  • pnpm build:docker
  • codex review --base origin/main
    • result: No discrete correctness issues were identified in the changed config-guard migration detection paths or adjacent migration contracts reviewed.
  • pnpm test:startup:gateway --runs 5 --warmup 1 --output .artifacts/gateway-startup-upstream/final-reviewed-gateway-startup.json

Earlier full-suite caveat still applies:

  • Full pnpm test was run before the review-fix loop and failed in unrelated shards outside this patch's two changed files. Failures included provider catalog expectations, timing/process tests, backup late-SQLite tests, security scan allowlist tests, and script resolver tests. The failure log is .artifacts/gateway-startup-upstream/pnpm-test-gateway-preflight-final.log.

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: L labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 24, 2026, 3:43 PM ET / 19:43 UTC.

Summary
This PR makes openclaw gateway run skip the doctor state-migration preflight unless lightweight legacy-state probes find known core, session, channel, or plugin migration inputs, with expanded config-guard tests.

PR surface: Source +728, Tests +876. Total +1604 across 2 files.

Reproducibility: yes. by source inspection: the PR awaits plugin doctor detectors without the current collector's try/catch, so a detector exception rejects ensureConfigReady. I did not run a failing detector fixture in this read-only review.

Review metrics: 1 noteworthy metric.

  • Gateway migration gate: 1 command added to legacy-input gating. gateway run now decides whether to run automatic state migration from detector coverage, so detector failure behavior is merge-critical.

Stored data model
Persistent data-model change detected: database schema: src/cli/program/config-guard.test.ts, serialized state: src/cli/program/config-guard.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🐚 platinum hermit
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Wrap plugin doctor detector failures so Gateway falls back to the existing preflight path.
  • [P2] Add a focused config-guard regression test with a throwing plugin detector on gateway run.

Risk before merge

  • [P1] Merging as-is can make gateway run fail during startup when a plugin doctor detector throws, instead of preserving the current warning-and-continue doctor behavior.
  • [P1] A missed or failed detector can skip automatic migration for existing session or plugin state during Gateway startup.

Maintainer options:

  1. Preserve fail-soft detector behavior (recommended)
    Wrap the plugin-doctor detector loop so exceptions fall back to the doctor preflight instead of rejecting Gateway startup.
  2. Accept stricter startup failure
    Maintainers could intentionally accept fail-closed Gateway startup on detector errors, but that would be a compatibility decision rather than a safe performance-only change.
  3. Pause for a plugin-owned detection seam
    If maintainers do not want more core-owned detector knowledge, pause this PR until plugin doctor contracts expose a cheap startup-detection API.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Wrap `snapshotHasPluginDoctorLegacyState` detector calls so exceptions conservatively require the existing doctor preflight instead of rejecting `ensureConfigReady`; add a config-guard regression test with a throwing plugin doctor detector for `gateway run`, then run `node scripts/run-vitest.mjs src/cli/program/config-guard.test.ts` and `node scripts/run-vitest.mjs extensions/msteams/doctor-contract-api.test.ts`.

Next step before merge

  • [P2] Queue a focused repair because the remaining blocker is a narrow, mechanical detector error-boundary fix with clear tests.

Security
Cleared: The diff does not add dependencies, workflows, secret handling, or new supply-chain surfaces; the blocker is startup and migration compatibility.

Review findings

  • [P1] Keep plugin detector failures on the preflight path — src/cli/program/config-guard.ts:875
Review details

Best possible solution:

Keep the Gateway skip path, but make detector failures conservatively run the existing doctor preflight and add regression coverage for a throwing plugin detector.

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

Yes by source inspection: the PR awaits plugin doctor detectors without the current collector's try/catch, so a detector exception rejects ensureConfigReady. I did not run a failing detector fixture in this read-only review.

Is this the best way to solve the issue?

No; gating Gateway preflight is a reasonable performance direction, but this implementation must preserve the existing fail-soft detector semantics before it is the best fix.

Full review comments:

  • [P1] Keep plugin detector failures on the preflight path — src/cli/program/config-guard.ts:875
    The new gate calls each plugin doctor detectLegacyState directly. The established migration collector catches detector errors and records warnings, so on current main a bad plugin detector does not abort gateway run; after this change the same error rejects ensureConfigReady before the doctor preflight can handle it. Please catch detector failures here and conservatively run the existing preflight.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 365279b86f47.

Label changes

Label justifications:

  • P2: This is a normal-priority Gateway startup improvement with a concrete upgrade-safety blocker before merge.
  • merge-risk: 🚨 compatibility: The PR changes when existing installations receive automatic doctor state migration during Gateway startup.
  • merge-risk: 🚨 session-state: The gated preflight covers persisted session and plugin state that existing users may still need migrated.
  • merge-risk: 🚨 availability: Unhandled plugin detector errors can abort gateway run startup before the existing preflight error handling runs.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes current-head copied benchmark/live output for the no-legacy Gateway startup improvement; that proof does not clear the detector-error correctness finding.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes current-head copied benchmark/live output for the no-legacy Gateway startup improvement; that proof does not clear the detector-error correctness finding.
Evidence reviewed

PR surface:

Source +728, Tests +876. Total +1604 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 736 8 +728
Tests 1 877 1 +876
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 1613 9 +1604

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/cli/program/config-guard.test.ts.
  • [P1] node scripts/run-vitest.mjs extensions/msteams/doctor-contract-api.test.ts.

What I checked:

  • PR head detector call lacks a catch boundary: The new snapshotHasPluginDoctorLegacyState loop directly awaits each plugin doctor detectLegacyState result and returns based on its preview, so a detector rejection escapes ensureConfigReady. (src/cli/program/config-guard.ts:875, 0149e9d9c166)
  • Current migration collector catches detector failures: Current main wraps plugin doctor detector calls in try/catch, records a warning, and continues collecting migration plans instead of aborting the whole preflight. (src/infra/state-migrations.ts:3904, 365279b86f47)
  • Current main runs Gateway preflight before this gate: On current main, gateway run is not part of the legacy-input-only command set, so ensureConfigReady runs the doctor state-migration preflight whenever state migration is considered. (src/cli/program/config-guard.ts:154, 365279b86f47)
  • Plugin-state reads can throw during detector work: The plugin-state SQLite store throws PLUGIN_STATE_CORRUPT when persisted JSON cannot be parsed, which is one concrete detector-read failure class that needs the existing fail-soft behavior. (src/plugin-state/plugin-state-store.sqlite.ts:115, 365279b86f47)
  • Gateway pre-bootstrap caller checked: Both normal CLI preaction and the fast gateway run entry route their beforeStateMigrations guard into ensureConfigReady, so the new detector path is on the actual foreground Gateway startup path. (src/cli/run-main.ts:208, 365279b86f47)
  • Feature history checked: History around gateway fast-path startup migrations, state migration detection, and plugin-state SQLite shows recent work by openperf, vincentkoc, and steipete on the relevant surfaces. (src/cli/program/config-guard.ts, df521a645977)

Likely related people:

  • openperf: Authored the merged Gateway fast-path startup migration guard change that this PR extends. (role: recent gateway startup contributor; confidence: high; commits: df521a645977; files: src/cli/program/config-guard.ts)
  • vincentkoc: Recent commits changed SQLite sidecar retry and config-health state migration behavior in the same startup/migration area. (role: recent state-migration contributor; confidence: high; commits: 6326395c0ae4, 6daabd23f821; files: src/cli/program/config-guard.ts, src/infra/state-migrations.ts)
  • steipete: History shows substantial work moving plugin state and task state into SQLite and maintaining doctor contract registry behavior that this PR depends on. (role: prior plugin-state and migration owner; confidence: high; commits: 33c246dbbaaa, 5443baa8527e, d115fb4cf9f9; files: src/plugins/doctor-contract-registry.ts, src/plugin-state/plugin-state-store.sqlite.ts, src/infra/state-migrations.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 proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 24, 2026
@efpiva
efpiva force-pushed the edpiva/gateway-startup-migration-preflight branch from 69ca69e to 0149e9d Compare June 24, 2026 19:28
@efpiva

efpiva commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the ClawSweeper review on the current PR head 0149e9d9c1 and updated the PR body with refreshed, reviewed benchmark evidence.

What changed:

  • Added MSTeams feedback-learning detection aligned to the doctor contract, including configured/default store paths and canonical agent-id normalization.
  • Replaced broad stateDir/agents and session.store presence checks for gateway run with actual legacy-state probes.
  • Preserved canonical session migrations: JSON5 stores, non-default/retired stores, ACP metadata, stale sessionFile repair, shared-store cross-agent/orphan-key behavior.
  • Covered Telegram sidecars with and without sessions.json, including named account store paths.
  • Fixed false positives for Voice Call under OPENCLAW_HOME and debug-proxy pointing at the shared state DB.
  • Added a generic public plugin doctor-contract-api state-migration probe so installed plugin migrations remain preserved.

Validation run locally:

  • pnpm test src/cli/program/config-guard.test.ts — 88 passed
  • pnpm test extensions/msteams/doctor-contract-api.test.ts — 5 passed
  • OPENCLAW_TSGO_HEAVY_CHECK_LOCK_HELD=1 pnpm tsgo:core
  • pnpm build:docker
  • codex review --base origin/main — no discrete correctness issues
  • pnpm test:startup:gateway --runs 5 --warmup 1 --output .artifacts/gateway-startup-upstream/final-reviewed-gateway-startup.json

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@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. proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 24, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Merged
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks @efpiva. Closing this implementation because its detector-empty fast path now bypasses the mandatory foreground-gateway migration checkpoint landed in #101881 for #98565. The branch also retains an unhandled plugin-detector exception path and a migration catalog that has drifted substantially from current main. The benchmark is still useful; please preserve it in a fresh checkpoint-aware performance follow-up rather than rebasing this 1,600-line branch.

@steipete steipete closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cli CLI command changes merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XL status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@efpiva @steipete