Skip to content

windows: unify non-core spawn handling across acp qmd and docker#31750

Merged
Takhoffman merged 2 commits intomainfrom
fix/windows-spawn-canonicalization-31740
Mar 2, 2026
Merged

windows: unify non-core spawn handling across acp qmd and docker#31750
Takhoffman merged 2 commits intomainfrom
fix/windows-spawn-canonicalization-31740

Conversation

@Takhoffman
Copy link
Contributor

@Takhoffman Takhoffman commented Mar 2, 2026

Summary

Closes #31740 by unifying remaining non-core Windows spawn behavior behind the shared Windows spawn resolver.

This removes mixed ad-hoc behavior across ACP client, QMD/mcporter memory path, and Docker sandbox command execution.

What changed

  • src/acp/client.ts
    • Added resolveAcpClientSpawnInvocation(...) using shared windows-spawn policy.
    • createAcpClient(...) now uses resolved command/argv/shell/windowsHide.
  • src/memory/qmd-manager.ts
    • Added shared spawn resolution helper for runQmd(...) and runMcporter(...).
    • Uses canonical policy for .cmd/.bat wrappers and shell fallback.
  • src/agents/sandbox/docker.ts
    • Added resolveDockerSpawnInvocation(...) using shared windows-spawn policy.
    • execDockerRaw(...) now spawns with resolved command/argv/shell/windowsHide.

Tests

  • Added: src/agents/sandbox/docker.windows.test.ts
  • Expanded: src/acp/client.test.ts
  • Expanded: src/memory/qmd-manager.test.ts

Verified locally:

  • pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Scope / risk

  • Scope is limited to Windows spawn invocation wiring and targeted tests.
  • No broad refactors or behavior changes outside spawn resolution paths.
  • extensions/acpx was already aligned and unchanged.

Notes

pnpm check currently fails on pre-existing TypeScript errors in src/slack/monitor/events/messages.ts unrelated to this PR.

@openclaw-barnacle openclaw-barnacle bot added agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Mar 2, 2026
@Takhoffman Takhoffman merged commit cd653c5 into main Mar 2, 2026
12 of 14 checks passed
@Takhoffman Takhoffman deleted the fix/windows-spawn-canonicalization-31740 branch March 2, 2026 14:05
@Takhoffman
Copy link
Contributor Author

Takhoffman commented Mar 2, 2026

PR #31750 - windows: unify non-core spawn handling across acp qmd and docker (#31750)

Merged via squash.

  • Merge commit: cd653c5
  • Verified:
    • pnpm install --frozen-lockfile
    • pnpm build
    • pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
    • pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts
  • Changes made:
    • Added changelog entry for Windows non-core spawn canonicalization under 2026.3.2 Fixes.
  • Why these changes were made:
    • Changelog is required for this user-facing reliability fix and autoland policy in this run.
  • Changelog: CHANGELOG.md updated=true required=true opt_out=false

Thanks @Takhoffman!

chatgpt-codex-connector[bot]
chatgpt-codex-connector bot reviewed Mar 2, 2026
View reviewed changes
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70d439c11b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/memory/qmd-manager.ts
env: params.env,
execPath: process.execPath,
packageName: params.packageName,
allowShellFallback: true,
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Disable shell fallback for qmd and mcporter spawns

This unconditional allowShellFallback: true enables the shell: true path for unresolved .cmd wrappers, but runQmd and runMcporter feed user-derived search text into argv (for example via buildSearchArgs), so on Windows this can turn query metacharacters like & or | into cmd.exe operators instead of literal arguments. Because Node shell mode builds a single command string from file + argv, unresolved-wrapper environments can hit command injection/command breakage from normal chat input; this new resolver wiring should fail closed (or strictly escape) instead of falling back to shell execution.

Useful? React with 👍 / 👎.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 2, 2026

Greptile Summary

Unifies Windows spawn handling across ACP client, QMD/mcporter memory manager, and Docker sandbox by leveraging the shared windows-spawn resolution policy from src/plugin-sdk/windows-spawn.ts.

Key improvements:

  • Intelligently unwraps .cmd/.bat wrappers to underlying executables (node scripts or .exe files) when possible
  • Falls back to shell mode only when wrappers cannot be resolved, improving security
  • Maintains backward compatibility by preserving the resolveWindowsCommandShim behavior in QMD manager
  • Non-Windows platforms remain unchanged

Implementation pattern:

  • Each module adds a resolve*SpawnInvocation function that wraps the shared policy
  • Spawn calls now use resolved command, args, shell, and windowsHide options
  • Comprehensive test coverage for non-Windows, unwrapping, and shell fallback scenarios

Testing:

  • New test file for Docker: docker.windows.test.ts
  • Expanded tests for ACP client and QMD manager
  • All tests verify correct resolution on Windows vs non-Windows platforms

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk.
  • The changes are well-scoped, thoroughly tested, and leverage an existing proven shared policy. The implementation is consistent across all three modules, maintains backward compatibility, and includes comprehensive test coverage for all resolution paths (direct, unwrapping, shell fallback). No breaking changes or risky modifications detected.
  • No files require special attention.

Last reviewed commit: fc8f412

hanqizheng pushed a commit to hanqizheng/openclaw that referenced this pull request Mar 2, 2026
@Takhoffman
windows: unify non-core spawn handling across acp qmd and docker (ope…
56c662f 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
obviyus pushed a commit that referenced this pull request Mar 2, 2026
@Takhoffman @obviyus
windows: unify non-core spawn handling across acp qmd and docker (#31750
2ab26c1 
) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
Linux2010 pushed a commit to Linux2010/openclaw that referenced this pull request Mar 2, 2026
@Takhoffman @Linux2010
windows: unify non-core spawn handling across acp qmd and docker (ope…
fd6f45f 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
execute008 pushed a commit to execute008/openclaw that referenced this pull request Mar 2, 2026
@Takhoffman @execute008
windows: unify non-core spawn handling across acp qmd and docker (ope…
de6409f 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
dawi369 pushed a commit to dawi369/davis that referenced this pull request Mar 3, 2026
@Takhoffman @dawi369
windows: unify non-core spawn handling across acp qmd and docker (ope…
231e364 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
OWALabuy pushed a commit to kcinzgg/openclaw that referenced this pull request Mar 4, 2026
@Takhoffman @OWALabuy
windows: unify non-core spawn handling across acp qmd and docker (ope…
790ca2c 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
zooqueen pushed a commit to hanzoai/bot that referenced this pull request Mar 6, 2026
@Takhoffman
windows: unify non-core spawn handling across acp qmd and docker (ope…
bcb0d52 
…nclaw#31750) thanks @Takhoffman

Verified:
- pnpm install --frozen-lockfile
- pnpm build
- pnpm check (fails on pre-existing unrelated src/slack/monitor/events/messages.ts typing errors)
- pnpm vitest run src/acp/client.test.ts src/memory/qmd-manager.test.ts src/agents/sandbox/docker.execDockerRaw.enoent.test.ts src/agents/sandbox/docker.windows.test.ts extensions/acpx/src/runtime-internals/process.test.ts

Co-authored-by: Takhoffman <781889+Takhoffman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows spawn follow-up: unify non-core call sites behind canonical strategy

1 participant

@Takhoffman