Skip to content

fix: resume Local AI setup with a new model - #1232

Open
RomneyDa wants to merge 3 commits into
mainfrom
fix/local-ai-onboarding-resume
Open

fix: resume Local AI setup with a new model#1232
RomneyDa wants to merge 3 commits into
mainfrom
fix/local-ai-onboarding-resume

Conversation

@RomneyDa

Copy link
Copy Markdown
Member

Summary

  • keep a visible progress spinner active while setup reconciles an existing Local AI installation after restart
  • treat an intentional model selection change as a replacement instead of a corrupt persisted installation
  • reuse the verified llama-server runtime, download the newly selected model, and atomically replace the manifest
  • restore the previous manifest and router preset if later setup rollback runs

Related context: #1206 and #1208 address complementary post-install Local AI availability and first-start behavior. They do not overlap this setup reconciliation path.

Validation

  • git diff --check - PASS
  • autoreview --mode local - PASS, no accepted/actionable findings after one rollback-preservation fix
  • Native Windows build and required test suites - pending CI

Real behavior proof

Not verified yet. Native Windows CI and focused setup-engine regression results are pending. The controller host is macOS, so no local WinUI claim is made.

Security impact

No new permissions, credentials, network endpoints, or command surfaces. Existing runtime and model receipts remain fail-closed; only an explicit catalog model-ID change enters the replacement path.

@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@RomneyDa RomneyDa added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Aug 26, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. 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. labels Aug 26, 2026
@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 26, 2026, 1:03 PM ET / 17:03 UTC.

ClawSweeper review

What this changes

This PR lets resumed Local AI setup reuse its verified llama-server runtime when a different catalog model is selected, replace the persisted installation and gateway route, and restore the prior state on rollback.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open. The prior gateway-route rollback concern is covered, but the replacement branch now bypasses the existing GPU-identity guard whenever the model also changes, expanding this from a model replacement into an unvalidated GPU migration.

Priority: P1
Reviewed head: f7d98e15d1ab659f8d446bebb4307564c9fb1cd1

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused rollback coverage and green CI provide useful signal, but the GPU-identity bypass is a merge-blocking correctness issue.
Proof confidence 🌊 off-meta tidepool Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; however, repository policy still makes an exact-head native replacement-and-rollback proof valuable before merge.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; however, repository policy still makes an exact-head native replacement-and-rollback proof valuable before merge.
Evidence reviewed 5 items GPU guard is bypassed on replacement: The new model-mismatch return occurs before ValidateSelectedModelMatch, which contains the SelectedGpuId comparison, so a changed GPU plus changed model enters replacement.
Previous behavior failed closed on GPU change: The baseline reconciliation required both model and selected GPU identity to match before reuse.
Replacement regression test omits changed-GPU case: The added replacement test uses GPU-0 for both the existing manifest and selected plan, so it does not exercise the bypassed mismatch.
Findings 1 actionable finding [P1] Reject GPU changes before replacing the model
Security None None.

Live Verification

Command: dotnet test ./tests/OpenClaw.SetupEngine.Tests/OpenClaw.SetupEngine.Tests.csproj --no-restore --filter "FullyQualifiedName~LocalAiInstallRecoveryTests"

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

Assertions:

  • FAIL expect_output: Passed!

How this fits together

Local AI setup persists a verified llama-server receipt, then configures the gateway’s local provider and primary model. Restart reconciliation compares the stored receipt with current hardware and model selection before reusing or replacing the installation.

flowchart LR
A[Saved Local AI receipt] --> B[Setup reconciliation]
C[Selected model and GPU] --> B
B --> D{Receipt matches?}
D -->|Same model| E[Reuse verified installation]
D -->|New model| F[Download replacement model]
F --> G[Persist receipt]
G --> H[Configure gateway route]
H --> I[Rollback prior receipt and route]
Loading

Before merge

  • Reject GPU changes before replacing the model (P1) - The model-mismatch return happens before ValidateSelectedModelMatch, which is the only place that checks SelectedGpuId. A persisted GPU-0 receipt plus a different model and GPU-1 therefore enters replacement and stamps GPU-1 into the new receipt, instead of preserving the prior fail-closed GPU mismatch behavior. Check the GPU before this return and add that combined-mismatch regression.
  • Resolve merge risk (P1) - Merging as written allows a model change to silently replace an installation whose selected GPU changed, rewriting the receipt and gateway route outside the prior fail-closed contract.
  • Resolve merge risk (P1) - The repository policy calls for current-head setup proof; CI passed, but no native replacement-and-rollback behavior proof is attached.

Findings

  • [P1] Reject GPU changes before replacing the model — src/OpenClaw.SetupEngine/LocalAiInstallReconciler.cs:81-92
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +174/-42, tests +210 The patch meaningfully changes persisted setup and gateway-route behavior while adding focused replacement and rollback coverage.

Merge-risk options

Maintainer options:

  1. Restore the GPU guard (recommended)
    Require the persisted and selected GPU IDs to match before entering model replacement, and add a regression covering a changed model plus changed GPU.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Reject replacement when SelectedGpuId differs from the persisted receipt, add a focused regression test, and retain the existing model-only replacement path.

Technical review

Best possible solution:

Preserve the existing GPU identity requirement around model replacement, then prove the unchanged-GPU replacement and a rejected changed-GPU attempt on the exact Windows head.

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

Yes. A saved GPU-0 receipt with a newly selected model and GPU-1 reaches the model-mismatch return before the only SelectedGpuId check, so the failure is directly reproducible from the source path.

