Pr Readiness

PR Readiness

PR Requirements Checklist

1. PR Title

The title must clearly and concisely describe the change from the user's perspective. It should:

• Start with a verb (e.g., "Fix", "Add", "Improve", "Remove", "Update").
• Mention the affected feature or area (e.g., presets, kits, CTest, build tasks, Project Outline).
• Be specific enough that a reader understands the change without opening the PR.

Good examples:

• Fix preset reloading loop when preset files are symlinks
• Add "Delete Build Directory and Reconfigure" command
• Improve CTest test ordering to match Test Explorer display

Bad examples:

• Fix bug (too vague)
• Update code (no useful information)
• WIP (not ready for review)

2. PR Description

The PR body must include:

• What changed: A short summary of the user-visible behavior change.
• Why: The motivation — link to a GitHub issue if one exists (e.g., Fixes #1234).
• How (if non-obvious): A brief explanation of the implementation approach when the change is complex.

3. CHANGELOG Entry

Every PR must add an entry to CHANGELOG.md.

Where to insert

Insert the entry under the most recent (topmost) version heading in CHANGELOG.md. The first version heading looks like ## <version> (e.g., ## 1.23). Always add the new entry at the bottom of the appropriate section (i.e., after all existing entries in that section).

Which section

Place the entry in exactly one of these three sections, creating the section if it does not already exist under the current version:

| Section | Use when… |

|---|---|

| Features: | A new user-visible capability is added (new command, new setting, new UI element). |

| Improvements: | An existing feature is enhanced, optimized, or has better UX — but no new capability is introduced. |

| Bug Fixes: | A defect is corrected. |

The sections appear in this fixed order: Features:, then Improvements:, then Bug Fixes:.

Entry format

Each entry follows this pattern:

- <Description>. [#<number>](<link>)

Where <Description> starts with a present-tense verb describing the user-visible change, and the link references either:

• The GitHub issue it solves: #<issue number>
• Or the PR itself: #<pr number>

An entry may optionally credit an external contributor at the end: @user.

Examples:

Features:
- Add "Delete Build Directory and Reconfigure" command that removes the entire build directory before reconfiguring, ensuring a completely clean state. [#4826](https://github.com/microsoft/vscode-cmake-tools/pull/4826)

Improvements:
- Run tests sequentially in alphabetical order (matching the Test Explorer display order) when `cmake.ctest.allowParallelJobs` is disabled. [#4829](https://github.com/microsoft/vscode-cmake-tools/issues/4829)

Bug Fixes:
- Fix `cmake.revealLog` set to `"focus"` not revealing the output panel or stealing focus. [#4471](https://github.com/microsoft/vscode-cmake-tools/issues/4471)
- Fix garbled characters in the Output panel when MSVC outputs UTF-8 on non-UTF-8 Windows systems. [#4520](https://github.com/microsoft/vscode-cmake-tools/issues/4520) [@contributor](https://github.com/contributor)

What NOT to do

• Do not add a new version heading — use the existing topmost one.
• Do not place the entry under an older version.
• Do not use past tense (write "Fix …", not "Fixed …").
• Do not omit the issue or PR link.

4. Correctness

Review the code changes for logical correctness:

• Both operating modes: If the change touches shared logic (configure, build, test, targets, environment), verify it handles both *presets mode* and *kits/variants mode*. Check for useCMakePresets branching where appropriate.
• Both generator types: If the change involves build-type logic, verify it handles both single-config generators (CMAKE_BUILD_TYPE at configure time) and multi-config generators (--config at build time).
• Edge cases: Look for off-by-one errors, null/undefined access, missing await on async calls, and unhandled promise rejections.
• Error handling: Verify errors are not silently swallowed. Top-level event handlers should use rollbar.invokeAsync() / rollbar.invoke(). Empty catch blocks are a red flag.
• Cross-platform: Check for hardcoded path separators (/ or \\), case-sensitive env var assumptions, or platform-specific APIs used without guards. Paths must use path.join() / path.normalize().

5. Regression Risks

Identify areas where the change could break existing behavior:

• Shared utilities: Changes to src/expand.ts, src/proc.ts, src/shlex.ts, or src/util.ts affect many callers — verify all call sites still behave correctly.
• Driver base class: Changes to cmakeDriver.ts propagate to cmakeFileApiDriver.ts, cmakeLegacyDriver.ts, and cmakeServerDriver.ts. Check that subclass overrides are still compatible.
• Preset merging: Changes to presetsController.ts or presetsParser.ts can alter how presets resolve — verify with nested include chains and CMakeUserPresets.json overrides.
• Settings: Adding or renaming a setting in package.json without updating src/config.ts (or vice versa) causes silent failures.
• Task provider: Changes to cmakeTaskProvider.ts can break tasks.json definitions that users have already configured.
• Public API / extensibility: Changes to exports in src/api.ts or types in EXTENSIBILITY.md can break dependent extensions.
• Test coverage: Flag changes to critical paths that lack corresponding test updates, especially in src/drivers/, src/presets/, and src/kits/. See also section 7 (Test Coverage) for detailed test review guidance.

6. Adherence to Existing Patterns

Verify the change follows the project's established conventions:

• Import style: Uses @cmt/* path aliases (not relative paths from outside src/). Uses import * as nls from 'vscode-nls' for localization.
• Logging: Uses logging.createLogger('module-name') — never console.log.
• Localization: All user-visible strings use localize('message.key', 'Message text') with the vscode-nls boilerplate at the top of the file.
• Settings access: Reads settings through ConfigurationReader (src/config.ts) — never calls vscode.workspace.getConfiguration() directly.
• Telemetry: Uses helpers from src/telemetry.ts — never calls the VS Code telemetry API directly.
• Data access: Uses canonical data paths (e.g., CMakeProject.targets for targets, CMakeDriver.cmakeCacheEntries for cache) — never parses CMake files or cache directly.
• Async patterns: Prefers async/await over .then() chains (exception: fire-and-forget UI calls).
• Naming and structure: New files are placed in the correct layer directory (see architecture table in .github/copilot-instructions.md). New commands are registered in src/extension.ts.

7. Test Coverage

Verify that the PR includes adequate tests for the changes:

• New functionality: Any new function, command, setting, or behavior branch should have corresponding tests. If the change adds a new helper or utility, check that it has unit tests covering its inputs and edge cases.
• Bug fixes: A bug fix should include at least one test that would have caught the original bug — i.e., a test that fails on the base branch and passes with the fix applied.
• Changed behavior: If existing behavior is modified, check that existing tests are updated to reflect the new behavior and that no tests are silently broken or deleted without justification.
• Test location: Unit tests that don't require VS Code APIs belong in test/unit-tests/backend/ and run via yarn backendTests. Tests that require VS Code belong in test/unit-tests/ and run via yarn unitTests. Check that new tests are placed in the correct location.
• Test quality: Tests should be specific and descriptive:
• Test names should clearly state what is being tested and the expected outcome.
• Tests should assert specific values, not just "no error thrown".
• Tests should cover both positive cases (expected input → expected output) and negative/edge cases (invalid input, boundary conditions, fallback paths).
• Comments in tests should be accurate and not misleading about what the test actually exercises.
• Coverage gaps to flag: Pay special attention to:
• Code paths that touch src/drivers/, src/presets/, or src/kits/ — these are critical and should have test coverage for any non-trivial changes.
• Fallback/error paths — if the PR adds a fallback (e.g., "if X fails, try Y"), both the success and fallback paths should be tested.
• Platform-specific branches — if the code branches on process.platform, each branch should ideally be covered.
• When tests are not feasible: Some changes (e.g., pure UI wiring, VS Code API integration) are difficult to unit test. In these cases, verify that the manual-checklist or PR description explains how to manually verify the change, and flag the gap rather than ignoring it.

8. Documentation Updates

Check whether the change requires documentation updates:

• New or changed settings: Must be reflected in all three locations — package.json (contributes.configuration), src/config.ts (ConfigurationReader), and docs/cmake-settings.md.
• New commands: Must be documented in package.json (contributes.commands) and referenced in the relevant docs page under docs/.
• User-visible behavior changes: If the change alters how a feature works (not just fixes a bug), check whether docs/ pages describing that feature need updating.
• Extensibility changes: If src/api.ts or public types change, update EXTENSIBILITY.md.
• README: If a new major feature is added, check whether README.md should mention it.

Applying This Skill

When reviewing or preparing a PR:

• Check the title — rewrite it if it is vague or missing context.
• Check the description — ensure it explains what, why, and (if needed) how.
• Check CHANGELOG.md — verify an entry exists under the current version in the correct section with the correct format. If missing, add one.
• Check correctness — review code for logical errors, missing mode/generator handling, cross-platform issues, and error handling gaps.
• Check regression risks — identify areas where the change could break existing behavior and flag missing test coverage for critical paths.
• Check pattern adherence — verify the change follows established import, logging, localization, settings access, and architectural conventions.
• Check test coverage — verify that new or changed behavior has corresponding tests, bug fixes include regression tests, and tests are placed in the correct location with good quality.
• Check documentation — verify that new or changed settings, commands, and behavior are reflected in the appropriate docs.