Escape --manifest-path arg in build --print-commands-only tests.

[fnando]

What

Escape the --manifest-path=<path> arg in the expected output of build::build_all, build::build_default_members, build::build_all_when_in_non_package_directory, and build::build_package_by_name so the assertions match what production emits on Windows.

Adds a manifest_path_arg helper in cmd/crates/soroban-test/tests/it/build.rs that wraps format!("--manifest-path={path}") with shell_escape::escape, the same way serialize_command (cmd/soroban-cli/src/commands/contract/build.rs:594) escapes each cargo arg.

Why

The Windows CI matrix has been red since #2502 (Shell-escape build command args in --print-commands-only output). On Windows PathBuf::join produces backslashes, and shell_escape::escape treats \ as a shell-special character and wraps the whole token in single quotes, so the CLI emits '--manifest-path=contracts\add\Cargo.toml'. The tests' expected strings interpolate the manifest path raw, so they never matched the actual output.

Fix is in the tests rather than production: both '--key=value' and --key='value' parse to the same arg token in a shell, so the current production output is functionally correct, just visually unusual on Windows. Mirroring it in the tests is the smaller, lower-risk change.

Verification

To run the Windows matrix on this PR, the if: gate on build-and-test-windows was temporarily commented out, then restored. Both Windows jobs passed with the fix:

• msrv: https://github.com/stellar/stellar-cli/actions/runs/26538566467/job/78173877101 (23m33s)
• latest: https://github.com/stellar/stellar-cli/actions/runs/26538566467/job/78173877224 (25m45s)

Known limitations

N/A

[claude]

Claude finished @fnando's task in 1m 30s —— View job

---

Claude PR Review

• Read PR context
• Read changed files (build.rs, rust.yml)
• Cross-reference production behavior (serialize_command at cmd/soroban-cli/src/commands/contract/build.rs:594)
• Check for other tests that interpolate manifest paths raw
• Post review feedback

Summary

The fix is correct, minimal, and well-justified. Wrapping --manifest-path=<path> with shell_escape::escape in the test expectations exactly mirrors how production builds each arg in serialize_command (cmd/soroban-cli/src/commands/contract/build.rs:604-608), so the test now models reality across platforms. The reasoning in the PR description (both '--key=value' and --key='value' parse to the same arg token in a POSIX shell, so production output is functionally correct just visually unusual) is right.

Findings

🔴 Blocker — must restore before merge (already called out)

• .github/workflows/rust.yml:97-98 — the if: gate on build-and-test-windows is commented out. The PR description flags this, but it bears repeating: this must be re-enabled before merging, otherwise the Windows job will run on every PR and every push to non-release branches. Suggest reverting that hunk once a green run is captured and then keeping cmd/crates/soroban-test/tests/it/build.rs as the only change in this PR.

  Note: I can't push edits to .github/workflows/** myself (GitHub App permissions), so this revert needs to be done by the author.

🟡 Consistency nit (non-blocking)

• cmd/crates/soroban-test/tests/it/build.rs:68 and :86 — build_package_by_current_dir and build_with_locked still interpolate --manifest-path=Cargo.toml raw. They pass today because Cargo.toml contains no shell-special characters, so shell_escape::escape leaves it untouched. But applying manifest_path_arg("Cargo.toml") here too would (a) keep the test surface uniform, and (b) prevent silent breakage if someone later changes the fixture layout so this path acquires a separator or space. Worth a follow-up.

🟢 Minor — code style

• cmd/crates/soroban-test/tests/it/build.rs:610-613 — the helper is fine, but escape accepts Cow<str>, so you can drop the explicit construction:

  fn manifest_path_arg(path: &str) -> String {
      escape(format!("--manifest-path={path}").into()).into_owned()
  }

  Pure taste. Either is correct.

🟢 Observation (not in scope for this PR)

• shell_escape::escape is the POSIX/unix variant — on Windows it still wraps \-containing tokens in single quotes, which cmd.exe doesn't treat as quoting. --print-commands-only output therefore isn't directly pipeable into a Windows shell. That's a property of the existing production code (Shell-escape build command args in --print-commands-only output. #2502), not a regression from this PR, and matching it in the tests (as you do) is the right call.

Verdict

LGTM on the test fix. Block the merge until the rust.yml if: line is restored. The author has my "approve once workflow is reverted" — but as an automated reviewer I can't formally approve, only flag.
· branch: windows-build-print-commands-tests

[Copilot]

Pull request overview

This PR updates build integration test expectations so --print-commands-only output matches the shell-escaped manifest path formatting emitted by production code, especially on Windows.

Changes:

• Adds a manifest_path_arg helper that shell-escapes --manifest-path=<path>.
• Updates affected build tests to use the escaped manifest path argument.
• Temporarily disables the Windows CI job gate to run Windows validation on this PR.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
cmd/crates/soroban-test/tests/it/build.rs	Updates expected cargo command strings to match escaped manifest path output.
.github/workflows/rust.yml	Temporarily ungates the Windows build-and-test job.

[claude]

Claude finished @fnando's task in 1m 28s —— View job

---

Claude PR Review (re-review after synchronize)

• Re-read updated changed files (build.rs, rust.yml)
• Verify Windows CI gate has been restored
• Cross-reference production serialize_command behavior
• Check for remaining raw manifest path interpolations
• Post review feedback

Summary

The previous review's blocker has been resolved: commit a6d756e restores the if: gate on build-and-test-windows in .github/workflows/rust.yml:97, matching the macOS job's gating. The PR diff against origin/main is now scoped to just cmd/crates/soroban-test/tests/it/build.rs — exactly the surface the test fix needed to touch.

I re-verified the fix against production: serialize_command (cmd/soroban-cli/src/commands/contract/build.rs:604-608) escapes each cargo arg through shell_escape::escape. The new manifest_path_arg helper (cmd/crates/soroban-test/tests/it/build.rs:610-613) mirrors that exactly. On Windows, where PathBuf::join emits \, the helper will wrap the token in single quotes the same way production does; on Unix the manifest path contains no shell-special characters and the helper is a no-op. Both branches are correct.

Findings

🟢 Blocker resolved

The if: line on build-and-test-windows (.github/workflows/rust.yml:97) is back in place. Nothing further required here.

🟡 Consistency nit (non-blocking, unchanged from prior review)

cmd/crates/soroban-test/tests/it/build.rs:68 and :86 — build_package_by_current_dir and build_with_locked still inline --manifest-path=Cargo.toml raw. They pass today because Cargo.toml has no shell-special characters, so shell_escape::escape would leave it unchanged. Routing them through manifest_path_arg("Cargo.toml") would make the test surface uniform and guard against a future fixture-layout change introducing a separator. Worth a follow-up — not a blocker for this PR.

Fix this →

🟢 Style nit (taste — fine as-is)

cmd/crates/soroban-test/tests/it/build.rs:610-613 — escape accepts Cow<str>, so the explicit Cow::Owned wrap could be elided via .into(). Pure style — current form is clear.

fn manifest_path_arg(path: &str) -> String {
    escape(format!("--manifest-path={path}").into()).into_owned()
}

🟢 Observation (out of scope — same as last review)

shell_escape::escape is the POSIX/unix variant. On Windows it wraps backslash-containing tokens in single quotes, which cmd.exe does not treat as quoting. So --print-commands-only output is not directly pipeable into a Windows shell. That's a property of production (#2502), not introduced here, and matching it in tests is correct.

Verdict

LGTM. The blocker is gone, the diff is now minimal and focused, and the test now models real production output across platforms. The two remaining notes are non-blocking nits.
· branch: windows-build-print-commands-tests