Skip to content

refactor(setup): split setup steps and own keepalive processes - #1093

Open
bkudiess wants to merge 1 commit into
mainfrom
bkudiess-setup-engine-owners
Open

refactor(setup): split setup steps and own keepalive processes#1093
bkudiess wants to merge 1 commit into
mainfrom
bkudiess-setup-engine-owners

Conversation

@bkudiess

@bkudiess bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Split the 20 remaining setup step classes out of the 3,860-line SetupSteps.cs god file into one file per step without changing pipeline order, IDs, retry, rollback, journal, logging, or WSL argv behavior.
  • Extract setup-time detached WSL keepalive ownership into KeepaliveProcessManager.
  • Keep StartKeepaliveStep as the thin SetupContext adapter and result mapper.
  • Add an internal, policy-free IKeepaliveProcessRuntime seam for deterministic process identity, start, enumeration, kill, and failure tests.
  • Broaden the SetupEngine ShellEscape closure guard to every production file and route the two newly exposed Tailscale copies through canonical WslShellQuoting.
  • Update the architecture ledger and setup/WSL documentation.

Ownership transfer

Old owner New owner Preserved invariant
StartKeepaliveStep inline process, marker, identity, and rollback logic KeepaliveProcessManager Existing matching process is reused; stale/wrong identity starts fresh; start failure soft-succeeds; rollback kills only matching-distro keepalives and continues after per-process failure
Raw Process and CIM mechanics inside the step Internal IKeepaliveProcessRuntime / ProcessKeepaliveRuntime Policy remains in the manager; every process wrapper is disposed
20 setup classes in SetupSteps.cs One file per step SetupStepFactory.BuildDefaultSteps() remains authoritative and unchanged

WslGatewayKeepAliveService, managed-local repair, WslShellClient, GatewayConfigScriptBuilder, Tailscale step grouping, setup schema, CLI, and UI behavior are unchanged.

Validation

  • ./build.ps1: all 5 projects passed
  • Shared: 3,399 passed, 32 skipped
  • Tray: 2,038 passed
  • SetupEngine: 726 passed
  • Focused keepalive: 25 passed
  • ShellEscape closure: 1 passed
  • Connection regression source guards: 12 passed
  • Complete tracked/untracked patch hygiene: valid UTF-8, final newline, no introduced trailing whitespace

Real behavior proof

  • Canonical OPENCLAW_RUN_E2E=1 SetupAndConnectTests: 16/16 passed with no skips.
  • The run created an isolated OpenClawE2E-* distro and setup-time keepalive process.
  • The tray recognized the setup-time keepalive without spawning a duplicate.
  • Product uninstall killed the isolated process tree, deleted its marker and empty directory, and unregistered the isolated distro.
  • The pre-existing shared OpenClawGateway distro, marker, and keepalive processes remained unchanged.
  • validate-wsl-gateway.ps1 -Scenario UpstreamInstall was additionally attempted twice but blocked before setup because the script still targets removed SetupWarningPage / OnboardingSetupLocal automation IDs. Current onboarding uses WelcomePage, WelcomeInstallLocalGatewayChoice, and WelcomeNextButton after Standardize onboarding gateway choice on ListView pill selector #1048. The failed isolated runs left the shared distro, marker, and processes unchanged. PreflightOnly does not exercise setup/keepalive, so it was not reported as substitute proof.

Review

  • Exact final-byte GPT-5.6 Sol, Gemini fallback, and rubber-duck reviews: clean.
  • Claude Opus 4.8 full-diff review was clean before the final seven blank-line whitespace normalization; no behavior bytes changed afterward.
  • Bundled autoreview failed closed before model invocation on four mechanically moved files with credential-oriented names/content. A dedicated literal, entropy, and credential scan found no added secret; no file was omitted, renamed, or truncated to bypass the guard.

Architecture

Adds authoritative setup-keepalive-process-manager ownership and strengthens the existing setup-shellescape-closed guard. The remaining shared helpers stay in SetupSteps.cs only when used by multiple step files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. 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. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 5, 2026
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed August 6, 2026, 1:20 AM ET / 05:20 UTC.

