Make the deterministic shell command E2E test platform-neutral - #327642
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
src/vs/platform/agentHost/test/node/e2e/suites/fileOperationsSuite.ts:183
- The updated prompt now requires replying with the exact value only, but this assertion still passes if extra text is included (e.g. explanations or formatting). To ensure the test is actually enforcing the intended behavior (and to reduce provider variance), tighten the assertion to match the full response (after any standard trimming performed by the harness).
assert.match(result.responseText, /SHELL_VALUE_73/);
src/vs/platform/agentHost/test/node/e2e/harness/ahpSnapshot.ts:472
- This mapping duplicates the shell-tool placeholder logic introduced in
capiReplayProxy.ts(SHELL_TOOL_PREFIXES/SHELL_TOOL_PLACEHOLDERS). Keeping two independent maps increases the risk of divergence (e.g. adding a new prefix/tool variant in one place but not the other). Consider centralizing the mapping in a shared helper (or exporting it from one module) so both AHP snapshot normalization and CAPI fixture normalization stay consistent.
function normalizeShellToolName(toolName: string): string {
const shellToolPlaceholders: Record<string, string> = {
bash: '${shell}', powershell: '${shell}',
read_bash: '${read_shell}', read_powershell: '${read_shell}',
write_bash: '${write_shell}', write_powershell: '${write_shell}',
stop_bash: '${stop_shell}', stop_powershell: '${stop_shell}',
bash_shutdown: '${shell_shutdown}', powershell_shutdown: '${shell_shutdown}',
list_bash: '${list_shell}', list_powershell: '${list_shell}',
};
return shellToolPlaceholders[toolName] ?? toolName;
}
src/vs/platform/agentHost/test/node/e2e/harness/ahpSnapshot.ts:446
- The new projection behavior is central to making previously-recorded snapshots replayable (dropping empty reasoning parts). Adding a focused unit test for
dropEmptyReasoningParts(or for snapshot serialization with an empty reasoning part present) would protect against regressions and ensure the intended unreplayable-case is covered.
function dropEmptyReasoningParts(actions: object[]): object[] {
return actions.filter(entry => {
const action = (entry as { action?: { type?: string; part?: { kind?: string; content?: string } } }).action;
return !(action?.type === ActionType.ChatResponsePart
&& action.part?.kind === ResponsePartKind.Reasoning
&& action.part.content === '');
});
}
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Low
roblourens
force-pushed
the
roblou/agents/e2e-cross-platform-captures
branch
from
July 27, 2026 22:02
1c340cd to
abc3ad8
Compare
Snapshot normalization rewrote working directories, home directories, user names, shell ids, and `ls -l` listing columns, but did nothing about line endings. A snapshot carrying literal tool output recorded on macOS/Linux would therefore mismatch on Windows purely because the text arrived as CRLF — a failure that looks like a product bug but isn't. Collapse CRLF before the path and line-anchored passes so everything downstream sees LF-only text. The escaped form is handled too, since tool inputs are frequently embedded JSON where the carriage return survives as a literal `\r` escape. Replace the now-fixed known issue with one found while verifying this: the trailing user-name scrub is an unanchored substring replacement, so on GitHub Actions Linux (`runner`) it rewrites the ordinary English word as well as the account name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prototype for porting the Windows-disabled shell tests. Pinning the
command in the prompt is necessary but not sufficient — three separate
things coupled this test to POSIX, only one of which was the command:
- The prompt described the command instead of specifying it, so the model
chose per provider and whatever it chose was frozen into the fixture
(Copilot picked `echo`, Claude picked `printf`). Pin `echo`, which
behaves the same under cmd/PowerShell and POSIX shells.
- The AHP snapshot recorded the Copilot shell tool's platform-specific
name (`bash` vs `powershell`). Normalize it to `${shell}`.
- The CAPI fixture recorded the same name in the tool_use block that
drives replay, so a macOS recording would tell a Windows agent to call
a tool that does not exist. Store the placeholder and expand it to the
running platform's name on replay.
Also drop reasoning parts that never receive content: a provider can open
and close one without emitting a delta, and replay rebuilds the stream
from the fixture's aggregated content where that leaves no trace. Any
snapshot recorded during such a turn was permanently unreplayable.
Split `shellToolReplayEnabled` so the blanket Windows exclusion is
separate from the provider's Linux shell-tool stability, and move this
test to the portable variant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The capture/replay round-trip for platform-specific shell tool names read `process.platform` at module load, so the Windows branch could not be exercised from a POSIX host — which is exactly the branch this work exists to protect. Take the platform as an optional parameter defaulting to the running one, and assert both directions for both platforms. Record what porting the prototype test actually required: pinning the command is per-test work, but the tool-name normalization on both the snapshot and capture sides, and dropping empty reasoning parts, are central fixes that should not need revisiting per test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Thirteen tests were disabled on Windows because their committed capture
contained a POSIX-only command. Two techniques apply, and which one is
needed depends on whether a file tool exists for the operation:
- Steer ("use your file tools; do not run a shell command") for reads,
edits, missing-file handling, and content creation. Those captures now
contain no shell command at all, which is the strongest outcome since
nothing is left to be platform-specific.
- Pin ("run exactly this command") for rename, delete, directory
creation, and listing. No file tool exists for these, so every provider
reaches for the shell; steering harder made one provider skip the
operation entirely rather than pick another tool. Pinned commands use
`node -e`, which is guaranteed present and quotes identically under cmd
and POSIX shells.
Two findings while re-recording:
- Reasoning traffic can never survive the capture round-trip, because
`capiWireCodec` drops reasoning items when aggregating a response. The
earlier filter only dropped empty parts; a partial one carrying a few
characters was equally unreplayable. Drop reasoning from snapshots.
- `counts lines in a file` seeded its fixture with a trailing newline,
making "how many lines" genuinely ambiguous once the agent read the
file instead of running `wc -l`. Remove the ambiguity from the input.
`worktree session uses the resolved worktree as working directory` stays
Windows-scoped: `pwd` is auto-approved as safe while a pinned `node -e`
is not, so the turn stops on a permission prompt the test never answers.
Its non-shell half still asserts worktree resolution on Windows.
With no test left on the Windows-excluded flag, remove it and keep only
the Linux shell-tool stability gate.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Porting the Windows-disabled tests fixed the captures that existed, but nothing stops the next re-record from reintroducing the problem. Nobody picks these command strings deliberately — the model produces them from a prompt that describes the goal instead of specifying the command — so a prompt that drifts will quietly produce a capture that cannot replay on Windows, and the failure appears on a CI leg the author may not run. Check the assistant's `tool_use` commands before writing the fixture and fail the recording with the two remedies spelled out. Only the response blocks are checked: those are what replay feeds back to the agent, so they are the commands that actually execute. The check throws before the write, so a rejected recording leaves the previous capture intact. The patterns are a blocklist of constructs known to fail under `cmd` rather than an allowlist of portable ones, and are anchored to command position so a coreutil name used as an argument does not trip them. A false positive would block a correct recording and push authors toward disabling the check, which is worse than missing a case. Verified by regressing a ported prompt back to `rm`: the recording fails with the expected message and the committed capture is left untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two fixes from CI feedback on the Windows leg. Clearing read-only attributes before retrying temp-directory removal. `inspects git status` now runs on Windows, and git marks the files under `.git/objects` read-only. A read-only file cannot be deleted on Windows, and `rmSync`'s `force` option only suppresses ENOENT — it does not override the attribute. The suite teardown therefore burned its full 30-second timeout and failed with an AggregateError even though every test had passed. Waiting cannot help a read-only file, so clear the attributes before the retry instead of spinning until the deadline. Reproduced locally with a read-only directory tree, which fails the same way on POSIX; verified the tree is removed after the fix. Narrowing the replay block rewrite to `tool_use`. Expanding the shell tool name was written as "not a text block" rather than "is a tool_use block". The current union has only those two members so this is not reachable today, but a future block kind would be rewritten as if it were a tool call. Match on `tool_use` explicitly and pass anything else through untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
roblourens
force-pushed
the
roblou/agents/e2e-cross-platform-captures
branch
from
July 28, 2026 01:11
abc3ad8 to
856ce8e
Compare
roblourens
marked this pull request as ready for review
July 28, 2026 03:15
roblourens
enabled auto-merge (squash)
July 28, 2026 03:15
DonJayamanne
approved these changes
Jul 28, 2026
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
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.
Draft. Follow-up to #327489, taking the first concrete step toward running the Agent Host E2E shell tests on Windows.
The main purpose of this PR is to get a Windows CI result. The changes are believed to make one test platform-neutral, but that is an inference from reading the replay code — it has not been proven on a real Windows run. See "What to look for in CI" below.
Background
16 tests are disabled on Windows. Almost all of them trace back to a POSIX-only command string frozen into a checked-in fixture — and no test author chose those strings. The prompt described what to do, the model picked how, and whatever it picked became the fixture.
The clearest case is the test ported here.
runs a deterministic shell commandwas skipped on Windows for all three providers, but Copilot's recorded command was alreadyecho SHELL_VALUE_73, which runs fine on Windows. It was blocked only because Claude happened to pickprintf '%s\n' "SHELL_VALUE_73".What changed
Porting one test surfaced four separate couplings, only one of which was the command:
printftoecho— confirming the model's choice was the whole problem. (per-test work)bashon POSIX,powershellon Windows) and that name reaches the client verbatim inchat/toolCallStart. Snapshots now record${shell}. (central)tool_useblock that drives replay, so a macOS recording would instruct a Windows agent to call a tool that does not exist. Captures now store a placeholder andCapiReplayProxyexpands it to the running platform's name on replay. (central)Also included:
normalizeSnapshotText. Snapshot normalization handled paths, user names, shell ids andls -lcolumns but not line endings, so a snapshot carrying literal tool output would fail on Windows for a reason unrelated to the behavior under test.shellToolReplayEnabledsplit into two flags. It conflated "the provider's shell-tool replay is unstable on Linux" with "this capture contains a POSIX-only command"; only the second is about Windows, and only for captures that haven't been ported. Ported tests useportableShellToolReplayEnabled.Tests
capiReplayProxyShellTools.test.tscovers the capture/replay round-trip for shell tool names. The platform is an injectable parameter rather than a module-load read ofprocess.platform, so both platform branches are asserted from either host — otherwise the Windows half would be dead code on a developer machine, which is exactly the branch this work exists to protect.What to look for in CI
On the Windows leg,
runs a deterministic shell commandcurrently reports as skipped for all three providers:Success is Claude and Copilot flipping to
✔. Codex should stay skipped — it is gated onstableNewScenarioResponse: false, which is unrelated to platform.If it fails, that is still a useful result: it means something beyond these four couplings is platform-specific, and it is much cheaper to learn that on one test than after porting all 15.
Verification so far
capiReplayProxyShellTools.test.ts: 3 passingtypecheck-client,valid-layers-check, hygiene: passKNOWN_ISSUES.mdFollow-ups (not in this PR)
KNOWN_ISSUES.md