From b4fd8dfd248f7ca634f379e99656c65017c43195 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Thu, 23 Jul 2026 16:03:38 -0700 Subject: [PATCH 01/10] Add code review instructions based on PR feedback patterns Analyzed review comments across 30+ PRs spanning Dec 2025 - Jul 2026 to extract recurring patterns and repo-specific best practices for code reviews: - General: avoid false-positive compile claims, respect test harness vs production distinction, recognize auto-generated files - Rust: cache keys, API visibility, fail-closed security, Windows FFI/COM safety, DLL loading security, what-if correctness, Option return types, schema coherence - Pester tests: cross-platform paths, env var isolation, ACL cleanup, exit code assertions, array comparisons, event subscriber cleanup, cmdlet skip guards - PowerShell adapters: module array returns, $using: in parallel blocks, terminating vs non-terminating error distinction - Design: settings precedence, backward compatibility, semver for resources, canonical property naming conventions, noFiltering semantics - CI/CD: fork permissions, conditional tool install, PS naming conventions - Docs: remove debug prints, locale string accuracy, dead i18n keys Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../instructions/code-review.instructions.md | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 .github/instructions/code-review.instructions.md diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md new file mode 100644 index 000000000..6903c55b1 --- /dev/null +++ b/.github/instructions/code-review.instructions.md @@ -0,0 +1,137 @@ +--- +applyTo: "**/*.rs, **/*.ps1, **/*.psm1, **/*.tests.ps1, **/*.json" +--- + +# Code Review Instructions for DSC Repository + +These instructions guide Copilot when performing code reviews on pull requests in this repository. +Focus on high-confidence, actionable findings. Do not comment on style, formatting, or trivial issues. + +## General Principles + +- **Do not flag issues that are intentional or already validated by CI**: If the code compiles and tests pass, do not claim it "will not compile" or "will fail." Verify your claim against actual Rust ownership/borrowing semantics before asserting a compile error. +- **Test resources vs production resources**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not apply production-quality error handling requirements (like replacing `expect()` with graceful errors) to test harnesses. +- **Automatically-generated files**: Files like `.versions.json` are updated by build automation. Do not flag version bumps in these files as unintentional unless the PR description explicitly contradicts them. + +## Rust Code Patterns + +### Caching and State Management + +- **Cache key correctness**: When caching by resource type/version, verify the cache key accounts for all dimensions that affect the cached value. For adapter resources with a `target_resource`, the cache key must include the target resource identity, not just the adapter's type/version. +- **Duplicate caching**: Watch for cache writes that duplicate logic already handled elsewhere. `BTreeMap::extend` overwrites existing entries — if multiple versions of a resource should coexist in the cache, use entry-based insertion instead. +- **Unused imports after refactoring**: When cache writes or other logic are centralized, verify that moved-from modules no longer import the now-unused symbols. + +### API Surface and Visibility + +- **Minimize `pub` exposure**: Internal helpers, submodules, and filter functions used only within their parent module should be `pub(crate)` or private, not `pub`. Expanding the public API surface creates long-term maintenance burden. +- **Breaking changes to public APIs**: Adding required parameters to public functions is a breaking change. Consider backward-compatible alternatives (default parameters, builder pattern, or a new function name). +- **Prefer extracting shared logic**: When the same logic appears in multiple subcommands (e.g., table formatting, resource listing), extract it into a helper function rather than duplicating code. However, large refactors (like introducing new traits) should be separate PRs. + +### Error Handling and Security + +- **Fail closed for security checks**: Security-sensitive functions (ACL verification, permission checks) must treat failures as "not secure." If reading a security descriptor, enumerating ACEs, or calling stat fails, return the restrictive/denied result — never fail open. +- **NULL DACL detection**: A NULL DACL means full access to everyone. Security checks must treat `p_dacl.is_null()` as insecure. +- **Complete permission checks**: Write-access checks must cover `GENERIC_WRITE` and `GENERIC_ALL` in addition to specific write flags. Also consider inherit-only ACEs that don't apply to the object itself. +- **Error promotion**: Converting a previously-ignored error into a hard error (e.g., `join_paths` failure) can break existing environments. Prefer warnings or graceful degradation unless the error is truly unrecoverable. +- **Return `Option` for fallible lookups**: When a function can legitimately fail to find a path or value, return `Option` (or similar) rather than an empty string, which can resolve to the current directory and cause confusing downstream behavior. + +### Windows FFI and COM Safety + +- **DLL loading security**: When loading system DLLs via `LoadLibraryW`, use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` to prevent DLL preloading/hijacking attacks. This is especially important for resources that run with elevated privileges. +- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and library handles (`HMODULE`) must be cleaned up even on error paths. Implement `Drop` for wrapper types or use RAII patterns to ensure `FreeLibrary`, `VariantClear`, etc. are always called. +- **Check HRESULT returns**: Windows API functions that return `HRESULT` (like `VariantClear`) should have their return values checked or at minimum logged, not silently ignored. +- **Iterative FFI operations**: When creating nested structures (e.g., registry keys path-by-path), ensure each iteration uses the result of the previous call as the parent handle, not always the root handle. + +### Serialization and Consistency + +- **Deterministic output**: If a function returns JSON, ensure the format (compact vs pretty) is consistent regardless of cache state. A cache hit should not produce differently-formatted output than a cache miss. +- **Avoid unnecessary serialize/parse roundtrips**: If you already have a `serde_json::Value`, cache or return it directly rather than serializing to string and re-parsing. +- **Schema/manifest version consistency**: When bumping a version in `Cargo.toml`, ensure the corresponding resource manifest `.dsc.resource.json` file is also updated to match. + +### Concurrency + +- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue` — `IsEmpty` is only an approximation under concurrency. Use `while queue.TryDequeue(...)` as the single loop condition. + +### What-If / Dry-Run Correctness + +- **No side effects during what-if**: When implementing `--what-if` mode, verify that no code path before the what-if gate can mutate state. Functions like `ensure_config_exists()` that create/copy files must be gated or skipped in what-if mode. +- **Consistent platform enforcement in what-if**: If an operation is platform-restricted (e.g., Windows-only), what-if mode must enforce the same restriction. Do not allow what-if to succeed on unsupported platforms. + +## PowerShell / Pester Test Patterns + +### Cross-Platform Correctness + +- **Path separators**: Never use hard-coded `\` in path construction. Always use `Join-Path` or `[System.IO.Path]::Combine()`. Tests with hard-coded backslashes will fail on Linux/macOS. +- **Platform-specific commands**: `stat -c` is GNU/Linux-specific. If a test should run on macOS as well, use PowerShell's `Get-Item` or gate the context on `$IsLinux` explicitly. +- **OS gating**: A Context labeled "Linux" that only checks `!$IsWindows` will also run on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. + +### Test Isolation and Cleanup + +- **Preserve and restore environment variables**: When tests modify `$env:` variables (e.g., `DSC_RESTRICTED_PATH`, `DSC_RESOURCE_PATH`), always capture the original value in `BeforeAll` and restore it in `AfterAll`. Setting to `$null` unconditionally can destroy pre-existing values. +- **Conflicting environment variables**: If one env var takes precedence over another (e.g., `DSC_RESTRICTED_PATH` over `DSC_RESOURCE_PATH`), tests for the lower-priority var must explicitly clear the higher-priority one. +- **ACL and permission restoration**: When tests modify filesystem ACLs or permissions, capture the full original state and restore it completely. Do not rely on partial undo (e.g., removing a single ACE) or hard-coded permission values like `755`. +- **Recursive ACL changes**: If `icacls /T` is used to recursively modify ACLs, `AfterAll` must restore children as well, not just the top-level directory. +- **Event subscriber cleanup**: When using `Register-ObjectEvent`, clean up only the subscribers you created (filter by `-SourceIdentifier`), not all subscribers in the session via blanket `Get-EventSubscriber | Unregister-Event`. + +### Test Assertions and Naming + +- **Test name must match assertions**: If a test is named "X happens only once," the assertions must verify the "only once" constraint — not just that X happened at least once. Either tighten assertions or rename the test. +- **Ordering assumptions**: `dsc resource list` returns items in alphabetical order. Tests should document this assumption or sort results before asserting on specific positions. +- **Test logic correctness**: Verify that test conditions actually exercise the code path under test. A condition that always fails regardless of the variable being tested is a no-op test. +- **Assert `$LASTEXITCODE`**: When testing CLI commands, assert `$LASTEXITCODE -eq 0` (or the expected exit code) in addition to output checks. A non-zero exit can go unnoticed if only output content is validated. +- **Avoid duplicate reads**: When asserting on file content, read it once into a variable rather than calling `Get-Content` multiple times (once for the assertion, again for `-Because`). +- **Array comparisons**: `Should -Be` can be unreliable for array comparisons. Normalize arrays through JSON conversion before comparing, or compare individual elements. +- **Skip guards for cmdlet availability**: Tests that depend on platform-specific cmdlets (e.g., `Get-NetFirewallRule`) should check cmdlet availability in their `-Skip` condition, not just elevation status. + +### Test Structure and Readability + +- **Use Context blocks for shared setup**: When multiple test cases share setup steps (e.g., found vs. not-found scenarios), use a Pester `Context` block to group and clarify shared setup. +- **Separation between sections**: Maintain blank lines between logical sections in configuration and test files for readability. +- **Helpers belong near their usage**: Helper functions used only in a specific test file should live in that file. Only promote to the shared helper module (`build.helpers.psm1`) if used across multiple scripts. + +## PowerShell Adapter Patterns + +- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects when multiple versions are loaded. Always select a single module (e.g., highest version) before calling methods on it. +- **Error handling around module imports**: When probing for DSC resource classes via module import (e.g., during cache refresh), wrap the import in try/catch so that one failing module doesn't abort the entire enumeration. +- **`$using:` in parallel blocks**: Variables from the parent scope are not automatically available inside `ForEach-Object -Parallel` script blocks. Use `$using:variableName` to capture them. +- **Distinguish terminating vs non-terminating errors**: Non-terminating errors written to the Error stream are counted in `$ps.HadErrors` but should not cause the adapter to report failure. Only terminating errors (exceptions) should produce a non-zero exit code. + +## Design and Architecture Patterns + +### Settings and Configuration Precedence + +- **Full precedence chain**: Settings resolution must account for all scopes: Policy > CommandLine > Environment > Workspace > User > Install. Missing scopes in the chain can allow lower-precedence values to bypass higher-precedence ones. +- **Resolved fields vs containers**: When implementing settings resolution, each leaf setting should be individually resolvable (with its source scope tracked), not just the container as a whole. +- **`allow_override` enforcement**: When a setting's `allow_override` is false (policy-scoped), ensure the code path actually prevents override from env vars and CLI args — not just one of those sources. + +### Backward Compatibility + +- **Breaking test expectations**: If a change makes a previously-optional directive required, existing tests must still pass as-is. Behavior that differs from what tests already validate is a potential breaking change that needs working group discussion. +- **Resource filtering logic**: When engine-side filtering is added alongside resource-side filtering, the logic should be: (1) if the resource supports filtering, let it handle it; (2) if not, use engine filtering with an INFO-level message. +- **Semantic versioning for resources**: Resources below 1.0 (e.g., `0.x.y`) are not design-stable. For breaking changes to resource input/output shapes, bump the minor version. Reserve 1.x for stable designs. + +### Regex and Wildcard Handling + +- **Escape all metacharacters**: When converting user-supplied wildcard patterns to regex, escape all regex metacharacters (`[`, `(`, `+`, `^`, `$`, `|`, `\`, etc.), not just `.`. Unescaped metacharacters can cause panics or unexpected matching behavior. + +### Resource Manifest and Schema Coherence + +- **Schema must match implementation**: If a resource manifest's embedded schema declares a property as required, the resource implementation must enforce that requirement. Conversely, if the implementation requires a field, the schema's `required` array must include it. +- **Export schema validation**: When `export.schema` is defined, the engine uses it for input validation. Ensure the schema is strict enough that invalid inputs are caught before reaching the resource. +- **`noFiltering` semantics**: When a resource declares `export.schema: noFiltering`, the export input should be treated as empty (no filter properties passed to the resource). +- **Naming for DSC-specific conventions**: Use leading underscore (`_`) only for canonical (cross-resource) properties. Resource-specific properties that might collide with future keywords should use descriptive names (e.g., `sshd_config_filepath`) and be discussed in the working group if elevation to canonical status is warranted. + +### CI/CD and GitHub Actions + +- **Fork permission limitations**: `GITHUB_TOKEN` on fork PRs is typically read-only even with `pull-requests: write`. Steps that post PR comments should be gated to same-repo PRs or use `continue-on-error: true`. +- **Conditional tool installation**: Install expensive tools (Rust toolchains, coverage tools, protoc) only after determining the PR actually needs them (e.g., after checking which files changed). +- **PowerShell conventions in build scripts**: Functions should be singular per PS convention (e.g., `Test-RustProject` not `Test-RustProjects`). Build helper failures should throw so the pipeline stops on error. + +## Documentation and Logging + +- **Accurate comments**: If code behavior changes, update comments to match. A comment that says "retains quoting" when the code actually strips quotes is misleading. +- **Log level appropriateness**: Full `PATH` contents should be `trace!` level, not `debug!`. Sensitive or extremely verbose data should use the lowest practical log level. +- **Doc comments matching implementation**: If a doc comment describes behavior that the implementation doesn't actually enforce (e.g., "controls both env var and CLI option" when only env var is gated), update the comment to match reality. +- **Remove debug print statements**: `println!` debug output must not be left in production or test code. Use `debug!`/`trace!` macros from the tracing crate for diagnostics, or remove entirely before merge. +- **Locale/i18n string accuracy**: When adding localized strings, verify the key name matches the keyword/function it describes. Copy-paste errors in locale files (e.g., wrong keyword name in the error message) are common and hard to catch later. +- **Dead locale strings**: Do not add i18n keys that are never referenced in code. Unused locale strings accumulate and make it harder to identify which messages are active. From d2be2160a1adb02ea3b6bfd524ddbdbfdac96de2 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Thu, 23 Jul 2026 17:52:52 -0700 Subject: [PATCH 02/10] Add comprehensive code review instructions from 455 PRs Extracted and analyzed 2,733 review comments across all 455 PRs with reviews in this repository (Dec 2025 - Jul 2026). Used parallel analysis agents to synthesize patterns by theme: security, performance, testing, maintainability, and manifest/schema coherence. Key sections: - False-positive avoidance (patterns Copilot should NOT flag) - Security (fail-closed checks, DLL hijacking, policy bypass, secret leaks) - Rust patterns (caching, API visibility, error handling, FFI safety, what-if) - Pester testing (cross-platform, isolation, assertions, structure) - PowerShell adapters (module arrays, parallel scoping, error types) - Design (settings precedence, backward compat, schema coherence) - CI/CD (fork permissions, artifact contracts, control flow) - Documentation (comments, logging, i18n accuracy) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../instructions/code-review.instructions.md | 191 +++++++++++------- 1 file changed, 115 insertions(+), 76 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 6903c55b1..6e8ecd3e1 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -1,137 +1,176 @@ --- -applyTo: "**/*.rs, **/*.ps1, **/*.psm1, **/*.tests.ps1, **/*.json" +applyTo: '**/*.rs,**/*.ps1,**/*.psm1,**/*.tests.ps1,**/*.json' +description: 'Repository-specific code review guidance for DSC (Rust, PowerShell/Pester, JSON)' --- # Code Review Instructions for DSC Repository These instructions guide Copilot when performing code reviews on pull requests in this repository. Focus on high-confidence, actionable findings. Do not comment on style, formatting, or trivial issues. +Derived from analysis of 2,700+ review comments across 455 PRs in this repository. -## General Principles +## General Principles (False-Positive Avoidance) -- **Do not flag issues that are intentional or already validated by CI**: If the code compiles and tests pass, do not claim it "will not compile" or "will fail." Verify your claim against actual Rust ownership/borrowing semantics before asserting a compile error. -- **Test resources vs production resources**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not apply production-quality error handling requirements (like replacing `expect()` with graceful errors) to test harnesses. -- **Automatically-generated files**: Files like `.versions.json` are updated by build automation. Do not flag version bumps in these files as unintentional unless the PR description explicitly contradicts them. +- **Do not claim code will not compile unless you are certain**: Multiple reviews were rejected because Copilot incorrectly claimed Rust ownership/borrowing errors. If the code compiles and tests pass in CI, do not assert otherwise. +- **Test resources are not production code**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not require production-grade error handling (replacing `expect()` with `Result`) in test harnesses unless panics would hide regressions. +- **Automatically-generated files**: Files like `lib/dsc-lib-jsonschema/.versions.json` are updated by build automation. Do not flag version bumps as unintentional. +- **Do not demand large abstractions in small PRs**: If duplicated logic exists, suggest extracting a helper function. Do not block a focused PR by requesting a full trait/framework redesign -- that is follow-up work. +- **Table output is not a stable API**: JSON/YAML are the canonical machine-readable outputs. Do not frame table column/layout changes as breaking changes. +- **Separate input/output structs can be intentional**: Resources often distinguish desired state input from observed state output. Do not assume duplicate structs are accidental. +- **Intentional design decisions**: When maintainers explicitly label behavior as "intentional" (e.g., PATH mutation behavior, error promotion choices), do not re-flag. Respect the context of intentional design. + +## Security + +- **Fail closed on all security checks**: Security-sensitive functions (ACL verification, permission checks, policy folder validation) must treat failures as "not secure." If reading a security descriptor, enumerating ACEs, or calling stat fails, return the restrictive/denied result -- never fail open. +- **NULL DACL detection**: A NULL DACL means full access to everyone. Security checks must treat `p_dacl.is_null()` as insecure. +- **Complete permission checks**: Write-access checks must cover `GENERIC_WRITE` and `GENERIC_ALL` in addition to specific write flags. Also consider inherit-only ACEs (which don't apply to the object itself) and non-standard ACE variants (callback ACEs can still grant access). +- **Do not let user-controlled inputs bypass policy**: CLI flags, env vars, or config precedence changes must not override policy-enforced settings. Policy sources should remain authoritative even when user convenience switches are added. +- **DLL loading security**: When loading system DLLs via `LoadLibraryW`, use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` to prevent DLL preloading/hijacking. Especially important for resources running elevated (DISM, offline registry, services). +- **Prevent command injection via string interpolation**: Flag code that embeds user-controlled values (secrets, vault names, paths) into PowerShell command strings. Prefer passing arguments/JSON rather than building quoted script fragments. +- **Never leak secrets into logs or output**: Verify redaction is consistently applied when logging JSON/resource output that may contain secure values. Review "show secrets" paths to ensure behavior is explicit. +- **Enforce declared security context at runtime**: If manifests declare `requireSecurityContext`, verify runtime enforcement exists for every operation (`get`, `set`, `test`, `export`). A schema field without enforcement is a security bug. +- **Do not cache or trust failed verifications**: Trust caches (e.g., Authenticode checks) should only be updated on successful validation, never on failure. +- **Watch for destructive modes with unresolvable system objects**: In list-reconciling resources (firewall, services), flag logic that disables/removes system-created entries (AppX/UWP rules) that users cannot reliably reference in their declared state. ## Rust Code Patterns -### Caching and State Management +### Caching and Performance -- **Cache key correctness**: When caching by resource type/version, verify the cache key accounts for all dimensions that affect the cached value. For adapter resources with a `target_resource`, the cache key must include the target resource identity, not just the adapter's type/version. -- **Duplicate caching**: Watch for cache writes that duplicate logic already handled elsewhere. `BTreeMap::extend` overwrites existing entries — if multiple versions of a resource should coexist in the cache, use entry-based insertion instead. -- **Unused imports after refactoring**: When cache writes or other logic are centralized, verify that moved-from modules no longer import the now-unused symbols. +- **Cache key correctness**: Cache keys must account for all dimensions affecting the cached value. For adapter resources with `target_resource`, cache by target identity, not just adapter type/version. +- **Centralize cache logic**: Flag duplicate cache writes or wrapper layers that both cache the same result. A single cache owner reduces redundant work and prevents divergent keying. +- **Avoid JSON serialize/parse roundtrips**: If you already have a `serde_json::Value`, cache or return it directly rather than stringifying and re-parsing. +- **Short-circuit redundant work**: Flag loops that keep scanning after all matches are found, or `contains_key` + `insert` patterns where entry APIs would do one pass. +- **Reduce allocations only when semantics stay intact**: Good flags: repeated `to_lowercase()`, cloning owned data that can be borrowed. Bad flags: clone removal that changes ownership semantics or breaks compilation. When in doubt, do not flag. ### API Surface and Visibility -- **Minimize `pub` exposure**: Internal helpers, submodules, and filter functions used only within their parent module should be `pub(crate)` or private, not `pub`. Expanding the public API surface creates long-term maintenance burden. -- **Breaking changes to public APIs**: Adding required parameters to public functions is a breaking change. Consider backward-compatible alternatives (default parameters, builder pattern, or a new function name). -- **Prefer extracting shared logic**: When the same logic appears in multiple subcommands (e.g., table formatting, resource listing), extract it into a helper function rather than duplicating code. However, large refactors (like introducing new traits) should be separate PRs. +- **Default to private/`pub(crate)`**: New modules, helpers, globals, and cache types should be private or `pub(crate)`. Require a clear external use case before exposing `pub`. +- **Breaking changes to public APIs**: Adding required parameters to public functions is a breaking change. Consider backward-compatible wrappers. +- **Extract shared logic into helpers**: When the same logic appears in multiple subcommands, extract a helper. But do not demand full trait redesigns in focused PRs. +- **Remove dead code immediately**: Unused imports, variables, and enums should be deleted when a refactor makes them unnecessary -- Rust warnings become CI failures. -### Error Handling and Security +### Error Handling -- **Fail closed for security checks**: Security-sensitive functions (ACL verification, permission checks) must treat failures as "not secure." If reading a security descriptor, enumerating ACEs, or calling stat fails, return the restrictive/denied result — never fail open. -- **NULL DACL detection**: A NULL DACL means full access to everyone. Security checks must treat `p_dacl.is_null()` as insecure. -- **Complete permission checks**: Write-access checks must cover `GENERIC_WRITE` and `GENERIC_ALL` in addition to specific write flags. Also consider inherit-only ACEs that don't apply to the object itself. -- **Error promotion**: Converting a previously-ignored error into a hard error (e.g., `join_paths` failure) can break existing environments. Prefer warnings or graceful degradation unless the error is truly unrecoverable. -- **Return `Option` for fallible lookups**: When a function can legitimately fail to find a path or value, return `Option` (or similar) rather than an empty string, which can resolve to the current directory and cause confusing downstream behavior. +- **Prefer `Result` over panics for user input**: Avoid `unwrap()`/`expect()` on user input, manifest data, regex creation, or serialization. Return clear `Result` or stable exit codes. (Exception: test harness code in `tools/dsctest/`.) +- **Error promotion risk**: Converting previously-ignored errors into hard errors can break existing environments. Prefer warnings or graceful degradation unless truly unrecoverable. +- **Return `Option` for fallible lookups**: When a function can legitimately fail to find a path or value, return `Option` rather than an empty string (which resolves to `.`). +- **`unwrap_or_default()` on serialization is a bug**: `serde_json::to_string(...).unwrap_or_default()` silently emits empty string on failure. Surface the error and exit non-zero. ### Windows FFI and COM Safety -- **DLL loading security**: When loading system DLLs via `LoadLibraryW`, use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` to prevent DLL preloading/hijacking attacks. This is especially important for resources that run with elevated privileges. -- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and library handles (`HMODULE`) must be cleaned up even on error paths. Implement `Drop` for wrapper types or use RAII patterns to ensure `FreeLibrary`, `VariantClear`, etc. are always called. -- **Check HRESULT returns**: Windows API functions that return `HRESULT` (like `VariantClear`) should have their return values checked or at minimum logged, not silently ignored. -- **Iterative FFI operations**: When creating nested structures (e.g., registry keys path-by-path), ensure each iteration uses the result of the previous call as the parent handle, not always the root handle. +- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and library handles (`HMODULE`) must be cleaned up even on error paths. Implement `Drop` for wrapper types. +- **Check HRESULT returns**: Windows API functions that return `HRESULT` should have their return values checked or at minimum logged. +- **Iterative FFI operations**: When creating nested structures (registry keys path-by-path), ensure each iteration uses the previous call's result as parent handle, not always root. -### Serialization and Consistency +### Consistency Across Operations -- **Deterministic output**: If a function returns JSON, ensure the format (compact vs pretty) is consistent regardless of cache state. A cache hit should not produce differently-formatted output than a cache miss. -- **Avoid unnecessary serialize/parse roundtrips**: If you already have a `serde_json::Value`, cache or return it directly rather than serializing to string and re-parsing. -- **Schema/manifest version consistency**: When bumping a version in `Cargo.toml`, ensure the corresponding resource manifest `.dsc.resource.json` file is also updated to match. +- **Behavior must be consistent across get/set/test/export**: If a validation, parameter, or output shape applies to one operation, verify it applies to all siblings. +- **Deterministic output**: JSON format (compact vs pretty) must be consistent regardless of cache state. A cache hit should not produce differently-formatted output than a miss. +- **Schema/manifest version consistency**: When bumping a version in `Cargo.toml`, ensure the corresponding `.dsc.resource.json` manifest is also updated. -### Concurrency +### What-If / Dry-Run Correctness -- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue` — `IsEmpty` is only an approximation under concurrency. Use `while queue.TryDequeue(...)` as the single loop condition. +- **No side effects during what-if**: Verify no code path before the what-if gate can mutate state. Functions like `ensure_config_exists()` that create/copy files must be gated. +- **Consistent platform enforcement**: If an operation is platform-restricted, what-if mode must enforce the same restriction. -### What-If / Dry-Run Correctness +### Naming and Serde Semantics + +- **Names must match semantics**: Flag singular/plural mismatches, stale help text, misleading test names, and deprecated options still visibly advertised. +- **Be precise with `Option`/`Result`/serde**: Review whether `None`, empty collections, borrowed-vs-moved values, and `rename_all`/`deny_unknown_fields` all mean what the API claims. + +### Concurrency -- **No side effects during what-if**: When implementing `--what-if` mode, verify that no code path before the what-if gate can mutate state. Functions like `ensure_config_exists()` that create/copy files must be gated or skipped in what-if mode. -- **Consistent platform enforcement in what-if**: If an operation is platform-restricted (e.g., Windows-only), what-if mode must enforce the same restriction. Do not allow what-if to succeed on unsupported platforms. +- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use `while queue.TryDequeue(...)` as the single loop condition. ## PowerShell / Pester Test Patterns ### Cross-Platform Correctness -- **Path separators**: Never use hard-coded `\` in path construction. Always use `Join-Path` or `[System.IO.Path]::Combine()`. Tests with hard-coded backslashes will fail on Linux/macOS. -- **Platform-specific commands**: `stat -c` is GNU/Linux-specific. If a test should run on macOS as well, use PowerShell's `Get-Item` or gate the context on `$IsLinux` explicitly. -- **OS gating**: A Context labeled "Linux" that only checks `!$IsWindows` will also run on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. +- **Path separators**: Never use hard-coded `\` in path construction. Always use `Join-Path`. Tests with hard-coded backslashes will fail on Linux/macOS. +- **Platform-specific commands**: `stat -c` is GNU/Linux-specific. Gate on `$IsLinux` explicitly or use PowerShell equivalents. +- **OS gating**: A Context that only checks `!$IsWindows` also runs on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. +- **Use realistic cross-platform fixtures**: Test data should use correct path separators and plausible file locations for the target OS. ### Test Isolation and Cleanup -- **Preserve and restore environment variables**: When tests modify `$env:` variables (e.g., `DSC_RESTRICTED_PATH`, `DSC_RESOURCE_PATH`), always capture the original value in `BeforeAll` and restore it in `AfterAll`. Setting to `$null` unconditionally can destroy pre-existing values. -- **Conflicting environment variables**: If one env var takes precedence over another (e.g., `DSC_RESTRICTED_PATH` over `DSC_RESOURCE_PATH`), tests for the lower-priority var must explicitly clear the higher-priority one. -- **ACL and permission restoration**: When tests modify filesystem ACLs or permissions, capture the full original state and restore it completely. Do not rely on partial undo (e.g., removing a single ACE) or hard-coded permission values like `755`. -- **Recursive ACL changes**: If `icacls /T` is used to recursively modify ACLs, `AfterAll` must restore children as well, not just the top-level directory. -- **Event subscriber cleanup**: When using `Register-ObjectEvent`, clean up only the subscribers you created (filter by `-SourceIdentifier`), not all subscribers in the session via blanket `Get-EventSubscriber | Unregister-Event`. +- **Preserve and restore environment variables**: Capture the original value in `BeforeAll` and restore in `AfterAll`. Never set to `$null` unconditionally. +- **Conflicting environment variables**: Tests for lower-priority env vars (e.g., `DSC_RESOURCE_PATH`) must explicitly clear higher-priority ones (`DSC_RESTRICTED_PATH`). +- **ACL and permission restoration**: Capture full original state and restore completely. Do not rely on partial undo or hard-coded permission values. +- **Recursive ACL changes**: If `icacls /T` is used, `AfterAll` must restore children too. +- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket `Get-EventSubscriber | Unregister-Event`. +- **Use `-ErrorAction Ignore` over `SilentlyContinue`**: When leftover error records could pollute later assertions. ### Test Assertions and Naming -- **Test name must match assertions**: If a test is named "X happens only once," the assertions must verify the "only once" constraint — not just that X happened at least once. Either tighten assertions or rename the test. -- **Ordering assumptions**: `dsc resource list` returns items in alphabetical order. Tests should document this assumption or sort results before asserting on specific positions. -- **Test logic correctness**: Verify that test conditions actually exercise the code path under test. A condition that always fails regardless of the variable being tested is a no-op test. -- **Assert `$LASTEXITCODE`**: When testing CLI commands, assert `$LASTEXITCODE -eq 0` (or the expected exit code) in addition to output checks. A non-zero exit can go unnoticed if only output content is validated. -- **Avoid duplicate reads**: When asserting on file content, read it once into a variable rather than calling `Get-Content` multiple times (once for the assertion, again for `-Because`). -- **Array comparisons**: `Should -Be` can be unreliable for array comparisons. Normalize arrays through JSON conversion before comparing, or compare individual elements. -- **Skip guards for cmdlet availability**: Tests that depend on platform-specific cmdlets (e.g., `Get-NetFirewallRule`) should check cmdlet availability in their `-Skip` condition, not just elevation status. - -### Test Structure and Readability - -- **Use Context blocks for shared setup**: When multiple test cases share setup steps (e.g., found vs. not-found scenarios), use a Pester `Context` block to group and clarify shared setup. -- **Separation between sections**: Maintain blank lines between logical sections in configuration and test files for readability. -- **Helpers belong near their usage**: Helper functions used only in a specific test file should live in that file. Only promote to the shared helper module (`build.helpers.psm1`) if used across multiple scripts. +- **Test name must match assertions**: If named "X happens only once," assertions must verify the constraint, not just that X happened. +- **Assert `$LASTEXITCODE`**: Always check exit code in addition to output content for CLI command tests. +- **Prove the intended failure mode**: Negative tests must fail for the exact reason under test, not a broader fallback condition. +- **Cover both branches**: Add explicit tests for both "present" and "absent" cases rather than depending on host configuration. +- **Array comparisons**: Normalize arrays through JSON conversion before comparing with `Should -Be`. +- **Avoid duplicate reads**: Read file content once into a variable, not multiple times per assertion. +- **Skip guards for cmdlet availability**: Check cmdlet availability in `-Skip` conditions, not just elevation. +- **Ordering assumptions**: `dsc resource list` returns alphabetical order. Document or sort before position-based assertions. +- **Manifest file naming**: Discovery only loads manifests with `.dsc.resource.(json|yaml|yml)` suffix. Tests using other extensions won't exercise discovery. +- **Prefer `Should -BeExactly`**: Use exact comparisons when full expected result is known to prevent regressions slipping through loose checks. + +### Test Structure + +- **Use Context blocks for shared setup**: Group found/not-found scenarios with shared `BeforeEach`. +- **Helpers belong near their usage**: Only promote to `build.helpers.psm1` if used across multiple scripts. +- **Test both adapters**: If both `PowerShellScript` and `WindowsPowerShellScript` share code, parameterize tests to cover both. +- **Distinguish "failed" from "succeeded with errors"**: Add tests that separate terminating errors, non-terminating errors, and successful runs with warnings. +- **Add edge-case and malformed-input coverage**: Test "almost valid" negative cases and boundary conditions whenever only the happy path is covered. +- **Test behavior end-to-end**: Favor tests demonstrating user-visible failure/fix paths; use logs only when output alone cannot distinguish the bug. ## PowerShell Adapter Patterns -- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects when multiple versions are loaded. Always select a single module (e.g., highest version) before calling methods on it. -- **Error handling around module imports**: When probing for DSC resource classes via module import (e.g., during cache refresh), wrap the import in try/catch so that one failing module doesn't abort the entire enumeration. -- **`$using:` in parallel blocks**: Variables from the parent scope are not automatically available inside `ForEach-Object -Parallel` script blocks. Use `$using:variableName` to capture them. -- **Distinguish terminating vs non-terminating errors**: Non-terminating errors written to the Error stream are counted in `$ps.HadErrors` but should not cause the adapter to report failure. Only terminating errors (exceptions) should produce a non-zero exit code. +- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects. Always select a single module (highest version) before calling methods. +- **Error handling around module imports**: Wrap import probes in try/catch so one failing module doesn't abort entire enumeration. +- **`$using:` in parallel blocks**: Variables from parent scope are not available inside `ForEach-Object -Parallel`. Use `$using:variableName`. +- **Distinguish terminating vs non-terminating errors**: Non-terminating errors in `$ps.HadErrors` should not produce non-zero exit. Only terminating errors (exceptions) should. +- **Use `$_` in catch blocks, not `$error`**: `$error` is the global error collection. Use `$_` (the caught exception) for condition checks in catch blocks. +- **Flush trace queues before exit on all paths**: If a catch/finally path exits the script, drain the trace queue first to avoid losing diagnostic messages. ## Design and Architecture Patterns ### Settings and Configuration Precedence -- **Full precedence chain**: Settings resolution must account for all scopes: Policy > CommandLine > Environment > Workspace > User > Install. Missing scopes in the chain can allow lower-precedence values to bypass higher-precedence ones. -- **Resolved fields vs containers**: When implementing settings resolution, each leaf setting should be individually resolvable (with its source scope tracked), not just the container as a whole. -- **`allow_override` enforcement**: When a setting's `allow_override` is false (policy-scoped), ensure the code path actually prevents override from env vars and CLI args — not just one of those sources. +- **Full precedence chain**: Policy > CommandLine > Environment > Workspace > User > Install. Missing scopes allow bypasses. +- **Resolved fields vs containers**: Each leaf setting should be individually resolvable with source scope tracked. +- **`allow_override` enforcement**: When false (policy-scoped), must prevent override from both env vars and CLI args. ### Backward Compatibility -- **Breaking test expectations**: If a change makes a previously-optional directive required, existing tests must still pass as-is. Behavior that differs from what tests already validate is a potential breaking change that needs working group discussion. -- **Resource filtering logic**: When engine-side filtering is added alongside resource-side filtering, the logic should be: (1) if the resource supports filtering, let it handle it; (2) if not, use engine filtering with an INFO-level message. -- **Semantic versioning for resources**: Resources below 1.0 (e.g., `0.x.y`) are not design-stable. For breaking changes to resource input/output shapes, bump the minor version. Reserve 1.x for stable designs. +- **Breaking test expectations**: If existing tests must change, that signals a potential breaking change needing working group discussion. +- **Resource filtering logic**: (1) if resource supports filtering, let it handle it; (2) if not, use engine filtering with INFO message. +- **Semantic versioning**: Resources below 1.0 are not design-stable. Reserve 1.x for stable designs. +- **Public signature changes are compatibility events**: Adding parameters, changing flags, or exposing new modules should trigger compatibility review. ### Regex and Wildcard Handling -- **Escape all metacharacters**: When converting user-supplied wildcard patterns to regex, escape all regex metacharacters (`[`, `(`, `+`, `^`, `$`, `|`, `\`, etc.), not just `.`. Unescaped metacharacters can cause panics or unexpected matching behavior. +- **Escape all metacharacters**: When converting wildcards to regex, escape `[`, `(`, `+`, `^`, `$`, `|`, `\` -- not just `.`. Unescaped metacharacters cause panics or unexpected matching. ### Resource Manifest and Schema Coherence -- **Schema must match implementation**: If a resource manifest's embedded schema declares a property as required, the resource implementation must enforce that requirement. Conversely, if the implementation requires a field, the schema's `required` array must include it. -- **Export schema validation**: When `export.schema` is defined, the engine uses it for input validation. Ensure the schema is strict enough that invalid inputs are caught before reaching the resource. -- **`noFiltering` semantics**: When a resource declares `export.schema: noFiltering`, the export input should be treated as empty (no filter properties passed to the resource). -- **Naming for DSC-specific conventions**: Use leading underscore (`_`) only for canonical (cross-resource) properties. Resource-specific properties that might collide with future keywords should use descriptive names (e.g., `sshd_config_filepath`) and be discussed in the working group if elevation to canonical status is warranted. +- **Schema must match implementation**: Required properties in schema must be enforced in code and vice versa. +- **`noFiltering` semantics**: Export input should be treated as empty when declared. +- **Canonical property naming**: Leading underscore (`_`) is only for cross-resource canonical properties. Resource-specific properties use descriptive names (e.g., `sshd_config_filepath`). +- **Behavior must match manifest metadata**: Versions, `requireSecurityContext`, adapter args, and defaults must reflect what the resource actually does. ### CI/CD and GitHub Actions -- **Fork permission limitations**: `GITHUB_TOKEN` on fork PRs is typically read-only even with `pull-requests: write`. Steps that post PR comments should be gated to same-repo PRs or use `continue-on-error: true`. -- **Conditional tool installation**: Install expensive tools (Rust toolchains, coverage tools, protoc) only after determining the PR actually needs them (e.g., after checking which files changed). -- **PowerShell conventions in build scripts**: Functions should be singular per PS convention (e.g., `Test-RustProject` not `Test-RustProjects`). Build helper failures should throw so the pipeline stops on error. +- **Fork permission limitations**: Steps posting PR comments should gate on same-repo PRs or use `continue-on-error: true`. +- **Conditional tool installation**: Install expensive tools only after determining the PR needs them. +- **Check control flow, not just syntax**: Verify `if: always()`, `continue-on-error`, and `needs`/stage dependencies match stated failure behavior. +- **Preserve downstream artifact contracts**: Flag changes to artifact names/paths when downstream jobs expect the old layout. +- **PowerShell conventions**: Singular function names (`Test-RustProject`). Build failures should throw. ## Documentation and Logging -- **Accurate comments**: If code behavior changes, update comments to match. A comment that says "retains quoting" when the code actually strips quotes is misleading. -- **Log level appropriateness**: Full `PATH` contents should be `trace!` level, not `debug!`. Sensitive or extremely verbose data should use the lowest practical log level. -- **Doc comments matching implementation**: If a doc comment describes behavior that the implementation doesn't actually enforce (e.g., "controls both env var and CLI option" when only env var is gated), update the comment to match reality. -- **Remove debug print statements**: `println!` debug output must not be left in production or test code. Use `debug!`/`trace!` macros from the tracing crate for diagnostics, or remove entirely before merge. -- **Locale/i18n string accuracy**: When adding localized strings, verify the key name matches the keyword/function it describes. Copy-paste errors in locale files (e.g., wrong keyword name in the error message) are common and hard to catch later. -- **Dead locale strings**: Do not add i18n keys that are never referenced in code. Unused locale strings accumulate and make it harder to identify which messages are active. +- **Accurate comments**: If code behavior changes, update comments to match. +- **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. +- **Doc comments matching implementation**: Update doc comments when described behavior doesn't match reality. +- **Remove debug print statements**: No `println!` debug output in production or test code. Use `debug!`/`trace!` macros. +- **Locale/i18n string accuracy**: Verify key names match the keyword/function they describe. Copy-paste errors in locale files are common. +- **Dead locale strings**: Do not add i18n keys never referenced in code. +- **Prefer scan-friendly wording**: Log/error strings should be concise and punctuated clearly for readability in diagnostics. From d0ae0f154b8ee84def729a44c5f871b9e680560d Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 09:14:29 -0700 Subject: [PATCH 03/10] Split code review instructions into area-specific files Reorganize the monolithic code-review.instructions.md into 11 focused instruction files under .github/instructions/code-review/, each with applyTo patterns targeting the relevant paths: - engine: configure, discovery, resource dispatch, functions, settings - resource: individual resource implementations - adapter: PowerShell/WMI adapter bridge code - extension: extension discovery and lifecycle - cli: command-line interface, subcommands, server mode - tests: Pester and Rust integration tests - library: shared libraries (dsc-lib, jsonschema, osinfo, etc.) - security: ACLs, policy, credentials, elevated resources - performance: allocations, caching, serialization hot paths - windows: FFI, COM, registry, DISM, services - linux: SSH config, apt/brew, platform-specific paths The top-level file now serves as a routing table mapping paths to areas, plus universal guidance (false-positive avoidance, documentation, CI/CD). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../instructions/code-review.instructions.md | 187 ++++-------------- .../code-review/adapter.instructions.md | 31 +++ .../code-review/cli.instructions.md | 30 +++ .../code-review/engine.instructions.md | 43 ++++ .../code-review/extension.instructions.md | 24 +++ .../code-review/library.instructions.md | 34 ++++ .../code-review/linux.instructions.md | 30 +++ .../code-review/performance.instructions.md | 31 +++ .../code-review/resource.instructions.md | 38 ++++ .../code-review/security.instructions.md | 42 ++++ .../code-review/tests.instructions.md | 47 +++++ .../code-review/windows.instructions.md | 35 ++++ 12 files changed, 421 insertions(+), 151 deletions(-) create mode 100644 .github/instructions/code-review/adapter.instructions.md create mode 100644 .github/instructions/code-review/cli.instructions.md create mode 100644 .github/instructions/code-review/engine.instructions.md create mode 100644 .github/instructions/code-review/extension.instructions.md create mode 100644 .github/instructions/code-review/library.instructions.md create mode 100644 .github/instructions/code-review/linux.instructions.md create mode 100644 .github/instructions/code-review/performance.instructions.md create mode 100644 .github/instructions/code-review/resource.instructions.md create mode 100644 .github/instructions/code-review/security.instructions.md create mode 100644 .github/instructions/code-review/tests.instructions.md create mode 100644 .github/instructions/code-review/windows.instructions.md diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 6e8ecd3e1..36c92c767 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -7,170 +7,55 @@ description: 'Repository-specific code review guidance for DSC (Rust, PowerShell These instructions guide Copilot when performing code reviews on pull requests in this repository. Focus on high-confidence, actionable findings. Do not comment on style, formatting, or trivial issues. -Derived from analysis of 2,700+ review comments across 455 PRs in this repository. -## General Principles (False-Positive Avoidance) - -- **Do not claim code will not compile unless you are certain**: Multiple reviews were rejected because Copilot incorrectly claimed Rust ownership/borrowing errors. If the code compiles and tests pass in CI, do not assert otherwise. -- **Test resources are not production code**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not require production-grade error handling (replacing `expect()` with `Result`) in test harnesses unless panics would hide regressions. -- **Automatically-generated files**: Files like `lib/dsc-lib-jsonschema/.versions.json` are updated by build automation. Do not flag version bumps as unintentional. -- **Do not demand large abstractions in small PRs**: If duplicated logic exists, suggest extracting a helper function. Do not block a focused PR by requesting a full trait/framework redesign -- that is follow-up work. -- **Table output is not a stable API**: JSON/YAML are the canonical machine-readable outputs. Do not frame table column/layout changes as breaking changes. -- **Separate input/output structs can be intentional**: Resources often distinguish desired state input from observed state output. Do not assume duplicate structs are accidental. -- **Intentional design decisions**: When maintainers explicitly label behavior as "intentional" (e.g., PATH mutation behavior, error promotion choices), do not re-flag. Respect the context of intentional design. - -## Security - -- **Fail closed on all security checks**: Security-sensitive functions (ACL verification, permission checks, policy folder validation) must treat failures as "not secure." If reading a security descriptor, enumerating ACEs, or calling stat fails, return the restrictive/denied result -- never fail open. -- **NULL DACL detection**: A NULL DACL means full access to everyone. Security checks must treat `p_dacl.is_null()` as insecure. -- **Complete permission checks**: Write-access checks must cover `GENERIC_WRITE` and `GENERIC_ALL` in addition to specific write flags. Also consider inherit-only ACEs (which don't apply to the object itself) and non-standard ACE variants (callback ACEs can still grant access). -- **Do not let user-controlled inputs bypass policy**: CLI flags, env vars, or config precedence changes must not override policy-enforced settings. Policy sources should remain authoritative even when user convenience switches are added. -- **DLL loading security**: When loading system DLLs via `LoadLibraryW`, use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` to prevent DLL preloading/hijacking. Especially important for resources running elevated (DISM, offline registry, services). -- **Prevent command injection via string interpolation**: Flag code that embeds user-controlled values (secrets, vault names, paths) into PowerShell command strings. Prefer passing arguments/JSON rather than building quoted script fragments. -- **Never leak secrets into logs or output**: Verify redaction is consistently applied when logging JSON/resource output that may contain secure values. Review "show secrets" paths to ensure behavior is explicit. -- **Enforce declared security context at runtime**: If manifests declare `requireSecurityContext`, verify runtime enforcement exists for every operation (`get`, `set`, `test`, `export`). A schema field without enforcement is a security bug. -- **Do not cache or trust failed verifications**: Trust caches (e.g., Authenticode checks) should only be updated on successful validation, never on failure. -- **Watch for destructive modes with unresolvable system objects**: In list-reconciling resources (firewall, services), flag logic that disables/removes system-created entries (AppX/UWP rules) that users cannot reliably reference in their declared state. - -## Rust Code Patterns - -### Caching and Performance - -- **Cache key correctness**: Cache keys must account for all dimensions affecting the cached value. For adapter resources with `target_resource`, cache by target identity, not just adapter type/version. -- **Centralize cache logic**: Flag duplicate cache writes or wrapper layers that both cache the same result. A single cache owner reduces redundant work and prevents divergent keying. -- **Avoid JSON serialize/parse roundtrips**: If you already have a `serde_json::Value`, cache or return it directly rather than stringifying and re-parsing. -- **Short-circuit redundant work**: Flag loops that keep scanning after all matches are found, or `contains_key` + `insert` patterns where entry APIs would do one pass. -- **Reduce allocations only when semantics stay intact**: Good flags: repeated `to_lowercase()`, cloning owned data that can be borrowed. Bad flags: clone removal that changes ownership semantics or breaks compilation. When in doubt, do not flag. - -### API Surface and Visibility - -- **Default to private/`pub(crate)`**: New modules, helpers, globals, and cache types should be private or `pub(crate)`. Require a clear external use case before exposing `pub`. -- **Breaking changes to public APIs**: Adding required parameters to public functions is a breaking change. Consider backward-compatible wrappers. -- **Extract shared logic into helpers**: When the same logic appears in multiple subcommands, extract a helper. But do not demand full trait redesigns in focused PRs. -- **Remove dead code immediately**: Unused imports, variables, and enums should be deleted when a refactor makes them unnecessary -- Rust warnings become CI failures. - -### Error Handling - -- **Prefer `Result` over panics for user input**: Avoid `unwrap()`/`expect()` on user input, manifest data, regex creation, or serialization. Return clear `Result` or stable exit codes. (Exception: test harness code in `tools/dsctest/`.) -- **Error promotion risk**: Converting previously-ignored errors into hard errors can break existing environments. Prefer warnings or graceful degradation unless truly unrecoverable. -- **Return `Option` for fallible lookups**: When a function can legitimately fail to find a path or value, return `Option` rather than an empty string (which resolves to `.`). -- **`unwrap_or_default()` on serialization is a bug**: `serde_json::to_string(...).unwrap_or_default()` silently emits empty string on failure. Surface the error and exit non-zero. - -### Windows FFI and COM Safety - -- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and library handles (`HMODULE`) must be cleaned up even on error paths. Implement `Drop` for wrapper types. -- **Check HRESULT returns**: Windows API functions that return `HRESULT` should have their return values checked or at minimum logged. -- **Iterative FFI operations**: When creating nested structures (registry keys path-by-path), ensure each iteration uses the previous call's result as parent handle, not always root. - -### Consistency Across Operations - -- **Behavior must be consistent across get/set/test/export**: If a validation, parameter, or output shape applies to one operation, verify it applies to all siblings. -- **Deterministic output**: JSON format (compact vs pretty) must be consistent regardless of cache state. A cache hit should not produce differently-formatted output than a miss. -- **Schema/manifest version consistency**: When bumping a version in `Cargo.toml`, ensure the corresponding `.dsc.resource.json` manifest is also updated. - -### What-If / Dry-Run Correctness - -- **No side effects during what-if**: Verify no code path before the what-if gate can mutate state. Functions like `ensure_config_exists()` that create/copy files must be gated. -- **Consistent platform enforcement**: If an operation is platform-restricted, what-if mode must enforce the same restriction. - -### Naming and Serde Semantics - -- **Names must match semantics**: Flag singular/plural mismatches, stale help text, misleading test names, and deprecated options still visibly advertised. -- **Be precise with `Option`/`Result`/serde**: Review whether `None`, empty collections, borrowed-vs-moved values, and `rename_all`/`deny_unknown_fields` all mean what the API claims. - -### Concurrency - -- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use `while queue.TryDequeue(...)` as the single loop condition. +Detailed review guidance is split by contribution area in `.github/instructions/code-review/`. +Use the path-based mapping below to determine which area instructions apply to the files under review. -## PowerShell / Pester Test Patterns +## Area Routing -### Cross-Platform Correctness +| Area | Paths | Instruction File | +|------|-------|------------------| +| Engine | `lib/dsc-lib/src/configure/`, `lib/dsc-lib/src/discovery/`, `lib/dsc-lib/src/dscresources/`, `lib/dsc-lib/src/functions/`, `lib/dsc-lib/src/settings/` | `code-review/engine.instructions.md` | +| Resources | `resources/` | `code-review/resource.instructions.md` | +| Adapters | `adapters/` | `code-review/adapter.instructions.md` | +| Extensions | `extensions/`, `lib/dsc-lib/src/extensions/` | `code-review/extension.instructions.md` | +| CLI | `dsc/src/` | `code-review/cli.instructions.md` | +| Tests | `**/*.tests.ps1`, `**/tests/` | `code-review/tests.instructions.md` | +| Libraries | `lib/` | `code-review/library.instructions.md` | +| Security | `lib/dsc-lib/src/util.rs`, `lib/dsc-lib-security_context/`, `lib/dsc-lib-registry/`, `resources/registry/`, `resources/windows_firewall/`, `resources/windows_service/`, `resources/dism_dsc/` | `code-review/security.instructions.md` | +| Performance | Any `*.rs` file in hot paths | `code-review/performance.instructions.md` | +| Windows | `resources/windows_*`, `resources/dism_dsc/`, `resources/registry/`, `lib/dsc-lib-registry/`, `pal/windows/` | `code-review/windows.instructions.md` | +| Linux/macOS | `resources/apt/`, `resources/sshdconfig/`, `resources/brew/`, `pal/linux/` | `code-review/linux.instructions.md` | -- **Path separators**: Never use hard-coded `\` in path construction. Always use `Join-Path`. Tests with hard-coded backslashes will fail on Linux/macOS. -- **Platform-specific commands**: `stat -c` is GNU/Linux-specific. Gate on `$IsLinux` explicitly or use PowerShell equivalents. -- **OS gating**: A Context that only checks `!$IsWindows` also runs on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. -- **Use realistic cross-platform fixtures**: Test data should use correct path separators and plausible file locations for the target OS. +Multiple areas may apply to a single file. For example, `resources/windows_firewall/` triggers +both the Resource, Windows, and Security instructions. -### Test Isolation and Cleanup - -- **Preserve and restore environment variables**: Capture the original value in `BeforeAll` and restore in `AfterAll`. Never set to `$null` unconditionally. -- **Conflicting environment variables**: Tests for lower-priority env vars (e.g., `DSC_RESOURCE_PATH`) must explicitly clear higher-priority ones (`DSC_RESTRICTED_PATH`). -- **ACL and permission restoration**: Capture full original state and restore completely. Do not rely on partial undo or hard-coded permission values. -- **Recursive ACL changes**: If `icacls /T` is used, `AfterAll` must restore children too. -- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket `Get-EventSubscriber | Unregister-Event`. -- **Use `-ErrorAction Ignore` over `SilentlyContinue`**: When leftover error records could pollute later assertions. - -### Test Assertions and Naming - -- **Test name must match assertions**: If named "X happens only once," assertions must verify the constraint, not just that X happened. -- **Assert `$LASTEXITCODE`**: Always check exit code in addition to output content for CLI command tests. -- **Prove the intended failure mode**: Negative tests must fail for the exact reason under test, not a broader fallback condition. -- **Cover both branches**: Add explicit tests for both "present" and "absent" cases rather than depending on host configuration. -- **Array comparisons**: Normalize arrays through JSON conversion before comparing with `Should -Be`. -- **Avoid duplicate reads**: Read file content once into a variable, not multiple times per assertion. -- **Skip guards for cmdlet availability**: Check cmdlet availability in `-Skip` conditions, not just elevation. -- **Ordering assumptions**: `dsc resource list` returns alphabetical order. Document or sort before position-based assertions. -- **Manifest file naming**: Discovery only loads manifests with `.dsc.resource.(json|yaml|yml)` suffix. Tests using other extensions won't exercise discovery. -- **Prefer `Should -BeExactly`**: Use exact comparisons when full expected result is known to prevent regressions slipping through loose checks. - -### Test Structure - -- **Use Context blocks for shared setup**: Group found/not-found scenarios with shared `BeforeEach`. -- **Helpers belong near their usage**: Only promote to `build.helpers.psm1` if used across multiple scripts. -- **Test both adapters**: If both `PowerShellScript` and `WindowsPowerShellScript` share code, parameterize tests to cover both. -- **Distinguish "failed" from "succeeded with errors"**: Add tests that separate terminating errors, non-terminating errors, and successful runs with warnings. -- **Add edge-case and malformed-input coverage**: Test "almost valid" negative cases and boundary conditions whenever only the happy path is covered. -- **Test behavior end-to-end**: Favor tests demonstrating user-visible failure/fix paths; use logs only when output alone cannot distinguish the bug. - -## PowerShell Adapter Patterns - -- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects. Always select a single module (highest version) before calling methods. -- **Error handling around module imports**: Wrap import probes in try/catch so one failing module doesn't abort entire enumeration. -- **`$using:` in parallel blocks**: Variables from parent scope are not available inside `ForEach-Object -Parallel`. Use `$using:variableName`. -- **Distinguish terminating vs non-terminating errors**: Non-terminating errors in `$ps.HadErrors` should not produce non-zero exit. Only terminating errors (exceptions) should. -- **Use `$_` in catch blocks, not `$error`**: `$error` is the global error collection. Use `$_` (the caught exception) for condition checks in catch blocks. -- **Flush trace queues before exit on all paths**: If a catch/finally path exits the script, drain the trace queue first to avoid losing diagnostic messages. - -## Design and Architecture Patterns - -### Settings and Configuration Precedence - -- **Full precedence chain**: Policy > CommandLine > Environment > Workspace > User > Install. Missing scopes allow bypasses. -- **Resolved fields vs containers**: Each leaf setting should be individually resolvable with source scope tracked. -- **`allow_override` enforcement**: When false (policy-scoped), must prevent override from both env vars and CLI args. - -### Backward Compatibility - -- **Breaking test expectations**: If existing tests must change, that signals a potential breaking change needing working group discussion. -- **Resource filtering logic**: (1) if resource supports filtering, let it handle it; (2) if not, use engine filtering with INFO message. -- **Semantic versioning**: Resources below 1.0 are not design-stable. Reserve 1.x for stable designs. -- **Public signature changes are compatibility events**: Adding parameters, changing flags, or exposing new modules should trigger compatibility review. +## General Principles (False-Positive Avoidance) -### Regex and Wildcard Handling +These apply to ALL areas: -- **Escape all metacharacters**: When converting wildcards to regex, escape `[`, `(`, `+`, `^`, `$`, `|`, `\` -- not just `.`. Unescaped metacharacters cause panics or unexpected matching. +- **Do not claim code will not compile unless you are certain**: Multiple reviews were rejected because Copilot incorrectly claimed Rust ownership/borrowing errors. If the code compiles and tests pass in CI, do not assert otherwise. +- **Test resources are not production code**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not require production-grade error handling in test harnesses unless panics would hide regressions. +- **Automatically-generated files**: Files like `lib/dsc-lib-jsonschema/.versions.json` are updated by build automation. Do not flag version bumps as unintentional. +- **Do not demand large abstractions in small PRs**: Suggest extracting a helper function. Do not block a focused PR by requesting trait/framework redesigns -- that is follow-up work. +- **Table output is not a stable API**: JSON/YAML are canonical machine-readable outputs. Table layout changes are not breaking. +- **Separate input/output structs can be intentional**: Resources often distinguish desired state from observed state. Do not assume duplicate structs are accidental. +- **Intentional design decisions**: When maintainers explicitly label behavior as "intentional", do not re-flag. -### Resource Manifest and Schema Coherence +## Documentation and Logging (All Areas) -- **Schema must match implementation**: Required properties in schema must be enforced in code and vice versa. -- **`noFiltering` semantics**: Export input should be treated as empty when declared. -- **Canonical property naming**: Leading underscore (`_`) is only for cross-resource canonical properties. Resource-specific properties use descriptive names (e.g., `sshd_config_filepath`). -- **Behavior must match manifest metadata**: Versions, `requireSecurityContext`, adapter args, and defaults must reflect what the resource actually does. +- **Accurate comments**: If code behavior changes, update comments to match. +- **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. +- **Doc comments matching implementation**: Update when described behavior doesn't match reality. +- **Remove debug print statements**: No `println!` in production or test code. Use `debug!`/`trace!` macros. +- **Locale/i18n string accuracy**: Verify key names match the keyword/function they describe. +- **Dead locale strings**: Do not add i18n keys never referenced in code. +- **Prefer scan-friendly wording**: Log/error strings should be concise for readability in diagnostics. -### CI/CD and GitHub Actions +## CI/CD (All Areas) - **Fork permission limitations**: Steps posting PR comments should gate on same-repo PRs or use `continue-on-error: true`. - **Conditional tool installation**: Install expensive tools only after determining the PR needs them. - **Check control flow, not just syntax**: Verify `if: always()`, `continue-on-error`, and `needs`/stage dependencies match stated failure behavior. - **Preserve downstream artifact contracts**: Flag changes to artifact names/paths when downstream jobs expect the old layout. - **PowerShell conventions**: Singular function names (`Test-RustProject`). Build failures should throw. - -## Documentation and Logging - -- **Accurate comments**: If code behavior changes, update comments to match. -- **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. -- **Doc comments matching implementation**: Update doc comments when described behavior doesn't match reality. -- **Remove debug print statements**: No `println!` debug output in production or test code. Use `debug!`/`trace!` macros. -- **Locale/i18n string accuracy**: Verify key names match the keyword/function they describe. Copy-paste errors in locale files are common. -- **Dead locale strings**: Do not add i18n keys never referenced in code. -- **Prefer scan-friendly wording**: Log/error strings should be concise and punctuated clearly for readability in diagnostics. diff --git a/.github/instructions/code-review/adapter.instructions.md b/.github/instructions/code-review/adapter.instructions.md new file mode 100644 index 000000000..9945cd2da --- /dev/null +++ b/.github/instructions/code-review/adapter.instructions.md @@ -0,0 +1,31 @@ +--- +applyTo: 'adapters/**' +description: 'Code review guidance for DSC adapters (PowerShell, WMI, etc.)' +--- + +# Adapter Code Review + +Adapters bridge DSC to external resource ecosystems (PowerShell DSC v1/v2 resources, WMI, etc.). +They run PowerShell runspaces, manage module loading, and translate between DSC and native formats. + +## Module Import Safety + +- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects when multiple versions are loaded. Always select a single module (highest version) before calling methods. +- **Error handling around module imports**: Wrap import probes in try/catch so one failing module doesn't abort entire resource enumeration during cache refresh. +- **`$using:` in parallel blocks**: Variables from parent scope are not available inside `ForEach-Object -Parallel`. Use `$using:variableName`. + +## Error Handling + +- **Distinguish terminating vs non-terminating errors**: Non-terminating errors in `$ps.HadErrors` should not produce non-zero exit. Only terminating errors (exceptions) should cause adapter failure. +- **Use `$_` in catch blocks, not `$error`**: `$error` is the global error collection. Use `$_` (the caught exception) for condition checks. +- **Flush trace queues before exit on all paths**: If a catch/finally path exits the script, drain the trace queue first to avoid losing diagnostic messages. + +## Concurrency and Event Handling + +- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use `while queue.TryDequeue(...)` as the single loop condition -- `IsEmpty` is only an approximation. +- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket `Get-EventSubscriber | Unregister-Event`. + +## Secrets and Security + +- **Prevent command injection**: Flag code that embeds user-controlled values (secrets, vault names, paths) into PowerShell command strings. Prefer passing arguments/JSON. +- **Never leak secrets into logs**: Verify redaction is applied when logging resource output that may contain secure values. diff --git a/.github/instructions/code-review/cli.instructions.md b/.github/instructions/code-review/cli.instructions.md new file mode 100644 index 000000000..2ce4fe609 --- /dev/null +++ b/.github/instructions/code-review/cli.instructions.md @@ -0,0 +1,30 @@ +--- +applyTo: 'dsc/src/**' +description: 'Code review guidance for DSC CLI (command-line interface, subcommands, server mode)' +--- + +# CLI Code Review + +The `dsc` crate is the main CLI binary. It handles argument parsing, subcommand dispatch, +output formatting (JSON/YAML/table), and the MCP server mode. + +## Output Formatting + +- **Deterministic output**: JSON format (compact vs pretty) must be consistent regardless of internal state. +- **Table output is not a stable API**: JSON/YAML are canonical. Table changes are not breaking. +- **Extract shared formatting logic**: When table/list formatting appears in multiple subcommands, extract a helper rather than duplicating. + +## Error Handling + +- **Prefer `Result` over panics**: Avoid `unwrap()`/`expect()` on user input or parsed data. Return clear error messages and exit codes. +- **Names must match semantics**: Flag singular/plural mismatches, stale help text, and deprecated options still visibly advertised. + +## Server Mode (MCP) + +- **Schema/typing for tool parameters**: Prefer typed parameters over generic `serde_json::Value` with runtime validation. This gives clients correct schemas. +- **Tool name accuracy**: Ensure locale strings and schema descriptions reference the correct tool name (e.g., `list_dsc_functions` not `list_dsc_function`). + +## Backward Compatibility + +- **CLI flag changes are compatibility events**: Renaming, removing, or changing the semantics of CLI flags requires consideration of existing users and scripts. +- **Breaking test expectations**: If existing tests must change due to CLI behavior changes, that signals a potential breaking change needing discussion. diff --git a/.github/instructions/code-review/engine.instructions.md b/.github/instructions/code-review/engine.instructions.md new file mode 100644 index 000000000..69a0dccc4 --- /dev/null +++ b/.github/instructions/code-review/engine.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: 'lib/dsc-lib/src/configure/**,lib/dsc-lib/src/discovery/**,lib/dsc-lib/src/dscresources/**,lib/dsc-lib/src/functions/**,lib/dsc-lib/src/settings/**' +description: 'Code review guidance for DSC engine (configure, discovery, resource dispatch, functions, settings)' +--- + +# Engine Code Review + +The engine is the core of DSC: configuration processing, resource discovery, function evaluation, +schema caching, and settings resolution. Changes here have broad impact. + +## Caching + +- **Cache key correctness**: Cache keys must account for all dimensions affecting the cached value. For adapter resources with `target_resource`, cache by target identity, not just adapter type/version. +- **Centralize cache logic**: Flag duplicate cache writes or wrapper layers that both cache the same result. A single cache owner reduces redundant work and prevents divergent keying. +- **Avoid JSON serialize/parse roundtrips**: If you already have a `serde_json::Value`, cache or return it directly rather than stringifying and re-parsing. + +## Settings Precedence + +- **Full precedence chain**: Policy > CommandLine > Environment > Workspace > User > Install. Missing scopes allow bypasses. +- **Resolved fields vs containers**: Each leaf setting should be individually resolvable with source scope tracked. +- **`allow_override` enforcement**: When false (policy-scoped), must prevent override from both env vars and CLI args. + +## Discovery + +- **Short-circuit redundant work**: Flag loops that keep scanning after all matches are found, or `contains_key` + `insert` patterns where entry APIs would do one pass. +- **Error promotion risk**: Converting previously-ignored errors into hard errors (e.g., `join_paths` failure) can break existing environments. Prefer warnings or graceful degradation unless truly unrecoverable. + +## Resource Dispatch Consistency + +- **Behavior must be consistent across get/set/test/export**: If a validation, parameter, or output shape applies to one operation, verify it applies to all siblings. +- **Deterministic output**: JSON format (compact vs pretty) must be consistent regardless of cache state. +- **Resource filtering logic**: (1) if resource supports filtering, let it handle it; (2) if not, use engine filtering with INFO message. + +## What-If / Dry-Run + +- **No side effects during what-if**: Verify no code path before the what-if gate can mutate state. Functions like `ensure_config_exists()` that create/copy files must be gated. +- **Consistent platform enforcement**: If an operation is platform-restricted, what-if mode must enforce the same restriction. + +## API Surface + +- **Default to private/`pub(crate)`**: New modules, helpers, globals, and cache types should be private or `pub(crate)`. Require a clear external use case before exposing `pub`. +- **Breaking changes to public APIs**: Adding required parameters to public functions is a breaking change. Consider backward-compatible wrappers. +- **Public signature changes are compatibility events**: Adding parameters, changing flags, or exposing new modules should trigger compatibility review. diff --git a/.github/instructions/code-review/extension.instructions.md b/.github/instructions/code-review/extension.instructions.md new file mode 100644 index 000000000..6ded06e21 --- /dev/null +++ b/.github/instructions/code-review/extension.instructions.md @@ -0,0 +1,24 @@ +--- +applyTo: 'extensions/**,lib/dsc-lib/src/extensions/**' +description: 'Code review guidance for DSC extensions (discovery, lifecycle)' +--- + +# Extension Code Review + +Extensions extend DSC's discovery and resource capabilities. They have their own discovery +protocol and manifest format. + +## Discovery Protocol + +- **Manifest content handling**: When deserializing discovered manifests, ensure the expected shape (`ImportedManifest` enum with `Resource`/`Extension` variants) matches what extensions actually emit. +- **Unused variables after refactoring**: When switching from reused helpers (e.g., `process_get_args`) to dedicated ones (e.g., `process_discover_args`), remove intermediate variables and imports that are no longer needed. +- **Doc comments must match parameter semantics**: If a doc comment says "file path" but the parameter actually receives an argument name or extension list, update the comment. + +## Schema Updates + +- **Update JSON schemas when adding protocol fields**: New fields in extension stdout or args (e.g., `manifestContent`, `extensionsArg`) must be reflected in the checked-in JSON schemas under `schemas/`. + +## PowerShell Extensions + +- **`$using:` in parallel blocks**: Variables from parent scope are not available inside `ForEach-Object -Parallel`. Use `$using:variableName`. +- **Error isolation**: One extension failing discovery should not abort the entire extension enumeration. diff --git a/.github/instructions/code-review/library.instructions.md b/.github/instructions/code-review/library.instructions.md new file mode 100644 index 000000000..0e47c8e44 --- /dev/null +++ b/.github/instructions/code-review/library.instructions.md @@ -0,0 +1,34 @@ +--- +applyTo: 'lib/**' +description: 'Code review guidance for DSC libraries (dsc-lib, jsonschema, osinfo, pal, registry, security_context)' +--- + +# Library Code Review + +Libraries under `lib/` provide shared functionality consumed by the engine, CLI, and resources. +They define the public API surface of the DSC codebase. + +## API Surface and Visibility + +- **Default to private/`pub(crate)`**: New modules, helpers, globals, and cache types should be private or `pub(crate)`. Require a clear external use case before `pub`. +- **Breaking changes**: Adding required parameters to public functions is breaking. Consider wrappers or new function names. +- **Remove dead code immediately**: Unused imports, variables, and enums should be deleted -- Rust warnings become CI failures. +- **Extract shared logic into helpers**: Deduplicate common patterns, but don't demand full trait redesigns in focused PRs. + +## Error Handling + +- **Prefer `Result` over panics for user input**: Return clear `Result` or stable exit codes. +- **Return `Option` for fallible lookups**: Rather than empty strings that resolve to `.`. +- **`unwrap_or_default()` on serialization is a bug**: Surface the error and exit non-zero. + +## Serde and Schema + +- **Be precise with serde**: Review `rename_all`, `deny_unknown_fields`, `Option` vs default, and wire-format names. +- **Names must match semantics**: Singular/plural, stale doc comments, misleading function names. +- **Schema version catalogs**: `lib/dsc-lib-jsonschema/.versions.json` is auto-generated by build. Do not flag version entries as unintentional. + +## Performance + +- **Reduce allocations only when semantics stay intact**: Good: repeated `to_lowercase()`, cloning data that can be borrowed. Bad: clone removal that changes ownership semantics. +- **Avoid serialize/parse roundtrips**: If you have a `Value`, don't stringify and re-parse. +- **Short-circuit redundant work**: Use entry APIs instead of `contains_key` + `insert`. diff --git a/.github/instructions/code-review/linux.instructions.md b/.github/instructions/code-review/linux.instructions.md new file mode 100644 index 000000000..ab15b975a --- /dev/null +++ b/.github/instructions/code-review/linux.instructions.md @@ -0,0 +1,30 @@ +--- +applyTo: 'resources/apt/**,resources/sshdconfig/**,pal/linux/**,resources/brew/**' +description: 'Code review guidance for Linux/macOS-specific code (SSH, apt, platform abstraction)' +--- + +# Linux/macOS Code Review + +Linux and macOS resources manage SSH configuration, package managers, and platform-specific state. +Cross-platform correctness is a recurring theme since the same test suite runs on all platforms. + +## Cross-Platform Path Handling + +- **Never hard-code path separators**: Use platform-appropriate path joining. Rust's `PathBuf`/`Path::join` handles this automatically. +- **Platform-conditional logic**: Gate Linux-specific behavior (like `/etc/dsc` permissions) on the correct platform check. Don't assume "not Windows" means "Linux" -- it could be macOS. + +## SSH Config Resource + +- **Quoting preservation**: Understand what the parser actually preserves vs strips. Comments claiming "retains quoting" when code strips quotes are misleading. +- **Match block handling**: Ensure match/criteria blocks are included in export results and not accidentally filtered out. +- **Repeatable keywords with operators**: Keywords with `+`/`-`/`^` operators have specific semantics about repetition. Operators on non-repeatable keywords are invalid. + +## Permission Checks (Linux) + +- **Fail closed on stat failures**: If `fs::metadata` fails for `/etc/dsc`, do not proceed as if the folder is trusted. +- **Correct permission checks**: Use PowerShell's `Get-Item` or explicit `$IsLinux` gating rather than GNU-specific `stat -c` (which fails on macOS). + +## Package Managers + +- **Idempotent operations**: Package install/remove operations should be safe to run repeatedly without error. +- **Version comparison**: OS and package versions don't follow semver. Don't assume semver parsing applies. diff --git a/.github/instructions/code-review/performance.instructions.md b/.github/instructions/code-review/performance.instructions.md new file mode 100644 index 000000000..352de8e87 --- /dev/null +++ b/.github/instructions/code-review/performance.instructions.md @@ -0,0 +1,31 @@ +--- +applyTo: '**/*.rs' +description: 'Code review guidance for performance (Rust allocations, caching, serialization)' +--- + +# Performance Code Review + +Performance matters most in the engine hot paths: resource discovery (many manifests), +schema caching, and configuration processing. + +## Caching + +- **Cache key correctness**: Keys must include all dimensions that affect the value. Adapter schemas vary by `target_resource`. +- **Centralize cache writes**: Duplicate cache mutations lead to inconsistent keying and redundant work. +- **Avoid serialize/parse roundtrips**: Don't stringify a `Value` just to re-parse it. +- **Deterministic output**: Cache hits and misses must produce identical format (compact vs pretty). + +## Allocations + +- **Reduce allocations only when semantics stay intact**: Flag repeated `to_lowercase()` or cloning owned data that can be borrowed. Do NOT flag clone removal that changes ownership or breaks compilation. +- **Short-circuit redundant work**: Use entry APIs instead of `contains_key` + `insert`. Exit loops early when all matches are found. +- **Avoid cloning before parsing**: Parse first, then return the original owned value. + +## Native Interop + +- **Resource cleanup as performance issue**: Missing `Drop`/RAII cleanup for handles, DLLs, or `VARIANT`s is both correctness and long-run efficiency problem. Especially in loops (firewall enumeration, registry key iteration). + +## False Positives to Avoid + +- **Do not flag performance when call order makes it moot**: If `get` always runs before `set`/`test`, redundant validation in later operations doesn't affect real-world performance. +- **Compact vs pretty JSON**: Maintainers have noted "doesn't matter here" for format differences in non-user-facing internal paths. Only flag when the difference is externally observable. diff --git a/.github/instructions/code-review/resource.instructions.md b/.github/instructions/code-review/resource.instructions.md new file mode 100644 index 000000000..d0537b95e --- /dev/null +++ b/.github/instructions/code-review/resource.instructions.md @@ -0,0 +1,38 @@ +--- +applyTo: 'resources/**' +description: 'Code review guidance for DSC resources (individual resource implementations)' +--- + +# Resource Code Review + +Resources are individual components that manage specific system state (registry, services, +firewall, SSH config, DISM features, etc.). They run as separate executables invoked by the engine. + +## Manifest and Schema Coherence + +- **Schema must match implementation**: Required properties in schema must be enforced in code and vice versa. +- **Behavior must match manifest metadata**: Versions, `requireSecurityContext`, adapter args, and defaults must reflect what the resource actually does. +- **Schema/manifest version consistency**: When bumping a version in `Cargo.toml`, ensure the corresponding `.dsc.resource.json` manifest is also updated. +- **`noFiltering` semantics**: Export input should be treated as empty when `noFiltering` is declared. +- **Canonical property naming**: Leading underscore (`_`) is only for cross-resource canonical properties. Resource-specific properties use descriptive names (e.g., `sshd_config_filepath`). + +## Operation Consistency + +- **All operations must validate consistently**: If a parameter is validated in `get`, it should also be validated in `set`, `test`, and `export`. +- **What-if must not mutate state**: Verify no code path before the what-if gate can create files, modify config, or change system state. +- **Consistent platform enforcement in what-if**: Platform-restricted operations must enforce restrictions even in what-if mode. + +## Error Handling + +- **Prefer `Result` over panics**: Avoid `unwrap()`/`expect()` on user input or manifest data. Return clear error messages and stable exit codes. +- **`unwrap_or_default()` on serialization is a bug**: `serde_json::to_string(...).unwrap_or_default()` silently emits empty string on failure. Surface the error and exit non-zero. +- **Return `Option` for fallible lookups**: Return `Option` rather than empty string (which resolves to `.`). + +## Destructive Operations + +- **Watch for unresolvable system objects**: In list-reconciling resources (firewall, services), flag logic that disables/removes system-created entries (AppX/UWP rules) that users cannot reliably reference in their declared state. +- **Semantic versioning**: Resources below 1.0 are not design-stable. Breaking changes to input/output shapes require a minor version bump. + +## Regex and Wildcard Handling + +- **Escape all metacharacters**: When converting wildcards to regex, escape `[`, `(`, `+`, `^`, `$`, `|`, `\` -- not just `.`. Unescaped metacharacters cause panics or unexpected matching. diff --git a/.github/instructions/code-review/security.instructions.md b/.github/instructions/code-review/security.instructions.md new file mode 100644 index 000000000..42de16d5c --- /dev/null +++ b/.github/instructions/code-review/security.instructions.md @@ -0,0 +1,42 @@ +--- +applyTo: 'lib/dsc-lib/src/util.rs,lib/dsc-lib-security_context/**,lib/dsc-lib-registry/**,resources/registry/**,resources/windows_firewall/**,resources/windows_service/**,resources/dism_dsc/**' +description: 'Code review guidance for security-sensitive code (ACLs, policy, credentials, elevated resources)' +--- + +# Security Code Review + +This repo manages system configuration. Many resources run elevated and modify security-sensitive +state (ACLs, registry, services, firewall rules, policy files). Security review is critical. + +## Fail Closed + +- **All security checks must fail closed**: If reading a security descriptor, enumerating ACEs, or calling stat fails, return the restrictive/denied result -- never fail open. +- **NULL DACL detection**: A NULL DACL means full access to everyone. Treat `p_dacl.is_null()` as insecure. +- **Do not cache or trust failed verifications**: Trust caches (Authenticode, ACL checks) should only be updated on successful validation. + +## ACL and Permission Checks + +- **Complete write-access checks**: Cover `GENERIC_WRITE` and `GENERIC_ALL` in addition to specific write flags. +- **Inherit-only ACEs**: ACEs with `INHERIT_ONLY_ACE` flag don't apply to the object itself -- don't treat them as granting access to the folder. +- **Non-standard ACE variants**: Callback ACEs and object ACEs can still grant write access. Don't skip them. + +## Policy Bypass Prevention + +- **User inputs must not bypass policy**: CLI flags, env vars, or config changes must not override policy-enforced settings. Policy sources remain authoritative. +- **Enforce declared security context**: If manifests declare `requireSecurityContext`, verify runtime enforcement for every operation. + +## DLL and Native Code Safety + +- **DLL loading security**: Use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` instead of `LoadLibraryW` to prevent DLL hijacking. Critical for elevated resources. +- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and library handles must be cleaned up on all paths including error paths. +- **Check HRESULT returns**: Don't silently ignore Windows API return codes. + +## Secrets + +- **Never leak secrets into logs or output**: Verify redaction when logging output that may contain secure values. +- **Prevent command injection**: Don't embed user-controlled values into PowerShell command strings. Pass arguments/JSON instead. + +## Destructive Operations + +- **Unresolvable system objects**: In list-reconciling resources (firewall, services), skip system-created entries (AppX/UWP rules) that users cannot reliably reference. +- **Service credential changes**: Flag service-configuration code that switches accounts without secure credential handling. diff --git a/.github/instructions/code-review/tests.instructions.md b/.github/instructions/code-review/tests.instructions.md new file mode 100644 index 000000000..4a786246b --- /dev/null +++ b/.github/instructions/code-review/tests.instructions.md @@ -0,0 +1,47 @@ +--- +applyTo: '**/*.tests.ps1,**/tests/**' +description: 'Code review guidance for DSC tests (Pester and Rust integration tests)' +--- + +# Test Code Review + +Tests use Pester 5 for end-to-end CLI testing and Rust's built-in test framework for unit tests. +Tests run cross-platform (Windows, Linux, macOS) in CI. + +## Cross-Platform Correctness + +- **Path separators**: Never use hard-coded `\` in path construction. Always use `Join-Path`. Tests with hard-coded backslashes will fail on Linux/macOS. +- **Platform-specific commands**: `stat -c` is GNU/Linux-specific. Gate on `$IsLinux` explicitly or use PowerShell equivalents. +- **OS gating**: A Context that only checks `!$IsWindows` also runs on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. +- **Use realistic cross-platform fixtures**: Test data should use correct path separators and plausible file locations for the target OS. + +## Test Isolation and Cleanup + +- **Preserve and restore environment variables**: Capture the original value in `BeforeAll` and restore in `AfterAll`. Never set to `$null` unconditionally. +- **Conflicting environment variables**: Tests for lower-priority env vars (e.g., `DSC_RESOURCE_PATH`) must explicitly clear higher-priority ones (`DSC_RESTRICTED_PATH`). +- **ACL and permission restoration**: Capture full original state and restore completely. Do not rely on partial undo or hard-coded permission values. +- **Recursive ACL changes**: If `icacls /T` is used, `AfterAll` must restore children too. +- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket unregister. +- **Use `-ErrorAction Ignore` over `SilentlyContinue`**: When leftover error records could pollute later assertions. + +## Assertions + +- **Test name must match assertions**: If named "X happens only once," assertions must verify the constraint. +- **Assert `$LASTEXITCODE`**: Always check exit code in addition to output content for CLI tests. +- **Prove the intended failure mode**: Negative tests must fail for the exact reason under test, not a broader fallback. +- **Cover both branches**: Test both "present" and "absent" cases explicitly. +- **Array comparisons**: Normalize arrays through JSON conversion before comparing with `Should -Be`. +- **Avoid duplicate reads**: Read file content once into a variable, not multiple times per assertion. +- **Skip guards for cmdlet availability**: Check cmdlet availability in `-Skip`, not just elevation. +- **Ordering assumptions**: `dsc resource list` returns alphabetical order. Sort before position-based assertions. +- **Manifest file naming**: Discovery only loads `.dsc.resource.(json|yaml|yml)` files. Tests with other extensions won't exercise discovery. +- **Prefer `Should -BeExactly`**: Use exact comparisons when full expected result is known. + +## Structure + +- **Use Context blocks for shared setup**: Group found/not-found scenarios with shared `BeforeEach`. +- **Helpers belong near their usage**: Only promote to `build.helpers.psm1` if used across multiple scripts. +- **Test both adapters**: If `PowerShellScript` and `WindowsPowerShellScript` share code, parameterize tests. +- **Distinguish "failed" from "succeeded with errors"**: Separate terminating errors, non-terminating errors, and successful runs with warnings. +- **Add edge-case and malformed-input coverage**: Test boundary conditions whenever only the happy path is covered. +- **Test behavior end-to-end**: Favor tests demonstrating user-visible paths. diff --git a/.github/instructions/code-review/windows.instructions.md b/.github/instructions/code-review/windows.instructions.md new file mode 100644 index 000000000..1b065a3a3 --- /dev/null +++ b/.github/instructions/code-review/windows.instructions.md @@ -0,0 +1,35 @@ +--- +applyTo: 'resources/windows_firewall/**,resources/windows_service/**,resources/WindowsUpdate/**,resources/windows_personalization/**,resources/dism_dsc/**,resources/registry/**,lib/dsc-lib-registry/**,pal/windows/**' +description: 'Code review guidance for Windows-specific code (FFI, COM, registry, DISM, services)' +--- + +# Windows Code Review + +Windows-specific code uses Win32 APIs, COM interfaces, registry access, DISM, and Windows +service management. These resources typically run elevated. + +## FFI Safety + +- **DLL loading security**: Use `LoadLibraryExW` with `LOAD_LIBRARY_SEARCH_SYSTEM32` to prevent DLL hijacking. +- **Resource cleanup with `Drop`**: COM objects, `VARIANT`s, and `HMODULE` handles must be cleaned up on all paths. Implement `Drop` for wrapper types. +- **Check HRESULT returns**: Don't silently ignore Windows API error codes. +- **Iterative handle operations**: When creating nested structures (registry keys), use previous call's result as parent handle, not always root. + +## ACL and Permissions + +- **Fail closed**: Security descriptor read failures must return "not secure." +- **NULL DACL = full access**: Always treat as insecure. +- **Complete write mask**: Check `GENERIC_WRITE`, `GENERIC_ALL`, and all relevant specific flags. +- **Inherit-only ACEs**: Don't treat `INHERIT_ONLY_ACE` as applying to the folder itself. +- **Callback/object ACEs**: Non-standard ACE types can still grant access. Don't skip them. + +## Service and Firewall Resources + +- **Unresolvable system objects**: AppX/UWP firewall rules with names like "ms-resource://" cannot be referenced by users. Skip them in destructive reconciliation. +- **Service credential handling**: Flag logon identity changes without secure credential supply. +- **Protocol normalization**: Handle `Option` correctly -- protocol may be required for the operation even if optional in the input struct. + +## COM Enumeration + +- **`VariantClear` on all paths**: Clear variants even when `.cast()` or other operations fail early via `?`. +- **Single-item vs collection**: COM enumerations may return collections. Handle both cases. From c90648b2ffc68890f33ff60c94b6a8d7d690ed06 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 09:19:30 -0700 Subject: [PATCH 04/10] Restructure adapter instructions for language-agnostic guidance Move general adapter design principles (single-mode operation, input contract, structured I/O, exit codes, resource enumeration) to the top. Add explicit guidance that the validate operation is deprecated and adapters should rely on JSONSchema for validation. PowerShell-specific patterns are now in their own subsection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../code-review/adapter.instructions.md | 70 ++++++++++++++----- 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/.github/instructions/code-review/adapter.instructions.md b/.github/instructions/code-review/adapter.instructions.md index 9945cd2da..7de30c845 100644 --- a/.github/instructions/code-review/adapter.instructions.md +++ b/.github/instructions/code-review/adapter.instructions.md @@ -5,27 +5,65 @@ description: 'Code review guidance for DSC adapters (PowerShell, WMI, etc.)' # Adapter Code Review -Adapters bridge DSC to external resource ecosystems (PowerShell DSC v1/v2 resources, WMI, etc.). -They run PowerShell runspaces, manage module loading, and translate between DSC and native formats. +Adapters bridge DSC to external resource ecosystems (PowerShell DSC v1/v2 resources, WMI providers, +etc.). An adapter is a single-mode executable that receives the path or content to the adapted +resource along with the type name and version information. -## Module Import Safety +## General Adapter Design -- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects when multiple versions are loaded. Always select a single module (highest version) before calling methods. -- **Error handling around module imports**: Wrap import probes in try/catch so one failing module doesn't abort entire resource enumeration during cache refresh. -- **`$using:` in parallel blocks**: Variables from parent scope are not available inside `ForEach-Object -Parallel`. Use `$using:variableName`. +- **Single-mode operation**: Adapters operate in a single mode -- they receive input describing + which adapted resource to invoke and pass through the operation. Do not add multi-mode dispatch + logic inside an adapter; each invocation handles exactly one resource call. +- **Input contract**: Adapters receive the resource type name, version information, and either a + path to or the content of the adapted resource definition. Verify the adapter correctly parses + and forwards all of these. +- **`validate` is deprecated**: The `validate` operation must NOT be implemented by adapters. + Validation is handled by JSONSchema. Flag any new code that adds a `validate` operation or + processes validate requests. +- **Schema-based validation**: Adapted resources should provide JSONSchema for input validation. + The adapter should not re-implement validation logic that the schema already covers. +- **Exit codes**: Adapters must exit 0 on success and non-zero on failure. Ensure all error paths + produce a non-zero exit code with structured error output on stderr. +- **Structured I/O**: Adapters communicate via JSON on stdin/stdout. Verify JSON serialization + round-trips correctly, especially for nested objects, null values, and arrays. +- **Resource enumeration**: When listing available resources, adapters must accurately report + type names, versions, and capabilities. Do not advertise capabilities the adapted resource + does not support. -## Error Handling +## Secrets and Security (All Adapters) -- **Distinguish terminating vs non-terminating errors**: Non-terminating errors in `$ps.HadErrors` should not produce non-zero exit. Only terminating errors (exceptions) should cause adapter failure. -- **Use `$_` in catch blocks, not `$error`**: `$error` is the global error collection. Use `$_` (the caught exception) for condition checks. -- **Flush trace queues before exit on all paths**: If a catch/finally path exits the script, drain the trace queue first to avoid losing diagnostic messages. +- **Prevent command injection**: Flag code that embeds user-controlled values (secrets, vault + names, paths) into command strings. Prefer passing arguments or structured data. +- **Never leak secrets into logs**: Verify redaction is applied when logging resource output + that may contain secure values. -## Concurrency and Event Handling +## PowerShell Adapter Specifics -- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use `while queue.TryDequeue(...)` as the single loop condition -- `IsEmpty` is only an approximation. -- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket `Get-EventSubscriber | Unregister-Event`. +The PowerShell adapter runs PowerShell runspaces to invoke DSC v1/v2/v3 resources. These +patterns apply specifically to PowerShell-based adapter code. -## Secrets and Security +### Module Import Safety -- **Prevent command injection**: Flag code that embeds user-controlled values (secrets, vault names, paths) into PowerShell command strings. Prefer passing arguments/JSON. -- **Never leak secrets into logs**: Verify redaction is applied when logging resource output that may contain secure values. +- **Module import can return arrays**: `Get-Module` can return multiple `PSModuleInfo` objects + when multiple versions are loaded. Always select a single module (highest version) before + calling methods. +- **Error handling around module imports**: Wrap import probes in try/catch so one failing module + doesn't abort entire resource enumeration during cache refresh. +- **`$using:` in parallel blocks**: Variables from parent scope are not available inside + `ForEach-Object -Parallel`. Use `$using:variableName`. + +### Error Handling + +- **Distinguish terminating vs non-terminating errors**: Non-terminating errors in `$ps.HadErrors` + should not produce non-zero exit. Only terminating errors (exceptions) should cause adapter failure. +- **Use `$_` in catch blocks, not `$error`**: `$error` is the global error collection. Use `$_` + (the caught exception) for condition checks. +- **Flush trace queues before exit on all paths**: If a catch/finally path exits the script, drain + the trace queue first to avoid losing diagnostic messages. + +### Concurrency and Event Handling + +- **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use + `while queue.TryDequeue(...)` as the single loop condition -- `IsEmpty` is only an approximation. +- **Event subscriber cleanup**: Filter by `-SourceIdentifier`, not blanket + `Get-EventSubscriber | Unregister-Event`. From a9092104cdc127e7d13c1eab420c0809faac369d Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:01:34 -0700 Subject: [PATCH 05/10] Add CLI/server mode parity requirement to review instructions CLI additions must be reflected in the DSC server mode JSONRPC APIs. Rename section from 'Server Mode (MCP)' to 'Server Mode (JSONRPC)' for accuracy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/code-review/cli.instructions.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/instructions/code-review/cli.instructions.md b/.github/instructions/code-review/cli.instructions.md index 2ce4fe609..e5ebc92da 100644 --- a/.github/instructions/code-review/cli.instructions.md +++ b/.github/instructions/code-review/cli.instructions.md @@ -19,8 +19,11 @@ output formatting (JSON/YAML/table), and the MCP server mode. - **Prefer `Result` over panics**: Avoid `unwrap()`/`expect()` on user input or parsed data. Return clear error messages and exit codes. - **Names must match semantics**: Flag singular/plural mismatches, stale help text, and deprecated options still visibly advertised. -## Server Mode (MCP) +## Server Mode (JSONRPC) +- **CLI additions must be reflected in server mode**: Any new CLI subcommand or capability must + also be exposed as a corresponding JSONRPC API in server mode. Flag PRs that add CLI + functionality without a matching server-mode implementation or a tracking issue for follow-up. - **Schema/typing for tool parameters**: Prefer typed parameters over generic `serde_json::Value` with runtime validation. This gives clients correct schemas. - **Tool name accuracy**: Ensure locale strings and schema descriptions reference the correct tool name (e.g., `list_dsc_functions` not `list_dsc_function`). From 106811342235319f454f59b9151b95caba490faa Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:05:37 -0700 Subject: [PATCH 06/10] Address PR review feedback: fix paths and overly broad guidance - Replace non-existent pal/windows/** and pal/linux/** with actual lib/dsc-lib-pal/** in applyTo patterns and routing table - Expand tests applyTo to include **/test/** and **/Tests/** directories (covers resources/apt/test/, adapters/powershell/Tests/, etc.) - Narrow println! guidance: debug println! should use tracing macros, but println! is acceptable for intentional CLI user-facing output Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/code-review.instructions.md | 8 ++++---- .github/instructions/code-review/linux.instructions.md | 2 +- .github/instructions/code-review/tests.instructions.md | 2 +- .github/instructions/code-review/windows.instructions.md | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 36c92c767..e40aee9f6 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -20,12 +20,12 @@ Use the path-based mapping below to determine which area instructions apply to t | Adapters | `adapters/` | `code-review/adapter.instructions.md` | | Extensions | `extensions/`, `lib/dsc-lib/src/extensions/` | `code-review/extension.instructions.md` | | CLI | `dsc/src/` | `code-review/cli.instructions.md` | -| Tests | `**/*.tests.ps1`, `**/tests/` | `code-review/tests.instructions.md` | +| Tests | `**/*.tests.ps1`, `**/tests/`, `**/test/`, `**/Tests/` | `code-review/tests.instructions.md` | | Libraries | `lib/` | `code-review/library.instructions.md` | | Security | `lib/dsc-lib/src/util.rs`, `lib/dsc-lib-security_context/`, `lib/dsc-lib-registry/`, `resources/registry/`, `resources/windows_firewall/`, `resources/windows_service/`, `resources/dism_dsc/` | `code-review/security.instructions.md` | | Performance | Any `*.rs` file in hot paths | `code-review/performance.instructions.md` | -| Windows | `resources/windows_*`, `resources/dism_dsc/`, `resources/registry/`, `lib/dsc-lib-registry/`, `pal/windows/` | `code-review/windows.instructions.md` | -| Linux/macOS | `resources/apt/`, `resources/sshdconfig/`, `resources/brew/`, `pal/linux/` | `code-review/linux.instructions.md` | +| Windows | `resources/windows_*`, `resources/dism_dsc/`, `resources/registry/`, `lib/dsc-lib-registry/`, `lib/dsc-lib-pal/` | `code-review/windows.instructions.md` | +| Linux/macOS | `resources/apt/`, `resources/sshdconfig/`, `resources/brew/`, `lib/dsc-lib-pal/` | `code-review/linux.instructions.md` | Multiple areas may apply to a single file. For example, `resources/windows_firewall/` triggers both the Resource, Windows, and Security instructions. @@ -47,7 +47,7 @@ These apply to ALL areas: - **Accurate comments**: If code behavior changes, update comments to match. - **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. - **Doc comments matching implementation**: Update when described behavior doesn't match reality. -- **Remove debug print statements**: No `println!` in production or test code. Use `debug!`/`trace!` macros. +- **Remove debug print statements**: Do not use `println!` for debugging -- use `debug!`/`trace!` macros. Note that `println!` is acceptable for intentional CLI user-facing output. - **Locale/i18n string accuracy**: Verify key names match the keyword/function they describe. - **Dead locale strings**: Do not add i18n keys never referenced in code. - **Prefer scan-friendly wording**: Log/error strings should be concise for readability in diagnostics. diff --git a/.github/instructions/code-review/linux.instructions.md b/.github/instructions/code-review/linux.instructions.md index ab15b975a..da320a7ce 100644 --- a/.github/instructions/code-review/linux.instructions.md +++ b/.github/instructions/code-review/linux.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: 'resources/apt/**,resources/sshdconfig/**,pal/linux/**,resources/brew/**' +applyTo: 'resources/apt/**,resources/sshdconfig/**,lib/dsc-lib-pal/**,resources/brew/**' description: 'Code review guidance for Linux/macOS-specific code (SSH, apt, platform abstraction)' --- diff --git a/.github/instructions/code-review/tests.instructions.md b/.github/instructions/code-review/tests.instructions.md index 4a786246b..ceabe713d 100644 --- a/.github/instructions/code-review/tests.instructions.md +++ b/.github/instructions/code-review/tests.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: '**/*.tests.ps1,**/tests/**' +applyTo: '**/*.tests.ps1,**/tests/**,**/test/**,**/Tests/**' description: 'Code review guidance for DSC tests (Pester and Rust integration tests)' --- diff --git a/.github/instructions/code-review/windows.instructions.md b/.github/instructions/code-review/windows.instructions.md index 1b065a3a3..54bc3ab30 100644 --- a/.github/instructions/code-review/windows.instructions.md +++ b/.github/instructions/code-review/windows.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: 'resources/windows_firewall/**,resources/windows_service/**,resources/WindowsUpdate/**,resources/windows_personalization/**,resources/dism_dsc/**,resources/registry/**,lib/dsc-lib-registry/**,pal/windows/**' +applyTo: 'resources/windows_firewall/**,resources/windows_service/**,resources/WindowsUpdate/**,resources/windows_personalization/**,resources/dism_dsc/**,resources/registry/**,lib/dsc-lib-registry/**,lib/dsc-lib-pal/**' description: 'Code review guidance for Windows-specific code (FFI, COM, registry, DISM, services)' --- From 96801a13866fda36e9d4d7aa93440e09efb7b665 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:13:05 -0700 Subject: [PATCH 07/10] Add rust-i18n t! macro requirement to general instructions All Rust user-facing strings must use the t! macro from rust-i18n with text defined in en-us.toml rather than hard-coded in source files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/code-review.instructions.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index e40aee9f6..4364fc673 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -48,10 +48,15 @@ These apply to ALL areas: - **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. - **Doc comments matching implementation**: Update when described behavior doesn't match reality. - **Remove debug print statements**: Do not use `println!` for debugging -- use `debug!`/`trace!` macros. Note that `println!` is acceptable for intentional CLI user-facing output. -- **Locale/i18n string accuracy**: Verify key names match the keyword/function they describe. -- **Dead locale strings**: Do not add i18n keys never referenced in code. - **Prefer scan-friendly wording**: Log/error strings should be concise for readability in diagnostics. +## Internationalization (All Rust Code) + +- **Use `rust-i18n` with the `t!` macro**: All user-facing strings in Rust code must use the `t!` macro from the `rust-i18n` crate. Do not hard-code English strings in source files. +- **Strings go in `en-us.toml`**: Localized text must be defined in the `en-us.toml` locale file, not inline in Rust source. +- **Locale key accuracy**: Verify key names match the keyword/function they describe. +- **No dead locale strings**: Do not add i18n keys that are never referenced in code. + ## CI/CD (All Areas) - **Fork permission limitations**: Steps posting PR comments should gate on same-repo PRs or use `continue-on-error: true`. From 649e4028dffefa2deaee6fd4a2a93f0308989fdf Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:14:41 -0700 Subject: [PATCH 08/10] Add helper function extraction guidance to general principles Repetitive code should be flagged and a shared helper function suggested. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/code-review.instructions.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 4364fc673..8dd6554c2 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -41,6 +41,7 @@ These apply to ALL areas: - **Table output is not a stable API**: JSON/YAML are canonical machine-readable outputs. Table layout changes are not breaking. - **Separate input/output structs can be intentional**: Resources often distinguish desired state from observed state. Do not assume duplicate structs are accidental. - **Intentional design decisions**: When maintainers explicitly label behavior as "intentional", do not re-flag. +- **Extract repetitive code into helper functions**: When the same logic appears in multiple places, flag it and suggest extracting a shared helper function to reduce duplication and improve maintainability. ## Documentation and Logging (All Areas) From 5fe848e128e3f4cfe424d0ab41870660dcf8356c Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:15:56 -0700 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/instructions/code-review/windows.instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/instructions/code-review/windows.instructions.md b/.github/instructions/code-review/windows.instructions.md index 54bc3ab30..2a6d5e545 100644 --- a/.github/instructions/code-review/windows.instructions.md +++ b/.github/instructions/code-review/windows.instructions.md @@ -27,7 +27,7 @@ service management. These resources typically run elevated. - **Unresolvable system objects**: AppX/UWP firewall rules with names like "ms-resource://" cannot be referenced by users. Skip them in destructive reconciliation. - **Service credential handling**: Flag logon identity changes without secure credential supply. -- **Protocol normalization**: Handle `Option` correctly -- protocol may be required for the operation even if optional in the input struct. +- **Protocol normalization**: Handle `Option` protocol correctly -- protocol may be required for the operation even if optional in the input struct. ## COM Enumeration From 596b5bd6d603a801d1cfadc4b4290235811a91a1 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 27 Jul 2026 10:17:03 -0700 Subject: [PATCH 10/10] Fix performance scope wording and compile-error guidance - Performance instructions: clarify they apply to all Rust code (matching the applyTo pattern) but are most critical in hot paths - Compile-error principle: rephrase to evidence-based ('cite a specific rule violation') instead of relying on CI results the reviewer may not have in context Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/code-review.instructions.md | 2 +- .github/instructions/code-review/performance.instructions.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 8dd6554c2..c159bebbf 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -34,7 +34,7 @@ both the Resource, Windows, and Security instructions. These apply to ALL areas: -- **Do not claim code will not compile unless you are certain**: Multiple reviews were rejected because Copilot incorrectly claimed Rust ownership/borrowing errors. If the code compiles and tests pass in CI, do not assert otherwise. +- **Do not claim code will not compile unless you can cite a specific rule violation**: Avoid speculative "this won't compile" claims about Rust ownership/borrowing. Only flag compile errors when you can point to a concrete language rule being violated. - **Test resources are not production code**: Code in `tools/dsctest/` is for testing only and is never user-facing. Do not require production-grade error handling in test harnesses unless panics would hide regressions. - **Automatically-generated files**: Files like `lib/dsc-lib-jsonschema/.versions.json` are updated by build automation. Do not flag version bumps as unintentional. - **Do not demand large abstractions in small PRs**: Suggest extracting a helper function. Do not block a focused PR by requesting trait/framework redesigns -- that is follow-up work. diff --git a/.github/instructions/code-review/performance.instructions.md b/.github/instructions/code-review/performance.instructions.md index 352de8e87..ae0ba8179 100644 --- a/.github/instructions/code-review/performance.instructions.md +++ b/.github/instructions/code-review/performance.instructions.md @@ -5,8 +5,8 @@ description: 'Code review guidance for performance (Rust allocations, caching, s # Performance Code Review -Performance matters most in the engine hot paths: resource discovery (many manifests), -schema caching, and configuration processing. +These patterns apply to all Rust code but are most critical in engine hot paths: resource +discovery (many manifests), schema caching, and configuration processing. ## Caching