Is this the best way to solve the issue?

No. The current approach should preserve the existing GPU guard before permitting the intended model-only replacement; that is narrower than silently accepting both changes.

Full review comments:

  • [P1] Reject GPU changes before replacing the model — src/OpenClaw.SetupEngine/LocalAiInstallReconciler.cs:81-92
    The model-mismatch return happens before ValidateSelectedModelMatch, which is the only place that checks SelectedGpuId. A persisted GPU-0 receipt plus a different model and GPU-1 therefore enters replacement and stamps GPU-1 into the new receipt, instead of preserving the prior fail-closed GPU mismatch behavior. Check the GPU before this return and add that combined-mismatch regression.
    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 1e18866d9b42.

Labels

Label changes:

  • add merge-risk: 🚨 auth-provider: The replacement path rewrites and rolls back the gateway’s Local AI provider and primary-model routing.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; however, repository policy still makes an exact-head native replacement-and-rollback proof valuable before merge.
  • 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: ⏳ waiting on author.

Label justifications:

  • P1: The patch affects an existing resumed Local AI workflow and can change a user’s persisted runtime selection and gateway route.
  • merge-risk: 🚨 compatibility: A changed GPU plus model can rewrite a previously verified Local AI installation receipt during upgrade or recovery.
  • merge-risk: 🚨 auth-provider: The replacement path rewrites and rolls back the gateway’s Local AI provider and primary-model routing.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; however, repository policy still makes an exact-head native replacement-and-rollback proof valuable before merge.

Evidence

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.SetupEngine.Tests/OpenClaw.SetupEngine.Tests.csproj --no-restore.

What I checked:

Likely related people:

  • Joel: The existing managed Local AI reconciliation and its original fail-closed GPU contract date to the Local AI introduction commit. (role: introduced baseline behavior; confidence: high; commits: ccd64bbb7d68; files: src/OpenClaw.SetupEngine/LocalAiInstallReconciler.cs)

Rank-up moves

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

  • Restore the GPU guard and add the combined model-and-GPU mismatch regression.
  • Attach redacted exact-head Windows evidence for successful model-only replacement and rollback recovery.

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 (2 earlier review cycles)
  • reviewed 2026-08-26T01:24:11.456Z sha 0bcb68a :: needs changes before merge. :: [P1] Preserve gateway state during model replacement
  • reviewed 2026-08-26T07:08:40.804Z sha d9fd022 :: needs maintainer review before merge. :: none

@RomneyDa RomneyDa removed the status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. label Aug 26, 2026
@RomneyDa

Copy link
Copy Markdown
Member Author

Addressed the ClawSweeper P1 in d9fd022.

  • Accepts the exact prior companion-managed provider/primary only during an explicit model replacement.
  • Carries the prior fallback into the replacement manifest.
  • Reconstructs the prior provider from the trusted old manifest and restores the old primary on rollback.
  • Adds a gateway-step regression covering successful old-to-new transition and rollback.
  • Fixes the prior CI compile failure by importing the shared GPU types.
  • Structured autoreview is clean with no actionable findings.

Native Windows CI is rerunning now. @clawsweeper re-review

@clawsweeper clawsweeper Bot added 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 26, 2026
@RomneyDa
RomneyDa marked this pull request as ready for review August 26, 2026 16:19
@RomneyDa

Copy link
Copy Markdown
Member Author

Fixed the Windows CI compile failure in the GPU test fixture. GpuInfo includes shared-memory parameters, so the positional driver/CUDA/stable-ID arguments had shifted. Commit uses named arguments and autoreview is clean. CI is rerunning.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. 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 26, 2026
@karkarl

karkarl commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Adversarial dual-model review

Verdict: SHIP AFTER FIXES

Claude Opus and GPT Codex independently reviewed the PR diff, then cross-examined the disputed interruption path. Both confirmed one release blocker.

HIGH consensus

Issue Opus Codex Fix confidence Release blocker
If setup is interrupted after persisting model B's manifest but before switching the gateway from model A to B, prior-install provenance is lost because ReplacedLocalAiInstall exists only in memory. A fresh run treats B as fully reused while the gateway remains on A, so ConfigureLocalAiGatewayStep rejects the exact old managed route. Repeated B retries remain stuck. Uninstall also cannot prove that gateway A belongs to manifest B, preserves the stale route, and continues best-effort cleanup of Local AI state. HIGH HIGH 85% Yes

Affected regions:

  • LocalAiSetupSteps.cs:568-580
  • LocalAiInstallReconciler.cs:76-103
  • LocalAiGatewayConfiguration.cs:100-129,247-303

Required fix direction: make the replacement transition durable across process termination, retaining enough trusted prior-install/provider provenance to complete or roll back the A-to-B gateway transition. Keep the exact-provider ownership checks. Do not broaden them to accept merely managed-looking configuration.

Required regression coverage: add a fresh-process crash/resume test for the window after manifest replacement and before gateway configuration, plus uninstall behavior from that drift state.

LOW consensus

Issue Opus Codex Fix confidence
matchesReplacementPrior can throw InvalidDataException if the old model is no longer catalog-qualified. The pipeline fails safely, but reports an opaque unhandled-step failure. LOW 95%

In-process rollback ordering, reused-runtime preservation, API-key redaction handling, and fallback/primary propagation were reviewed and found sound for uninterrupted replacement.

Regression risk: Medium-high for interrupted model replacement; low for the uninterrupted happy path.

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

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. 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.

2 participants