|
| 1 | +--- |
| 2 | +applyTo: "**/*.rs, **/*.ps1, **/*.psm1, **/*.tests.ps1, **/*.json" |
| 3 | +--- |
| 4 | + |
| 5 | +# Code Review Instructions for DSC Repository |
| 6 | + |
| 7 | +These instructions guide Copilot when performing code reviews on pull requests in this repository. |
| 8 | +Focus on high-confidence, actionable findings. Do not comment on style, formatting, or trivial issues. |
| 9 | + |
| 10 | +## General Principles |
| 11 | + |
| 12 | +- **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. |
| 13 | +- **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. |
| 14 | +- **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. |
| 15 | + |
| 16 | +## Rust Code Patterns |
| 17 | + |
| 18 | +### Caching and State Management |
| 19 | + |
| 20 | +- **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. |
| 21 | +- **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. |
| 22 | +- **Unused imports after refactoring**: When cache writes or other logic are centralized, verify that moved-from modules no longer import the now-unused symbols. |
| 23 | + |
| 24 | +### API Surface and Visibility |
| 25 | + |
| 26 | +- **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. |
| 27 | +- **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). |
| 28 | +- **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. |
| 29 | + |
| 30 | +### Error Handling and Security |
| 31 | + |
| 32 | +- **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. |
| 33 | +- **NULL DACL detection**: A NULL DACL means full access to everyone. Security checks must treat `p_dacl.is_null()` as insecure. |
| 34 | +- **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. |
| 35 | +- **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. |
| 36 | +- **Return `Option` for fallible lookups**: When a function can legitimately fail to find a path or value, return `Option<PathBuf>` (or similar) rather than an empty string, which can resolve to the current directory and cause confusing downstream behavior. |
| 37 | + |
| 38 | +### Windows FFI and COM Safety |
| 39 | + |
| 40 | +- **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. |
| 41 | +- **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. |
| 42 | +- **Check HRESULT returns**: Windows API functions that return `HRESULT` (like `VariantClear`) should have their return values checked or at minimum logged, not silently ignored. |
| 43 | +- **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. |
| 44 | + |
| 45 | +### Serialization and Consistency |
| 46 | + |
| 47 | +- **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. |
| 48 | +- **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. |
| 49 | +- **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. |
| 50 | + |
| 51 | +### Concurrency |
| 52 | + |
| 53 | +- **`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. |
| 54 | + |
| 55 | +### What-If / Dry-Run Correctness |
| 56 | + |
| 57 | +- **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. |
| 58 | +- **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. |
| 59 | + |
| 60 | +## PowerShell / Pester Test Patterns |
| 61 | + |
| 62 | +### Cross-Platform Correctness |
| 63 | + |
| 64 | +- **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. |
| 65 | +- **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. |
| 66 | +- **OS gating**: A Context labeled "Linux" that only checks `!$IsWindows` will also run on macOS. Be explicit with `$IsLinux` or `$IsMacOS`. |
| 67 | + |
| 68 | +### Test Isolation and Cleanup |
| 69 | + |
| 70 | +- **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. |
| 71 | +- **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. |
| 72 | +- **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`. |
| 73 | +- **Recursive ACL changes**: If `icacls /T` is used to recursively modify ACLs, `AfterAll` must restore children as well, not just the top-level directory. |
| 74 | +- **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`. |
| 75 | + |
| 76 | +### Test Assertions and Naming |
| 77 | + |
| 78 | +- **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. |
| 79 | +- **Ordering assumptions**: `dsc resource list` returns items in alphabetical order. Tests should document this assumption or sort results before asserting on specific positions. |
| 80 | +- **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. |
| 81 | +- **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. |
| 82 | +- **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`). |
| 83 | +- **Array comparisons**: `Should -Be` can be unreliable for array comparisons. Normalize arrays through JSON conversion before comparing, or compare individual elements. |
| 84 | +- **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. |
| 85 | + |
| 86 | +### Test Structure and Readability |
| 87 | + |
| 88 | +- **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. |
| 89 | +- **Separation between sections**: Maintain blank lines between logical sections in configuration and test files for readability. |
| 90 | +- **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. |
| 91 | + |
| 92 | +## PowerShell Adapter Patterns |
| 93 | + |
| 94 | +- **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. |
| 95 | +- **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. |
| 96 | +- **`$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. |
| 97 | +- **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. |
| 98 | + |
| 99 | +## Design and Architecture Patterns |
| 100 | + |
| 101 | +### Settings and Configuration Precedence |
| 102 | + |
| 103 | +- **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. |
| 104 | +- **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. |
| 105 | +- **`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. |
| 106 | + |
| 107 | +### Backward Compatibility |
| 108 | + |
| 109 | +- **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. |
| 110 | +- **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. |
| 111 | +- **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. |
| 112 | + |
| 113 | +### Regex and Wildcard Handling |
| 114 | + |
| 115 | +- **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. |
| 116 | + |
| 117 | +### Resource Manifest and Schema Coherence |
| 118 | + |
| 119 | +- **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. |
| 120 | +- **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. |
| 121 | +- **`noFiltering` semantics**: When a resource declares `export.schema: noFiltering`, the export input should be treated as empty (no filter properties passed to the resource). |
| 122 | +- **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. |
| 123 | + |
| 124 | +### CI/CD and GitHub Actions |
| 125 | + |
| 126 | +- **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`. |
| 127 | +- **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). |
| 128 | +- **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. |
| 129 | + |
| 130 | +## Documentation and Logging |
| 131 | + |
| 132 | +- **Accurate comments**: If code behavior changes, update comments to match. A comment that says "retains quoting" when the code actually strips quotes is misleading. |
| 133 | +- **Log level appropriateness**: Full `PATH` contents should be `trace!` level, not `debug!`. Sensitive or extremely verbose data should use the lowest practical log level. |
| 134 | +- **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. |
| 135 | +- **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. |
| 136 | +- **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. |
| 137 | +- **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. |
0 commit comments