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: needs real behavior proof before merge. Reviewed October 7, 2026, 12:47 PM ET / 16:47 UTC (Revision 7). ClawSweeper reviewWhat this changesThe setup engine quotes Gateway bind, authentication mode, and reload mode as single shell arguments and adds regression coverage. Merge readiness⛔ Blocked before merge - 2 items remain The fix remains necessary: main and the latest release still emit these values without quoting. No introduced correctness defect was found, but the declared setup E2E proof requirement remains unmet. Priority: P2 Review scores
Verification
How this fits togetherThe Windows setup engine turns Gateway settings into a script executed inside WSL. That script configures the Gateway before subsequent startup and pairing steps. flowchart TD
A[Setup settings] --> B[Validate configuration]
B --> C[Quote configuration values]
C --> D[Send script through WSL stdin]
D --> E[Gateway configuration CLI]
E --> F[Startup and pairing]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the focused quoting repair using the existing POSIX helper, with successful product setup integration demonstrated before landing. Do we have a high-confidence way to reproduce the issue? Yes, from source: a shell-metacharacter value in Gateway.AuthMode or Gateway.ReloadMode reaches an unquoted command on main. This review did not execute that failing path. Is this the best way to solve the issue? Yes. Reusing the canonical POSIX quoting helper preserves valid arguments and avoids a parallel escaping implementation; setup integration proof remains incomplete. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5e3fb40bcc89. LabelsLabel 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 (6 earlier review cycles)
|
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 72%; recommendation confidence 88%; effort small; risk medium. Reviewed exact head |
ExtraConfig reload mode is the unquoted value that can still reach the command. ExecuteAsync now rejects or quotes a reload mode that contains shell metacharacters. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
What Problem
BuildConfigCommands wrote gateway.bind, gateway.auth.mode, and gateway.reload.mode into a bash -c script with no quoting. Extra config values were already single-quoted. The script also contained $PATH, which wsl.exe expands before bash.
Why
A setup value with a semicolon, quote, or command substitution ran in the gateway distro.
User Impact
Those three fields are now POSIX single-quoted, and the configure script is sent with inputViaStdin true.
Evidence
Head
47e860bb02ea3e2c310716da33f40443b6f6cd17.Production
ConfigureGatewayStep.BuildConfigCommandsbuilt the configure script for bindloopback, auth modetoken, and reload modehybrid;$(id); echo PWNED.CommandRunner.RunInWslAsyncsent that script towsl.exe -d Ubuntu-24.04 -- bash -s(inputViaStdin: true). A shell function namedopenclawprinted each argument so the real CLI was not invoked and no gateway config was changed.wsl.exeexited 0.gateway.reload.modearrived as one argument,hybrid;$(id); echo PWNED. There was no second line that executedPWNED.gateway.bindwasloopback.gateway.auth.modewastoken. The auth token argument was the fake valueproof-token-not-real.On 2026-09-25 the same quoting was sent to the real binary
/root/.openclaw/bin/openclaw, OpenClaw 2026.9.6 (eb377ac).gateway.reload.modewas unset.openclaw config set gateway.reload.modewith that value as one single-quoted argument exited 1. The output had nouid=line, and the key stayed unset.openclaw config set gateway.reload.mode hybridthenopenclaw config get gateway.reload.modereturnedhybrid. The key was unset again afterward. Windowshttp://127.0.0.1:18789/still returned HTTP 200.The production configure script was then piped to
bash -swith that same binary onPATH. It ranopenclaw plugins registry --refreshand theconfig setlines for mode, port18789, bindloopback, auth modetoken, the existing gateway token, reload modehybrid, the node command allowlist, and device-pair public URL plus enabled. Exit 0. Stdout containedGATEWAY_CONFIGURED. Token length stayed 48. Read-back was modelocal, port18789, bindloopback, auth modetoken, reload modehybrid, public URLhttp://127.0.0.1:18789, device-pair enabledtrue. Reload mode and the other keys that had been unset were unset again afterward. Windowshttp://127.0.0.1:18789/returned HTTP 200 after the gateway restart.Required proof pools
GATEWAY_CONFIGUREDthrough the real OpenClaw 2026.9.6 CLI. A full setup wizard was not run.Validation
47e860bb02ea3e2c310716da33f40443b6f6cd17.dotnet build src/OpenClaw.SetupEngine/OpenClaw.SetupEngine.csproj -p:Platform=x64succeeded.ConfigureGateway_ExecuteAsyncQuotesOrRejectsHostileReloadMode: 1 passed, 0 failed../build.ps1exited 0.Real behavior proof
bash -s.47e860bb02ea3e2c310716da33f40443b6f6cd17.BuildConfigCommandsplusRunInWslAsynctowsl.exe -d Ubuntu-24.04 -- bash -s. Then the same configure script piped tobash -swith/root/.openclaw/bin/openclawonPATH, using the existing gateway token. Read the keys back, then unset the ones that had been unset.GATEWAY_CONFIGURED. Token length stayed 48. Reload mode read back ashybridduring the run and was unset again afterward.$(id)did not run, and the product configure script completed against the real CLI.