diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md new file mode 100644 index 00000000..a04b86f7 --- /dev/null +++ b/.github/skills/code-review/SKILL.md @@ -0,0 +1,25 @@ +--- +name: code-review +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 + +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. + +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. +- 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..39591fc4 --- /dev/null +++ b/.github/skills/code-review/rubric.md @@ -0,0 +1,255 @@ +# 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 / 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. + +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 + +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`), 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) + +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. + +### 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, + `package.json` contributions via `package.nls.json`). Legacy `localize(...)` is gone; new + 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 +- Mixing `try/catch` with promises **without** `await` hides errors -- ensure rejections are + 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, + 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. 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`, and guard the resulting possibly-undefined access. + +### TypeScript typing +- 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` + (`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 -- flag one + when you can point to the concrete input it would mis-match, not merely because it looks + complex. + +### UI / UX +- Accessibility: refer to UI elements by name, not relative position ("the button on the + 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`. + +### 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 +- Linux filesystems are case-sensitive: import path casing must match exactly or the Linux + build breaks. +- 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) +**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`, + `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 +- 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 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) + +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. +- 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 +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 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. diff --git a/.github/skills/review-pr/SKILL.md b/.github/skills/review-pr/SKILL.md new file mode 100644 index 00000000..07c36d90 --- /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. +--- + +# 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. + +If `gh` is unavailable, use the equivalent read-only GitHub tools exposed by the current host; +tool names vary between hosts. + +## 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. + +- 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. + +When using host-provided GitHub tools, prefer their pending-review workflow so inline comments +are submitted as one 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.