Default to an installed build, and report missing video tooling - #331881
Merged
Conversation
Three problems surfaced running the skill from a fresh checkout. Nothing ran without a target. With no flags the runner used the build from the checkout, which only exists after compiling the product, so the documented starting point failed to launch. Reproducing a reported issue means running the shipped product anyway, so with no target flag it now finds an installed VS Code Insiders (falling back to Stable) and logs which one it chose. `--dev` selects the checkout build, and `--build` still pins an exact install. A missing ffmpeg was only discovered after the run, as a raw ENOENT, and it threw out of `runScenario` after the report had been written. The runner now checks for ffmpeg and ffprobe before launching anything and prints the install command for the platform, and caption rendering can no longer fail a run that has already produced its evidence. The window did not fill the recording. The canvas is 1920x1080 while VS Code sizes its own window (1440x900 with a workspace, 1200x800 empty), so the capture showed the window in the top-left corner surrounded by dead space. The window is now sized to the canvas once recording is on; a window larger than the display still renders at that size, so this holds on smaller screens. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adb443eb-11e5-40a1-8608-7f593fa79485
Contributor
There was a problem hiding this comment.
Pull request overview
Improves UI scenario validation for fresh checkouts and installed VS Code builds.
Changes:
- Defaults to installed Insiders/Stable, with
--devfor checkout builds. - Reports missing video tools and makes caption failures non-fatal.
- Sizes desktop windows to the recording canvas and updates guidance.
Show a summary per file
| File | Description |
|---|---|
test/scenario/src/runScenario.ts |
Adds video-tool checks and safe caption rendering. |
test/scenario/src/renderEvidenceChapters.ts |
Exposes non-failing caption rendering. |
test/scenario/src/options.ts |
Adds the --dev option. |
test/scenario/src/application.ts |
Discovers installed VS Code builds by default. |
test/automation/src/playwrightElectron.ts |
Resizes recorded Electron windows. |
.github/skills/validate-ui-scenario/SKILL.md |
Documents new targeting and tooling behavior. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
Follow-up to the same skill run. Captions were missing even though ffmpeg was installed. A PATH edit only reaches processes started afterwards, so an editor that was already running never sees it, and the runner concluded ffmpeg was absent. It now looks in the usual install locations as well as PATH, which is the difference between an annotated recording and a raw one on a machine that already has ffmpeg. Steps flowed past too quickly to read. A caption is only legible for as long as its step is on screen, and steps that assert rather than type can complete in a few hundred milliseconds. Each finished step is now held briefly, controlled by `stepPauseMs` and disabled with `0` for timing-sensitive scenarios. Steps that cannot be automated were indistinguishable from ones that were merely unavailable. `skip` now takes `needs: human` or `needs: infrastructure`: the first means a person has to check it, the second means the harness could do it but cannot yet, which is an enhancement request rather than a permanent limit. The distinction is recorded in the manifest, highlighted in the report, shown on the video caption, and printed at the end of the run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adb443eb-11e5-40a1-8608-7f593fa79485
Bryan Chen (bryanchen-d)
marked this pull request as draft
August 20, 2026 23:19
Review follow-ups. The evidence labelled every installed run `Dev`. Quality was read from the environment, which only describes a build made from this checkout, so a run against installed Insiders was reported as Dev in both the manifest and the report - the evidence named the wrong product. An installed build stamps its own quality in `product.json`, so that is now the source when a build path is given. This also corrects `--build`, which had the same problem before this change. Linux missed Snap installs. Snap keeps the app under a read-only revision root, so a machine with VS Code installed only through Snap found nothing and fell back to the unbuilt checkout - the exact failure the new default exists to avoid. The web launcher recorded 1920x1080 while sizing the page to 1440x900, so the no-empty-margins claim did not hold there. It now matches the canvas while recording and keeps its established size otherwise, so smoke runs are unchanged. `--dev` was accepted but undocumented in the runner's own help, which now lists all three targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adb443eb-11e5-40a1-8608-7f593fa79485
VS Code on Windows applies a downloaded update by swapping the executable during startup, so a launch attempt exits before showing a window. Playwright then waits the full launch timeout and reports that the process "likely crashed or hung", which sends the reader looking for crash dumps that do not exist. Insiders downloads an update most days, so anyone reproducing an issue will meet this. A `new_<exe>` beside the target is the marker, and checking for it turns a 60s misleading timeout into an immediate statement of the cause and the fix. Confirmed the launch failure is environmental rather than harness behaviour: a bare Playwright launch of the same installed build, with none of this code in the path, also never receives a window while the update is staged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adb443eb-11e5-40a1-8608-7f593fa79485
Bryan Chen (bryanchen-d)
marked this pull request as ready for review
August 21, 2026 17:27
Vijay Upadya (vijayupadya)
approved these changes
Aug 21, 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.
Three problems found running the
validate-ui-scenarioskill from a fresh checkout against #244597.Nothing ran without a target flag
With no flags the runner used the build from the checkout, which only exists after compiling the product. Verified on unmodified
main: the documented starting point fails to launch.Reproducing a reported issue means running the shipped product anyway, so with no target flag the runner now finds an installed VS Code Insiders — falling back to Stable — and logs the choice:
--devselects the checkout build, and--build <app-root>still pins an exact install. Existing callers that pass--buildare unaffected.A missing ffmpeg was only discovered after the run
Captions are rendered once the scenario finishes, so a missing ffmpeg surfaced as a raw
spawnSync ffprobe ENOENTafter VS Code had been driven and the report written — and it threw out ofrunScenario, so the run never printed its outcome.The runner now checks before launching anything and names the fix for the platform:
Caption rendering can no longer fail a run that has already produced its evidence, and the post-run message is actionable rather than an ENOENT stack.
The window did not fill the recording
The recording canvas is 1920x1080, but VS Code sizes its own window —
DEFAULT_WORKSPACE_WINDOW_SIZEis 1440x900 andDEFAULT_EMPTY_WINDOW_SIZEis 1200x800 — so the capture showed the window in the top-left corner of a larger frame, wasting about 30% of every pixel recorded.Measured on the reporting machine:
The window is now sized to the canvas once recording is enabled. The display being smaller than the canvas is fine — confirmed by probing before implementing: after
setBounds,innerWidth/innerHeightreport 1920x1080 and both the screenshot and the video come back full frame on a 1512x901 display. Sizing only happens when video recording is on, so non-recording smoke runs are untouched.Verification
Against installed Insiders on Windows, with the skill's own documented example:
1for the reproduced failurevideos/annotated.mp4at 1920x1300 (1080 of workbench + the caption band) with no dead space--devstill resolves the checkout build; it fails to launch here only because this checkout is not built, identically to unmodifiedmainffmpeg was installed, and still not found
A PATH edit only reaches processes started afterwards, so an editor that was already running never sees it. On the reporting machine ffmpeg was installed and present in the persisted user PATH, yet invisible to the shell the skill ran in — so the run produced no annotated video.
The runner now probes the usual install locations (winget
LinksandPackages, chocolatey, Program Files, Homebrew,/usr/local/bin) in addition to PATH. Verified with ffmpeg deliberately off PATH:No warning, no manual setup.
Steps flowed past too quickly to read
A caption is only legible for as long as its step is on screen, and an asserting step can finish in a few hundred milliseconds. Each finished step is now held briefly —
stepPauseMs, default1000, set0for timing-sensitive scenarios.Blocked steps are classified and reported
skippreviously recorded only that a step did not run. It now takesneeds:needshumaninfrastructureThe distinction is recorded in
manifest.json, highlighted in a Needs attention section ofreport.html, shown on the caption band, and printed at the end of the run:Both variants were exercised end to end; the caption band renders
SKIPPED - NEEDS HUMANin amber, later steps do not run, and the outcome isabortedrather than passed.test/mcpstill compiles against the widenedEvidenceService.stepsignature.