Improve macOS onboarding UX and gateway setup#47263
Conversation
openclaw-barnacle
Bot
added
app: macos
Greptile SummaryThis PR improves the macOS onboarding UX for local installs by inserting a CLI installation step (with Xcode CLT preflight) before the wizard, adding an onboarding gate to the node-mode connection loop, and refreshing the wizard's provider-selection layout so the primary action always stays visible. The TypeScript side updates provider hint copy and bumps default model strings. Key changes and findings:
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func shouldSkipWizard(root: [String: Any]) -> Bool { | ||
| if let wizard = root["wizard"] as? [String: Any], !wizard.isEmpty { | ||
| return true | ||
| } | ||
| if let gateway = root["gateway"] as? [String: Any], | ||
| let auth = gateway["auth"] as? [String: Any] | ||
| { | ||
| if let mode = auth["mode"] as? String, | ||
| !mode.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | ||
| { | ||
| return true | ||
| } | ||
| if let token = auth["token"] as? String, | ||
| !token.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | ||
| { | ||
| return true | ||
| } | ||
| if let password = auth["password"] as? String, | ||
| !password.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | ||
| { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Existing users may be re-shown the wizard after update
The previous shouldSkipWizard also returned true when gateway.auth.mode, gateway.auth.token, or gateway.auth.password were set in the config. With those checks removed, any user who configured gateway auth manually (without the wizard producing a wizard key) will now encounter the wizard when they next open the app — even though they are already fully configured.
This is a meaningful regression if there is any population of users on the legacy auth-only config format. Consider either keeping the fallback gateway-auth checks in place, or adding a one-time migration that writes a sentinel wizard key for users whose config already contains auth credentials.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/macos/Sources/OpenClaw/OnboardingWizard.swift
Line: 199-204
Comment:
**Existing users may be re-shown the wizard after update**
The previous `shouldSkipWizard` also returned `true` when `gateway.auth.mode`, `gateway.auth.token`, or `gateway.auth.password` were set in the config. With those checks removed, any user who configured gateway auth manually (without the wizard producing a `wizard` key) will now encounter the wizard when they next open the app — even though they are already fully configured.
This is a meaningful regression if there is any population of users on the legacy auth-only config format. Consider either keeping the fallback gateway-auth checks in place, or adding a one-time migration that writes a sentinel `wizard` key for users whose config already contains auth credentials.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01a6e0da81
ℹ️ 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".
|
|
||
| if (params.setDefaultModel) { | ||
| const model = "github-copilot/gpt-4o"; | ||
| const model = "github-copilot/gpt-5.4"; |
There was a problem hiding this comment.
This now writes github-copilot/gpt-5.4 as the default model, but the forward-compat path only synthesizes gpt-5.4 for openai-codex (see src/agents/model-forward-compat.ts where CODEX_GPT54_ELIGIBLE_PROVIDERS excludes github-copilot), so Copilot onboarding can end up with a default model the resolver cannot provide unless that exact model is already present in the live catalog. In environments where Copilot does not expose gpt-5.4, users will immediately hit unknown-model failures after auth setup.
Useful? React with 👍 / 👎.
| if !self.desiredActive { return false } | ||
| do { | ||
| _ = try await self.connection.requestRaw(method: .health, timeoutMs: 1500) | ||
| _ = try await self.probeLocalGatewayHealth(timeoutMs: 1500) |
There was a problem hiding this comment.
Switching readiness checks to probeLocalGatewayHealth bypasses self.connection, so the existing DEBUG injection hook (setTestingConnection) no longer affects waitForGatewayReady and related attach/start paths. That breaks deterministic testing/mocking of gateway readiness (for example apps/macos/Tests/OpenClawIPCTests/GatewayProcessManagerTests.swift now cannot drive this path with its fake GatewayConnection) and makes these flows depend on a real local socket even under tests.
Useful? React with 👍 / 👎.
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟠 GatewayProcessManager health probe can connect to non-loopback host and leak gateway credentials
Description
This behavior is newly exercised in the local gateway process manager polling loops (attach/wait), so it can happen automatically and repeatedly during startup/health checks. Vulnerable code: let config = GatewayEndpointStore.localConfig()
let channel = GatewayChannelActor(
url: config.url,
token: config.token,
password: config.password)
return try await channel.request(method: GatewayConnection.Method.health.rawValue, params: nil, timeoutMs: timeoutMs)RecommendationConstrain the "local" health probe to a local-only endpoint and avoid transmitting secrets to non-loopback addresses. Recommended hardening options (pick one depending on intended feature set):
private func probeLocalGatewayHealth(timeoutMs: Double) async throws -> Data {
let port = GatewayEnvironment.gatewayPort()
let scheme = "ws" // or "wss" if you require TLS
let url = URL(string: "\(scheme)://127.0.0.1:\(port)")!
let channel = GatewayChannelActor(url: url, token: nil, password: nil)
defer { Task.detached { await channel.shutdown() } }
return try await channel.request(method: GatewayConnection.Method.health.rawValue,
params: nil,
timeoutMs: timeoutMs)
}If the gateway requires auth even for health, consider adding a dedicated unauthenticated health path on the gateway, or restrict auth usage to loopback only.
Also consider using 2. 🟡 Unpinned/unchecked CLI install for .debug builds via expectedGatewayVersionString() returning nil
Description
Downstream call sites treat
This creates a supply-chain exposure for debug builds:
Vulnerable flow (key parts): // GatewayEnvironment.swift
if let bundleIdentifier,
bundleIdentifier.trimmingCharacters(in: .whitespacesAndNewlines).hasSuffix(".debug")
{
return nil // triggers downstream fallback to "latest"
}
// CLIInstaller.swift
let expected = GatewayEnvironment.expectedGatewayVersionString() ?? "latest"
curl -fsSL https://openclaw.bot/install-cli.sh | bash -s -- ... --version \(expected)While this is gated on RecommendationAvoid installing/executing unpinned code for debug builds, and add integrity verification for any network-delivered installer. Recommended options (best → better):
Example approach (hash verification sketch): // Pseudocode
let url = URL(string: "https://openclaw.bot/releases/openclaw-\(version).tar.gz")!
let data = try await URLSession.shared.data(from: url).0
let actual = sha256Hex(data)
guard actual == expectedSha256 else { throw InstallError.integrityFailed }
// proceed to unpack/install
3. 🔵 Onboarding security acknowledgement can be bypassed via page navigation dots
DescriptionThe new onboarding “security notice acknowledged” gate only blocks the Next button ( As a result, a user can:
This makes the acknowledgement requirement trivially bypassable in-app and undermines it as a security/consent control. Vulnerable code (page dot navigation ignores Button {
withAnimation { self.currentPage = index }
} label: {
Circle()
.fill(index == self.currentPage ? Color.accentColor : Color.gray.opacity(0.3))
.frame(width: 8, height: 8)
}
.disabled(isLocked)RecommendationTreat the acknowledgement as a navigation invariant, not just a “Next button” state. Option A (minimal UI fix): disable non-zero page dots until acknowledged when currently on page 0. let blockedBySecurityNotice = !self.securityNoticeAcknowledged && self.currentPage == 0 && index > 0
Button {
withAnimation { self.currentPage = index }
} label: {
// ...
}
.disabled(isLocked || blockedBySecurityNotice)
.opacity((isLocked || blockedBySecurityNotice) ? 0.3 : 1)Option B (stronger): centralize page transitions through a function that enforces the rule, and use it for all navigation paths (dots, next/back, programmatic changes). Also consider not setting Analyzed PR: #47263 at commit Last updated on: 2026-03-18T07:01:38Z |
f135473 to
fbbca10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbbca10c83
ℹ️ 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".
| Text("Prefer to install manually?") | ||
| .font(.footnote) | ||
| .foregroundStyle(.secondary) | ||
| Button(self.copied ? "Copied" : "Copy install command") { | ||
| self.copyToPasteboard(self.devLinkCommand) |
There was a problem hiding this comment.
If users follow the new “Prefer to install manually?” path here, onboarding has no way to notice success while they stay on this page: Next is disabled until cliInstalled becomes true (apps/macos/Sources/OpenClaw/Onboarding.swift:151-154), but the only refreshes now happen on page entry or after installCLI() runs (apps/macos/Sources/OpenClaw/OnboardingView+Monitoring.swift:42-49, 63-79). Running the copied command in Terminal leaves the page stuck with Next disabled until the user backs out and re-enters the page or restarts onboarding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f2f9a05a3
ℹ️ 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".
| UserDefaults.standard.set(true, forKey: onboardingSecurityAcknowledgedKey) | ||
| UserDefaults.standard.set(true, forKey: "openclaw.onboardingSeen") |
There was a problem hiding this comment.
This now records openclaw.onboardingSecurityAcknowledged unconditionally when finish() runs. The new warning only blocks the Next button on page 0, but the progress dots in OnboardingView.navigationBar are still clickable, so a first-run user can jump straight to a later page (for example the final “All set” page) and finish onboarding without ever ticking the acknowledgement checkbox. That makes the new security gate ineffective for anyone who navigates with the dots.
Useful? React with 👍 / 👎.
| // Preserve runtime connectivity for existing local installs when a newer | ||
| // app build refreshes onboarding copy or flow. | ||
| return OnboardingWizardModel.hasExistingLocalSetup(root: root) |
There was a problem hiding this comment.
This fallback now depends entirely on OnboardingWizardModel.hasExistingLocalSetup(root:), which only inspects openclaw.json. However, the macOS app already treats OPENCLAW_GATEWAY_TOKEN / OPENCLAW_GATEWAY_PASSWORD and the saved launchd snapshot as valid local auth sources in GatewayEndpointStore.resolveGatewayToken/Password. On an upgraded local install that keeps its gateway auth in the environment or service plist, shouldConnectNodeMode() will now return false here and run() will disconnect node mode until the user reruns onboarding, even though the local gateway is still configured.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
the
rating: 🌊 off-meta tidepool
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 9:35 AM ET / 13:35 UTC. Summary PR surface: Source 0, Other +421. Total +421 across 17 files. Reproducibility: yes. for the review findings: source inspection of the PR head shows the credentialed Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Proof path suggestion Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Close this PR as superseded by #99767. Do we have a high-confidence way to reproduce the issue? Yes for the review findings: source inspection of the PR head shows the credentialed Is this the best way to solve the issue? No. The PR is a plausible old UX direction, but the best current solution is the newer main onboarding path plus narrow rebased fixes for any remaining gaps, not merging this stale branch wholesale. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f9e194e4a122. Label changesLabel justifications:
Evidence reviewedPR surface: Source 0, Other +421. Total +421 across 17 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
clawsweeper
Bot
removed
the
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
status: 📣 needs proof
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Thanks @ImLukeF. Closing this stale implementation branch while preserving #90665 for the remaining macOS onboarding work. The useful current-main direction has since moved through #99767, #99935, and #102677; this head is conflicting, lacks current proof, and still has unresolved authentication/security findings. Any remaining gap should be a focused patch on current main rather than a conflict resolution of this branch. |
2 participants
Summary
Testing