[pull] main from microsoft:main

.github/skills/validate-ui-scenario/SKILL.md

@@ -18,21 +18,36 @@ step boundary, writes the report, and captions the recording with each step and
 ## Prepare
 
 ```bash
-npm install                        # once
+npm install                             # once
 npm --prefix test/scenario run compile  # after any change under test/scenario
 ```
 
-Add `ffmpeg` and `ffprobe` to `PATH` to get the caption band on the video. Without them the run still
-succeeds and the raw recording is kept.
-
-| Target | Extra flags | Also required | Use for |
-|--------|-------------|---------------|---------|
-| Installed Insiders | `--build <app-root>` | nothing | Reproducing a report against shipped behavior |
-| Dev build from this checkout | *(none)* | `npm run electron`, `npm run transpile-client` | Verifying an unmerged change |
+**Check `ffmpeg` and `ffprobe` are available before running.** The runner looks on `PATH` and in the
+usual install locations, so an ffmpeg installed after the editor started is still found. Without them
+the scenario still runs and keeps the raw recording, but the video is not captioned with step titles.
+The runner warns at startup; if they are missing, tell the user how to install them rather than
+silently returning an unannotated video:
+
+| Platform | Install |
+|----------|---------|
+| Windows | `winget install Gyan.FFmpeg` |
+| macOS | `brew install ffmpeg` |
+| Linux | `sudo apt install ffmpeg` |
+
+A new terminal may be needed for `PATH` to pick them up, or set `FFMPEG_PATH` and `FFPROBE_PATH`. An
+existing run can be annotated afterwards with
+`node test/scenario/out/renderEvidenceChapters.js <run-dir>`.
+
+| Target | Flags | Also required | Use for |
+|--------|-------|---------------|---------|
+| Installed Insiders, else Stable | *(none — the default)* | nothing | Reproducing a report against shipped behavior |
+| Dev build from this checkout | `--dev` | `npm run electron`, `npm run transpile-client` | Verifying an unmerged change |
+| A specific install | `--build <app-root>` | nothing | Pinning an exact build |
 | Web | `--web --headless` | `npm run transpile-client` | Browser-only behavior |
 
-`--build` takes the application root — the install directory on Windows and Linux, or the `.app`
-bundle on macOS:
+With no target flag the runner finds an installed VS Code Insiders (falling back to Stable) and logs
+which one it chose. `--build` takes the application root — the install directory on Windows and
+Linux, or the `.app` bundle on macOS:
 
 ```bash
 # Windows
@@ -41,9 +56,12 @@ bundle on macOS:
 --build "/Applications/Visual Studio Code - Insiders.app"
 ```
 
-An installed build runs with its own profile and extensions directory, so your extensions and
-settings never leak into the recording. Insiders only reproduces **shipped** behavior — to validate
-an unmerged change, run the dev build from a checkout that contains it.
+Every target runs with its own profile and extensions directory, so your extensions and settings
+never leak into the recording, and the window is sized to the recording canvas so the capture has no
+empty margins. The evidence records the quality of the build that actually ran (`Insiders`,
+`Stable`, `Dev`), so a report always names the product it validated. An installed build only
+reproduces **shipped** behavior — to validate an unmerged change, use `--dev` in a checkout that
+contains it.
 
 ## Write the scenario
 
