Repository navigation
Conversation
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 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. |
|
Codex review: blocked before merge. Reviewed September 28, 2026, 3:51 PM ET / 19:51 UTC (Revision 11). ClawSweeper reviewWhat this changesThe branch validates the configured Linux username before setup-wizard pairing or WSL commands, sends reload scripts through WSL standard input, and adds invalid-input regression tests. Merge readiness⛔ Blocked before merge - 2 items remain Keep this PR open. Current main handles the WSL stdin path but still lacks the wizard’s early invalid-user check. The proposed guard has direct behavior proof; the remaining blocker is integrating the head with current main and validating that result. Priority: P2 Review scores
Verification
How this fits togetherThe setup engine reads a local setup configuration, runs the Gateway wizard, and issues commands to the managed WSL distro. Its result controls whether pairing and the rest of setup can proceed. flowchart LR
A[Setup configuration] --> B[Wizard-only setup]
B --> C{Linux user valid?}
C -->|No| D[Terminal diagnostic]
C -->|Yes| E[Gateway pairing and WSL commands]
E --> F[Setup result]
Before merge
Agent review detailsSecurityNone. Review metricsNone. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the shared username guard at wizard entry while retaining stdin-based WSL execution, with the resolved head proving both invalid-user rejection and valid-user setup. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source shows that wizard-only setup skips the existing WSL-user validation step; the PR body also records production-path invalid-user runs with zero WSL commands after the change. Is this the best way to solve the issue? Yes. Reusing the existing username contract at the wizard boundary is a narrow repair, and stdin execution follows the repository’s documented WSL argument-handling pattern. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 373887bbe211. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (10 earlier review cycles; latest 8 shown)
|
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 68%; recommendation confidence 90%; effort small; risk medium. Reviewed exact head |
|
Head f2f7c33 records this closeout in the PR body. Invalid user: SetupWizard_RejectsInvalidLinuxUserBeforeAnyWslCommand passed for bad"user and bad$(id). The outcome is FailedTerminal, and the fake runner recorded no WSL call. Default user openclaw: live OPENCLAW_RUN_E2E FullSetup_TrayConnects_OperatorAndNode created OpenClawE2E-782a01ca, paired the operator and the node, and logged Gateway wizard completed. The following openclaw gateway restart exited 1 with GATEWAY_RESTART_PREPARATION_REFUSED, then the fixture rolled the distro back. That restart refusal is after wizard completion. Shared: 4093 passed, 0 failed, 32 skipped. Tray: 3059 passed, 5 failed. Those five source-contract failures are the same on 1488, 1489, 1490, and 1491. |
The username check ran only when reload mode was suspended or restored. Wizard startup with auto-approval reached the pairing WSL command first. RunCoreAsync now rejects the same invalid user before registry lookup, pairing, or any other wizard command. Tests: SetupWizard_RejectsInvalidLinuxUserBeforeAnyWslCommand passed, 2 of 2. ./build.ps1 exit 0. Shared 4093 passed, 32 skipped. Tray 3059 passed. Five source-contract tests still fail on an LF checkout because they expect CRLF snippets. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ac1f11f-ab65-454b-b841-11c6cafe7381
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
Maintainer landing assessment is complete at |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ac1f11f-ab65-454b-b841-11c6cafe7381
|
Current |
What Problem
--wizard-onlydoes not runConfigureWslInstanceStep, which was previously the only caller ofWslConfig.IsValidLinuxUserName. The wizard path could therefore reach operator pairing and WSL reload commands with an invalid configured Linux user. The old username regex also accepted a single trailing newline because it used$rather than an absolute end anchor.What Changed
bash -sstdin sowsl.exedoes not rewrite$PATHin argv.\A...\zanchors.null, and trailing-newline input.Required proof pools
windows-wsl-gateway-e2e: wizard-only WSL username handling and the valid-user product setup path changed.Validation
PR head:
f4006f0c957863948089b10e15e60e56aa516c83(empty validation refresh commit on the contributor branch).Locally integrated merge candidate: tree
3c7a7428510151d6f82553ac50fee2d8a4001ecb, combining the PR head with currentmain60e978c594e582586c1e27e8624d1073294622cb, which includes #1507 (fix(setup): retry guarded restart intent contention)../build.ps1: passed.dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 4,145 passed, 33 skipped, 0 failed.dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 3,180 passed, 0 failed.dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore: 1,393 passed, 1 skipped, 0 failed.setup-connectshard with task-owned processTEMP/TMPonD:\: 32 passed, 13 failed, 1 skipped. Both real setup fixtures passed WSL preflight, operator pairing, node pairing, andGateway wizard completed; fix(setup): retry guarded restart intent contention #1507's restart-intent contention no longer failed setup. Later failures were host tray/MCP connection timeouts and disconnected-state fanout. Cleanup completed, no test distro remained, and the task-owned temp directory was deleted.Real behavior proof
The real
OpenClaw.SetupEngine.Program.Mainwizard-only path was invoked with its productionCommandRunner, not a fake runner.bad$(id)FailedTerminal, exit 1nullFailedTerminal, exit 1openclaw\nFailedTerminal, exit 1Each run stopped at
run-wizardwith the actionableInvalid WSL userdiagnostic before registry loading, pairing, or WSL execution.The integrated valid-user setup proof installed fresh WSL distros using the default
openclawuser, completed operator and node pairing, completed the Gateway wizard, restored reload mode through stdin, exercised #1507's guarded restart-intent retry, and reached post-setup tray/MCP testing.Publication blocker
Not merged yet. A clean two-parent merge with current
mainexists locally as commit56504a32f2c0d7b1c0aac428962b22f450adfee0(parentsf4006f0cand60e978c5; tree3c7a7428). Direct push is rejected because this OAuth token lacksworkflowscope and currentmainchanges.github/workflows/ci.yml. GitHub's update-branch REST endpoint returns HTTP 422merge conflict, while localgit merge-tree --write-treesucceeds with no conflict.A workflow-authorized maintainer must push that prepared merge commit to SebTardif's original
fix/f026-invalid-linux-userbranch, or perform the equivalent normal merge in GitHub's web interface. Then required PR CI can run against currentmain; merge only if it is green.