Add code review instructions derived from PR feedback patterns

.github/instructions/code-review.instructions.md

@@ -0,0 +1,67 @@
+---
+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.
+
+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.
+
+## Area Routing
+
+| 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/`, `**/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/`, `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.
+
+## General Principles (False-Positive Avoidance)
+
+These apply to ALL areas:
+
+- **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.
+- **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)
+
+- **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**: Do not use `println!` for debugging -- use `debug!`/`trace!` macros. Note that `println!` is acceptable for intentional CLI user-facing output.
+- **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`.
+- **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.

.github/instructions/code-review/adapter.instructions.md

@@ -0,0 +1,69 @@
+---
+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 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.
+
+## General Adapter Design
+
+- **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.
+
+## Secrets and Security (All Adapters)
+
+- **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.
+
+## PowerShell Adapter Specifics
+
+The PowerShell adapter runs PowerShell runspaces to invoke DSC v1/v2/v3 resources. These
+patterns apply specifically to PowerShell-based adapter code.
+
+### 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`.

.github/instructions/code-review/cli.instructions.md

@@ -0,0 +1,33 @@
+---
+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 (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`).
+
+## 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.

.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.

.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.

.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`.

.github/instructions/code-review/linux.instructions.md

@@ -0,0 +1,30 @@
+---
+applyTo: 'resources/apt/**,resources/sshdconfig/**,lib/dsc-lib-pal/**,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.