@@ -117,20 +135,41 @@ module.exports = {
 | `workspacePath` | Disposable folder to open |
 | `userSettings` | Settings seeded into the profile before launch |
 | `extraArgs` | Extra VS Code command-line arguments |
+| `stepPauseMs` | How long to hold each finished step so its caption is readable. Defaults to `1000`; set `0` when the scenario is timing-sensitive |
 
-Each step receives a `context` with `app`, `workbench`, `code`, `page`, and `skip(reason)`.
+Each step receives a `context` with `app`, `workbench`, `code`, `page`, and `skip(reason, options)`.
 `workbench` exposes the feature helpers (`settingsEditor`, `quickaccess`, `editors`, `terminal`,
 `chat`, …); `page` is the Playwright page for anything they do not cover.
 
 - **Return a string** describing how the step was validated. It appears in the report.
 - **Throw** to fail the step. The message is recorded, and the run stops.
-- **Call `skip(reason)`** when hardware, an account, or a service is unavailable. The run stops and
-  is reported as `aborted`, never as passed.
+- **Call `skip(reason, { needs })`** when the step cannot be validated automatically. The run stops
+  and is reported as `aborted`, never as passed.
+
+## Steps that cannot be automated
+
+Decide this while planning, before writing the scenario, and classify each one — the two kinds have
+different consequences:
+
+| `needs` | Meaning | What to do |
+|---------|---------|------------|
+| `human` | A person is required: physical hardware, a subjective judgement, a sign-in that cannot be scripted | Report the step so someone can check it by hand |
+| `infrastructure` | Automatable in principle, but the harness cannot do it yet | Report it as an **enhancement to this skill**, naming the missing capability |
+
+```js
+ctx.skip('Comparing physical print output requires a person with a printer.', { needs: 'human' });
+ctx.skip('The harness cannot drive native OS file dialogs.', { needs: 'infrastructure' });
+```
+
+Blocked steps are recorded in `manifest.json`, highlighted in a **Needs attention** section of
+`report.html`, marked on the video caption (`SKIPPED - NEEDS HUMAN`), and printed at the end of the
+run. Surface them in your summary — never quietly drop a step you could not perform, and never
+weaken an assertion so that it passes.
 
 ## Run it
 
 ```bash
-node test/scenario/out/runScenario.js <scenario.cjs> --build "<app-root>"
+node test/scenario/out/runScenario.js <scenario.cjs>
 ```
 
 Exit code `0` means every step passed, `1` means the run failed or was aborted, `2` a usage error.
@@ -170,6 +209,12 @@ Summarize the outcome, list failed or skipped steps, link `report.html`, and sta
 VS Code version and quality (both are in `manifest.json`), and the source issue. Attach the video to
 the issue or pull request by dragging it into the comment box.
 
+Always call out, separately from the pass/fail result:
+
+- **steps that need a person**, so someone knows what is still unverified;
+- **steps blocked on a missing harness capability**, named as a concrete enhancement to this skill;
+- **anything that degraded the evidence**, such as a missing ffmpeg leaving the video uncaptioned.
+
 ## Related
 
 - **Interactive exploration.** `test/mcp` also serves these tools over MCP (`vscode_automation_*`),
@@ -181,16 +226,17 @@ the issue or pull request by dragging it into the comment box.
   skill when a scenario is not yet covered there, or to iterate locally before proposing one.
 
 <example>
-User: "/validate-ui-scenario reproduce https://github.com/microsoft/vscode/issues/250159 against my
-installed VS Code Insiders, and give me the report and the annotated video."
+User: "/validate-ui-scenario reproduce https://github.com/microsoft/vscode/issues/250159"
 
-1. Read the issue and identify the observable claim: searching `chat confirm` in the Settings editor
+1. Confirm `ffmpeg`/`ffprobe` are available; if not, say so and give the install command before
+   running, so the user is not surprised by a video without step titles.
+2. Read the issue and identify the observable claim: searching `chat confirm` in the Settings editor
    should match **Max Requests**, whose description mentions confirmation.
-2. Add a baseline step (`max requests` finds the setting) so a failure cannot be explained by the
+3. Add a baseline step (`max requests` finds the setting) so a failure cannot be explained by the
    setting being missing from the build.
-3. Write `.build/vscode-playwright-mcp/issue-250159.cjs`, run it with `--build`, and read the
-   printed report path.
-4. Report the outcome per step, link `report.html`, and attach `videos/annotated.mp4`.
+4. Write `.build/vscode-playwright-mcp/issue-250159.cjs` and run it with no target flag, which uses
+   the installed Insiders; read the printed report path.
+5. Report the outcome per step, link `report.html`, and attach `videos/annotated.mp4`.
 
 The run fails at the search step, and that is the answer: the issue reproduces. Report it as a
 successful reproduction, not as a broken scenario.

src/vs/editor/browser/widget/multiDiffEditor/multiDiffEditorLogging.ts

@@ -0,0 +1,185 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { Disposable, toDisposable } from '../../../../base/common/lifecycle.js';
+import { autorun, autorunWithStore, IObservable, observableFromEvent } from '../../../../base/common/observable.js';
+import { basename } from '../../../../base/common/resources.js';
+import { URI } from '../../../../base/common/uri.js';
+import { ILogService, LogLevel } from '../../../../platform/log/common/log.js';
+import { MultiDiffEditorViewModel } from './multiDiffEditorViewModel.js';
+
+/** A diff item of the multi diff editor, as far as logging is concerned. */
+export interface ILoggedDiffItem {
+	getKey(): string;
+	/** Short, log friendly name of the item. */
+	getLabel(): string;
+	readonly collapsed: IObservable<boolean>;
+	readonly contentHeight: IObservable<number>;
+}
+
+/** The widget state traced by {@link MultiDiffEditorLogger.logStateChanges}. */
+export interface ILoggedEditorState {
+	readonly viewModel: IObservable<MultiDiffEditorViewModel | undefined>;
+	readonly items: IObservable<readonly ILoggedDiffItem[]>;
+	readonly spaceBetweenPx: number;
+	readonly getScrollTop: () => number;
+	readonly isPreserveFocusOnLoad: () => boolean;
+}
+
+/**
+ * Trace logger for the multi diff editor. Logs the state transitions that are
+ * hard to observe from the outside (scroll offsets that are set programmatically,
+ * content height changes, collapsed state changes and view state save/restore),
+ * so bugs like "the editor jumped while scrolling" or "this file was expanded
+ * even though it was collapsed" can be reconstructed from the log.
+ *
+ * Enable with `Developer: Set Log Level...` > `Window` > `Trace`.
+ */
+export class MultiDiffEditorLogger extends Disposable {
+	/** Last logged collapsed state per diff item, to only log actual changes. */
+	private readonly _lastLoggedCollapsed = new Map<string, boolean>();
+	/** Last logged content height per diff item, to only log actual changes. */
+	private readonly _lastLoggedContentHeight = new Map<string, number>();
+
+	private readonly _isEnabled: IObservable<boolean>;
+
+	constructor(private readonly _logService: ILogService) {
+		super();
+
+		this._isEnabled = observableFromEvent(this, this._logService.onDidChangeLogLevel, () => this._logService.getLevel() <= LogLevel.Trace);
+	}
+
+	/**
+	 * Whether trace logging is on. Check this before computing data that is only
+	 * needed for logging.
+	 */
+	public get isEnabled(): boolean {
+		return this._isEnabled.get();
+	}
+
+	public log(message: string, data?: Record<string, unknown>): void {
+		if (!this.isEnabled) {
+			return;
+		}
+		const formattedData = data ? Object.entries(data).map(([key, value]) => `${key}: ${formatValue(value)}`).join(', ') : undefined;
+		this._logService.trace(`[MultiDiffEditor] ${message}${formattedData ? ` (${formattedData})` : ''}`);
+	}
+
+	/**
+	 * Traces the state transitions that cannot be reconstructed after the fact:
+	 * view model/loading changes, collapsed state changes (no matter who caused
+	 * them) and content height changes, which are the usual cause of the view
+	 * jumping while scrolling. Per-frame rendering is deliberately not traced.
+	 *
+	 * The observers only exist while trace logging is on, so nothing is observed
+	 * (and no state is tracked) in the default case; they are recreated when the
+	 * log level is raised to trace again.
+	 */
+	public logStateChanges(state: ILoggedEditorState): void {
+		this._register(autorunWithStore((reader, store) => {
+			if (!this._isEnabled.read(reader)) {
+				return;
+			}
+
+			// Reset the tracked state when tracing is turned off, so the first logs
+			// after re-enabling report the current state instead of a diff against
+			// stale values.
+			store.add(toDisposable(() => {
+				this._lastLoggedCollapsed.clear();
+				this._lastLoggedContentHeight.clear();
+			}));
+
+			store.add(autorun(reader => {
+				const viewModel = state.viewModel.read(reader);
+				this.log('view model changed', {
+					hasViewModel: !!viewModel,
+					isLoading: viewModel?.isLoading.read(reader),
+					preserveFocusOnLoad: state.isPreserveFocusOnLoad(),
+				});
+			}));
+
+			store.add(autorun(reader => {
+				const changed: string[] = [];
+				for (const item of state.items.read(reader)) {
+					const collapsed = item.collapsed.read(reader);
+					const key = item.getKey();
+					if (this._lastLoggedCollapsed.get(key) !== collapsed) {
+						this._lastLoggedCollapsed.set(key, collapsed);
+						changed.push(`${item.getLabel()}=${collapsed}`);
+					}
+				}
+				if (changed.length > 0) {
+					this.log('collapsed state changed', { changed });
+				}
+			}));
+
+			store.add(autorun(reader => {
+				// Not read via the reader: this must not re-run on every scroll event.
+				const scrollTop = state.getScrollTop();
+				const changed: string[] = [];
+				let totalHeight = 0;
+				for (const item of state.items.read(reader)) {
+					const contentHeight = item.contentHeight.read(reader);
+					const key = item.getKey();
+					const lastContentHeight = this._lastLoggedContentHeight.get(key);
+					if (lastContentHeight !== contentHeight) {
+						// A height change above the current scroll offset shifts
+						// everything below it and makes the view jump.
+						const abovePosition = totalHeight < scrollTop ? ' (above scroll offset)' : '';
+						changed.push(`${item.getLabel()}: ${lastContentHeight ?? '?'} -> ${contentHeight}${abovePosition}`);
+						this._lastLoggedContentHeight.set(key, contentHeight);
+					}
+					totalHeight += contentHeight + state.spaceBetweenPx;
+				}
+				if (changed.length > 0) {
+					this.log('content height changed', { changed, totalHeight, scrollTop });
+				}
+			}));
+		}));
+	}
+}
+
+/** Short, log friendly name of a diff item resource. */
+export function formatUri(uri: URI | undefined): string {
+	if (!uri) {
+		return '<none>';
+	}
+	// Pathless URIs (e.g. `changes-multi-diff-source:?<query>`) have an empty
+	// basename, so fall back to something that still identifies the resource.
+	return basename(uri) || uri.authority || uri.scheme;
+}
+
+/** Turns a {@link DocumentDiffItemViewModel.getKey} value into a short log label. */
+export function formatDiffItemKey(key: string | undefined): string {
+	if (key === undefined) {
+		return '<none>';
+	}
+	try {
+		const [original, modified] = JSON.parse(key) as (string | undefined)[];
+		const uri = modified ?? original;
+		return uri ? formatUri(URI.parse(uri)) : '<none>';
+	} catch {
+		return key;
+	}
+}
+
+function formatValue(value: unknown): string {
+	if (value === undefined) {
+		return '<undefined>';
+	}
+	if (URI.isUri(value)) {
+		return formatUri(value);
+	}
+	if (Array.isArray(value)) {
+		return `[${value.map(formatValue).join(', ')}]`;
+	}
+	if (typeof value === 'number') {
+		return String(Math.round(value * 100) / 100);
+	}
+	if (typeof value === 'object') {
+		return JSON.stringify(value);
+	}
+	return String(value);
+}