agentHost: Separate substantive first progress from host bookkeeping - #336845
Merged
Vijay Upadya (vijayupadya) merged 6 commits intoSep 19, 2026
Merged
Vijay Upadya (vijayupadya) merged 6 commits into
Vijay Upadya (vijayupadya) merged 6 commits into
Conversation
Agent Host turns report `timeToFirstProgress` as one opaque number, so the host's own share of time-to-first-token cannot be attributed to any particular piece of work. Add per-stage timings, then fix the two largest contributors they expose. Attribution: record the duration of each bounded pre-send stage (workingDirectory, modelSelection, attachments, contributions, checkpoint) plus `timeToProviderDispatch`, the total host cost before the provider is called. This uses its own vocabulary rather than widening `AgentHostTurnFailureStage`: "where did the turn break" and "where did the turn spend its time" are different questions with different consumers. A stage that ran instantly reports 0; one that never ran is absent. Checkpoint overlap: the turn-start checkpoint ran a full git working-tree snapshot (status --untracked-files=all, read-tree, write-tree) inline before `sendMessage` — roughly 400ms-1s per turn on a large repository. Start it as soon as the working directory is known so it overlaps the provider round-trips, and keep awaiting it before the message is sent, so the snapshot still reflects the tree the agent starts from. A turn already cancelled skips the capture entirely, and because the cancel-time discard now runs before the capture settles, the send path discards again afterwards. Model validation: `_validateModelSelection` awaited an in-flight model-catalog refresh on every turn. That await only guards against rejecting a model a pending refresh is about to add, so skip it when the catalog already lists the model — a refresh landing later cannot invalidate a selection that is valid now. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Vijay Upadya (vijayupadya)
September 18, 2026 21:08
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved cross-provider metric, catalog freshness, timing consistency, and checkpoint cleanup defects can produce incorrect telemetry or retained state.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 5
Open (5)
What changed in this PR
Adds more accurate Agent Host time-to-first-progress telemetry and pre-send timing attribution.
Changes:
- Separates substantive progress from chat-renaming bookkeeping.
- Measures pre-send stages and overlaps checkpoint capture with preparation.
- Avoids unnecessary model-catalog waits for known models.
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/node/copilotAgent.test.ts | Tests model-refresh behavior. |
| src/vs/platform/agentHost/test/node/agentSideEffects.test.ts | Tests concurrent checkpoint capture and cancellation. |
| src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts | Tests substantive-progress and stage telemetry. |
| src/vs/platform/agentHost/test/node/agentHostTelemetryReporter.test.ts | Updates telemetry report fixtures. |
| src/vs/platform/agentHost/node/copilot/copilotAgent.ts | Optimizes model-selection validation. |
| src/vs/platform/agentHost/node/agentSideEffects.ts | Classifies progress and overlaps pre-send work. |
| src/vs/platform/agentHost/node/agentHostTurnTracker.ts | Tracks substantive progress and send-stage timings. |
| src/vs/platform/agentHost/node/agentHostTelemetryReporter.ts | Defines and emits the new measurements. |
| src/vs/platform/agentHost/common/agentHostTelemetry.ts | Defines pre-send stage types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
Base:
|
Four defects found in review, all real: Catalog freshness: a scheduled model refresh is *invalidating* — it follows a token rotation or client restart, and `_capiModels` is not cleared first, so the published catalog still belongs to the previous credential. The fast path therefore accepted a model the new credential may not have. Skip only an ordinary in-flight refresh, which re-enumerates the same credential. Failure cleanup: starting the checkpoint earlier meant a turn that failed during model selection, attachment resolution, or contributions left a turn-start checkpoint behind, which the non-resumable error path then paired with an end checkpoint. Previously such a turn never captured at all. Discard the pending capture unless `sendMessage` has begun. Timing consistency: `turnCompleted` sampled `totalTime` before closing the open stage, so a millisecond tick between the two samples could attribute stage time past the reported turn duration. Close the stage first. Checkpoint test fidelity: the fake let a discard complete while a capture was in flight, which the production service's per-session sequencer forbids. The test asserted an impossible interleaving, and the comment justifying the second discard was wrong. Model the sequencer, assert the real guarantee, and document the discard as idempotent cleanup that the failure path genuinely requires. Both new behaviours are covered by tests confirmed to fail without their fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Vijay Upadya (vijayupadya)
force-pushed
the
vijayupadya/agent-host-substantive-first-progress
branch
from
September 18, 2026 21:56
4b0d08d to
3f64ffb
Compare
The previous guard checked `_scheduledModelRefresh`, but the scheduler clears that field before it awaits the `models.list` request. The entire request window therefore looked like an ordinary refresh, and a model from the superseded catalog was accepted through the fast path — the exact case the guard was added to prevent. The earlier regression test passed only because it selected while the refresh was still scheduled, never after the request began. Track `_invalidatingModelRefresh` instead, held from the credential or client change that schedules the refresh until the replacement catalog is published, which is strictly later than the scheduler clearing its own bookkeeping. The new test waits for the credential-triggered request to start, keeps it gated, and asserts the selection stays pending and then rejects. Confirmed to fail against the scheduled-only guard (resolved while gated, setModel called). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`timeToFirstProgress` counts any visible output, including the `rename_chat` call the host itself induces via the injected rename reminder. A turn whose first visible act is naming the chat therefore looks fast while the user is still waiting for real output — the same step-selection flaw that makes the eval dashboard's Agent Host and Local numbers non-comparable. Add `timeToFirstSubstantiveProgress`, which excludes the chat rename call. Renaming is bookkeeping rather than work on the user's request, so it never qualifies regardless of whether the host or the agent initiated it. Substantive progress also marks plain progress, so the two keep the invariant that `timeToFirstSubstantiveProgress >= timeToFirstProgress` whenever both are set. Both are reported: comparing against another harness needs the substantive value, comparing against "something appeared on screen" needs the plain one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two defects found in review, both real: Cross-provider metric: Claude surfaces host server tools through its MCP bridge as `mcp__host__rename_chat`, so the bare-name comparison treated the injected rename as substantive and defeated the metric for every Claude turn. Match through a shared `isRenameChatTool` predicate that accepts both forms, following the existing convention in `agentFeedbackAnnotations`. Timing consistency: the turn stopwatch has millisecond resolution, and marking plain then substantive progress read it twice. A millisecond boundary between the reads implied a delay that never happened and could flake the equality test. Stamp both metrics from a single reading. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The classifier excluded rename tool calls but counted every response part and text/reasoning delta regardless of content. Providers open a part before streaming into it — Claude emits empty `text`/`thinking` parts on `content_block_start`, Codex an empty reasoning part — so the metric dated to the moment the agent began thinking rather than the moment it produced something, and a turn that ended without any content still reported a value instead of none. Classify content rather than shape: text and reasoning deltas must be non-empty, markdown and reasoning parts must carry content, and host-authored system notifications are structural. Tool calls other than the chat rename, content references, and input requests remain substantive. Legacy `timeToFirstProgress` is unchanged: everything rejected here still marks plain progress, so only the new metric is affected. Covers the mapper sequences directly — an empty opener followed by a delayed delta, and empty openers with no content at all. Confirmed to fail against content-blind classification (substantive populated at the empty opener). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Vijay Upadya (vijayupadya)
force-pushed
the
vijayupadya/agent-host-substantive-first-progress
branch
from
September 18, 2026 23:21
3f64ffb to
2d28806
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Vijay Upadya (vijayupadya)
marked this pull request as ready for review
September 19, 2026 00:13
Vijay Upadya (vijayupadya)
enabled auto-merge (squash)
September 19, 2026 00:14
roblourens
approved these changes
Sep 19, 2026
Vijay Upadya (vijayupadya)
deleted the
vijayupadya/agent-host-substantive-first-progress
branch
September 19, 2026 00:23
Vijay Upadya (vijayupadya)
added
the
agent-host
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.
Follows microsoft/vscode#. Part of the Agent Host TTFT work
(microsoft/vscode-internalbacklog#8816).
timeToFirstProgresscounts any visible output, including therename_chatcall the host itself induces via its injected rename reminder — which is on by default whereverproduct.quality !== 'stable', i.e. exactly where evals run.A turn whose first visible act is naming the chat therefore looks fast while the user is still waiting for real output. That is the step-selection flaw the issue documents: re-scoring one matched task on first substantive step reversed the result (VS Code 2.060s vs AHP 1.716s, against a dashboard reading of 1.269s vs 1.588s).
Adds
timeToFirstSubstantiveProgress, which excludes the chat rename call. Renaming is bookkeeping rather than work on the user's request, so it never qualifies regardless of whether the host or the agent initiated it — a simpler and more defensible rule than trying to detect whether the reminder induced it.Substantive progress also marks plain progress, so the two cannot drift: the invariant
timeToFirstSubstantiveProgress >= timeToFirstProgressholds whenever both are set. Both are reported — comparing against another harness needs thesubstantive value, comparing against "something appeared on screen" needs the plain one.