fix: setup fails closed for invalid configuration arguments

[calebeden]

What Problem This Solves

Fixes an issue where users running SetupEngine with invalid explicit configuration or malformed arguments could silently load bundled defaults and start setup with settings they did not request. This included missing option values, unknown or misspelled options, unconsumed positional arguments, nonexistent paths, unreadable or malformed files, and semantically empty JSON.

For example, with --config --headless, the old parser treated --headless as the config filename. Because that file did not exist, SetupEngine silently loaded default-config.json while separately recognizing --headless. Since those defaults enable CleanBeforeRun, a malformed unattended invocation could unregister the existing app-owned WSL distro and remove setup-managed gateway state before rebuilding it.

The tray-hosted setup window also parsed arguments independently, ignored equals syntax and malformed/unknown input, and had no visible recovery path for invalid or JSON null configuration.

Why This Change Was Made

SetupEngine now parses its complete option contract in one shared, fail-closed pass. It supports case-insensitive --name value and --name=value syntax for value options; rejects unknown options, positional tokens, bare --, values on boolean flags, missing values, and duplicate value options; and preserves idempotent duplicate bare flags. Explicit configuration failures never fall back to bundled defaults.

The tray-hosted setup window uses the same parser with a narrow contract for --config and --no-rollback-on-failure. A focused projection removes only valid tray restart/deep-link host arguments. Invalid, missing, self, signed, overflow, or whitespace-padded restart PIDs and unknown post-setup launch targets remain visible to strict parsing and render the failure page before configuration loading, setup-lock acquisition, or pipeline startup.

CLI and WinUI also share expected configuration-load error handling, so missing, unreadable, malformed, directory-path, and JSON null configuration failures are visible in the existing setup failure UI.

User Impact

Operators and automation now receive immediate deterministic errors for malformed setup invocations instead of risking unintended setup or cleanup using bundled defaults. Valid existing invocations continue to work, value options additionally accept equals syntax, legitimate tray restart/deep-link launches remain compatible, and WinUI users receive a visible error with a Close action.

Evidence

• Added complete option-contract snapshots and behavior matrices for all nine value options and eight boolean flags.
• Added parser and Program.Main coverage for separated/equals syntax, mixed case, embedded equals, missing/blank values, unknown and misspelled options, positional tokens, bare --, flag values, duplicate policy, first-error behavior, and bundled-fallback prevention.
• Added setup-window parser and tray projection coverage, including valid restart/deep-link arguments and invalid/self/signed/overflow/whitespace PID and launch-target cases.
• Added shared configuration-loader coverage and a WinUI contract regression ensuring errors are shown before setup starts.
• Hanselman dual-model review found one medium edge case involving invalid host values; it was fixed. The final focused follow-up review found no actionable issues.

CLI behavior screenshots

Links reverified successfully on 2026-07-17.

1. Motivating malformed command

2. Nonexistent explicit config

3. Valid explicit config dry run

Change Type

• Bug fix
• Feature
• Refactor
• Docs or instructions
• Tests or validation
• Security hardening
• Chore or infrastructure

Scope

• Tray or WinUI UX
• Windows node capability
• Local MCP or winnode
• Gateway, connection, or pairing
• Setup or onboarding
• Permissions, privacy, or security
• Tests, CI, or docs

Validation

• .\build.ps1 — passed
• dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore — 588 passed, 0 failed, 0 skipped
• dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore — 2,866 passed, 0 failed, 31 conditionally skipped
• dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore — 1,767 passed, 0 failed, 0 skipped
• Focused final tray parser/projection/ordering selection — 80 passed, 0 failed, 0 skipped

Real Behavior Proof

• Environment tested: Windows ARM64, .NET SDK 10.0.301
• PR head or commit tested: b31d9b35b7e7b3f478b9f6e326974b10935ab062
• Exact steps or command run: launched the current built tray with isolated data, forced onboarding, and --wait-for-pid abc; inspected the active SetupWindow through UI Automation.
• Evidence after fix:

  Setup failed
  Invalid setup arguments: Unknown option '--wait-for-pid'.
  Close
  

• Observed result: the invalid tray host value was preserved for strict setup parsing, the real setup window remained visible on its failure page, and no configuration load, setup lock, or pipeline run began.
• Screenshot or artifact links verified? (Yes/No/N/A): Yes; all three embedded CLI screenshots returned HTTP 200. A current-head WinUI screenshot was captured locally, and copied UI Automation output is included above.
• Not verified or blocked: None

Security Impact

• New permissions or capabilities? (Yes/No): No
• Secrets or tokens handling changed? (Yes/No): No
• New or changed network calls? (Yes/No): No
• Command or tool execution surface changed? (Yes/No): Yes
• Data access scope changed? (Yes/No): No
• If any answer is Yes, explain the risk and mitigation: Existing setup command-line handling is stricter and supports equals syntax for existing value options. No commands or privileges were added; malformed input is rejected before setup execution.

