Assert the recorded model request on replay in the agent host E2E suite - #327955
Draft
roblourens wants to merge 12 commits into
Draft
Assert the recorded model request on replay in the agent host E2E suite#327955roblourens wants to merge 12 commits into
roblourens wants to merge 12 commits into
Conversation
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>
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Response selection stays ordinal, but every replayed turn now compares the live model request against the one recorded in the capture, through a projection applied symmetrically to both sides. Captures keep their existing shape and stay readable. The request body is the host's own product - prompt assembly, retained history, truncation, attachment marshalling, and tool-result hand-back - and none of it was checked before, so a regression replayed green and became the new expected value at the next re-record. Asserts host-authored structure (roles and ordering, retained history, system prompt presence, text and attachment content, tool names/inputs, tool_use_id wiring) and elides what replay cannot reproduce or what is environment-derived (tool_result payloads, run-time identifiers, reasoning blocks, the model id). Switching this on found seven stale captures: four whose prompt text had been edited without re-recording, one holding a one-off ordering of parallel tool_result blocks, and one Codex capture still carrying the pre-pinned pwd prompt - all re-recorded here. The seventh, Claude's side-chat capture, cannot be refreshed because recording it hits the known provider-context fork defect, so it is listed as an exception and documented in KNOWN_ISSUES. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ert-model-requests # Conflicts: # src/vs/platform/agentHost/test/node/e2e/harness/agentHostE2ETestHarness.ts # src/vs/platform/agentHost/test/node/e2e/harness/capiReplayProxy.ts
…ert-model-requests
Contributor
Screenshot ChangesBase: Changed (6)
|
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (panel)/Light
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (wide)/Dark
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (wide)/Light
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (narrow)/Dark
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (narrow)/Light
| Before | After |
|---|---|
 |
 |
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.
This PR makes agent host E2E replay fixtures assert the recorded model request (via a stable projection) in addition to selecting responses ordinally, closing a gap where prompt/request regressions could replay green.
Changes:
- Add a request projection + mismatch reporting and enforce it during replayed model turns in
CapiReplayProxy. - Extract shell tool name placeholder logic into a dedicated helper module and update related tests.
- Re-record/update several E2E capture fixtures and expand documentation for the new assertion behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts | Plumbs allowStaleRecordedRequest through server start options into the replay proxy. |
| src/vs/platform/agentHost/test/node/modelRequestProjection.test.ts | Adds unit tests for the request projection/matching/mismatch formatting behavior. |
| src/vs/platform/agentHost/test/node/e2e/harness/modelRequestProjection.ts | Implements projection logic to compare live vs recorded requests while eliding unstable fields. |
| src/vs/platform/agentHost/test/node/e2e/harness/shellToolNames.ts | Centralizes platform-neutral shell tool naming normalization/expansion. |
| src/vs/platform/agentHost/test/node/e2e/harness/capiReplayProxy.ts | Captures recorded requests per turn and asserts replayed requests against them; improves replay error reporting. |
| src/vs/platform/agentHost/test/node/e2e/harness/agentHostE2ETestHarness.ts | Adds per-capture stale-request exception handling and wires replay error lifecycle to the test lease. |
| src/vs/platform/agentHost/test/node/e2e/README.md | Documents model request assertion behavior and new replay mismatch error mode. |
| src/vs/platform/agentHost/test/node/e2e/KNOWN_ISSUES.md | Updates known-issues guidance and documents the stale-capture exception rationale. |
| src/vs/platform/agentHost/test/node/capiReplayProxyShellTools.test.ts | Updates imports to the new shellToolNames module. |
| src/vs/platform/agentHost/test/node/e2e/**/captures/*.yaml | Updates recorded fixtures to match current host request behavior and tool/result ordering. |
| src/vs/platform/agentHost/test/node/e2e/providers/snapshots/*.yaml | Updates provider traffic snapshot to reflect fixture/toolcall ordering changes. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Low
Comment on lines
+128
to
+131
| /** Compares two projections. */ | ||
| export function modelRequestsMatch(expected: IProjectedModelRequest, actual: IProjectedModelRequest): boolean { | ||
| return JSON.stringify(expected) === JSON.stringify(actual); | ||
| } |
Comment on lines
+120
to
+126
| /** Projects a readable model request down to its host-authored structure. */ | ||
| export function projectModelRequest(request: IReadableAnthropicRequest): IProjectedModelRequest { | ||
| return { | ||
| system: request.system, | ||
| messages: request.messages.map(message => ({ role: message.role, content: projectContent(message.content) })), | ||
| }; | ||
| } |
Comment on lines
+105
to
+117
| return content.filter(block => !isReasoningBlock(block)).map(block => { | ||
| const b = block as { type?: string; text?: string; name?: string; input?: unknown; tool_use_id?: string }; | ||
| switch (b.type) { | ||
| case 'text': | ||
| return { type: 'text', text: elideRuntimeIds(b.text ?? '') }; | ||
| case 'tool_use': | ||
| return { type: 'tool_use', name: normalizeShellToolNameForCapture(b.name ?? ''), input: projectValue(b.input) }; | ||
| case 'tool_result': | ||
| return { type: 'tool_result', tool_use_id: elideRuntimeIds(b.tool_use_id ?? ''), content: TOOL_RESULT_PLACEHOLDER }; | ||
| default: | ||
| return { type: b.type }; | ||
| } | ||
| }); |
Windows CI failed three model-request assertions. None was a host regression:
each was the same file addressed differently than the macOS-recorded capture.
Three separate spellings were involved - an unsubstituted \${workdir}, an
unsubstituted \${homedir}, and a backslash separator - which is the point: a
directory has too many per-machine spellings (separator, drive letter, 8.3
short names, /var vs /private/var, and whether the recorder substituted its
placeholder at all) to be compared literally across platforms.
The projection now collapses a path to \${path}/<basename>. Which file the
host referenced stays asserted; where it lives does not. A different filename
is still a mismatch.
Also fixes a real gap in the recorder: \`homeDir\` was normalized only in its
raw form, while \`workDir\` also had a JSON-escaped variant, so a Windows home
directory embedded in a JSON body was never substituted.
(Written by Copilot)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The remaining Windows failure was a recorded bare `${workdir}` (the workspace
directory itself) against a live raw directory path. Keeping the basename
cannot reconcile those two, because the live basename is the temp directory's
own name.
Paths are now elided entirely rather than reduced to their filename. A path has
too many per-machine spellings to compare literally - separator, drive letter,
8.3 short names, /var vs /private/var, whether the recorder substituted its
placeholder, and whether the value is a file or the directory itself - and two
rounds of Windows CI failures came from exactly those, none a real regression.
This is a deliberate narrowing: the projection no longer asserts which file a
prompt referenced. That is a weaker oracle than it looks, because the tests
that care assert the filesystem effect directly. The instruction wrapped around
the path is still compared.
(Written by Copilot)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
CapiReplayProxyselects a recorded response ordinally — the Nth request to an endpoint replays the Nth recorded response. Nothing ever compared what was actually asked.That left the request body completely unasserted, and the request body is the host's own product: prompt assembly, retained conversation history, truncation, attachment marshalling, and how tool results are handed back to the model. A regression in any of it replayed green, and was silently promoted to the new expected value the next time somebody re-recorded. Several
multiChattests already hand-rolled assertions overobservedModelRequestBodiesto compensate — that was the signal the gap was real.What changed
Response selection stays ordinal (matching on request bodies would be brittle against volatile fields and would desync the agent loop). Separately, every replayed turn now compares the live request against the recorded one through a projection in
harness/modelRequestProjection.ts.The same projection is applied to both sides, so captures keep their existing shape and stay readable — the previously decorative
request:block simply became executable.tool_resultpayloadstool_use_idwiringEvery elision is load-bearing, and each was established by running the assertion against the committed captures rather than guessed:
tool_resultpayloads would reintroduce exactly the platform coupling the portable-command work removed — command output, line endings and listing formats all differ per OS — and would need a per-tool normalizer layer to stay stable. Presence and wiring are asserted; the text is not.${uuid_0}) assigned at write time, which a live run cannot reproduce.What it found immediately
Switching it on surfaced seven stale captures:
tool_resultblocks (verified the live ordering is stable across three runs before re-recording, rather than weakening the assertion by sorting).pwdprompt.Invalid upToMessageId) that already gatessupportsChatForkE2E. It is listed inSTALE_RECORDED_REQUEST_EXCEPTIONS— keyed by provider and test title, so it silences only the capture it was written for — and documented inKNOWN_ISSUES.mdwith a re-enable path.The first six are re-recorded here.
Validation
Conformance 52, Copilot 49, Claude 44, Codex 8 — 0 failing, re-verified after merging main. 23 harness unit tests.
typecheck-clientand hygiene clean.Independent of #327934 — no textual conflicts; the two can land in either order.
(Written by Copilot)