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>

Author: SteveL-MSFT

.github/instructions/code-review.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.

.github/instructions/code-review/adapter.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.

.github/instructions/code-review/cli.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/engine.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/extension.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/library.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.

.github/instructions/code-review/linux.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.

.github/instructions/code-review/performance.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<PathBuf>` 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.

.github/instructions/code-review/resource.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.