Compatibility and Migration

• Backward compatible? (Yes/No): No for malformed or duplicate-value invocations; valid invocations remain compatible
• Config or environment changes? (Yes/No): No
• Migration needed? (Yes/No): Only for callers that repeat value options
• If yes, list the exact upgrade steps: Remove duplicate value options and pass each value option once. Unknown, positional, and malformed arguments must be corrected instead of relying on them being ignored.

Review Conversations

• I replied to or resolved every bot review conversation addressed by this PR.
• I left unresolved only conversations that still need maintainer judgment.

Scott's feedback about unknown options, equals syntax, duplicate policy, synchronized option coverage, visible WinUI configuration failures, and canonical tray-hosted parsing is addressed in the current branch.

[clawsweeper]

Codex review: found issues before merge. Reviewed July 17, 2026, 8:14 PM ET / July 18, 2026, 00:14 UTC.

Summary
The branch replaces permissive SetupEngine and tray setup argument handling with shared fail-closed parsing, configuration-load errors, tray-host argument projection, documentation, and focused tests.

Reproducibility: yes. for the original bug at source level: the supplied review and branch tests identify the old malformed --config path and the branch adds direct regression coverage, but this read-only review did not execute the pre-fix binary.

Review metrics: 2 noteworthy metrics.

• Patch surface: 15 files affected; 1,120 additions and 78 deletions. The hardening spans shared parsing, native setup UX, tray argument projection, documentation, and multiple test suites.
• CLI contract: 9 value options and 8 boolean flags. The new duplicate policy affects the full value-option contract rather than only --config.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• Get an explicit maintainer decision on duplicate value-option semantics, then align the parser and compatibility tests with that decision.

Risk before merge

• [P1] Merging changes existing scripts that repeat any value option from running with their prior first value to exiting with an argument error; documentation explains the break but does not establish that maintainers want this repository-wide CLI contract.

Maintainer options:

1. Restore the legacy duplicate contract (recommended)
   Preserve first-value-wins for existing duplicate value options while retaining strict parsing for malformed and unknown input.
2. Approve the breaking CLI contract
   Explicitly accept duplicate-value rejection for all existing value options and retain the migration guidance as the supported upgrade path.

Next step before merge

• [P1] A maintainer must choose the supported duplicate-value CLI contract; automation cannot safely decide whether this compatibility break is acceptable.

Maintainer decision needed

• Question: Should SetupEngine permanently reject duplicate occurrences of all nine existing value options, replacing the legacy first-value-wins CLI behavior?
• Rationale: The reported safety bug does not require changing duplicate semantics across unrelated established options, and the supplied discussion contains no maintainer confirmation that this upgrade break is desired.
• Likely owner: shanselman — This reviewer surfaced the concrete setup CLI compatibility boundary, though the automated-review disclaimer means ownership confidence is low.
• Options:
  • Preserve existing duplicate semantics (recommended): Retain first-value-wins for duplicate value options and keep the fail-closed handling limited to malformed, unknown, missing, and explicit-config load failures.
  • Adopt strict duplicate rejection: Accept exit-code failure for existing scripts with repeated value options and record explicit maintainer approval for the documented migration.

Security
Cleared: The diff adds no dependencies, permissions, network access, or new command capability; it narrows existing setup execution by rejecting invalid arguments before pipeline startup.

Review findings

• [P1] Preserve the prior duplicate-value CLI contract — src/OpenClaw.SetupEngine/SetupArgumentParser.cs:34-39

Review details

Best possible solution:

Keep strict rejection for malformed, unknown, and missing arguments, while preserving legacy duplicate-value behavior unless the setup owner explicitly approves a documented breaking change for all value options.

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

Yes for the original bug at source level: the supplied review and branch tests identify the old malformed --config path and the branch adds direct regression coverage, but this read-only review did not execute the pre-fix binary.

Is this the best way to solve the issue?

Unclear: shared fail-closed parsing is the maintainable fix for malformed setup arguments, but applying strict duplicate rejection to every value option is broader than the reported defect without an explicit compatibility decision.

Full review comments:

• [P1] Preserve the prior duplicate-value CLI contract — src/OpenClaw.SetupEngine/SetupArgumentParser.cs:34-39
  The parser now rejects a second occurrence of every value option, replacing legacy first-value-wins behavior for unrelated established options such as --log-path and --gateway-port. The malformed-config safety fix does not require this broad upgrade break; preserve the old contract or obtain explicit setup-owner approval for the migration before merging.
  Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4518dd87ed2b.

Label changes

Label changes:

• add proof: sufficient: Contributor real behavior proof is sufficient. The PR provides current-head CLI screenshots plus copied native UI Automation output demonstrating that an invalid tray-host argument reaches the setup failure page before configuration loading or pipeline startup.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR provides current-head CLI screenshots plus copied native UI Automation output demonstrating that an invalid tray-host argument reaches the setup failure page before configuration loading or pipeline startup.
• remove status: 📣 needs proof: Current PR status label is status: ⏳ waiting on author.
• remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

• P1: The PR fixes a destructive malformed-setup path but presently introduces an unconfirmed breaking behavior for existing automation.
• merge-risk: 🚨 compatibility: Rejecting duplicate value options changes the established first-value-wins behavior for existing setup scripts.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR provides current-head CLI screenshots plus copied native UI Automation output demonstrating that an invalid tray-host argument reaches the setup failure page before configuration loading or pipeline startup.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR provides current-head CLI screenshots plus copied native UI Automation output demonstrating that an invalid tray-host argument reaches the setup failure page before configuration loading or pipeline startup.

Evidence reviewed

What I checked:

• Shared parser expands duplicate rejection beyond the reported malformed-config case: The new shared parser rejects any repeated value option before host-specific processing; this applies to all nine value options, including established operational options such as --log-path and --gateway-port. (src/OpenClaw.SetupEngine/SetupArgumentParser.cs:34, b31d9b35b7e7)
• The branch documents an intentional compatibility break: The setup-engine redesign documentation says duplicate value options are rejected and directs scripts that relied on legacy first-value-wins behavior to change before upgrading. (docs/SETUP_ENGINE_REDESIGN.md:321, b31d9b35b7e7)
• Earlier review identified the same unresolved contract decision: The latest prior ClawSweeper review required preservation of the prior duplicate-value CLI contract unless the setup owner explicitly accepts strict rejection; the subsequent branch update retained rejection and added documentation, but no maintainer decision is present in the supplied discussion. (b31d9b35b7e7)
• Current-head tray parsing and failure ordering are covered in the branch: The SetupWindow now invokes its canonical narrow parser before configuration loading and setup-lock acquisition, with contract tests covering malformed setup arguments and failure-page ordering. (src/OpenClaw.SetupEngine.UI/SetupWindow.xaml.cs:102, b31d9b35b7e7)
• Validation and current-head behavior proof were supplied: The PR reports successful build, setup-engine, shared, and tray test suites, completed CI checks, CLI screenshots, and current-head UI Automation output showing an invalid tray argument reaches the visible failure page before setup starts. (b31d9b35b7e7)

Likely related people:

• shanselman: The supplied review discussion contains the only concrete routing signal for the setup argument contract and explicitly called out the duplicate-value compatibility decision. (role: reviewer; confidence: low; files: src/OpenClaw.SetupEngine/Program.cs, src/OpenClaw.SetupEngine/SetupArgumentParser.cs, docs/SETUP_ENGINE_REDESIGN.md)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

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

• 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.

Review history (7 earlier review cycles)

• reviewed 2026-07-17T01:39:17.645Z sha 11fd480 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T02:20:52.775Z sha 11fd480 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T03:47:28.443Z sha 11fd480 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T18:34:47.327Z sha b478685 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T19:16:16.034Z sha b478685 :: found issues before merge. :: [P1] Preserve the prior duplicate-value CLI contract
• reviewed 2026-07-17T21:17:24.625Z sha df077e4 :: needs real behavior proof before merge. :: [P1] Route SetupWindow through the canonical argument parser | [P1] Preserve the prior duplicate-value CLI contract
• reviewed 2026-07-17T23:33:00.376Z sha b31d9b3 :: needs real behavior proof before merge. :: [P1] Preserve the prior duplicate-value CLI contract

[shanselman]

GitHub Copilot maintainer-assistant review — this is automated feedback, not a comment written personally by Scott.

Reviewed head: 11fd4808c38b1c387a24e2f05def60283a494e7f

The core bug is real and worth fixing: --config --headless must not consume a flag as a filename and then silently fall back to bundled defaults. However, the new parser does not yet fully establish fail-closed argument handling.

Merge blockers / required decisions:

1. TryParseValueArguments ignores every unknown token. Forms such as --config=missing.json or a typo such as --confg missing.json are not recognized as explicit config attempts and can still fall through to bundled defaults. Either support --name=value for value options or reject unknown --... options with exit code 2. Add direct process/tests for both equals-form and misspelled config options.
2. Duplicate value options now fail globally, replacing the old first-value-wins behavior for all nine options. That may be the right safer contract, but it is a compatibility change for scripts. Please document the intended duplicate policy and add migration/compatibility tests; otherwise narrow strict duplicate rejection to the security-sensitive options being fixed.
3. SetupConfig.LoadFromFile now throws for JSON null. The CLI catches this through TryLoadConfig, but SetupWindow.xaml.cs calls LoadFromFile directly. A config containing null can now crash the WinUI setup path. Catch and surface the error in the UI, or keep the shared method contract stable and put CLI-specific null rejection inside TryLoadConfig.