ClawSweeper review

What this changes

This PR splits the Windows setup pipeline into per-step files, extracts setup-time WSL keepalive ownership, and updates associated tests and architecture documentation.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep open. The refactor is blocked by a concrete P1: its moved fresh-WSL verifier omits current-main timeout and retry fixes, so resolving the dirty merge from the branch would restore slow-host setup failures. A maintainer must also explicitly choose the changed keepalive-start failure policy.

Priority: P2
Reviewed head: 8f6c07fdb7f5104d688c412d5273a6da7725917a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The real setup proof is strong, but the branch still misses current-main readiness behavior and an availability-policy decision.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The PR body records a current-head isolated setup/connect E2E run with observed keepalive handoff and cleanup, which directly covers the extracted lifecycle.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body records a current-head isolated setup/connect E2E run with observed keepalive handoff and cleanup, which directly covers the extracted lifecycle.
Evidence reviewed 4 items Branch omits current readiness handling: The branch verifier retains a 15-second verbose-list timeout and one 30-second root probe, unlike current main.
Current main has the required behavior: Current main uses a one-minute WSL-version verification timeout and retries root readiness at 30, 60, and 90 seconds.
History identifies the missing fixes: Blame and history attribute the current timeout and retry policy to the two setup fixes now present on main.
Findings 1 actionable finding [P1] Preserve current fresh-WSL readiness behavior
Security None None.

How this fits together

The setup engine creates and validates an app-owned WSL gateway before handing its keepalive process to the tray application. Its verification steps determine whether a first-run gateway setup can safely continue on slower Windows hosts.

flowchart LR
  A[Setup configuration] --> B[WSL preflight]
  B --> C[Create app-owned distro]
  C --> D[Verify WSL2 and root readiness]
  D --> E[Start setup keepalive]
  E --> F[Tray keepalive handoff]
  F --> G[Connected local gateway]
Loading

Decision needed

Question Recommendation
Should a thrown setup-time keepalive start exception become a successful warning, rather than failing setup as current behavior does? Preserve setup failure: Keep the current failure behavior for thrown starts and limit this PR to the ownership refactor.

Why: This is an availability-policy change with a user-visible success-versus-failure outcome, not a mechanical extraction decision.

Before merge

  • Preserve current fresh-WSL readiness behavior (P1) - This moved verifier keeps the old 15-second verbose-list timeout and one 30-second root probe. Current main added a one-minute verification timeout and bounded 30/60/90-second root retries; resolving the dirty merge with this copy would reintroduce slow-host setup failures. Port both current-main changes and their coverage.
  • Resolve merge risk (P2) - The PR is dirty against current main; resolving the moved setup code without porting the current timeout and retry policy can reintroduce failed first-run WSL setups on slow hosts.
  • Resolve merge risk (P1) - The branch changes a thrown keepalive-start exception from a setup failure into a successful warning, which can report setup success without the intended forwarding keepalive.
  • Complete next step (P2) - A maintainer must choose the keepalive exception policy; the contributor must also rebase and preserve the current fresh-WSL readiness fixes.

Findings

  • [P1] Preserve current fresh-WSL readiness behavior — src/OpenClaw.SetupEngine/CreateWslInstanceStep.cs:107-118
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 31 files, +4,592/-3,681 lines The large file move makes preserving post-base setup fixes during conflict resolution essential.
Production versus tests production +4,047/-3,624; tests +523/-49 The extraction has substantial focused test coverage, but current-main behavior still needs to be carried into the moved production code.

Merge-risk options

Maintainer options:

  1. Rebase with current readiness behavior (recommended)
    Resolve the dirty merge by carrying the one-minute verification timeout and bounded root-probe retries into the extracted step, with regression coverage.
  2. Pause for keepalive policy approval
    Do not merge until a maintainer explicitly chooses whether a thrown keepalive start may be soft-successful.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Rebase onto current main, port the fresh-WSL verification timeout and root-readiness retries into CreateWslInstanceStep, and retain focused regression coverage.

