Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
167 changes: 167 additions & 0 deletions .github/skills/mxc-code-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -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
Comment thread
bbonaby marked this conversation as resolved.

- 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
Comment thread
bbonaby marked this conversation as resolved.

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
Comment thread
bbonaby marked this conversation as resolved.
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.
Comment thread
bbonaby marked this conversation as resolved.

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.
Comment thread
bbonaby marked this conversation as resolved.

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.
81 changes: 81 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# MXC agent instructions

Read and follow [.github/copilot-instructions.md](.github/copilot-instructions.md)
Comment thread
bbonaby marked this conversation as resolved.
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
Comment thread
bbonaby marked this conversation as resolved.
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.
Comment thread
bbonaby marked this conversation as resolved.
5 changes: 5 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
@@ -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).
16 changes: 15 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.

Expand Down
Loading