Please also avoid maintaining a second unsynchronized option-name list in tests. Expose the canonical internal option set or add an assertion that test coverage and production options stay aligned.

Useful live/process proof at the final head:

• --config --headless → exit 2;
• --config <missing> → exit 2;
• --config=<missing> → exit 2 (or documented supported parsing followed by load failure);
• --confg <path> → exit 2 as unknown;
• valid explicit config + --dry-run → exit 0;
• bundled default with no explicit config still works;
• chosen duplicate policy has a documented, tested result.

The current test additions are substantial, but until unknown/equals-form options and the WinUI caller are covered, the implementation still has fail-open/crash edges adjacent to the bug class it is fixing.

[calebeden]

Caleb Eden + GitHub Copilot pair-programming follow-up

@shanselman Thanks for the detailed review. We replaced the separate value and flag scans with one case-insensitive parser backed by canonical option sets. SetupEngine now accepts both --name value and --name=value, while rejecting unknown options, positional arguments, bare --, values on boolean flags, and missing or blank values.

We kept strict duplicate rejection for value options because repeated setup values are ambiguous and fail-closed behavior is safer here. Duplicate bare flags remain idempotent. That policy is now documented and covered by compatibility tests. Tests also combine production-owned option sets with hard-coded contract snapshots so option coverage cannot silently drift.

The WinUI path now uses the same fail-closed configuration loader as the CLI. Missing, invalid, unreadable, malformed, directory-path, and JSON null configurations are shown on the setup failure page before the setup lock or pipeline starts.

At the current head, the full build passes along with 573 SetupEngine tests, 2,863 Shared tests, and 1,718 Tray tests. The PR description and behavior proof have also been updated to reflect the complete change.

[shanselman]

GitHub Copilot maintainer-assistant follow-up — automated feedback, not written personally by Scott.

Re-reviewed head: b478685b33b227c3b88da1b20685cfaa6b7345a5

The shared fail-closed loader, canonical CLI option sets, equals-form support, unknown/positional rejection, option-drift coverage, and visible WinUI configuration failure page are meaningful improvements.

However, current-head SetupWindow.xaml.cs still obtains its explicit config path with the legacy local helper:

var explicitConfigPath = configPath ?? GetArg(args, "--config");

That helper only recognizes --config <value> and performs no unknown/missing-value validation. Therefore the WinUI-hosted path still differs from the canonical parser: --config=..., bare --config, or --confg ... can be ignored and fall through to bundled defaults. Using the shared TryLoadFromFile fixes malformed file handling only after a path was recognized; it does not close this argument-parsing gap.

Please route the WinUI command-line path through the same canonical parser (or extract a shared parser owner consumed by both hosts) and add direct WinUI/host tests for equals-form, missing value, typo/unknown option, positional input, and valid explicit config. The failure must occur before setup lock acquisition or pipeline startup.

Separately, duplicate value rejection is now correctly implemented and documented, but it remains a deliberate compatibility decision. Our recommendation is to preserve first-value-wins unless the setup owner explicitly accepts strict duplicate rejection as the new supported contract. If strict rejection is retained, call that breaking behavior out prominently in migration/CLI docs.

The current red test job is the unrelated ConnectAndReconnect_EmitCompletedOperatorSpans flake, but required CI must still be rerun green before merge.

[calebeden]

Caleb Eden + GitHub Copilot pair-programming follow-up

@shanselman Thanks for the follow-up review. We addressed the remaining WinUI argument-parsing gap in b31d9b35.

The tray-hosted SetupWindow now uses the same canonical fail-closed parser as SetupEngine, with a narrow UI contract. It supports --config=<path> and rejects missing values, typo or unknown options, positional input, and malformed tray-host arguments before configuration loading, setup-lock acquisition, or pipeline startup.

Valid restart and deep-link arguments remain supported. Invalid, self, signed, overflow, or whitespace-padded restart PIDs and unknown post-setup launch targets remain visible to strict parsing instead of being discarded.

We retained strict duplicate-value rejection as an intentional compatibility decision and now call it out prominently in the migration/CLI documentation. Direct parser, host-projection, configuration-loading, and lifecycle-ordering tests cover the requested cases.

At the current head, the full build passes along with 588 SetupEngine tests, 2,866 Shared tests, and 1,767 Tray tests. The PR description and current-head behavior proof have also been updated. Fresh CI is currently running.

[shanselman]

Maintainer decision recorded by GitHub Copilot assistant at Scott’s direction.

The strict duplicate-value contract is accepted: SetupEngine should reject repeated value options with exit code 2 rather than preserve legacy first-value-wins behavior. The current head documents and tests that compatibility change, while duplicate bare flags remain idempotent.

This resolves the remaining policy/governance question from review.