From b09bf04448e182c2c1cacd10bb3c717afe539ca8 Mon Sep 17 00:00:00 2001 From: Nathan Carlson Date: Thu, 16 Jul 2026 16:37:40 -0700 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=A4=96=20Add=20PR-review=20agent=20sk?= =?UTF-8?q?ills=20(code-review=20+=20review-pr)=20sharing=20one=20rubric?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two Copilot agent skills under .github/skills, both driven by a single shared grading rubric distilled from this repo's maintainer review history: - code-review/: rubric-only skill the cloud Copilot code reviewer auto-invokes (review-focused name); works from the diff it is given, never approves. - review-pr/: on-demand CLI skill to review a PR by number/URL, fetching via gh/GitHub MCP, report by default, optional posting with a reviewer marker. rubric.md is the single source of truth (localization, async/resource correctness, TypeScript conventions, command-line construction, telemetry, settings, cross-platform), graded by severity/confidence for high signal, low noise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14b94f62-70f3-4e6d-8d2d-cd5a318a26ca --- .github/skills/code-review/SKILL.md | 24 ++++ .github/skills/code-review/rubric.md | 198 +++++++++++++++++++++++++++ .github/skills/review-pr/SKILL.md | 77 +++++++++++ 3 files changed, 299 insertions(+) create mode 100644 .github/skills/code-review/SKILL.md create mode 100644 .github/skills/code-review/rubric.md create mode 100644 .github/skills/review-pr/SKILL.md diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md new file mode 100644 index 00000000..8b76d58b --- /dev/null +++ b/.github/skills/code-review/SKILL.md @@ -0,0 +1,24 @@ +--- +name: code-review +description: Review a pull request in this repository (the vscode-containers / Container Tools extension) against the maintainers' house style. Use this whenever reviewing a pull request or its changed files to check localization, async/resource correctness, TypeScript conventions, command-line construction, telemetry, settings, and cross-platform issues distilled from this repo's review history. +--- + +# Container Tools code review + +Apply the shared grading rubric in [`rubric.md`](./rubric.md) to the pull request under +review. That file is the single source of truth for what to look for and how to grade it -- +read it and follow it. + +You already have the pull request's diff and changed files provided to you, and your review +comments are published through the normal review mechanism. So: + +- **Do not** fetch the PR yourself or run `gh` -- work from the diff you were given. +- **Do not** approve. Leave comments / request changes only; approval is a human decision. +- Focus your comments on the changed lines. Ground every point in the diff or a concrete repo + convention, and cite the exact helper or pattern the author should use instead of a generic + complaint. +- Prefer high signal, low noise: raise real, high-confidence issues; route genuine "did you + intend X" points as questions rather than assertions; drop speculation. + +Follow the rubric's severity/confidence grading and the "What NOT to flag" list so the review +stays useful and does not bury maintainers in nits. diff --git a/.github/skills/code-review/rubric.md b/.github/skills/code-review/rubric.md new file mode 100644 index 00000000..05d26988 --- /dev/null +++ b/.github/skills/code-review/rubric.md @@ -0,0 +1,198 @@ +# Container Tools review rubric (shared) + +This is the shared grading rubric used by both PR-review skills in this repo: + +- `code-review` -- the skill the cloud Copilot code reviewer auto-invokes on a PR. +- `review-pr` -- the on-demand skill a maintainer runs from the CLI against any PR. + +It defines **what to look for and how to grade** a change to +`microsoft/vscode-containers` (formerly `microsoft/vscode-docker`). It says nothing about +*how* to fetch a PR or *where* to post -- each skill handles that. Review the change as a +maintainer would: not just "does it work," but "does it fit this codebase" -- established +patterns, maintainability, localization, resource/error handling, and no more complexity than +needed. + +The **Repo-specific patterns** below were distilled from thousands of real maintainer review +comments on this repo. Treat them as house style. They are curated and **not exhaustive** -- +they capture the issues maintainers raise most often, not every possible issue. Use them to +prioritize, then apply general engineering judgment for anything they do not cover. + +## How to grade + +Judge each finding on two axes and only surface what earns its place: + +- **Severity**: blocking (correctness / resource / localization / clear pattern violation) vs + non-blocking (nit, style, optional improvement) vs open question (ask, do not assert). +- **Confidence**: only raise a point you can ground in the diff or a concrete repo + convention, and cite the exact helper/pattern the author should use instead. If you are + guessing, downgrade it to an open question or drop it. + +Prefer **high signal, low noise**. A short review that names three real problems beats a long +one padded with speculation. + +## Higher scrutiny for agent-authored PRs + +If the PR is authored by an agent (title starts with an agent marker such as the robot emoji, +or the author is a bot like `Copilot` / `dependabot`), review with **higher scrutiny** -- +agents commonly over-engineer, duplicate existing helpers, hard-code strings that should be +localized, add unnecessary defensive code, or leave PR-narrative comments in source. For human +authors, give benefit of the doubt and **ask** when something looks odd but could be +intentional. + +## Scope and intent (every PR) + +1. Does the title/description explain *why*, not just *what*? Will it read cleanly as a + squash-merge commit message? +2. Do the changes match the stated scope? Flag unexplained scope creep or missing changes the + description implies. +3. If a linked issue exists, does the PR actually address it? (A partial fix is fine if the PR + says so; a silent mismatch is not.) + +Classify the changed files to decide which patterns apply most: command/UI code +(`extensions/vscode-containers/src/commands/**`, tree items), tasks/debugging +(`extensions/vscode-containers/src/tasks/**`, `extensions/vscode-containers/src/debugging/**`), +scaffolding, runtime/CLI clients, `package.json` / `package.nls.json` contributions and +settings, or tests. + +## Repo-specific patterns + +Apply these. Cite the concrete helper or convention the author should use. + +### Localization +- **Every user-facing string must be localized** with `vscode.l10n.t('...')`. Flag any + hard-coded visible string (messages, quick-pick labels/placeholders, notifications, errors, + `package.json` contributions via `package.nls.json`). Legacy `localize(...)` is gone; new + code uses `vscode.l10n.t`. +- Message style: avoid contractions in user-facing text; keep trailing-period usage + consistent; prefer clear, full wording. + +### Async / await +- Do not mark a function `async` if its body is synchronous. +- Mixing `try/catch` with promises **without** `await` hides errors -- ensure rejections are + awaited inside the `try`. +- Intentional fire-and-forget must be explicit: prefix the call with `void` **and** a short + comment that you are deliberately not awaiting. +- `vscode.window.withProgress(...)` returns the inner callback's value -- await it instead of + stashing extra locals. +- After an `await`, re-check any guard state you relied on before it (reentrancy). +- Consider `Promise.all` for independent CLI calls. + +### Disposables / resource lifetime +- `CancellationTokenSource`, `EventEmitter`, `createOutputChannel()`, event subscriptions, + and timers are disposable. Dispose them in a `finally`, or push them onto + `context.subscriptions` for extension-lifetime cleanup. `clearTimeout` disposes timers. + +### Error handling +- Prefer specific `catch` blocks; use `parseError` (from `@microsoft/vscode-azext-utils`) to + classify errors rather than assuming the cause. +- Cancellation is signaled by throwing `UserCancelledError`. Many helpers (e.g. the shared + quick-pick / UI helpers) already throw it, so redundant "user cancelled" checks are not + needed. +- Wrap command entry points in `callWithTelemetryAndErrorHandling`. Error / no-item messages + must be localized. + +### null / undefined +- Choose `||` vs `??` deliberately: `||` treats `''` / `0` / `false` as "use default"; `??` + only defaults on null/undefined. This matters for settings where empty string is a real + value. +- Guard array access on possibly-undefined values; a non-empty check often needs both a + defined check **and** `length > 0`. +- Keep return types honest -- if a value can be undefined, the signature must be + `T | undefined`. +- Prefer real `undefined` over `''` / "(not set)" in telemetry; do not write explicit + default/`undefined` config into `tasks.json`. + +### TypeScript typing +- TS is **structurally** typed -- two interfaces with the same shape are interchangeable, so + distinct names alone do not prevent mix-ups. +- Prefer an `enum` or string-literal union over a bare `boolean` parameter so call sites are + self-documenting. +- Avoid `any`; keep public/extension API surface minimal (add options only when needed). + +### Declarations / style +- Prefer `const`; use `private readonly` on constructor parameter properties. +- Single quotes for strings; `export const X` over `export default`; avoid `=== true`. + +### Command-line / regex construction +- Build container/CLI commands with the arg helpers from `@microsoft/vscode-processutils` + (`composeArgs`, `withArg`, `withNamedArg`, `withQuotedArg`, with the `shouldQuote` / + `assignValue` options) instead of string interpolation -- they handle quoting/escaping. + Escaping differs per shell/OS (especially Windows); prefer `withQuotedArg` / `shouldQuote` + over hand-rolled quoting. See `extensions/vscode-containers/src/tasks/netSdk/netSdkTaskUtils.ts` and + `extensions/vscode-containers/src/debugging/netcore/*` for current usage. +- Regex: escape `.` as `\.`, anchor correctly, and watch for accidental matches elsewhere in a + path or command line. + +### UI / UX +- Prefer a QuickPick (shows all options at once) over multi-click "cycle" buttons. +- Accessibility: refer to UI elements by name, not relative position ("the button on the + left") -- screen readers cannot rely on position. +- Avoid redundant toasts / progress notifications. +- Commands that should not appear in the Command Palette need a `when` clause / menu + restriction in `package.json`. + +### Telemetry +- Do not emit telemetry from tests (`DEBUGTELEMETRY=1` is set for tests). +- Renaming/moving/removing telemetry breaks existing queries and dashboards -- flag it. +- Do not inflate counts by invoking a command internally just to reuse its logic. + +### Configuration / settings +- Prefer a contributed **setting** over `globalState` (discoverable, per-workspace, + shareable). Get the setting **scope** right (e.g. machine-scoped to separate remote vs + local). +- Deprecate a setting with `deprecationMessage` for at least one release before removing; + check telemetry for usage first. +- Mind precedence between VS Code settings and environment variables (e.g. `docker.host` vs + `DOCKER_HOST`). + +### Cross-platform +- Prefer Node `os.platform()` / `os.arch()` over bespoke OS-provider abstractions. +- Linux filesystems are case-sensitive: import path casing must match exactly or the Linux + build breaks. +- Consider cmd, PowerShell, and git bash behavior on Windows. + +### Comments / docs / tests +- Comments explain **why**, not what; add a source link when porting logic from elsewhere. +- Replace "what does `true` mean" comments with a typed parameter. +- Factor pure logic (version comparison, parsing) into unit-testable functions; use test data + that is not already sorted so sorting logic is actually exercised. + +## General correctness (every PR) + +Beyond the repo patterns, check: logic / off-by-one / null-path bugs, race conditions and +reentrancy, resource leaks, error masking (caching/telemetry/logging swallowing a primary +error), duplication of an existing helper (name the helper that should be reused), dead code +and debug leftovers, new public API only consumed by tests, and inconsistencies with +surrounding code (naming, types, logging, error handling) without a clear reason. + +This is a pnpm monorepo with ESLint. Flag obvious lint/type violations, but do not run heavy +builds as part of a review unless explicitly asked. + +## What NOT to flag + +- Style already consistent with the codebase. +- Pre-existing issues in unchanged code (mention in the summary if relevant; do not block on + them). +- Alternative approaches that are merely different, not clearly better. +- Missing XML/JSDoc unless it is public API or the surrounding code documents everything else. +- Theoretical concerns that do not manifest in practice. +- Files that must not be hand-edited / reviewed line-by-line (generated or curated files) -- + see the repo's custom instructions. + +When you do propose an alternative, make it concrete (sketch the code / name the files) and say +*why* it is better -- simpler, safer, faster, or more maintainable. "Did you consider X?" is +fine when you have thought X through enough to defend it. + +## Verdict shape + +Frame the review as: + +1. **Verdict** -- one line: ready / needs changes / needs discussion. +2. **Blocking issues** -- correctness, resource, localization, or pattern violations that must + be fixed, each with file:line and the concrete fix. +3. **Non-blocking suggestions** -- improvements and nits. +4. **What looks good** -- brief. +5. **Open questions** -- things to ask the author rather than assert. + +Never approve -- approval is a human maintainer's decision. A review either raises comments or +requests changes; it does not sign off. diff --git a/.github/skills/review-pr/SKILL.md b/.github/skills/review-pr/SKILL.md new file mode 100644 index 00000000..84e9ef2d --- /dev/null +++ b/.github/skills/review-pr/SKILL.md @@ -0,0 +1,77 @@ +--- +name: review-pr +description: Review a specific vscode-containers pull request on demand from the CLI (or any interactive agent), the way a Container Tools maintainer would. Use when asked to review a PR by number, URL, or "the current branch", to produce a written review and, only when explicitly asked, post comments on the PR. Fetches the PR with the GitHub CLI and applies the shared review rubric. +allowed-tools: shell +--- + +# Review a Container Tools PR (on demand) + +You are reviewing a pull request to `microsoft/vscode-containers` the way a maintainer would. +Apply the shared grading rubric in +[`../code-review/rubric.md`](../code-review/rubric.md) -- read it first; it is the single +source of truth for what to look for and how to grade. This skill only adds how to **get** the +PR and, optionally, how to **post**. + +## Modes + +Default to **report** unless the user explicitly asks you to comment on / review the PR on +GitHub. + +- **report** (default): produce a written review; touch nothing on GitHub. +- **post**: publish review comments on the PR (see Posting). Only when explicitly requested. + +Never approve (`gh pr review --approve`) in either mode -- approval is a human decision. + +## Get the PR + +Accept a PR number (`123`), a URL +(`https://github.com/microsoft/vscode-containers/pull/123`), or "the current branch". +Resolve with the GitHub CLI: + +```bash +gh pr view --json number,title,body,author,files,url,state,isDraft,headRefOid +gh pr diff +``` + +For prior review threads and discussion (so you do not repeat points already made, and can see +what the author pushed back on): + +```bash +gh api repos/microsoft/vscode-containers/pulls//comments # inline review comments +gh pr view --comments # issue-level discussion +``` + +If the PR references an issue, read it (`gh issue view `) to confirm the PR addresses it. + +If the PR is already **merged or closed**, do a retrospective review in report mode only -- do +not try to post change requests on it. + +(Equivalent GitHub MCP Server tools -- `get_pull_request`, `get_pull_request_diff`, +`get_pull_request_files`, `get_pull_request_comments` -- work too if `gh` is unavailable.) + +## Produce the review + +Apply the rubric to the diff and emit its **Verdict shape** (verdict, blocking issues, +non-blocking suggestions, what looks good, open questions). Ground every point in the diff or a +concrete repo convention and cite the helper/pattern to use. + +## Posting (post mode only) + +Keep each comment specific and actionable; reference the exact file/line. + +- Inline comment on a line: + `gh api repos/microsoft/vscode-containers/pulls//comments -f body='...' -f commit_id='' -f path='' -F line= -f side=RIGHT` +- Summary review (comment, or request changes when the author must act -- never approve): + `gh pr review --comment --body '...'` / `gh pr review --request-changes --body '...'` +- Prefix each agent-posted review comment with a clear reviewer marker (e.g. a magnifying + glass or detective emoji) so it is distinguishable from a human review. + +(Equivalent GitHub MCP Server tools: `create_pending_pull_request_review` -> +`add_comment_to_pending_review` -> `submit_pending_pull_request_review`, or +`create_and_submit_pull_request_review` for a summary-only review.) + +## Rules + +- Never approve. Read-only otherwise: do not modify code, commit, or push as part of a review. +- In report mode, post nothing to GitHub. +- Prefer asking over asserting when intent is genuinely unclear. From 125df729a3dde9307adad488c679f290232e235c Mon Sep 17 00:00:00 2001 From: Nathan Carlson Date: Fri, 17 Jul 2026 09:47:28 -0700 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=A4=96=20Tune=20review=20rubric=20fro?= =?UTF-8?q?m=20backtest=20+=20incoming-repo=20mining?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backtested the rubric against real maintainer (SME) comments on historical vscode-docker PRs (blind reviewer + judge agents), then A/B-tested two variants on a held-out set. Adopted the higher-value variant: - Reframe "high signal, low noise" as *no speculation*, not *few findings*, and ask for thoroughness on changed lines. A/B (6 held-out PRs): +40% relative recall and ~65% more real issues surfaced, at ~4% noise cost (96% vs 100% precision). - Add "Reuse before adding (check first)" -- reuse was the single most common SME request across the corpus -- plus a minimal-API rule. Mined the two repos being imported into the monorepo (vscode-docker-extensibility, compose-language-service) for recurring, generalizable maintainer patterns and added coverage for where the repo is going: - Contracts / client / registry packages: self-contained public contracts, CancellationToken ownership, zod schema parsing, command-runner stream lifecycle, image/tag modeling, registry connection-state, export loadability. - Dependencies / manifests: keep dependency/lockfile churn scoped and Node-engine compatible. - Tests/docs: avoid flaky wall-clock upper bounds; preserve line endings. Also migrated rubric path references to the monorepo layout (extensions/vscode-containers/src) and corrected the package manager to pnpm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14b94f62-70f3-4e6d-8d2d-cd5a318a26ca --- .github/skills/code-review/rubric.md | 65 ++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/.github/skills/code-review/rubric.md b/.github/skills/code-review/rubric.md index 05d26988..8777cbde 100644 --- a/.github/skills/code-review/rubric.md +++ b/.github/skills/code-review/rubric.md @@ -27,8 +27,13 @@ Judge each finding on two axes and only surface what earns its place: convention, and cite the exact helper/pattern the author should use instead. If you are guessing, downgrade it to an open question or drop it. -Prefer **high signal, low noise**. A short review that names three real problems beats a long -one padded with speculation. +Prefer **high signal, low noise** -- but "low noise" means *no speculation*, not *few +findings*. Be **thorough on the changed lines**: surface every substantive issue you can ground +in the diff or a concrete convention, not just the top three. Maintainers review closely and +raise many small, real points per PR; a review that grounds ten real issues is better than one +that stops at three and leaves grounded problems uncommented. The bar for including a point is +confidence, not scarcity: if you can name the exact line and the concrete fix, include it; if +you are guessing, drop it or make it an open question. ## Higher scrutiny for agent-authored PRs @@ -58,6 +63,18 @@ settings, or tests. Apply these. Cite the concrete helper or convention the author should use. +### Reuse before adding (check first) +- This is the **single most common** maintainer request. Before accepting a new helper, + constant, URL, type, or utility the PR introduces, check whether one already exists and should + be reused instead: an existing constant/URL (e.g. a shared `dockerHubUrl`, `learnMore` + property), an existing helper doing the same job (e.g. `fse.readJSON` instead of `readFile` + + `JSON.parse`, `path.resolve`/`path.normalize` instead of hand-rolled separator logic, an + existing `addVolumeWithoutConflicts`-style helper), or an existing base class the new code + should extend rather than duplicate. Name the concrete existing thing to reuse. +- Keep the public/extension **API surface minimal**: flag new parameters, options, or config + fields the caller did not ask for and that nothing yet consumes. Add options only when a real + caller needs them. + ### Localization - **Every user-facing string must be localized** with `vscode.l10n.t('...')`. Flag any hard-coded visible string (messages, quick-pick labels/placeholders, notifications, errors, @@ -151,11 +168,53 @@ Apply these. Cite the concrete helper or convention the author should use. build breaks. - Consider cmd, PowerShell, and git bash behavior on Windows. +### Contracts / client / registry packages (imported extensibility) +The monorepo is importing `vscode-docker-extensibility` (the `@microsoft/vscode-container-client` +and `@microsoft/vscode-docker-registries` packages) and `compose-language-service`. For changes +under those packages, apply these -- they are the patterns their maintainers raise most: +- **Self-contained public contracts.** Define the types a contract exposes in the contract file; + do not couple a public API to implementation-only types. Re-export intended public symbols from + the package entrypoint, and share cross-cutting errors (e.g. `CancellationError`, + `CommandNotSupportedError`) rather than redefining them. Duplicate a small type before coupling + a contract to an implementation. +- **CancellationToken ownership.** Public APIs should accept a `CancellationToken`, not a + `CancellationTokenSource` -- the caller that creates the `CTS` owns cancellation and disposal. + Long/streaming operations (including generators and parsers) should take and honor a token and + throw `CancellationError` on cancel. +- **Schema-based parsing.** Validate and normalize external CLI/registry JSON with shared `zod` + schemas (`preprocess` around `JSON.parse`, `transform` for normalization) instead of ad-hoc + `JSON.parse` plus manual conversion; remember MSBuild returns `"true"`/`"false"` strings + (`z.stringbool()`), and fields like `Created` can be null. +- **Command-runner stream lifecycle.** Runners that pipe or generate process output must drain or + close every stream copy (pipe copies cause back-pressure), await the process, and propagate a + process failure as a thrown error rather than silently returning empty output. +- **Image/tag modeling.** Represent image references with the original text plus parsed optional + parts; treat Docker sentinels like `` as missing data, and cover parser edge cases + (`alpine:5` is a tag, not a registry host) with tests. Prefer the client's parsed fields over + re-parsing references by hand. +- **Registry connection state.** Connect/disconnect flows must keep persisted IDs, caches, + tree-provider state, and stored credentials in sync; re-adding a registry should be idempotent + (overwrite), and removal should clean up the stored session. +- **Package export loadability.** Ensure an exported entrypoint's module format matches the + package `type` (no ESM `.js` behind a CommonJS package), and that everything a public + entrypoint imports is a real `dependency`/`peerDependency`, not a `devDependency`. + +### Dependencies / manifests +- Do not change dependencies, the root manifest, or the lockfile unless the owning package + actually needs it; flag spurious dependency additions and stray root `devDependencies`. +- Keep dependency bumps scoped to the intended package, and verify new deps satisfy the Node + engine in `.nvmrc` (e.g. a package requiring a newer Node than the repo targets). + ### Comments / docs / tests -- Comments explain **why**, not what; add a source link when porting logic from elsewhere. +- Comments explain **why**, not what; add a source link (permalink) when porting logic from + elsewhere. - Replace "what does `true` mean" comments with a typed parameter. - Factor pure logic (version comparison, parsing) into unit-testable functions; use test data that is not already sorted so sorting logic is actually exercised. +- Avoid tight wall-clock upper-bound assertions in timing tests (CI VMs stall); assert ordering + or use repeated samples if a bound is essential. +- Preserve original line endings and avoid whole-file rewrites / formatting-only churn that + bury the real change and pollute `git blame`. ## General correctness (every PR) From 8fe2a8f258a9b035ae1adc785b77e72079e67f3b Mon Sep 17 00:00:00 2001 From: Nathan Carlson Date: Fri, 17 Jul 2026 11:27:06 -0700 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=A4=96=20Trim=20review=20rubric=20to?= =?UTF-8?q?=20precision-first=20(drop=20style/noise=20rules)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noise-reduction pass validated by A/B backtest (v3 pre-cut vs this leaner version) over 6 held-out PRs plus a live demo on current open PRs: SME-validated recall held within noise while ~20% of finding volume was shed and false-positive rate stayed flat (~78% of emitted findings are genuine issues). - Remove the Declarations/style section (const/quotes/named-export/=== true) which contradicted the repo's mixed-style source and earned no catches. - Narrow async/await, TypeScript, localization, UI/UX, cross-platform and regex rules to concrete, demonstrable cases; drop copy-editing nits. - Hard-gate the imported-extensibility section to packages/** so it cannot fire noise on today's extensions/** PRs but is ready for the import PRs. - Scope dependencies/manifests review; stop reviewing lockfile lines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14b94f62-70f3-4e6d-8d2d-cd5a318a26ca --- .github/skills/code-review/rubric.md | 111 +++++++++++++-------------- 1 file changed, 54 insertions(+), 57 deletions(-) diff --git a/.github/skills/code-review/rubric.md b/.github/skills/code-review/rubric.md index 8777cbde..285f5c41 100644 --- a/.github/skills/code-review/rubric.md +++ b/.github/skills/code-review/rubric.md @@ -38,11 +38,11 @@ you are guessing, drop it or make it an open question. ## Higher scrutiny for agent-authored PRs If the PR is authored by an agent (title starts with an agent marker such as the robot emoji, -or the author is a bot like `Copilot` / `dependabot`), review with **higher scrutiny** -- -agents commonly over-engineer, duplicate existing helpers, hard-code strings that should be -localized, add unnecessary defensive code, or leave PR-narrative comments in source. For human -authors, give benefit of the doubt and **ask** when something looks odd but could be -intentional. +or the author is a bot like `Copilot` / `dependabot`), look **more carefully** at the known +agent failure modes -- over-engineering, duplicated helpers, hard-coded strings that should be +localized, unnecessary defensive code, PR-narrative comments left in source. This raises where +you look, not the bar: every finding still needs the same grounding. For human authors, give +benefit of the doubt and **ask** when something looks odd but could be intentional. ## Scope and intent (every PR) @@ -79,20 +79,18 @@ Apply these. Cite the concrete helper or convention the author should use. - **Every user-facing string must be localized** with `vscode.l10n.t('...')`. Flag any hard-coded visible string (messages, quick-pick labels/placeholders, notifications, errors, `package.json` contributions via `package.nls.json`). Legacy `localize(...)` is gone; new - code uses `vscode.l10n.t`. -- Message style: avoid contractions in user-facing text; keep trailing-period usage - consistent; prefer clear, full wording. + code uses `vscode.l10n.t`. Spell terms out in user-facing text (e.g. "Software Bill of + Materials", not "SBOM") where a maintainer has asked for it; skip routine grammar/punctuation + nits. ### Async / await -- Do not mark a function `async` if its body is synchronous. - Mixing `try/catch` with promises **without** `await` hides errors -- ensure rejections are - awaited inside the `try`. -- Intentional fire-and-forget must be explicit: prefix the call with `void` **and** a short - comment that you are deliberately not awaiting. -- `vscode.window.withProgress(...)` returns the inner callback's value -- await it instead of - stashing extra locals. -- After an `await`, re-check any guard state you relied on before it (reentrancy). -- Consider `Promise.all` for independent CLI calls. + awaited inside the `try` (a fire-and-forget call in a `try` will not be caught). +- Intentional fire-and-forget must be explicit: prefix the call with `void`, and make sure a + rejection cannot go unhandled (attach a handler or `await` in a `try/catch` when the call can + fail meaningfully). Do not demand an explanatory comment on every `void`. +- After an `await`, re-check any guard state you relied on before it -- but only when that state + is shared/mutable and can actually change across the await (reentrancy). ### Disposables / resource lifetime - `CancellationTokenSource`, `EventEmitter`, `createOutputChannel()`, event subscriptions, @@ -111,24 +109,16 @@ Apply these. Cite the concrete helper or convention the author should use. ### null / undefined - Choose `||` vs `??` deliberately: `||` treats `''` / `0` / `false` as "use default"; `??` only defaults on null/undefined. This matters for settings where empty string is a real - value. -- Guard array access on possibly-undefined values; a non-empty check often needs both a - defined check **and** `length > 0`. + value. Flag it when you can point to a legitimate falsy case the `||` mishandles. - Keep return types honest -- if a value can be undefined, the signature must be - `T | undefined`. -- Prefer real `undefined` over `''` / "(not set)" in telemetry; do not write explicit - default/`undefined` config into `tasks.json`. + `T | undefined`, and guard the resulting possibly-undefined access. ### TypeScript typing -- TS is **structurally** typed -- two interfaces with the same shape are interchangeable, so - distinct names alone do not prevent mix-ups. -- Prefer an `enum` or string-literal union over a bare `boolean` parameter so call sites are - self-documenting. -- Avoid `any`; keep public/extension API surface minimal (add options only when needed). - -### Declarations / style -- Prefer `const`; use `private readonly` on constructor parameter properties. -- Single quotes for strings; `export const X` over `export default`; avoid `=== true`. +- Prefer an `enum` or string-literal union over a bare `boolean` parameter **for an + exported/public API or an ambiguous call site** so callers are self-documenting; do not + demand it for a clear private helper. +- Avoid introducing new avoidable `any`; the command-wrapper signatures that legitimately use + `any[]` are an established boundary, not a target. ### Command-line / regex construction - Build container/CLI commands with the arg helpers from `@microsoft/vscode-processutils` @@ -137,14 +127,15 @@ Apply these. Cite the concrete helper or convention the author should use. Escaping differs per shell/OS (especially Windows); prefer `withQuotedArg` / `shouldQuote` over hand-rolled quoting. See `extensions/vscode-containers/src/tasks/netSdk/netSdkTaskUtils.ts` and `extensions/vscode-containers/src/debugging/netcore/*` for current usage. -- Regex: escape `.` as `\.`, anchor correctly, and watch for accidental matches elsewhere in a - path or command line. +- Regex: escape `.` as `\.`, anchor correctly, and watch for accidental matches -- flag one + when you can point to the concrete input it would mis-match, not merely because it looks + complex. ### UI / UX -- Prefer a QuickPick (shows all options at once) over multi-click "cycle" buttons. - Accessibility: refer to UI elements by name, not relative position ("the button on the - left") -- screen readers cannot rely on position. -- Avoid redundant toasts / progress notifications. + left") -- screen readers cannot rely on position. (Open question unless clearly wrong.) +- Flag a redundant toast / progress notification only when you can name the concrete duplicate + or misleading path (e.g. two popups for one failure). - Commands that should not appear in the Command Palette need a `when` clause / menu restriction in `package.json`. @@ -163,15 +154,19 @@ Apply these. Cite the concrete helper or convention the author should use. `DOCKER_HOST`). ### Cross-platform -- Prefer Node `os.platform()` / `os.arch()` over bespoke OS-provider abstractions. - Linux filesystems are case-sensitive: import path casing must match exactly or the Linux build breaks. -- Consider cmd, PowerShell, and git bash behavior on Windows. +- Container/CLI command construction must work across cmd, PowerShell, and git bash on Windows + -- lean on the `@microsoft/vscode-processutils` arg helpers (above) rather than hand-rolled + quoting. ### Contracts / client / registry packages (imported extensibility) -The monorepo is importing `vscode-docker-extensibility` (the `@microsoft/vscode-container-client` -and `@microsoft/vscode-docker-registries` packages) and `compose-language-service`. For changes -under those packages, apply these -- they are the patterns their maintainers raise most: +**Apply this section ONLY to changes under `packages/**`** -- the imported source of +`vscode-docker-extensibility` (`@microsoft/vscode-container-client`, +`@microsoft/vscode-docker-registries`) and `compose-language-service`, arriving via the open +import PRs. These do **not** apply to `extensions/**` consumer code, which merely depends on the +published packages. If `packages/**` is not in the diff, skip this section. For changes that +are under those packages, apply -- they are the patterns those maintainers raise most: - **Self-contained public contracts.** Define the types a contract exposes in the contract file; do not couple a public API to implementation-only types. Re-export intended public symbols from the package entrypoint, and share cross-cutting errors (e.g. `CancellationError`, @@ -200,21 +195,22 @@ under those packages, apply these -- they are the patterns their maintainers rai entrypoint imports is a real `dependency`/`peerDependency`, not a `devDependency`. ### Dependencies / manifests -- Do not change dependencies, the root manifest, or the lockfile unless the owning package - actually needs it; flag spurious dependency additions and stray root `devDependencies`. -- Keep dependency bumps scoped to the intended package, and verify new deps satisfy the Node - engine in `.nvmrc` (e.g. a package requiring a newer Node than the repo targets). +- Keep dependency changes scoped to the workspace that needs them: flag a dependency added to + the root manifest (or a stray root `devDependency`) when only one extension/package uses it. + Name the workspace that should own it. +- When a **changed** dependency declares its own engine requirement, check it against `.nvmrc`. + Do not review generated `pnpm-lock.yaml` entries line by line -- confirm the lockfile change + is the expected result of the manifest change. ### Comments / docs / tests -- Comments explain **why**, not what; add a source link (permalink) when porting logic from - elsewhere. -- Replace "what does `true` mean" comments with a typed parameter. -- Factor pure logic (version comparison, parsing) into unit-testable functions; use test data - that is not already sorted so sorting logic is actually exercised. -- Avoid tight wall-clock upper-bound assertions in timing tests (CI VMs stall); assert ordering - or use repeated samples if a bound is essential. -- Preserve original line endings and avoid whole-file rewrites / formatting-only churn that - bury the real change and pollute `git blame`. +- Comments should explain **why** for surprising / platform / protocol logic; add a source + permalink when porting logic from elsewhere. Do not ask for comments on obvious code. +- Factor pure logic (version comparison, parsing) into unit-testable functions; when a test + exercises sorting, use input that is not already sorted. +- In timing tests, avoid tight wall-clock upper-bound assertions (CI VMs stall); assert + ordering or use repeated samples if a bound is essential. +- Flag a genuine whole-file rewrite or line-ending conversion that buries the real change and + pollutes `git blame`; ignore ordinary formatter output. ## General correctness (every PR) @@ -249,9 +245,10 @@ Frame the review as: 1. **Verdict** -- one line: ready / needs changes / needs discussion. 2. **Blocking issues** -- correctness, resource, localization, or pattern violations that must be fixed, each with file:line and the concrete fix. -3. **Non-blocking suggestions** -- improvements and nits. -4. **What looks good** -- brief. -5. **Open questions** -- things to ask the author rather than assert. +3. **Non-blocking suggestions** -- improvements and nits worth making; omit if there are none. +4. **What looks good** -- optional, one line; skip it rather than pad the review. +5. **Open questions** -- material ambiguities to ask the author about rather than assert (not a + dumping ground for low-confidence findings). Never approve -- approval is a human maintainer's decision. A review either raises comments or requests changes; it does not sign off. From 010ce6fedc18771d6157e190741a7d2dd85bddb3 Mon Sep 17 00:00:00 2001 From: "Brandon Waterloo [MSFT]" <36966225+bwateratmsft@users.noreply.github.com> Date: Thu, 23 Jul 2026 14:07:19 -0400 Subject: [PATCH 4/4] Refine PR review skill guardrails Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe20b6f7-b994-413a-9ff1-3daae41f1225 --- .github/skills/code-review/SKILL.md | 7 ++++--- .github/skills/code-review/rubric.md | 9 +++++---- .github/skills/review-pr/SKILL.md | 14 +++++++------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 8b76d58b..a04b86f7 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -1,6 +1,6 @@ --- name: code-review -description: Review a pull request in this repository (the vscode-containers / Container Tools extension) against the maintainers' house style. Use this whenever reviewing a pull request or its changed files to check localization, async/resource correctness, TypeScript conventions, command-line construction, telemetry, settings, and cross-platform issues distilled from this repo's review history. +description: Review a pull request in this repository (the vscode-containers / Container Tools extension) when the automated reviewer has already supplied the diff and changed files. Checks localization, async/resource correctness, TypeScript conventions, command-line construction, telemetry, settings, and cross-platform issues distilled from this repo's review history. For an on-demand review by PR number, URL, or current branch, use review-pr instead. --- # Container Tools code review @@ -9,8 +9,9 @@ Apply the shared grading rubric in [`rubric.md`](./rubric.md) to the pull reques review. That file is the single source of truth for what to look for and how to grade it -- read it and follow it. -You already have the pull request's diff and changed files provided to you, and your review -comments are published through the normal review mechanism. So: +This skill is for the automated review context, where the pull request's diff and changed files +are already provided and review comments are published through the normal review mechanism. If +that context is not present, use `review-pr` instead. So: - **Do not** fetch the PR yourself or run `gh` -- work from the diff you were given. - **Do not** approve. Leave comments / request changes only; approval is a human decision. diff --git a/.github/skills/code-review/rubric.md b/.github/skills/code-review/rubric.md index 285f5c41..39591fc4 100644 --- a/.github/skills/code-review/rubric.md +++ b/.github/skills/code-review/rubric.md @@ -21,8 +21,10 @@ prioritize, then apply general engineering judgment for anything they do not cov Judge each finding on two axes and only surface what earns its place: -- **Severity**: blocking (correctness / resource / localization / clear pattern violation) vs - non-blocking (nit, style, optional improvement) vs open question (ask, do not assert). +- **Severity**: blocking (correctness / resource / localization / a pattern violation that + creates a concrete functional or maintenance risk) vs non-blocking (nit, style, optional + improvement) vs open question (ask, do not assert). A stated preference is not blocking by + itself; explain the concrete risk when grading a house-style violation as blocking. - **Confidence**: only raise a point you can ground in the diff or a concrete repo convention, and cite the exact helper/pattern the author should use instead. If you are guessing, downgrade it to an open question or drop it. @@ -231,8 +233,7 @@ builds as part of a review unless explicitly asked. - Alternative approaches that are merely different, not clearly better. - Missing XML/JSDoc unless it is public API or the surrounding code documents everything else. - Theoretical concerns that do not manifest in practice. -- Files that must not be hand-edited / reviewed line-by-line (generated or curated files) -- - see the repo's custom instructions. +- Do not review `NOTICE.html`; it is a curated artifact that must not be hand-edited. When you do propose an alternative, make it concrete (sketch the code / name the files) and say *why* it is better -- simpler, safer, faster, or more maintainable. "Did you consider X?" is diff --git a/.github/skills/review-pr/SKILL.md b/.github/skills/review-pr/SKILL.md index 84e9ef2d..07c36d90 100644 --- a/.github/skills/review-pr/SKILL.md +++ b/.github/skills/review-pr/SKILL.md @@ -1,7 +1,6 @@ --- name: review-pr description: Review a specific vscode-containers pull request on demand from the CLI (or any interactive agent), the way a Container Tools maintainer would. Use when asked to review a PR by number, URL, or "the current branch", to produce a written review and, only when explicitly asked, post comments on the PR. Fetches the PR with the GitHub CLI and applies the shared review rubric. -allowed-tools: shell --- # Review a Container Tools PR (on demand) @@ -46,8 +45,8 @@ If the PR references an issue, read it (`gh issue view `) to confirm the PR a If the PR is already **merged or closed**, do a retrospective review in report mode only -- do not try to post change requests on it. -(Equivalent GitHub MCP Server tools -- `get_pull_request`, `get_pull_request_diff`, -`get_pull_request_files`, `get_pull_request_comments` -- work too if `gh` is unavailable.) +If `gh` is unavailable, use the equivalent read-only GitHub tools exposed by the current host; +tool names vary between hosts. ## Produce the review @@ -59,16 +58,17 @@ concrete repo convention and cite the helper/pattern to use. Keep each comment specific and actionable; reference the exact file/line. -- Inline comment on a line: +- Prefer one summary review so comments are submitted together. Use inline comments only when + the user explicitly asks for them; the REST endpoint below posts each comment immediately. +- Inline comment on a changed line (GitHub rejects lines outside the PR diff): `gh api repos/microsoft/vscode-containers/pulls//comments -f body='...' -f commit_id='' -f path='' -F line= -f side=RIGHT` - Summary review (comment, or request changes when the author must act -- never approve): `gh pr review --comment --body '...'` / `gh pr review --request-changes --body '...'` - Prefix each agent-posted review comment with a clear reviewer marker (e.g. a magnifying glass or detective emoji) so it is distinguishable from a human review. -(Equivalent GitHub MCP Server tools: `create_pending_pull_request_review` -> -`add_comment_to_pending_review` -> `submit_pending_pull_request_review`, or -`create_and_submit_pull_request_review` for a summary-only review.) +When using host-provided GitHub tools, prefer their pending-review workflow so inline comments +are submitted as one review. ## Rules