feat(setup): inject Windows-node guidance into AGENTS.md after pairing#659
Draft
paulcam206 wants to merge 1 commit into
Draft
feat(setup): inject Windows-node guidance into AGENTS.md after pairing#659paulcam206 wants to merge 1 commit into
paulcam206 wants to merge 1 commit into
Conversation
Add WindowsNodeBootstrapContextStep that runs after node pairing and injects a managed BEGIN/END block into the gateway's AGENTS.md (workspace resolved from `openclaw config get agents.defaults.workspace`, or from a `WindowsNodeContext.WorkspacePath` override). The injected block tells the running OpenClaw agent how to use the paired Windows tray node (nodes tool, exec host=node, tools.exec defaults, etc.). The file mutation is pure POSIX shell + awk + base64 — no Node.js dependency, no embedded JS, no heredocs. Bare WSL distros (and the gateway distro) do not have node installed, so the previous node-based approach failed at runtime. Scripts that need bash variable handling are piped to `bash -s` via stdin instead of being passed as argv to `bash -c`, because wsl.exe performs shell variable expansion on argv before bash sees it and silently drops user-defined $var references. New docs/WSL_EXE_ARGV_PITFALL.md documents the footgun, the empirical reproduction against fresh Ubuntu-26.04, and the ranked fixes. Notable changes: * CommandRunner.RunInWslAsync gains opt-in `inputViaStdin: true` that switches argv to `bash -s` and pipes the script over stdin * WindowsNodeBootstrapContextStep computes the absolute workspace path once via ExpandLinuxPath and threads it through both `openclaw setup --workspace` and the apply script, so `~/foo` or relative-path overrides cannot land in different directories * RunOpenclawSetupAsync and ResolveWorkspacePathAsync use the stdin path because their scripts include $PATH via WslPathPrefix * Apply script is idempotent and handles missing/symlink/malformed AGENTS.md; rollback removes the managed block in-place * Cross-references in AGENTS.md and docs/WINDOWS_NODE_TESTING.md surface the pitfall doc from likely discovery points * Tests cover apply/rollback shape, override expansion, per-call stdin-vs-argv invariants, and a tilde-override regression * Verified end-to-end against bare Ubuntu-26.04 (no openclaw, no node): apply-to-missing, apply-preserves-content, idempotency (exactly 1 BEGIN marker after second apply), rollback restores original content Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: found issues before merge. Reviewed June 18, 2026, 3:26 AM ET / 07:26 UTC. Summary Reproducibility: not applicable. This PR adds new setup behavior rather than fixing a reproduced current-main bug. Review confidence comes from source/diff inspection, PR discussion, and the contributor's live smoke proof. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land only after the managed guidance is scoped to intentional use of this paired Windows node, permissive gateway exec recommendations are removed or moved behind explicit consent/docs, and AGENTS enrichment becomes non-blocking after pairing. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds new setup behavior rather than fixing a reproduced current-main bug. Review confidence comes from source/diff inspection, PR discussion, and the contributor's live smoke proof. Is this the best way to solve the issue? No, not as-is: the stdin WSL execution and managed-marker mechanics are useful, but the global guidance content and fail-closed enrichment behavior need revision before this is the maintainable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 234fa8ff097f. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity 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
|
clawsweeper
Bot
added
proof: sufficient
Contributor
Author
|
just as a heads up -- this change is a little wonky in terms of what we're doing... it works, but I wouldn't say that it's elegant. anyways, here's a completely clean install being asked to pop a notification: |
Contributor
|
@paulcam206 does it only do it if the message is coming in via the Windows app? |
Contributor
|
Automated Copilot review note from the wide PR triage pass: I would not treat this as safe to merge at 90% confidence yet, even aside from the current conflict state. The core concern is scope: this PR injects Windows-node guidance into the gateway workspace's global AGENTS.md, so it can affect agent behavior for requests that did not originate from the local Windows tray chat — for example Telegram-originated work, remote/gateway sessions, or future multi-node scenarios. Two things I think should be tightened before merge:
Separately, because this is post-pairing enrichment, I would prefer injection failures/timeouts/malformed existing markers to be non-terminal or self-healing where possible. Optional guidance should not block onboarding after pairing succeeds. |
Contributor
|
Thanks for the proof and the implementation here. I don't think this is safe to merge as-is yet because the generated Could you please revise this so the injected guidance is clearly scoped to tasks that intentionally need this paired Windows node, remove or soften the permissive |
clawsweeper
Bot
added
proof: 📸 screenshot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new setup step (
WindowsNodeBootstrapContextStep) that runs after node pairing and injects a managed<!-- BEGIN ... -->/<!-- END ... -->block into the gateway'sAGENTS.md. The injected block tells the OpenClaw agent how to use the paired Windows tray node (thenodestool,exec host=node,tools.execdefaults, etc.) so the agent stops trying to do Windows-only work inside the WSL gateway.Without this step, the agent has no idea a Windows companion node exists and treats the gateway as the only execution surface.
Behavior
openclaw config get agents.defaults.workspace(or aWindowsNodeContext.WorkspacePathoverride).AGENTS.mdif missing; appends the managed block; preserves any existing content above it.Implementation notes
1. Pure POSIX shell, no Node.js dependency
The previous internal iteration of this step embedded a Node.js script for the file mutation. That broke at runtime in bare WSL distros:
openclaw setupis the Rust CLI, not a Node.js app, so node is not guaranteed to be installed. This PR usesawk+base64+ standard POSIX builtins instead — no node, no embedded JS, no heredocs.2. Stdin path on
RunInWslAsync(new opt-in)While debugging this step against a fresh Ubuntu-26.04 we re-discovered a long-standing wsl.exe gotcha already half-documented at
SetupSteps.cs:937-939:So
workspace='/home/openclaw/...'; mkdir -p "$workspace"arrives at bash asmkdir -p ""and dies withcannot create directory ''. This PR adds an opt-ininputViaStdin: trueparameter toICommandRunner.RunInWslAsyncthat pipes the script tobash -sover stdin (which wsl.exe doesn't touch). The new step opts in; existing callers are unchanged.A new
docs/WSL_EXE_ARGV_PITFALL.mddocuments the pitfall with the empirical reproduction table, ranked fixes, and what does NOT work (single quotes, double quotes, braces, subshells, prefix assignment,-e VAR=valforwarding — all verified). The existingValidateWslLockdownStepcomment is expanded with a cross-reference, andAGENTS.mdplusdocs/WINDOWS_NODE_TESTING.mdget pointers so future agents discover it from the right entry points.3. Workspace-path consistency
WindowsNodeContext.WorkspacePathoverrides like~/fooor relative paths used to be passed raw toopenclaw setup --workspacewhile the apply script ran against theExpandLinuxPath-resolved version, which could land setup and injection in different directories. The override is now expanded once and threaded through both invocations.4. Helper calls that include
$PATHRunOpenclawSetupAsyncandResolveWorkspacePathAsyncbuild their command viactx.WslPathPrefix, which references$PATH. On the argv path, wsl.exe expands$PATHto the Windows$PATH— usually harmless because the Linux openclaw bin is prepended, but fragile. Both helpers now use the stdin path so the Linux$PATHis preserved.Validation
./build.ps1✅OpenClaw.Shared.Tests— 2045 passed / 29 skipped ✅OpenClaw.Tray.Tests— 934 passed ✅OpenClaw.SetupEngine.Tests— 222 passed (added tilde-override regression + per-call stdin-vs-argv assertions) ✅--mode local):autoreview clean: no accepted/actionable findings reported · patch is correct (0.82)✅Files
src/OpenClaw.SetupEngine/CommandRunner.csinputViaStdinparameter; XML docs explaining the pitfallsrc/OpenClaw.SetupEngine/SetupSteps.csWindowsNodeBootstrapContextStep(apply + rollback); expanded existing pitfall commentsrc/OpenClaw.SetupEngine/SetupPipeline.csPairNodeStepand beforeVerifyEndToEndStepsrc/OpenClaw.SetupEngine/SetupContext.csWindowsNodeContextConfig(Enabled, WorkspacePath, TimeoutSeconds)src/OpenClaw.SetupEngine/WindowsNodeContextSection.cssrc/OpenClaw.SetupEngine/default-config.jsonWindowsNodeContextsectiontests/OpenClaw.SetupEngine.Tests/*docs/WSL_EXE_ARGV_PITFALL.mddocs/WINDOWS_NODE_TESTING.md,docs/ONBOARDING_WIZARD.md,AGENTS.md