Close the remaining Windows gaps in the agent host E2E suite - #327761
Merged
Conversation
Base automatically changed from
roblou/agents/e2e-cross-platform-captures
to
main
July 28, 2026 04:46
roblourens
changed the title
Snapshot and capture normalization ended with an unanchored
`replaceAll(userName, '${user}')`. With the GitHub Actions Linux account
name `runner` — an ordinary English word — captured text such as
`the runner completed` was rewritten to `the ${user} completed`.
That is worse than cosmetic. It only misfires on platforms whose account
name happens to be a common word, so the same recording normalizes
differently on macOS and Linux CI, which is exactly the cross-platform
mismatch this normalization exists to prevent.
Replace only the two positions where the name genuinely identifies a
user: after a path separator (including the escaped form found in
embedded JSON), and the owner/group columns of an `ls -l` listing. The
listing case is load-bearing — the committed subagent capture records the
account name there, outside any path — so it cannot simply be dropped.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test was disabled on Windows because its temporary git repository could not be deleted, which failed suite teardown even though every assertion passed. Its assertions were always platform-independent. Two independent causes, both now addressed: - Read-only `.git/objects` files. Fixed by clearing read-only attributes before retrying removal, which also unblocked `inspects git status`. - Background `git gc` holding handles under `.git` after the test finishes. `initTestGitRepo` now sets `gc.auto 0`; these repositories never create enough objects to need it. Collapse the duplicated init/identity setup in three suites into `initTestGitRepo` so the gc setting cannot be forgotten by the next test that needs a repository. Whether this holds can only be confirmed on a Windows run; the remaining risk is a handle neither mechanism covers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The comment claiming this test could not be made portable was wrong on two of its three counts: - The path assertions use `URI.fsPath`, which is platform-native, and the expected value is derived at runtime from the host's own `sessionAdded` notification rather than hardcoded as a POSIX path. - The recorded `pwd` is only reached when the provider routes commands through the host terminal tool, which on Windows is PowerShell, where `pwd` is an alias for `Get-Location`. The third — that pinning a command stops the turn on an unanswered permission prompt — was observed in a different test. This one already handles confirmation, dispatching `ChatToolCallConfirmed` when `toolCallReady` arrives unconfirmed. Drop `pwd` from the record-time command blocklist for the same reason, which empties `POSIX_COMMAND_EXCEPTIONS`; a recording now passes the check without an opt-out. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…mmand Tried replacing `pwd` with `node -e "console.log(process.cwd())"` on both of the test's turns. Both stalled on `session/inputNeededSet`: providers auto-approve `pwd` as a safe read-only command, and an arbitrary `node -e` invocation is not on that list, so pinning turns a silent tool call into one that waits for a confirmation this test's flow does not answer — even on the turn that does dispatch `ChatToolCallConfirmed`. `pwd` is portable regardless, since PowerShell defines it as an alias for `Get-Location`, so the test still runs on Windows. Correct the stale comment, which still claimed the assertions were POSIX-shaped, and record the general point: pinning a command changes its permission posture, so prefer steering to a file tool where one exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Answers why pinning `node -e` failed here when it works in every other ported test: those all go through `driveTurnToCompletion`, which confirms each unconfirmed `chat/toolCallReady` as it arrives. This test drives its turn by hand, and its host-terminal branch approved exactly once while its SDK-shell branch already used `startBackgroundApprovalLoop`. A pinned command is not on the provider's auto-approve list the way `pwd` is, so it adds an approval round-trip. With single-shot approval a later request stays pending and the turn stalls on `session/inputNeededSet`, which reads as a hang rather than a permission problem. Both branches now use the shared loop, and the command is pinned like everywhere else. Verified by re-recording both providers. This also removes the last special case: no test now depends on a command being auto-approved, and `POSIX_COMMAND_EXCEPTIONS` stays empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
roblourens
force-pushed
the
roblou/agents/e2e-windows-followups
branch
from
July 28, 2026 04:58
c2bed94 to
4b09421
Compare
roblourens
marked this pull request as ready for review
July 28, 2026 05:10
roblourens
enabled auto-merge (squash)
July 28, 2026 05:10
roblourens
marked this pull request as draft
July 28, 2026 05:14
auto-merge was automatically disabled
July 28, 2026 05:14
Pull request was converted to draft
Windows CI failed it for two reasons, neither about command portability, and both specific to this test's output assertions: - The expected path comes from `os.tmpdir()`, which on Windows CI returns an 8.3 short form (`C:\Users\CLOUDT~1\...`) while the shell reports the long form, so the `includes()` assertion can never match and the wait times out. Same class of mismatch as `/var` versus `/private/var` on macOS, which snapshot normalization already special-cases. - The Copilot branch waits for a `chat/toolCallContentChanged` carrying a terminal resource, and on Windows that notification never arrives even though the tool call starts, is confirmed, and completes. Keep the pinned command and the shared approval loop from the previous commit — both are correct regardless of platform — and record what CI showed so the next attempt starts from evidence rather than a fresh guess. Reworking this needs `realpathSync.native` on both sides of the path comparison, and the missing terminal content understood first; neither is reproducible without a Windows machine. `session configuration resolves and completes git branches` passed on Windows in the same run, so the git lock fixes hold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Closes remaining Windows portability gaps in the agent host E2E suite by tightening fixture normalization (avoid over-scrubbing usernames), stabilizing git-backed tests on Windows, and making shell-driven turns handle tool approval correctly.
Changes:
- Introduces a targeted
scrubUserNamenormalizer + unit tests; integrates it into snapshot and capture normalization. - Adds
initTestGitRepoto centralize git test repo setup (identity +gc.auto 0) and uses it across suites. - Updates worktree E2E flow to pin a portable “print cwd” command and approve tool calls in a loop; updates recorded captures and docs.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/node/userNameScrub.test.ts | Adds unit coverage for username scrubbing behavior and regex escaping. |
| src/vs/platform/agentHost/test/node/posixCommandLint.test.ts | Updates lint expectations to treat pwd as allowed (PowerShell alias). |
| src/vs/platform/agentHost/test/node/e2e/suites/workspaceSuite.ts | Pins portable cwd command, uses approval loop, and refactors git init to shared helper. |
| src/vs/platform/agentHost/test/node/e2e/suites/hostFeaturesSuite.ts | Uses shared git init helper and enables the git-branch completion test on Windows. |
| src/vs/platform/agentHost/test/node/e2e/suites/fileOperationsSuite.ts | Uses shared git init helper for Windows-stable temp repo cleanup. |
| src/vs/platform/agentHost/test/node/e2e/harness/userNameScrub.ts | Implements targeted username scrubbing (path segments + ls -l owner/group columns). |
| src/vs/platform/agentHost/test/node/e2e/harness/posixCommandLint.ts | Removes pwd from POSIX-only patterns and documents rationale. |
| src/vs/platform/agentHost/test/node/e2e/harness/capiReplayProxy.ts | Switches capture normalization from naive replaceAll to scrubUserName. |
| src/vs/platform/agentHost/test/node/e2e/harness/ahpSnapshot.ts | Switches snapshot normalization from naive replaceAll to scrubUserName. |
| src/vs/platform/agentHost/test/node/e2e/harness/agentHostE2ETestHarness.ts | Adds initTestGitRepo helper and removes the POSIX-command exception list. |
| src/vs/platform/agentHost/test/node/e2e/captures/copilotcli-worktree-session-uses-the-resolved-worktree-as-working-directory.yaml | Re-records worktree capture using pinned Node cwd command instead of pwd. |
| src/vs/platform/agentHost/test/node/e2e/captures/claude-worktree-session-uses-the-resolved-worktree-as-working-directory.yaml | Re-records worktree capture using pinned Node cwd command instead of pwd. |
| src/vs/platform/agentHost/test/node/e2e/KNOWN_ISSUES.md | Updates Windows-scoped status and documents approval-loop + git-temp-repo lessons. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Low
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
roblourens
marked this pull request as ready for review
July 28, 2026 20:55
roblourens
enabled auto-merge (squash)
July 28, 2026 20:55
joshspicer
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. Stacked on #327642 — base that PR's branch, not
main, so the diff shows only the commits here. Retarget tomainonce #327642 lands.Follow-ups from the cross-platform capture work. Two commits close the last Windows gaps that were portability problems; two fix bugs found while verifying them.
Scrub the account name only where it identifies a user
Snapshot and capture normalization ended with an unanchored
replaceAll(userName, '${user}'). With the GitHub Actions Linux account namerunner— an ordinary English word — captured text such asthe runner completedwas rewritten tothe ${user} completed.That is worse than cosmetic. It only misfires where the account name happens to be a common word, so the same recording normalizes differently on macOS and Linux CI — exactly the cross-platform mismatch this normalization exists to prevent.
The obvious fix (restrict to path contexts) is wrong. The committed subagent capture records the account name in
ls -lowner columns, outside any path:Dropping that would leak developer identity into committed fixtures. The scrub now handles exactly two positions — after a path separator (including the escaped
\\form found in embedded JSON), and the owner/group columns of a listing — and leaves prose alone. Shared by both normalizers, which had duplicated the same bug.Enable
session configuration resolves and completes git brancheson WindowsDisabled because its temporary git repository could not be deleted, failing suite teardown even though every assertion passed. The assertions were always platform-independent.
Two independent causes:
.git/objectsfiles. A read-only file cannot be deleted on Windows, andrmSync'sforceonly suppressesENOENT— it does not override the attribute, so the retry loop burned its full timeout on a condition waiting can never fix. Fixed in Make the deterministic shell command E2E test platform-neutral #327642, which also unblockedinspects git status.git gcholding handles under.git. New here:initTestGitReposetsgc.auto 0. These repositories never create enough objects to need it.The init/identity setup duplicated across three suites is collapsed into
initTestGitRepo, so the next test needing a repository cannot forget the gc setting.Enable
worktree session uses the resolved worktree as working directoryon WindowsTwo claims in the old comment were wrong: the path assertions use
URI.fsPath(platform-native, compared against a value derived at runtime from the host's ownsessionAddednotification, not a hardcoded POSIX path), and the shell turn is only reached when the provider routes commands through the host terminal tool.Approve tool calls in a loop, not once
Pinning
node -e "…"in the worktree test initially stalled the turn, which looked like the pinned command being unsupported. It was not.A pinned command is not on the provider's auto-approve list the way
pwdis, so it adds an approval round-trip. Every other ported test handles this transparently becausedriveTurnToCompletionconfirms each unconfirmedchat/toolCallReadyas it arrives. This test drives its turn by hand, and its two branches disagreed: the SDK-shell branch already usedstartBackgroundApprovalLoop, while the host-terminal branch approved exactly once.A turn can raise more than one approval, so the second stayed pending and the turn stalled on
session/inputNeededSet— presenting as a hang rather than a permission problem.Both branches now use the shared loop, and the command is pinned like everywhere else. Verified by re-recording both providers. This removes the last special case: no test depends on a command being auto-approved, and
POSIX_COMMAND_EXCEPTIONSstays empty.Result
The Windows-disabled list is down to one row —
a bang command runs locally and exposes terminal output— which is a turn-completion problem, not a portability one.Verification
forerunner,runneradmin,runner.js, and regex escapingtypecheck-client,valid-layers-check, hygiene: passWhat CI has to confirm: both newly enabled tests pass on Windows. The assertions are platform-independent by inspection, but the failure modes here were teardown and approval behavior — neither reproducible on macOS. If either fails, that is a real finding rather than a flake.