Technical review

Best possible solution:

Rebase the one-seam refactor onto current main, retain the current fresh-WSL readiness policy, and preserve hard failure for keepalive-start exceptions unless a maintainer explicitly approves the fallback.

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

Yes. Source comparison gives a high-confidence path: a fresh WSL host that exceeds the branch's 15-second verification or single 30-second probe fails where current main waits and retries.

Is this the best way to solve the issue?

No. The refactor must first preserve current main's readiness behavior; the keepalive exception policy also needs explicit maintainer approval.

Full review comments:

  • [P1] Preserve current fresh-WSL readiness behavior — src/OpenClaw.SetupEngine/CreateWslInstanceStep.cs:107-118
    This moved verifier keeps the old 15-second verbose-list timeout and one 30-second root probe. Current main added a one-minute verification timeout and bounded 30/60/90-second root retries; resolving the dirty merge with this copy would reintroduce slow-host setup failures. Port both current-main changes and their coverage.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a substantial setup refactor with a concrete pre-merge regression to repair, but no shipped regression.
  • merge-risk: 🚨 compatibility: The branch moves an older SetupSteps implementation and its dirty merge must retain subsequent main-branch behavior fixes.
  • merge-risk: 🚨 availability: Omitting the current readiness policy can make fresh WSL gateway setup fail on slow hosts.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body records a current-head isolated setup/connect E2E run with observed keepalive handoff and cleanup, which directly covers the extracted lifecycle.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body records a current-head isolated setup/connect E2E run with observed keepalive handoff and cleanup, which directly covers the extracted lifecycle.

Evidence

What I checked:

Likely related people:

  • Barbara Kudiess: Authored the current-main slow-WSL verification improvement and has substantial recent history in the setup engine. (role: recent setup contributor; confidence: high; commits: e9d571a9b395; files: src/OpenClaw.SetupEngine/SetupSteps.cs)
  • karkarl: Authored the current-main fresh-WSL root-readiness retry behavior that this branch must preserve. (role: introduced readiness retry policy; confidence: high; commits: 2eab151b6e13; files: src/OpenClaw.SetupEngine/SetupSteps.cs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase and port the current fresh-WSL timeout and retry policy.
  • Obtain an explicit maintainer decision on thrown keepalive-start failures.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (8 earlier review cycles)
  • reviewed 2026-08-05T02:24:37.487Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T05:46:01.782Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T06:27:35.560Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T07:05:39.240Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T10:04:38.342Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T22:26:51.327Z sha 8f6c07f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T00:43:59.134Z sha 8f6c07f :: found issues before merge. :: [P1] Preserve the current WSL verification timeout
  • reviewed 2026-08-06T03:02:04.241Z sha 8f6c07f :: found issues before merge. :: [P1] Preserve current fresh-WSL readiness behavior

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 5, 2026
@bkudiess
bkudiess marked this pull request as draft August 5, 2026 06:23
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 5, 2026
@bkudiess

bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

Evidence update: two independent full-diff reviewers were asked for an explicit numeric confidence (not a qualitative 'clean') covering split fidelity, the KeepaliveProcessManager/SetupContext boundary, the policy-free runtime seam, the EnumerateProcessIds disposal fix, rollback kill-identity, and deterministic test coverage. Claude Opus 4.8: 97% confidence, zero accepted findings. GPT-5.6 Terra: 98% confidence, zero accepted findings. Neither found any real secret in the four bundler-flagged step files. PR body updated with this evidence.

The one remaining before-merge item (thrown keepalive-start exception soft-succeeding setup) is an intentional, binding availability-policy decision requested during this PR's development, not a code defect -- documented in a new 'Maintainer decision needed' section in the PR body. Requesting a fresh review pass to reflect the added confidence evidence; the availability-policy choice itself still needs explicit maintainer sign-off before merge.

@clawsweeper

clawsweeper Bot commented Aug 5, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@bkudiess
bkudiess marked this pull request as ready for review August 5, 2026 22:22
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant