From 2404912fb4ce5f33766dd6a71a7d362a15d5f169 Mon Sep 17 00:00:00 2001 From: Branden Bonaby <105318831+bbonaby@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:04:07 -0700 Subject: [PATCH] Codify MXC agent and code-review guidance Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c5f0ea5-7586-4184-a1b2-e26c9ea00a2b --- .github/copilot-instructions.md | 10 ++ .github/skills/mxc-code-review/SKILL.md | 167 ++++++++++++++++++++++++ AGENTS.md | 81 ++++++++++++ CLAUDE.md | 5 + CONTRIBUTING.md | 16 ++- 5 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 .github/skills/mxc-code-review/SKILL.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b2ec44d7a..38ae36778 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -78,14 +78,17 @@ Prefer the smallest test command covering the change. Host-dependent backend sui - Production parsing dispatches through exact closed contracts and their version-specific adapters into private `CommonRequestIR` normalization input. - Preserve optional-field presence through parsing and binding. Apply defaults and semantic validation in the backend. + - New features use their intended permanent JSON location in the exact development contract. JSON placement, publication eligibility, and runtime experimental authorization are separate. Update the matching closed request types, adapt them into `CommonRequestIR` or a phase-specific runtime config, and regenerate the registered exact schema and TypeScript artifacts. + - Never edit `schemas/stable/`; released schemas are immutable. - Never hand-edit generated development schemas or generated TypeScript wire types. - `schemas/schema-version.json` is the canonical source for compatibility constants. + - In the .NET SDK (`sdk/dotnet/`), every serialized or deserialized wire type must be registered as a `[JsonSerializable]` root in `MxcJsonContext` (`MxcJsonSerialization.cs`) and routed through the `MxcJson` helpers, so the @@ -152,3 +155,10 @@ When creating issues through an API, explicitly set the issue type and these exa | Task | `Task` | `Issue-Task`, `Needs-Triage` | Do not fabricate missing required issue fields. For security issues or BSODs, do not attach dumps, logs, or traces to a public GitHub issue; direct sensitive material to `secure@microsoft.com` and reference the issue. + +## Engineering and review guidance + +Follow the additional [design, communication, and validation standards](../AGENTS.md#design-for-the-codebase-not-just-the-feature) +in `AGENTS.md`. Use the [MXC review skill](skills/mxc-code-review/SKILL.md) +when reviewing changes, including its requirements for functional CI validation +and bug regression coverage. diff --git a/.github/skills/mxc-code-review/SKILL.md b/.github/skills/mxc-code-review/SKILL.md new file mode 100644 index 000000000..5249b604b --- /dev/null +++ b/.github/skills/mxc-code-review/SKILL.md @@ -0,0 +1,167 @@ +--- +name: mxc-code-review +description: Review MXC pull requests and diffs for SDK breaking changes, CI gate regressions, useful SDK documentation, maintainable design, and containment. +--- + +# MXC code review + +Read `AGENTS.md`, `.github/copilot-instructions.md`, matching language +instructions, and the affected subsystem docs. Follow the changed flow through +its callers, shared code, backends, and failure paths. Do not make edits during +review or stop after finding one working path. + +## Check SDK compatibility first + +- Compare changed public APIs with the existing supported version in Rust, + .NET, and Node. Check removals, renames, signatures, types, import paths, + serialization, defaults, errors, cancellation, and resource ownership. + Look for source, binary, and behavioral breaks, not only compile failures. + +- Report every confirmed breaking change to an existing supported SDK API in + the **highest review category: P0 / critical**. Put these findings first and + label them **SDK breaking change**. This is a maintainer compatibility alert; + it does not imply a security vulnerability or universal runtime failure. + +- Identify the affected SDK and version, the old and new behavior, and a + concrete caller that breaks. Call it out even when the PR describes the + change as intentional. Maintainers decide whether to accept it; the reviewer + must not silently waive it or assume acceptance. + +- Published SDK APIs require a new versioned surface for breaking changes. + Check that the existing version and its references remain intact. Adding + a new version while preserving the old one is not itself a breaking change. + +## Check the design in context + +- Does the change fit the existing design and remain understandable to the + next maintainer, or does it merely get the requested feature working? + +- Look for duplicated paths, feature-specific helper variants, narrow + exceptions, and unnecessary abstractions. Prefer fixing the responsible + shared component over adding another special case. + +- Check responsibility boundaries. Parsing should decode contracts and retain + field presence; backend-specific defaults, policy checks, and enforcement + belong in the backend, not the shared parser. + +- If a feature requires a broad rewrite or large deletion, call out the + maintenance risk and recommend smaller, reviewable vertical slices with + behavior coverage. Do not demand a rewrite for a local problem. + +## Check MXC rules + +- **Rust package boundaries:** Inspect `src/Cargo.toml` workspace members and + changed Cargo dependencies. Internal engine, contract, shared-runtime, + backend, build, and schema implementations belong as modules in `mxc-sdk`, + not new workspace crates or publishable path dependencies. Required process + boundaries use existing `[[bin]]` patterns; distinguish legitimate tools and + FFI packages from unnecessary SDK implementation crates. + +- **Dispatch and build:** `mxc_engine` owns backend routing. No dispatch or + backend implementation dependencies in `mxc_common`, backend-selection + matches in thin executors, or recursive Cargo invocation in `build.rs`. + Keep platform/feature gates and build-time staging intact. + +- **Containment and errors:** Unsupported policy fails closed before container + creation. Preserve failure propagation, cancellation, cleanup, handle + ownership, and panic containment across the C ABI. Check each changed + execution surface, not only create-and-run. + +- **Contracts:** Preserve optional-field presence and version-specific parsing. + Do not edit released schemas or hand-edit generated artifacts. Development + fields need the proper contract, adapter, regenerated artifacts, and tests; + experimental authorization does not replace contract or host validation. + +- **SDKs:** Compare affected Rust, .NET, and Node versioned references. Preserve + field meanings, optional-field presence, and language-appropriate naming. + Confirm .NET JSON source-generation registration and AOT smoke coverage. + +- **Host compatibility:** New Windows backend features need concrete host + prerequisites, runtime capability checks, actionable unsupported-feature + errors, and a brief update to existing user guidance when requirements or + behavior change. Document supported builds, limits, and remedies, not a + backend implementation walkthrough. SDK/schema versions alone cannot prove + host support. Telemetry requires MXC-owned consent and restrictive admin policy. + +## Keep SDK documentation useful + +SDK READMEs and versioned API references are consumer-facing entry points. +Review additions and removals against what a caller needs to install, choose, +use, and safely clean up an operation. + +- Keep signatures, examples, defaults, prerequisites, limitations, and error + remedies accurate and consistent with the supported API. Check affected + links and examples when content is moved or removed. + +- Flag changes that hide supported behavior, remove necessary usage guidance, + contradict the API, or bury instructions under irrelevant implementation + detail. Keep backend internals and contributor workflows in developer docs; + link them only when they help the reader's task. + +- Make additions and removals leave a coherent final document. Avoid agent + reasoning, change diaries, discarded designs, and repetition. Do not demand + more documentation merely because code changed, or report wording nits as + defects. + +## Check for weakened CI gates + +Inspect removed or changed checks, including workflows, reusable jobs, build +scripts, test commands, and matrix configuration. Do not assume a green check +means the same validation still runs. + +- Check trigger and path filters, job conditions and dependencies, platform + and feature coverage, test discovery, exclusions, and skipped tests. + Look for checks that disappear or stop covering the changed behavior. + +- Check failure propagation. `continue-on-error`, ignored exit codes, removed + assertions, and success-shaped fallbacks can make a failing build or test + appear green. Trace the relevant command through its callers to the final + check result. + +- Flag removed or weakened validation that lets a concrete regression pass. + Name the lost check, the affected path or platform, and what CI no longer + detects. Do not accept disabling the failing check as a fix for the failure. + +- Legitimate replacement or retirement is allowed. Verify equivalent coverage + still runs and gates the result, or that the covered behavior is actually + removed. An explanation alone does not establish equivalent coverage. + +## Require behavior coverage + +Feature changes need functional validation in the applicable CI suite. +Backend behavior needs coverage in the scheduled/nightly validation runs on +suitable hosts. Bug fixes need a regression scenario for the reported failure, +preferably shown to fail before the fix and pass afterward. Verify the scenario +is wired into CI and checks observable behavior through the relevant API or +backend, including affected interactions and failures. + +Do not accept a collection of tiny unit tests as a substitute for that coverage. +Unit tests, including those at the bottom of `.rs` files, can support isolated +logic; passing tests that mirror new helpers do not establish that a feature +works in MXC. Prefer extending existing functional scenarios over test-count +growth. Follow the validation runbook and backend guide; report specific gaps, +not demands for unrelated tests. Documentation-only edits need no runtime tests. + +## Write useful findings + +Focus on high- and medium-severity defects; critical failures always qualify. +Leave out low-severity issues, style nits, speculative scenarios, and optional +refactoring. Apply the highest-priority SDK breaking-change rule above; +otherwise, do not inflate severity just to include a finding. + +Review all affected paths before submitting, combine related findings, and +post one consolidated review. Do not drip-feed findings across rounds or +reopen resolved points without new evidence. New edits still need review for +material regressions, not unrelated cleanup. + +Use plain language an everyday developer can act on. Give the severity, +file/line, triggering situation, incorrect behavior, and consequence. Include +enough detail to explain the issue without jargon, long essays, or wording so +short that the reader must guess. Do not include an agent's internal reasoning. + +Keep comments and requested documentation focused on non-obvious reasons and +useful behavior, usage, and limits, not implementation diaries. Separate +paragraph-length list items with a blank line; keep short lists compact. +Report concrete defects and material maintenance risks, not speculative bugs +or style preferences. Distinguish missing test evidence from a confirmed +runtime failure. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..789797fbe --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,81 @@ +# MXC agent instructions + +Read and follow [.github/copilot-instructions.md](.github/copilot-instructions.md) +before writing or reviewing code. It contains MXC's architecture, compatibility, +security, build, and SDK rules. The expectations below apply in addition. + +## Design for the codebase, not just the feature + +- Understand the existing flow before editing: callers, shared helpers, + backend ownership, errors, cleanup, and affected SDK entry points. A working + happy path is not enough; check how the change composes with existing behavior. + +- Put behavior where it belongs. Parsers decode contracts and preserve field + presence; backend-specific defaults, policy checks, and enforcement belong + in the backend, not shared parsing code. + +- Reuse the normal implementation path. Do not add a parallel helper, a + feature-specific variant, or a narrow exception just to make a feature work. + If the design cannot express the feature cleanly, refactor the responsible + component while preserving existing callers and behavior. + +- Choose the simplest design that fits the current requirements. Add an + abstraction only when it solves a real problem; do not build for imagined + reuse. Name functions for their responsibility, not a growing list of cases. + +- Consider whether a maintainer can understand and safely extend the result. + Completing the requested feature does not excuse duplication, misplaced + responsibilities, weakened checks, or hidden failure paths. + +- If adding a feature requires replacing or deleting a large part of a + subsystem, split the work into reviewable vertical slices: a small part of + the feature working through the relevant layers, with its tests. Separate + prerequisite refactoring from behavior changes; keep each step buildable + and preserve existing behavior. Do not attempt the whole rewrite at once. + +## Write for everyday developers + +Use plain, precise language in code reviews, comments, documentation, and +handoffs. Explain the concrete behavior and consequence; avoid invented jargon +and research-paper language. Be concise without leaving out the detail needed +to understand or act. Neither verbosity nor oversimplification is helpful. + +Keep comments sparse and short: explain a non-obvious reason or constraint, +not what the next line does. Prefer clear names and straightforward code. + +Documentation should describe the finished design, behavior, and usage, not +an agent's thought process, implementation diary, or discarded approaches. +Update the relevant canonical document rather than adding overlapping Markdown. +Document only what the reader needs: supported behavior, usage, prerequisites, +limitations, and useful error remedies. Include implementation detail only when +it explains a necessary maintenance or design decision, in developer docs. + +Separate paragraph-length Markdown list items with a blank line; keep short +lists compact. + +## Require meaningful behavior coverage + +- For a feature, add or extend a functional validation test in the applicable + CI suite. Backend behavior must run in scheduled/nightly validation on + the relevant hosts; adding a test file without wiring it into CI is not enough. + +- For a bug fix, add a regression scenario that exercises the reported failure + through the relevant public API or backend flow and checks the correct result. + Where practical, show that it fails before the fix and passes afterward. + +- Prefer tests of useful behavior, feature interactions, and failure handling + over many tiny checks of implementation details. Extend existing scenarios + rather than duplicating them. + +- Unit tests, including tests at the bottom of `.rs` files, are useful for + isolated logic but do not replace functional validation. Do not judge + coverage by test count or tests that merely repeat the new implementation. + +- Follow the [validation runbook](docs/development/build-and-test/ci-validation-infrastructure.md) + and backend guide. Check that the intended scenario actually runs and asserts + behavior on a suitable host. State what was not run or cannot be covered; + do not invent evidence. Documentation-only changes do not need runtime tests. + +For reviews, use the [MXC review skill](.github/skills/mxc-code-review/SKILL.md). +Focus on high- and medium-severity defects and consolidate findings into one +review, rather than creating extra review rounds for nits or optional cleanup. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..ef16b15b4 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,5 @@ +# MXC Claude instructions + +@AGENTS.md + +For code review, follow [.github/skills/mxc-code-review/SKILL.md](.github/skills/mxc-code-review/SKILL.md). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9948f562a..53d29023c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -253,6 +253,13 @@ The Rust style guidelines used in this repo are documented in [`.github/instruct Testing is a key component in the development workflow. We expect contributors to add or update tests alongside their changes. +Features need functional coverage in the applicable CI suite, and bug fixes +need a regression scenario for the reported failure. Backend behavior belongs +in the scheduled/nightly validation suites on suitable hosts; see the +[validation runbook](docs/development/build-and-test/ci-validation-infrastructure.md). +Unit tests support isolated logic but do not replace this behavior coverage. +Documentation-only edits do not need runtime tests. + ```bash # Rust unit tests (from src/) cargo test --workspace @@ -272,7 +279,14 @@ PowerShell and shell helper scripts that drive the executor end-to-end live unde ### Code review -When the change is ready, mark the Draft PR as **Ready for Review**. The PR template asks you to confirm CLA acceptance and to update [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) if your change affects build commands, project architecture, or key conventions. +When the change is ready, mark the Draft PR as **Ready for Review**. The PR +template asks you to confirm CLA acceptance and update agent guidance if build +commands, architecture, or key conventions change. Technical repository rules +live in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md). +[`AGENTS.md`](./AGENTS.md) references those rules and adds design, communication, +and validation expectations; `CLAUDE.md` points to that guidance. +Repository-specific review checks live in the +[MXC code-review skill](./.github/skills/mxc-code-review/SKILL.md). PR builds don't run automatically — Microsoft ADO policy requires a Microsoft employee to comment `/azp run` to start the `MXC-PR-Build` pipeline. See [pull request builds](./docs/development/build-and-test/pull-requests.md) for details.