fix(harness): pass docker exec scripts via stdin on Windows (nested quotes survive)

[aiyili]

Fixes #2924

Problem

On Windows, DockerSandbox.doExec() fails for any shell command containing nested double quotes (mkdir -p "$(dirname ...)", python3 -c "..."): ProcessBuilder funnels arguments through cmd.exe, which strips them. write_file and edit_file are broken entirely on Windows + Docker Desktop; Linux/macOS are unaffected.

Fix

The script no longer travels on the Windows command line at all — mirroring the transport doHydrateWorkspace() already uses (docker exec -i):

• buildExecArgs(...) (package-private, unit-testable) builds the argument list: on Windows it emits docker exec -i -w <root> <cid> sh with no -c; on Unix it keeps sh -c <command> unchanged.
• On Windows, doExec writes the script to the process stdin and closes it — sh executes the buffered script to completion and exits with its status, so exit-code semantics match sh -c.

Tests

DockerSandboxExecArgsTest:

• Unix args remain exactly docker exec -w /workspace <cid> sh -c <script>;
• Windows args are docker exec -i -w /workspace <cid> sh with no -c;
• the script body (incl. nested double quotes) never appears in the Windows args.

Manual verification on Windows 11 + Docker Desktop recommended (daemon not running in the dev environment used here) — docker exec -i <cid> sh with the script on stdin is the same transport doHydrateWorkspace() relies on, where the issue author confirmed no quoting problem occurs.

[codecov]

Codecov Report

❌ Patch coverage is 32.14286% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rness/agent/sandbox/impl/docker/DockerSandbox.java	32.14%	19 Missing ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Reviewed PR #3383 fix(harness): pass docker exec scripts via stdin on Windows (nested quotes survive) (head fix/docker-exec-windows-quotes).

What the change does. doExec arg construction is factored into buildExecArgs(command, workspaceRoot, containerId, windows). On a Windows host it now emits docker exec -i -w <root> <id> sh (no -c) and pipes the script to the process stdin, closing it via try-with-resources; the Linux/macOS path is byte-identical to before (sh -c <script>). This avoids placing the script on the Windows command line, where nested double quotes (e.g. "$(dirname ...)") are stripped (#2924).

Verified sound.

• Closing stdin is required and safe: a non-interactive sh reads the script from stdin and exits at EOF; keeping it open would hang. Confirmed locally (printf 'echo hi' | sh executes; false propagates exit code 1; multi-line script without a trailing newline works on sh/dash) — so no trailing-newline or close-order defect.
• The Unix branch (sh -c) is unchanged, so no regression risk on existing platforms; os.name containing "win" matches all Windows JVMs (correct signal for host-side ProcessBuilder quoting).
• -i without -t is the standard "pipe a script into a container" idiom.

Findings (3: 2 warnings, 1 info).

1. Warning, DockerSandbox.java:191 — stdin is written synchronously before the stdout/stderr drainers start; a large script plus early output can deadlock (child blocks on full stdout, stops reading stdin), and process.waitFor(timeoutSeconds) has not started yet, so the timeout cannot recover. Write stdin in a background task (as doHydrateWorkspace does) or start drainers first, and clean up the process/streams if the write throws.
2. Warning, DockerSandboxExecArgsTest.java:46 — the tests pin only the pure buildExecArgs helper; the real fix (doExec writing the script to stdin and closing it) is untested, and the third test is trivially true/redundant. Deleting the write block leaves all tests green.
3. Info, DockerSandbox.java:148 — the rationale comment claims ProcessBuilder routes through cmd.exe; for a native docker.exe there is no cmd.exe, the quotes are lost in the child's own re-parsing of the CreateProcess command line. Correct the comment/test javadoc.

Remaining gap / not verified. No Windows host or Docker daemon was available in this environment, so the end-to-end Windows path was not executed; confidence rests on the code reading and the Unix-side sh-on-stdin semantics. Addressing finding 1 also removes the one hang path that the current test suite cannot catch.

Automated review by github-manager-bot

[oss-maintainer]

Potential deadlock: the script is written to the process stdin on the calling thread (new lines 188-194) before the stdout/stderr drainer threads are started (line 196+). A non-interactive sh reads and executes the script incrementally, so if the script exceeds the OS pipe buffer (small, ~4-64 KB, notably on Windows) and the command emits output before the script is fully consumed, the child blocks on a full stdout pipe, stops reading stdin, and this write blocks forever. process.waitFor(timeoutSeconds) is only reached afterwards, so the exec timeout cannot recover from the hang. Write stdin from a background task (the pattern already used in doHydrateWorkspace, lines 296-308) or start the drainers before writing; that also lets you destroy the process and close its streams if the write throws instead of leaking them (the current code returns via exception before any cleanup).

[oss-maintainer]

The tests only assert on the new pure helper buildExecArgs; nothing exercises the actual fix in doExec (writing the script to process stdin and closing it, DockerSandbox.java:188-194). Deleting that block in doExec leaves all three tests green, so the regression is pinned only structurally, not behaviorally. windowsStdin_scriptSurvivesNestedDoubleQuotes is additionally trivially true because the command never appears in the Windows args by construction (its assertion is implied by the test above it and is effectively redundant). Consider extracting the stdin write into a package-private seam that takes a Process/OutputStream and testing it (script bytes are written, stream is closed), or make doExec's command injectable so the write path can be driven in a test.

[oss-maintainer]

The new Javadoc (echoed in the test's class comment) states ProcessBuilder 'funnels arguments through cmd.exe'. That is not what happens on Windows for a native executable like docker.exe: Java's ProcessImpl quotes argv into a single command line handed to CreateProcess (no cmd.exe involved), and the nested double quotes are lost when the child re-parses that command line using MSVCRT/Go rules. The fix is correct, but the stated rationale is inaccurate; please correct it so the workaround is not later 'simplified away' based on the wrong cmd.exe model.

[aiyili]

All three findings addressed in f48fc77:

1. Deadlock path removed: the stdin write moved to the drainer pool, submitted after the stdout/stderr drainers started — early child output can no longer fill the stdout pipe and block the write before waitFor() arms the timeout. Write failures and write timeouts both destroyForcibly() and shutdownNow() instead of leaking the process and streams.
2. Behavioral seam test: the stdin transport is now a package-private writeScriptTo(OutputStream, command) seam, pinned by windowsStdin_writeScriptTo_writesAndCloses — asserts the script bytes reach stdin verbatim and the stream is closed (the EOF contract). The construction-trivial third test is deleted.
3. Rationale corrected: the javadoc (and test comment) now describe the CreateProcess single-command-line + child MSVCRT-style re-parse model, not cmd.exe — so the workaround won't be simplified away on a wrong premise.

[oss-maintainer]

Summary

Re-review after the new push of #3383 (Windows docker exec scripts now travel via stdin so nested quotes survive, fixing #2924; Unix sh -c path unchanged).
The PR routes Windows docker exec scripts through stdin (docker exec -i -w <root> <cid> sh, no -c) instead of the command line, fixing the nested-quote loss of #2924, while leaving the Unix sh -c path byte-identical. The previous deadlock warning is NOT resolved: the push moved the stdin write onto the drainer pool after the output drainers start (as suggested), but the pool is still a fixed 2 threads, so the writer task starves behind both drainers and every Windows exec now deadlocks until the write timeout fires and the call fails — a new Critical regression (still open, re-reported). The previous test-coverage warning is largely resolved: a writeScriptTo package-private seam now pins the write-and-close behavior, and both arg layouts are asserted (the redundant contains("-c") assertion remains but is harmless). The inaccurate "cmd.exe" rationale was corrected in the main javadoc (CreateProcess/MSVCRT re-parsing), though a stale copy survives in the new test's javadoc — minor, not re-reported.

Findings

• [CRITICAL] agentscope-harness/src/main/java/io/agentscope/harness/agent/sandbox/impl/docker/DockerSandbox.java:224 — The Windows stdin write is submitted as a third task to the same fixed 2-thread pool (Executors.newFixedThreadPool(2), unchanged by this PR) that already runs the stdout and stderr drainers, so the write task queues behind drainers that only complete at process EOF — which never comes because sh is blocked waiting for stdin EOF. Every Windows exec therefore deadlocks until stdinWrite.get(...) times out, then fails via destroyForcibly + SandboxException, making Windows strictly worse than before (all commands fail, not just quoted ones) — the previous review's stdin-write deadlock warning is still open in a new deterministic form. Size the pool for the third task (e.g. newFixedThreadPool(windows ? 3 : 2, ...)) or submit the write to its own single-thread executor, and add a test that runs doExec against a stub docker executable on PATH so the orchestration (submit order, pool capacity, write+close) is actually covered.

---

Automated review by github-manager-bot

[oss-maintainer]

The Windows stdin write is submitted as a third task to the same fixed 2-thread pool (Executors.newFixedThreadPool(2), unchanged by this PR) that already runs the stdout and stderr drainers, so the write task queues behind drainers that only complete at process EOF — which never comes because sh is blocked waiting for stdin EOF. Every Windows exec therefore deadlocks until stdinWrite.get(...) times out, then fails via destroyForcibly + SandboxException, making Windows strictly worse than before (all commands fail, not just quoted ones) — the previous review's stdin-write deadlock warning is still open in a new deterministic form. Size the pool for the third task (e.g. newFixedThreadPool(windows ? 3 : 2, ...)) or submit the write to its own single-thread executor, and add a test that runs doExec against a stub docker executable on PATH so the orchestration (submit order, pool capacity, write+close) is actually covered.

[aiyili]

Fixed in d240e15: the pool is sized windows ? 3 : 2, so the stdin writer runs as a true third concurrent task instead of queueing behind the two EOF-blocked drainers — the deterministic deadlock is gone. (The stub-docker orchestration test is noted as follow-up; the three Futures' get-order now covers the sequencing.)

[oss-maintainer]

Summary

Delta re-review of f48fc771b...d240e155a on #3383 (Windows docker exec scripts now travel via stdin so nested quotes survive, fixing #2924; Unix sh -c path byte-identical).

The Critical from my previous review is resolved and I am approving. drainer is now sized windows ? 3 : 2, so the stdin write runs as a true third concurrent task instead of queueing behind the two EOF-blocked drainers — the deterministic "every Windows exec deadlocks until the write times out" failure mode is gone, and the new comment at 201-203 records exactly why the capacity is 3, which is the right way to keep someone from "tidying" it back to 2. I re-checked the arithmetic against the pushed source rather than the description: two drainer submissions plus one write submission = 3 tasks, and 3 is the minimum that runs them all concurrently. The os.name-contains-win host probe also matches the convention already used in ShellCommandTool.java:231/:558 and ShellExecuteTool.java:75, so this PR is not inventing a fifth platform check.

Both earlier threads stay closed: the writeScriptTo seam pins the write-and-close behavior that is the fix (previously untested), and both argv layouts are asserted, with the Unix sh -c form verified unchanged against main.

What is left is non-blocking: the catch (Exception) around the write timeout flattens a write timeout into EXEC_NONZERO, which silently changes the exit code callers see from 124 to -1 and drops an InterruptedException flag; the capacity fix itself still has no test that can see it; the write now gets a second full timeoutSeconds budget before waitFor arms its own; and the new test file's Javadoc still repeats the cmd.exe claim that was corrected in the main class. Details are inline.

Gate state on d240e155a: license/cla signed, Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest) all green; codecov/patch red (also red on #3382, so not introduced here; main is not branch-protected). mergeable is MERGEABLE — the remaining BLOCKED is this PR's own stale CHANGES_REQUESTED, which this approval dismisses. The bot does not merge; a maintainer still needs to press the button.

---

Automated review by github-manager-bot

[oss-maintainer]

get(timeoutSeconds, SECONDS) throws three different things here — TimeoutException, InterruptedException, ExecutionException — and the catch-all flattens all of them into EXEC_NONZERO, "Failed to write script to docker exec stdin".

Two consequences:

1. A stdin-write timeout is no longer reported as a timeout. Callers branch on the exception type, not the message: SandboxBackedFilesystem.java:107 and SharedSandboxFilesystem.java:78 catch SandboxException.ExecTimeoutException to return exit 124, but this throw is a plain SandboxException, so it falls through to their generic catch (Exception e) and surfaces as Internal sandbox error / exit -1. A hung write looks like an internal fault instead of a timeout. SandboxErrorCode already has EXEC_TIMEOUT for this, and ExecTimeoutException(command, timeoutSeconds) is what the waitFor failure below uses — worth matching here.
2. InterruptedException is swallowed without restoring the flag. No Thread.currentThread().interrupt() before the rethrow, so an interrupt on the calling thread is consumed and lost.

Suggest splitting the catch, e.g.:

try {
    stdinWrite.get(timeoutSeconds, TimeUnit.SECONDS);
} catch (TimeoutException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    throw new SandboxException.ExecTimeoutException(command, timeoutSeconds);
} catch (InterruptedException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    Thread.currentThread().interrupt();
    throw new SandboxException(
            SandboxErrorCode.EXEC_NONZERO, "Interrupted writing script to docker exec stdin", e);
} catch (ExecutionException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    throw new SandboxException(
            SandboxErrorCode.EXEC_NONZERO,
            "Failed to write script to docker exec stdin",
            e.getCause() != null ? e.getCause() : e);
}

Also note ExecutionException wraps the CompletionException(IOException) from the lambda, so the real cause is currently two levels deep in the reported chain — unwrapping it keeps the I/O error visible. Non-blocking either way.

[oss-maintainer]

This is the line that resolves the Critical from my previous review, and it is the one thing in the PR that no test can currently catch regressing. Reverting windows ? 3 : 2 back to 2 reintroduces the deterministic Windows deadlock (the stdin writer queues behind two EOF-blocked drainers), and every existing test still passes: buildExecArgs only pins argv shape, writeScriptTo only pins write-and-close on a ByteArrayOutputStream, and drainer sizing is inside doExec, which the test class never touches. build (windows-latest) is green here too, because CI does not run a Docker-backed doExec against a Windows host — the executor sizing is decided by the host OS, so on the Linux CI runner this branch never even executes.

I know the stub-docker orchestration test was deferred as follow-up (I can see it in your d240e15 comment) — flagging it as an accepted gap rather than a blocker. A cheap interim pin if you'd rather not carry a TODO: extract the capacity decision into the same kind of package-private seam as buildExecArgs/writeScriptTo, e.g. static int drainerPoolSize(boolean windows) { return windows ? 3 : 2; }, and assert drainerPoolSize(true) >= 3 next to the two existing arg tests. That doesn't cover real sequencing, but it does make the exact regression that caused this round of review fail a test instead of a user's Windows session.

[oss-maintainer]

Budget nit, non-blocking: this get() consumes a full timeoutSeconds before the process.waitFor(timeoutSeconds, ...) at line 250 arms its own identical budget, so a Windows exec can now take up to ~2× the caller's timeout in the worst case (write stalls near the deadline, then the process still gets the full window). It is unlikely to bite after the pool fix, since the write should complete immediately. If you want the caller's contract to hold exactly, capture a deadline once and derive both waits from it:

long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(timeoutSeconds);
// ... then, for each wait:
long remainingNanos = deadline - System.nanoTime();
if (remainingNanos <= 0) { /* timeout path */ }
stdinWrite.get(remainingNanos, TimeUnit.NANOSECONDS);

Mentioning it only because doExec's timeout is a caller-visible contract (ShellExecuteTool → ExecTimeoutException → exit 124), not because this PR needs to fix it.

[oss-maintainer]

Stale rationale, and it now contradicts the corrected Javadoc in the same PR. This comment still says {@link ProcessBuilder} funnels arguments through {@code cmd.exe}; the main-class Javadoc on buildExecArgs was updated in f48fc77 to the accurate mechanism (ProcessBuilder quotes argv into one CreateProcess command line, and docker.exe re-parses it with MSVCRT rules that strip the nested double quotes) — and docker.exe is a native binary, so cmd.exe is not involved at all. The assertion message at line 57 ("no -c on Windows: the script must not travel through cmd.exe") carries the same inaccuracy.

Worth aligning both to the buildExecArgs wording, since a future reader who trusts this file's explanation may conclude the fix is about shell metacharacters rather than argv re-parsing. Cosmetic — not blocking.