Repository navigation
fix(setup): preserve sanitized Local AI verification errors - #1716
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
@clawsweeper review Please review the current head (dabce8e). The PR body includes the Windows 11 ARM64 test machine validation, component receipts/screenshots, and the remaining proof limits. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. What this changesThis PR displays sanitized verification failures and preserves the current error when a retry fails. Example: Model verification returns HTTP 503 with apiKey=demo-secret, then Check again encounters a distinct RPC failure.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Partial Merge readiness✅ Ready for maintainer review Keep open for landing: current main still loses verification details, both reflection callers are corrected, and no actionable defect remains. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherGatewayAiSetupClient receives verification replies from the connected Gateway; AiSetupPage presents their sanitized details alongside localized recovery guidance. flowchart LR
A[Setup action] --> B[GatewayAiSetupClient]
B --> C[Gateway verification RPC]
C --> D[Verification result or exception]
D --> E[AiSetupPage]
E --> F[Existing sanitizer]
F --> G[Bounded error display]
Technical reviewBest possible solution: Land the focused diagnostic repair while retaining the underlying inference investigation in #1713. Do we have a high-confidence way to reproduce the issue? Current-main source deterministically discards returned verification details and overwrites rejected-state errors; inspected baseline captures support that path, without reproducing native inference instability. Is this the best way to solve the issue? Retaining the validated failure in the existing client and sanitizing it in the existing page fixes lost diagnostics without changing verification or recovery ownership. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 9e4999c6d43f. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (6 earlier review cycles)
Reviewed October 15, 2026, 12:35 AM ET / 04:35 UTC (Revision 7). |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
@clawsweeper re-review Proof capture is completed for the documented diagnostic scope at dabce8e. The PR description now records that scope explicitly, with current-head before/after screenshots and controlled verification/retry receipts demonstrating HTTP 503 error retention, redaction, and current retry-error retention. The product AI setup page was exercised in a temporary WinUI host. Please reassess the proof labels against this scope: |
|
🦞👀 Re-review progress:
|
|
🦞👀 Re-review progress:
|
|
The diagnostics change looks reasonable, but I found one introduced UI-test regression that needs fixing before merge. Reviewed head: Update the UI tests for the new
|
Keep the Gateway verification failure available to the AI setup page and display bounded, sanitized details. Clear stale details on retry and preserve the current RPC exception when a prior rejection settles.
dabce8e to
0285783
Compare
|
Both existing reflection call sites now pass the optional On the requested Windows ARM64 device, the normal Light/Dark variants reproduced The two additional native capture variants were attempted and failed their foreground-window assertion ( Independent code review, structured review, and final commit-coherence checks passed. @clawsweeper review Please re-review head |
|
🦞👀 Re-review progress:
|
|
Independent local validation completed on a native Windows ARM64 device at exact head Local build and tests
Fresh actual-Companion behavior proofThe additional interactive run used the unmodified built Copied live UI output from the visible verification error: Copied live UI output after clicking Check again and receiving a distinct RPC failure: A later UI Automation read confirmed that the current retry error remained displayed, with neither the old HTTP 503 detail nor the synthetic credential present. The server recorded exactly two verification RPCs: a negative verification result in a successful RPC envelope, followed by the controlled retry RPC error. Focused screenshots of both actual-application states were captured with owned-window Scope: this covers the diagnostic behavior on @clawsweeper re-review Please re-review head |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Repeat Hanselman review: no actionable findingsVerdict: no blocking or actionable introduced defects found. Both independent reviewers support landing this narrowly scoped diagnostic fix, subject to the normal final head/base readiness check. Reviewed head: Independent reviewers ran in parallel:
Both inspected pinned Git blobs, not the newer code in the working checkout. Their conclusions were reconciled against the exact-head source. Both models agree: HIGH consensus
Both checked that verification detail is cleared before a retry and retained only after cancellation, authority, connection-generation, and response checks. Both confirmed that current exception detail takes precedence, sanitization occurs before the 2,048-character limit, request settlement preserves the current error, and both reflection callers match the new method signature. Existing client/page ownership and inline-error routing remain intact. Key source: verification lifecycle, request settlement, and sanitized presentation. Only one model noted concerns: LOW consensus
Neither concern established an actionable regression after reconciliation. No fix is proposed. Fix confidence is a coordinator assessment, not a vote; no numeric fix rating is assigned where no verified repair is needed. Evidence reused and limitsThe published exact-head ARM64 validation records a passing full build, 10,550 tests passed, 36 skipped, 0 failed, and actual-Companion verification-error/redaction/retry output. These executions were reused, not rerun for this source review. The refreshed PR check rollup has 11 successes and 17 skips, with no failed or pending checks. The HTTP 503 was an intentional negative-test response. Passing proof means the cause was visible, credentials were redacted, and the current retry error replaced stale detail. It does not mean model inference worked. The visible run covered the detected-provider route, not every ClawSweeper's completed re-review also reports no findings, no before-merge work, and sufficient live-output proof. Its broader-issue scope caveat is separate from this diagnostic patch's correctness. No product files were changed, no workflows rerun, and no merge was performed as part of this review. |
A user reported native Gateway Local AI verification failing despite retries. The setup page replaces the detailed cause with generic provider rejection text. Retain the returned verification error, display bounded sanitized details, clear stale details on retry, and preserve the current RPC exception when an earlier rejection settles.
Improves diagnostics for #1713. This does not claim to resolve the underlying native inference instability. Installer lifecycle changes remain separate in #1715.
The review update fixes both existing reflection calls to
ShowErrorby supplying its optional detail argument explicitly. No new test files or test methods. One commit:02857835f2b692ce04eddf4870a2cfaf6052f48d.Required proof pools
windows-11-arm64: independent branch build and regression tests.windows-winui-interactive: both affected normal UI variants pass; earlier diagnostic component captures are below. The additional foreground-dependent capture variants are blocked as described below.Validation
All new execution/testing used the requested Windows 11 ARM64 device, in a separate checkout and isolated profile.
./build.ps1ProviderLoading_StaysInlineAndCanCancelWithoutOpeningAPopup(Light/Dark)FollowupProof_ProviderInlineLoadingCancelAndInput(Light/Dark)Baseline: both normal UI variants failed with
System.Reflection.TargetParameterCountException: Parameter count mismatch.Both pass with the two existing call sites corrected. Tests restored/built explicitly; none of these results are empty--no-restoreruns.Independent code review and structured review found no actionable findings. Commit-coherence advisory: OK. All changed source files match the tested checkout after CRLF/LF normalization.
Proof completed
The required build/regression validation and the two affected normal UI variants are complete. The production diagnostic files are unchanged in this review revision, so the earlier controlled component captures below still show the same returned-cause, redaction, and retry behavior.
Real behavior proof
Earlier product-page component proof, using controlled Gateway replies:
The fresh run also launched the actual
OpenClaw.Tray.WinUI.exe, discovered local MCP tools before invocation, connected to an authenticated controlled Gateway, and navigated through the existing-Gateway connection and PC-access pages to the actual Connect your AI page. Copied live UI diagnostics contained:Not verified / blocked
The two opt-in native capture variants failed their foreground-window assertion: Windows returned
GetForegroundWindow() == 0. The live setup window could be read, but automated card activation/capture could not complete in this desktop state. Fresh shipped-app rejection/retry screenshots remain unverified; no foreground or capability bypass was substituted. Controlled component diagnostics and normal UI regression tests passed as reported above.Native model startup/GPU inference was not exercised by these controlled diagnostics checks. The reported runtime instability stays tracked separately in #1713.