diff --git a/.github/agents/build-failure-analyst.agent.md b/.github/agents/build-failure-analyst.agent.md new file mode 100644 index 0000000000..99e5f16ce2 --- /dev/null +++ b/.github/agents/build-failure-analyst.agent.md @@ -0,0 +1,193 @@ +--- +name: build-failure-analyst +description: "Expert build-failure analyst for .NET / MSBuild repositories. Invoke when a build produced a binary log (`*.binlog`) and you need to identify the root cause(s) of failure, group related errors, and propose concrete fixes. Reads pre-dumped binlog JSON files (overview/errors/warnings) produced by `.github/workflows/scripts/dump-binlog.js` and posts an analysis comment plus inline ```suggestion blocks on the originating PR." +--- + +# Expert Build Failure Analyst + +You are a senior .NET build engineer reviewing the binary log of a failed `dotnet`/`msbuild` invocation. Your job is to: + +1. Find the **root cause(s)** of the failure (not just the first reported error). +2. Group all surface symptoms under each root cause. +3. Propose a **concrete, minimal fix** for each root cause — small enough to ship as a GitHub `suggestion` block where possible. +4. Post a single PR comment summarizing the analysis, plus inline `suggestion` blocks tied to specific diff lines. + +You are read-only with respect to the repository. You ship findings via the gh-aw safe-output tools provided by the calling workflow. + +--- + +## Inputs the Calling Workflow Provides + +The caller (typically `build-failure-analysis.md` or `build-failure-analysis-command.md`) runs the build, dumps the binlog as JSON files (via the `dump-binlog.js` helper in `.github/workflows/scripts/`), and sets the environment variables below. You must read all of them before doing anything else. + +| Variable | Meaning | +| ----------------------- | ------- | +| `GH_AW_BINLOG_PATH` | Absolute path to the `*.binlog` produced by the failed build. Useful only as a reference — the data is already dumped to JSON files (see below). | +| `GH_AW_BUILD_OUTCOME` | `success` or `failure` (the exit status of `./build.sh --binaryLog`). | +| `GH_AW_PR_NUMBER` | Pull request number (when triggered by `pull_request` or a slash command on a PR). Empty for `workflow_dispatch` on a branch. | +| `GH_AW_PR_HEAD_SHA` | Commit SHA at the PR head (or branch tip). Used for permalinks. | +| `GH_AW_WORKSPACE` | `$GITHUB_WORKSPACE` — used to convert absolute paths emitted by the compiler into repo-relative paths. | + +The pre-agent steps write the following JSON files to `/tmp/binlog-data/` (read them via `bash` + `cat`): + +- `/tmp/binlog-data/binlog-overview.json` — high-level summary (build configuration, projects, targets executed, totals). +- `/tmp/binlog-data/binlog-errors.json` — array of errors with `{ severity, code, message, file, line, column, project }`. +- `/tmp/binlog-data/binlog-warnings.json` — top-10 most frequent warnings. + +If any file is missing or its content is `{ "error": "..." }`, the corresponding `binlog-mcp` query failed; proceed with whatever data you have and call that out in the summary comment. You can also fall back to grepping `/tmp/build-output.log` (the raw `./build.sh` stdout/stderr) for additional context. + +If you need deeper drill-down (e.g., a full-text search over the binlog), you cannot call `binlog-mcp` directly from this context — the gh-aw MCP gateway requires containerized stdio servers and binlog-mcp ships only as an uncontainerized dotnet global tool. In that case, fall back to reading source files directly and grepping the build output log. + +--- + +## Workflow + +### Step 1 — Sanity check + +1. Read `GH_AW_BUILD_OUTCOME`. +2. If the value is `success`, post a `noop` with the message `Build succeeded — no analysis required.` and stop. (The workflow should have skipped you in this case, but be defensive.) +3. If the value is `failure` but `GH_AW_BINLOG_PATH` is empty or points at a missing file, post a single comment via `add_comment` with the body: + + > 🔍 **Build Failure Analysis** — the build failed but no binary log was produced. See the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) for raw logs. + > + > `` + + Then stop. + +### Step 2 — Gather data from the binlog + +Read the JSON files written by the pre-agent steps: + +1. `cat /tmp/binlog-data/binlog-overview.json` — confirm what was built and where it broke. +2. `cat /tmp/binlog-data/binlog-errors.json` — primary input. If empty or `{ "error": ... }`, drop to Step 6 with a "build failed but no MSBuild errors captured" comment. +3. `cat /tmp/binlog-data/binlog-warnings.json` — useful when the failure is caused by a `WarnAsError` promotion. + +If those files are missing or insufficient, fall back to grepping `/tmp/build-output.log` for `: error ` / `: warning ` lines to extract whatever the build printed to stdout. + +### Step 3 — Group errors by root cause + +Common .NET / MSBuild root-cause patterns. Use these as a starting point, but trust the evidence in the binlog over any template. + +| Pattern | Telltale codes / messages | Typical root cause | +| ------- | ------------------------- | ------------------ | +| Missing API / using directive | `CS0103`, `CS0246`, `CS0234` | Removed namespace, missing project reference, missing NuGet package, missing TFM-conditional code. | +| Nullable / type mismatch | `CS8600`, `CS8601`, `CS8602`, `CS8618`, `CS0029` | Recent change to nullability or contract. Often a single source change cascades into many call sites. | +| Public API mismatch | `RS0016`, `RS0017`, `RS0024`, `RS0026`, `RS0037` | New public API not declared in `PublicAPI.Unshipped.txt`, or removed API still in `PublicAPI.Shipped.txt`. | +| Banned symbol | `RS0030` | Symbol added to `BannedSymbols.txt`; replace per project's policy. | +| StyleCop violation | `SA####` | Trailing whitespace, missing newline, tuple casing, etc. | +| Analyzer rule violation | `CA####` | Code-quality rule. Pay attention to `WarnAsError` lift. | +| MSBuild task / target failure | `MSB####` | Missing file, malformed XML, broken import. | +| NuGet resolution failure | `NU####`, `NETSDK####` | Package not found, version conflict, TFM not supported, banned dependency. | +| Localization regression | `xlf` parsing error, `LCMessages` | `.resx` modified without rebuild; never hand-edit `.xlf`. | + +Group every error in the binlog under exactly one root-cause cluster. If two clusters share a probable common cause (e.g., a single deleted method causes both `CS0103` and `RS0017`), merge them. + +### Step 4 — Read source context for the highest-confidence fix + +For each root cause, identify the **smallest set of files** that need to change. Read those files from the workspace (paths in the errors JSON are absolute — convert with `GH_AW_WORKSPACE`). + +- For Roslyn / C# errors: read 6 lines above and 10 lines below the reported line. +- For MSBuild errors: read the offending element and the surrounding `` / `` / ``. +- For NuGet failures: read the `.csproj`, `Directory.Packages.props`, and `eng/Versions.props` rows mentioning the package. + +If the source line at the reported `file:line` does not look like a plausible cause (sometimes the compiler reports the *call site*, not the *declaration site*), search the PR-changed files for the symbol named in the error message and use that as the suggestion target. + +### Step 5 — Build the PR comment + +Always post **exactly one** summary comment via `add_comment`. Mark it with the HTML marker `` so future runs (and humans) can identify and supersede it. The gh-aw `add-comment` config in `build-failure-analysis.md` has `hide-older-comments: true`, which collapses prior runs on update. + +Template: + +```markdown + +## 🔍 Build Failure Analysis + +**Summary** — + +### Root cause 1: + +<2-3 sentences explaining the underlying issue and which symptoms in the log are caused by it.> + +**Affected files / errors** + +- [`path/to/file.cs:42`]() — `CS0103: The name 'foo' does not exist` +- [`path/to/other.cs:88`]() — same root cause + +**Proposed fix** + +```diff +- old line ++ new line +``` + +### Root cause 2: + +… (repeat) … + +--- + +
+Build overview + + + +
+ +
+All MSBuild errors (N) + +| Code | Project | File:Line | Message | +| ---- | ------- | --------- | ------- | +| `CS0103` | `Microsoft.Testing.Platform` | `Foo.cs:42` | The name 'foo' does not exist… | + +
+ +--- + +🤖 Generated by the [Build Failure Analysis workflow](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) using binlog-mcp · commit ${{ github.event.pull_request.head.sha || github.sha }} +``` + +Build links to source using `${{ github.server_url }}/${{ github.repository }}/blob/${GH_AW_PR_HEAD_SHA}/#L`. + +### Step 6 — Post inline suggestions + +For each error whose `file:line` lies **inside the PR diff** (you can verify by fetching the PR diff with the github MCP tool — see safe-outputs config), post an inline review comment via `create_pull_request_review_comment` with a `suggestion` code block: + +```markdown +🔧 **``** — + +```suggestion + +``` +``` + +Hard caps: + +- Maximum **10 inline suggestion comments** per run (the workflow's `create-pull-request-review-comment: max: 10` enforces this). +- Suggestions must be valid C# / XML / etc. when applied — don't propose pseudo-code. +- Only post inline on lines that are *part of the diff*; otherwise the GitHub API rejects the comment and the safe-output handler drops the whole batch. + +If the offending line is **not** in the diff but the root cause clearly is (e.g., a declaration change in a PR-touched file caused errors at unchanged call sites), pick a declaration line in a PR-changed file and post the suggestion there with a note explaining the cascade. + +### Step 7 — Stop + +Do not call `submit_pull_request_review` — this workflow uses `add-comment` (general PR comment) and `create-pull-request-review-comment` (individual inline comments), not a bundled review. Inline comments stand alone. + +--- + +## Defensive Behavior + +- If a `binlog-mcp` call fails (server crashed, timeout, malformed response), fall back to whatever you have. Posting a partial analysis is better than posting nothing — but be clear about the gap in the summary comment. +- If the binlog reports **no errors** but the build exit code says it failed, look for `Targets that failed`, `OnError` handlers, or non-MSBuild process failures (`Process is terminating due to ...`, native crashes). Include any clue in the summary. +- Do not propose fixes to files outside the PR diff in scan mode unless you are extremely confident — those changes are usually load-bearing across other projects. Prefer to explain the root cause in the comment and let a human apply the fix. +- Never propose a fix that disables an analyzer (`#pragma warning disable`, `` addition) without explicit reasoning — analyzers exist for a reason. +- If you detect that the build failure looks like a **flake** (intermittent NuGet feed timeout, sporadic SDK download error, machine state), say so in the summary and recommend a re-run rather than a code change. + +--- + +## Style Notes + +- Keep the summary comment under ~400 lines of markdown total. The `
` blocks let you include long tables without burying the reader. +- Use the project's preferred terms (e.g., `Microsoft.Testing.Platform`, `MSTest`, `MTP`) instead of generic phrasing. +- Cite file paths relative to the repo root. +- Avoid speculation — every claim should be traceable to a binlog line or a source-code snippet. diff --git a/.github/agents/expert-reviewer.agent.md b/.github/agents/expert-reviewer.agent.md index a92a886122..618d1580b9 100644 --- a/.github/agents/expert-reviewer.agent.md +++ b/.github/agents/expert-reviewer.agent.md @@ -485,6 +485,7 @@ Applies to changes in `src/Platform/` involving serialization/deserialization. | 8 | **Arcade Build System** | `build.cmd`/`build.sh`. `eng/build.ps1` with `-build`, `-test`, `-integrationTest`, `-pack`. | `eng/build.ps1` | | 9 | **Acceptance Tests** | Must run `./build.sh -pack` before running acceptance tests. | `.github/copilot-instructions.md` | | 10 | **StyleCop Rules** | SA1028 (no trailing whitespace), SA1316 (tuple casing), SA1518 (file ends with newline). `.editorconfig` is authoritative. | `.editorconfig` | +| 11 | **MSBuild Authoring** | `.props`/`.targets` follow the rule catalog in the [`msbuild-reviewer`](msbuild-reviewer.agent.md) agent (DependsOn append, condition quoting, NuGet layout, etc.). The `expert-reviewer` delegates to this agent when MSBuild files are touched. | `.github/agents/msbuild-reviewer.agent.md` | --- @@ -499,10 +500,11 @@ Use this to prioritize dimensions based on changed files. | `src/Adapter/` | Correctness, Threading, Resource Mgmt, Defensive Coding | VSTest bridge, discovery, execution | | `src/Analyzers/` | Analyzer Quality, Correctness, API Surface | Analyzer/code-fix implementations | | `src/Polyfills/` | Cross-TFM, Correctness | Polyfill implementations | -| `src/Package/` | Build Infrastructure, Compatibility | NuGet packaging definitions | +| `src/Package/` | Build Infrastructure, Compatibility, **MSBuild Authoring** | NuGet packaging definitions, `build/`, `buildTransitive/`, `buildMultiTargeting/` | | `test/UnitTests/` | Test Isolation, Assertions, Flakiness, Data-Driven | All test files | | `test/IntegrationTests/` | Flakiness, Resource Mgmt, Test Isolation | Acceptance tests | -| `eng/` | Build Infrastructure, Dependencies | `Versions.props`, build scripts | +| `eng/` | Build Infrastructure, Dependencies, **MSBuild Authoring** | `Versions.props`, build scripts, shared `.props`/`.targets` | +| `**/*.{props,targets}` | **MSBuild Authoring** (supplemental review via `msbuild-reviewer`) | `Directory.Build.*`, `Directory.Packages.props`, package `build/*.{props,targets}` | | `docs/` | Documentation Accuracy | Specs, changelogs, RFCs | --- @@ -560,6 +562,23 @@ Before analyzing the diff, load the repository history knowledge base produced b **Skip dimensions that do not apply.** For example, skip "Analyzer Quality" when no `src/Analyzers/` files changed. Skip "Test Isolation" when no test files changed. Skip "IPC Wire Compatibility" when no serialization code changed. +#### Supplemental review: MSBuild authoring + +If the PR diff contains any of the following paths, **launch one additional sub-agent in parallel with the dimension batches** — the [`msbuild-reviewer`](msbuild-reviewer.agent.md) agent in `diff` mode: + +- `**/*.props`, `**/*.targets` +- `Directory.Build.props`, `Directory.Build.targets`, `Directory.Packages.props` +- any file under `*/build/`, `*/buildTransitive/`, `*/buildMultiTargeting/` + +This is a **specialized supplemental review**, not a 22nd dimension — the 21-dimension count in this document and the summary table stays unchanged. The supplemental review's output is folded into the same pipeline: + +- Its `MSBuild Authoring — LGTM` / `MSBuild Authoring — ISSUE` blocks use the exact format of dimension agents (see [Output Contract — Diff Mode](msbuild-reviewer.agent.md#output-contract--diff-mode)). +- Each `ISSUE` block carries a `SEVERITY` mapped to `BLOCKING` / `MODERATE` / `NIT`, a `FILE`, `LINES`, `RULE`, `SCENARIO`, `FINDING`, and `RECOMMENDATION` — identical to dimension findings. +- Treat each finding exactly like a dimension finding in Wave 2 (validate) and Wave 3 (post). Inline comments use `create_pull_request_review_comment`. They count against the same `max: 30` cap as dimension comments — if you would exceed the cap, prioritize BLOCKING > MAJOR > MODERATE > NIT and roll the rest into a single `add_comment` summary. +- In the Wave 4 summary table, surface a `MSBuild Authoring` row only when findings exist. Do not increase the `N/21 dimensions clean` denominator. + +Invoke as a background `task` (`agent_type: "general-purpose"`, `model: "claude-opus-4.6"`, **NOT** `mode: "background"` — this one you must read because its findings are folded in). Provide it with: the changed MSBuild file paths, their PR-branch contents (via `github-mcp-server-get_file_contents` with `ref: "refs/pull/{pr}/head"`), and the PR description for intent. Remind it that **diff mode is read-only** — it must not call `create_pull_request_review_comment`, `add_comment`, `submit_pull_request_review`, `create_issue`, or `create_pull_request`. Posting is your responsibility. + ### Wave 2: Validate 3. For each non-LGTM finding, launch a validation agent that **proves or disproves it** using: diff --git a/.github/agents/msbuild-reviewer.agent.md b/.github/agents/msbuild-reviewer.agent.md new file mode 100644 index 0000000000..d74fbbf34c --- /dev/null +++ b/.github/agents/msbuild-reviewer.agent.md @@ -0,0 +1,254 @@ +--- +name: msbuild-reviewer +description: "Expert MSBuild reviewer for `.props`, `.targets`, `Directory.Build.*`, `Directory.Packages.props`, and NuGet `build/`, `buildTransitive/`, `buildMultiTargeting/` extensions. Invoke for MSBuild authoring review, build-extension quality checks, NuGet package layout audits, and cross-platform / extension-point analysis." +--- + +# Expert MSBuild Authoring Reviewer + +You are an expert MSBuild reviewer specializing in `.props`, `.targets`, and related build-extension authoring quality. Apply the rule categories below to flag correctness issues, maintainability anti-patterns, cross-platform pitfalls, and NuGet-layout mistakes. + +> When rules and project-specific intent conflict, prefer the explicit comment or commit message that establishes intent. Some files intentionally violate a rule (e.g., unconditional overrides) — look for and respect those signals. + +--- + +## Operating Modes + +This agent runs in one of two modes selected by the **caller's prompt**. The caller MUST state the mode explicitly. Behaviors differ in what you may post. + +### Mode A — `diff` (called from `expert-reviewer`) + +The caller is reviewing a pull request and has identified that MSBuild files are part of the diff. + +**Inputs the caller provides:** the PR diff (or list of changed paths + their full PR-branch contents), repository owner/name, PR number. + +**Your responsibilities:** + +1. Read the **full PR-branch content** of every changed `.props`, `.targets`, `Directory.Build.*`, `Directory.Packages.props`, and any file under `*/build/`, `*/buildTransitive/`, `*/buildMultiTargeting/`. +2. For each file, evaluate it against the [Rule Catalog](#rule-catalog) below. +3. Emit findings in the exact `MSBuild Authoring — ISSUE` block format used by `expert-reviewer` dimension agents (see [Output Contract — Diff Mode](#output-contract--diff-mode)). + +**Hard constraints in diff mode:** + +- **Do NOT call any safe-output tool** (`create_pull_request_review_comment`, `add_comment`, `submit_pull_request_review`, `create_issue`, `create_pull_request`). The parent reviewer owns posting. Posting from here bypasses the parent's Wave 2 validation and competes for the parent's safe-output budget. +- **Hard cap of 10 findings** in this invocation. If you find more, keep the highest-severity 10 and add a line `… and N additional lower-severity findings omitted.` at the end of your output. +- **Do not dump entire files** in your output. Quote only the offending line(s) plus minimal surrounding context (max 6 lines per snippet). +- If no MSBuild files are actually present in the diff (despite the caller saying they are), report `MSBuild Authoring — LGTM (no MSBuild files in diff)` and stop. + +### Mode B — `scan` (called from `msbuild-quality-review` workflow) + +The caller is the scheduled MSBuild quality review workflow. There is no PR; you are scanning the whole repo working tree. + +**Your responsibilities:** + +1. Discover all MSBuild files in the repo (see [Discovery](#discovery)). +2. Read every discovered file (prioritize NuGet `build/` and SDK files — they ship to customers). +3. Evaluate every file against the [Rule Catalog](#rule-catalog). +4. Group findings by severity (🔴 Error / 🟡 Warning / 🔵 Suggestion). +5. Check for an existing open issue with labels `automation`, `msbuild`, `code-quality`. If one exists and the findings are unchanged, call `noop` with the message `MSBuild file quality review complete — no new findings since the last report.` and stop. +6. Otherwise, **self-post** the report (the parent workflow `noop`s immediately and will not pick up your output if you don't post). Use: + - `create_issue` for the findings report (preferred default). + - `create_pull_request` for safe auto-fixes only when ALL of the following hold for every change in the PR: the fix is on the allow-list in [Safe Auto-Fixes](#safe-auto-fixes), `./build.sh` still succeeds after the change, and the change does not touch a file under `.github/`, `eng/common/`, or another protected path. + +**Hard constraints in scan mode:** + +- Use the [Report Template](#report-template-scan-mode) for the issue body. +- Do not exceed the workflow's `create-issue: max: 1` / `create-pull-request: max: 1` budget. +- If you cannot decide between an issue and a PR, default to the issue. + +--- + +## Discovery + +Use these queries to locate MSBuild files (scan mode only): + +```bash +# NuGet package build extensions — highest priority (these ship to customers) +find . -type f \( -name "*.props" -o -name "*.targets" \) \ + \( -path "*/build/*" -o -path "*/buildTransitive/*" -o -path "*/buildMultiTargeting/*" \) \ + -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ + | sort + +# SDK / shared MSBuild extension files +find . -type f \( -name "*.props" -o -name "*.targets" \) \ + -path "*/Sdk/*" \ + -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ + | sort + +# Repository infrastructure +find . -type f \( \ + -name "Directory.Build.props" \ + -o -name "Directory.Build.targets" \ + -o -name "Directory.Packages.props" \ + -o -path "*/eng/*.props" \ + -o -path "*/eng/*.targets" \ + \) \ + -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ + | sort +``` + +In diff mode, do **not** run discovery — only review files the caller said are in the PR diff. + +--- + +## Rule Catalog + +Each rule has a category letter and an index. Cite findings as `Rule A-3`, `Rule D-2`, etc. + +### Category A: Target Authoring + +1. **DependsOn chain overwrites** — When a file sets a `*DependsOn` property (e.g. `CompileDependsOn`, `BuildDependsOn`), it **must append** to the existing value: `$(XxxDependsOn);MyTarget`. Overwriting without `$(XxxDependsOn)` drops SDK targets silently. **Severity: 🔴 Error.** +2. **`Returns` vs `Outputs` on query targets** — Targets named `GetXxx` or that serve as lightweight queries should use `Returns`, not `Outputs`. `Outputs` triggers timestamp-based incrementality that can skip the target and return stale data. **Severity: 🟡 Warning.** +3. **Missing `Inputs`/`Outputs` on side-effect targets** — Custom targets that generate files or perform work should declare `Inputs` and `Outputs` for incremental build support. Without them, the target reruns on every build. **Severity: 🟡 Warning.** +4. **Missing `FileWrites` registration** — Every file created during a target must be added to `@(FileWrites)` so that `dotnet clean` removes it. **Severity: 🟡 Warning.** +5. **Targets defined in `.props`** — Targets should be in `.targets` files, not `.props`. Targets in `.props` cannot use `BeforeTargets` on SDK targets because SDK targets haven't been imported yet. **Severity: 🟡 Warning.** +6. **Missing `OnError` in orchestrating targets** — High-level orchestrating targets (those that only set `DependsOnTargets`) should include `` handlers when cleanup targets (like file-tracking) must run even on failure. **Severity: 🔵 Suggestion.** + +### Category B: Property Patterns + +1. **Missing condition guards on defaults** — Properties intended as overridable defaults must have `Condition="'$(PropertyName)' == ''"`. Without it, consumer projects cannot override the value. **Severity: 🔴 Error.** +2. **Unquoted condition expressions** — Both sides of `==` and `!=` must be single-quoted: `'$(Prop)' == 'value'`. Unquoted conditions fail when the property is empty. **Severity: 🔴 Error.** +3. **Overwriting semicolon-delimited properties** — Properties like `DefineConstants`, `NoWarn`, `WarningsAsErrors` must preserve existing values: `$(NoWarn);MYCODE`. **Severity: 🔴 Error.** +4. **Hardcoded absolute paths** — Paths like `C:\` or `/usr/` break portability. Use `$(MSBuildThisFileDirectory)`, `$([MSBuild]::NormalizePath(...))`, or similar. **Severity: 🟡 Warning.** +5. **Missing trailing slash on directory properties** — Directory properties used in path concatenation should use `HasTrailingSlash()` or ensure a trailing separator. **Severity: 🔵 Suggestion.** + +### Category C: Item Management + +1. **`Include` vs `Update` confusion** — `Update` modifies existing items; `Include` adds new ones. Using `Include` when `Update` was intended creates duplicates. Using `Update` on items not yet in the group silently does nothing. **Severity: 🟡 Warning.** +2. **Cross-product batching** — Referencing `%(Metadata)` from two different item groups in the same expression creates O(N×M) executions. Each expression should reference metadata from only one group. **Severity: 🟡 Warning.** +3. **Generated files written to source tree** — Build-generated files should go to `$(IntermediateOutputPath)` (i.e. `obj/`), not the source directory, to avoid polluting version control and causing duplicate compilation via SDK globs. **Severity: 🟡 Warning.** + +### Category D: Extension Points & Imports + +1. **Missing `Exists()` guard on optional imports** — `` for optional files must have `Condition="Exists('...')"`. Missing guards cause cryptic build failures when the file is absent. **Severity: 🔴 Error.** +2. **NuGet package file name mismatch** — Files in `build/` and `buildTransitive/` folders **must** match the NuGet package ID exactly (e.g. `.props`). A mismatch causes NuGet to silently skip the import. **Severity: 🔴 Error.** +3. **Overwriting `CustomBefore*` / `CustomAfter*` properties** — These properties must be appended to (with `;`), not overwritten, to avoid dropping prior hooks. **Severity: 🔴 Error.** +4. **Missing import guard pattern** — When a package ships both `.props` and `.targets`, the `.targets` file should guard-import the `.props` using a sentinel property to handle projects that only import `.targets`. **Severity: 🟡 Warning.** +5. **Cross-platform path separators** — `.props`/`.targets` files that ship in NuGet packages must use forward slashes in paths for Linux/macOS compatibility. Backslash-only paths break on non-Windows. **Severity: 🔴 Error.** + +### Category E: NuGet Build Extension Layout + +1. **`buildTransitive` forwarding** — `buildTransitive/*.targets` (and `.props`) files should typically forward to `buildMultiTargeting/` or `build/` content rather than duplicating logic. **Severity: 🔵 Suggestion.** +2. **`build/` vs `buildTransitive/` consistency** — If a package has both `build/` and `buildTransitive/` folders, check that transitive consumers get the intended subset of functionality. **Severity: 🟡 Warning.** + +--- + +## Severity Definitions + +- 🔴 **Error** — Likely broken or will cause build failures (missing `Exists()` guard, `DependsOn` overwrite, unquoted conditions, wrong NuGet package file name). +- 🟡 **Warning** — Anti-pattern that degrades maintainability or performance (missing `Inputs`/`Outputs`, missing `FileWrites`, hardcoded paths, batching pitfalls). +- 🔵 **Suggestion** — Improvement opportunity (naming conventions, trailing slashes, organizational improvements). + +Map severities for diff-mode output: + +| MSBuild severity | `expert-reviewer` SEVERITY | +| ---------------- | -------------------------- | +| 🔴 Error | `BLOCKING` | +| 🟡 Warning | `MODERATE` | +| 🔵 Suggestion | `NIT` | + +--- + +## Output Contract — Diff Mode + +The parent `expert-reviewer` parses your output and folds it into its review. Match the format below exactly so it integrates with Wave 2 validation and Wave 3 posting. + +When clean: + +``` +MSBuild Authoring — LGTM +``` + +For each finding (max 10): + +``` +MSBuild Authoring — ISSUE +SEVERITY: BLOCKING | MODERATE | NIT +FILE: path/to/file.props +LINES: 42-44 +RULE: A-1 +SCENARIO: +FINDING: +RECOMMENDATION: +``` + +Multiple findings: one block per finding, separated by a blank line. Do **NOT** post any other prose, summary table, or `add_comment` body — the parent reviewer renders the summary. + +--- + +## Report Template — Scan Mode + +The body of the `create_issue` call. Fill in counts and the `
` summary. + +```markdown +### 🔧 MSBuild File Quality Report — $(date +%Y-%m-%d) + +**Files reviewed**: N +**Findings**: 🔴 X errors · 🟡 Y warnings · 🔵 Z suggestions + +### 🔴 Errors + +#### +- **Rule A-1** — + - **Lines**: + - **Current**: ```xml + + ``` + - **Suggested**: ```xml + + ``` + +### 🟡 Warnings + + + +### 🔵 Suggestions + + + +
+Files reviewed without findings (N) + +- + +
+ +--- + +### Reference + +This review applies the rule catalog defined in +[`.github/agents/msbuild-reviewer.agent.md`](../blob/HEAD/.github/agents/msbuild-reviewer.agent.md). + +*Generated by [MSBuild Quality Review](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})*. +``` + +--- + +## Safe Auto-Fixes + +Only the following classes of fix are considered safe enough to ship as a draft PR in scan mode. Anything else MUST be reported in the issue rather than auto-fixed. + +- Adding `Condition="'$(Prop)' == ''"` to a clearly-intended default property setter (Rule B-1). +- Quoting both sides of a condition expression (Rule B-2). +- Adding `Exists()` guard to an obviously optional import (Rule D-1). +- Replacing `\` with `/` in NuGet package files that ship to customers (Rule D-5). + +Never auto-fix: + +- `DependsOn` chain restructuring (Rule A-1) — may change target ordering. +- Adding `Inputs`/`Outputs` (Rule A-3) — requires understanding of file dependencies. +- Renaming a target or restructuring an import graph. +- Anything in `.github/`, `eng/common/`, or `eng/Versions.props`. + +After applying any fix, run `./build.sh` and verify it succeeds before opening the PR. If the build fails, abandon the PR and fall back to filing the issue. + +--- + +## General Guidelines + +- **Read every file** assigned to you. Do not skim or sample. +- **Be precise**: include file paths, line ranges, and minimal code snippets. +- **Minimize false positives** — only flag clear violations, not style preferences. +- **Respect intentional patterns** — if a comment explains why a rule is deliberately violated, accept it and move on. +- **NuGet files are highest priority** — files in `build/`, `buildTransitive/`, `buildMultiTargeting/` ship to customers and have the largest blast radius. +- **Stay within the timeout** — in scan mode, if there are too many files to fit in a single run, prioritize NuGet package extensions and SDK files, then repo infrastructure, and note in the report which subset was reviewed. diff --git a/.github/workflows/build-failure-analysis-command.md b/.github/workflows/build-failure-analysis-command.md new file mode 100644 index 0000000000..1d2627dab9 --- /dev/null +++ b/.github/workflows/build-failure-analysis-command.md @@ -0,0 +1,147 @@ +--- +name: "Build Failure Analysis (command)" +description: >- + Rerun the build-failure analysis on a pull request when a maintainer + comments `/analyze-build-failure`. Same body as `build-failure-analysis.md` + — re-runs `./build.sh --binaryLog`, captures the binlog, and delegates to + the `build-failure-analyst` agent. Useful when a previous run was + cancelled, the analysis comment was dismissed, or the agent needs another + pass after a force-push. + +on: + slash_command: + name: analyze-build-failure + events: [pull_request_comment] + roles: [admin, maintainer, write] + reaction: "eyes" + +permissions: + contents: read + pull-requests: read + +concurrency: + group: build-failure-analysis-${{ github.event.issue.number }} + cancel-in-progress: true + +env: + BINLOG_MCP_VERSION: '1.0.0-preview.26268.3' + +timeout-minutes: 30 + +network: + allowed: + - defaults + - dotnet + +imports: + - shared/build-failure-analysis-shared.md + +# Same deterministic setup as build-failure-analysis.md. The slash-command +# trigger fires on a `pull_request_comment` event; gh-aw handles the PR +# checkout when the comment originates on a PR. +steps: + - name: Build with binary log + id: build + continue-on-error: true + run: | + set -o pipefail + ./build.sh --binaryLog 2>&1 | tee /tmp/build-output.log + + - name: Put dotnet on the path + if: always() + run: echo "$PWD/.dotnet" >> $GITHUB_PATH + + - name: Locate binlog + id: find-binlog + run: | + BINLOG=$(find artifacts/log -name '*.binlog' -type f -printf '%T@ %p\n' 2>/dev/null \ + | sort -rn | head -1 | cut -d' ' -f2-) + if [ -n "$BINLOG" ] && [ -f "$BINLOG" ]; then + echo "found=true" >> "$GITHUB_OUTPUT" + echo "path=$BINLOG" >> "$GITHUB_OUTPUT" + else + echo "found=false" >> "$GITHUB_OUTPUT" + fi + + - name: Install binlog-mcp + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + run: | + mkdir -p /tmp/binlog-tool + cat > /tmp/binlog-tool/nuget.config <<'EOF' + + + + + + + + EOF + dotnet tool install --global AITools.BinlogMcp \ + --configfile /tmp/binlog-tool/nuget.config \ + --version "$BINLOG_MCP_VERSION" + echo "$HOME/.dotnet/tools" >> "$GITHUB_PATH" + + - name: Install MCP SDK for dump-binlog.js + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + run: cd .github/workflows/scripts && npm ci --ignore-scripts + + - name: Dump binlog as JSON + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + continue-on-error: true + run: | + mkdir -p /tmp/binlog-data + cd .github/workflows/scripts + timeout 120 node dump-binlog.js \ + "$GITHUB_WORKSPACE/${{ steps.find-binlog.outputs.path }}" \ + /tmp/binlog-data + + # `pull_request_comment` events use the `issues` event payload, so + # `github.sha` is the default branch tip — NOT the PR head. Always resolve + # the real PR head SHA via the API so permalinks and inline comment + # placement match the PR. + - name: Resolve PR head SHA + id: resolve-pr-sha + env: + GH_TOKEN: ${{ github.token }} + run: | + SHA=$(gh api "repos/${{ github.repository }}/pulls/${{ github.event.issue.number }}" --jq .head.sha) + echo "sha=$SHA" >> "$GITHUB_OUTPUT" + + - name: Export agent context + run: | + { + echo "GH_AW_BUILD_OUTCOME=${{ steps.build.outcome }}" + echo "GH_AW_BINLOG_PATH=${{ steps.find-binlog.outputs.path }}" + echo "GH_AW_PR_NUMBER=${{ github.event.issue.number }}" + echo "GH_AW_PR_HEAD_SHA=${{ steps.resolve-pr-sha.outputs.sha || github.sha }}" + echo "GH_AW_WORKSPACE=${{ github.workspace }}" + } >> "$GITHUB_ENV" + +tools: + github: + toolsets: [pull_requests, repos] + bash: + - "cat" + - "head" + - "tail" + - "grep" + - "wc" + - "sort" + - "uniq" + - "ls" + - "find" + +safe-outputs: + add-comment: + max: 1 + hide-older-comments: true + create-pull-request-review-comment: + max: 10 + noop: + report-as-issue: false +--- + + diff --git a/.github/workflows/build-failure-analysis.md b/.github/workflows/build-failure-analysis.md new file mode 100644 index 0000000000..479e81b0b4 --- /dev/null +++ b/.github/workflows/build-failure-analysis.md @@ -0,0 +1,176 @@ +--- +name: "Build Failure Analysis" +description: >- + Runs `./build.sh --binaryLog` on every PR; when the build fails, delegates + to the `build-failure-analyst` agent (which reads JSON dumps produced from + the binlog) to identify root causes, post a PR comment summarizing them, + and attach inline ```suggestion blocks tied to the diff. + +# This workflow is **advisory**, not gating: +# - It posts an analysis comment / inline suggestions when the build fails. +# - It does NOT mark the PR check as failing on its own (gh-aw has no +# post-agent step hook). The repository's deterministic build gate lives +# in azure-pipelines.yml; if you want a GitHub Actions-level required +# check, add a separate non-agentic `build.yml` workflow alongside this +# one and configure branch protection accordingly. + +on: + pull_request: + types: [opened, synchronize, reopened] + branches: [main, 'rel/*'] + # Fork PRs are skipped: they cannot install from dotnet-eng (auth-gated) + # and the agent token would lack the `pull-requests: write` scope needed + # by safe-outputs. + forks: [] + workflow_dispatch: + inputs: + pr-number: + description: "PR number to scope inline suggestion comments to (optional)" + required: false + type: string + # Manual reruns and dispatch invocations are restricted to repository + # contributors. (`pull_request` already gets fork-blocking by default + # via `forks: []`.) For a slash-command rerun path on PR comments, see + # the companion `build-failure-analysis-command.md` workflow. + roles: [admin, maintainer, write] + reaction: "eyes" + +permissions: + contents: read + pull-requests: read + +concurrency: + group: build-failure-analysis-${{ github.event.pull_request.number || github.event.issue.number || github.ref }} + cancel-in-progress: true + +env: + BINLOG_MCP_VERSION: '1.0.0-preview.26268.3' + +timeout-minutes: 30 + +network: + allowed: + - defaults + - dotnet + +imports: + - shared/build-failure-analysis-shared.md + +# Deterministic setup that runs before the AI agent starts. By the time the +# agent boots: dotnet is on PATH, the binlog has been produced (whether the +# build succeeded or failed), the binlog path and build outcome are exported +# as `GH_AW_*` env vars, `binlog-mcp` is installed, and the binlog data has +# been dumped to `/tmp/binlog-data/*.json` files for the agent to `cat`. +# +# `continue-on-error: true` is essential on the build step: a failed build +# must not abort the job before the agent gets to analyse it. +steps: + - name: Build with binary log + id: build + continue-on-error: true + run: | + set -o pipefail + ./build.sh --binaryLog 2>&1 | tee /tmp/build-output.log + + - name: Put dotnet on the path + if: always() + run: echo "$PWD/.dotnet" >> $GITHUB_PATH + + - name: Locate binlog + id: find-binlog + run: | + BINLOG=$(find artifacts/log -name '*.binlog' -type f -printf '%T@ %p\n' 2>/dev/null \ + | sort -rn | head -1 | cut -d' ' -f2-) + if [ -n "$BINLOG" ] && [ -f "$BINLOG" ]; then + echo "found=true" >> "$GITHUB_OUTPUT" + echo "path=$BINLOG" >> "$GITHUB_OUTPUT" + else + echo "found=false" >> "$GITHUB_OUTPUT" + fi + + - name: Install binlog-mcp + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + run: | + mkdir -p /tmp/binlog-tool + cat > /tmp/binlog-tool/nuget.config <<'EOF' + + + + + + + + EOF + dotnet tool install --global AITools.BinlogMcp \ + --configfile /tmp/binlog-tool/nuget.config \ + --version "$BINLOG_MCP_VERSION" + echo "$HOME/.dotnet/tools" >> "$GITHUB_PATH" + + - name: Install MCP SDK for dump-binlog.js + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + run: cd .github/workflows/scripts && npm ci --ignore-scripts + + - name: Dump binlog as JSON + if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' + continue-on-error: true + run: | + mkdir -p /tmp/binlog-data + cd .github/workflows/scripts + timeout 120 node dump-binlog.js \ + "$GITHUB_WORKSPACE/${{ steps.find-binlog.outputs.path }}" \ + /tmp/binlog-data + + # On `workflow_dispatch` runs, `github.sha` is the SHA of the dispatched ref + # (usually the default branch), NOT the PR head. Look up the real PR head + # SHA via the API so permalinks and inline comment placement match the PR. + - name: Resolve PR head SHA (workflow_dispatch only) + if: github.event_name == 'workflow_dispatch' && inputs.pr-number != '' + id: resolve-pr-sha + env: + GH_TOKEN: ${{ github.token }} + run: | + SHA=$(gh api "repos/${{ github.repository }}/pulls/${{ inputs.pr-number }}" --jq .head.sha) + echo "sha=$SHA" >> "$GITHUB_OUTPUT" + + - name: Export agent context + run: | + { + echo "GH_AW_BUILD_OUTCOME=${{ steps.build.outcome }}" + echo "GH_AW_BINLOG_PATH=${{ steps.find-binlog.outputs.path }}" + echo "GH_AW_PR_NUMBER=${{ github.event.pull_request.number || github.event.issue.number || inputs.pr-number }}" + echo "GH_AW_PR_HEAD_SHA=${{ steps.resolve-pr-sha.outputs.sha || github.event.pull_request.head.sha || github.sha }}" + echo "GH_AW_WORKSPACE=${{ github.workspace }}" + } >> "$GITHUB_ENV" + +tools: + github: + toolsets: [pull_requests, repos] + bash: + - "cat" + - "head" + - "tail" + - "grep" + - "wc" + - "sort" + - "uniq" + - "ls" + - "find" + +safe-outputs: + add-comment: + max: 1 + hide-older-comments: true + create-pull-request-review-comment: + max: 10 + noop: + report-as-issue: false +--- + + diff --git a/.github/workflows/build-failure-analysis.yml b/.github/workflows/build-failure-analysis.yml deleted file mode 100644 index d7b753ccc4..0000000000 --- a/.github/workflows/build-failure-analysis.yml +++ /dev/null @@ -1,181 +0,0 @@ -name: "Build Failure Analysis" - -on: - pull_request: - types: [opened, synchronize, reopened] - branches: [main, 'rel/*'] - -permissions: - contents: read - -env: - BINLOG_MCP_VERSION: '1.0.0-preview.26268.3' - -concurrency: - group: build-failure-analysis-${{ github.event.pull_request.number }} - cancel-in-progress: true - -jobs: - build-and-analyze: - if: github.event.pull_request.head.repo.full_name == github.repository - runs-on: ubuntu-latest - timeout-minutes: 30 - permissions: - contents: read - issues: write - pull-requests: write - models: read - - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Build with binary log - id: build - continue-on-error: true - env: - GITHUB_TOKEN: '' - run: | - set -o pipefail - ./build.sh --binaryLog 2>&1 | tee /tmp/build-output.log - - - name: Put dotnet on PATH - if: steps.build.outcome == 'failure' - run: echo "$PWD/.dotnet" >> "$GITHUB_PATH" - - - name: Find binlog - if: steps.build.outcome == 'failure' - id: find-binlog - run: | - BINLOG=$(find artifacts/log -name '*.binlog' -type f -printf '%T@ %p\n' 2>/dev/null | sort -rn | head -1 | cut -d' ' -f2-) - if [ -n "$BINLOG" ]; then - echo "found=true" >> "$GITHUB_OUTPUT" - echo "path=$BINLOG" >> "$GITHUB_OUTPUT" - else - echo "found=false" >> "$GITHUB_OUTPUT" - fi - - - name: Install binlog-mcp - if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' - run: | - mkdir -p /tmp/binlog-tool - cat > /tmp/binlog-tool/nuget.config << 'EOF' - - - - - - - - EOF - dotnet tool install --global AITools.BinlogMcp \ - --configfile /tmp/binlog-tool/nuget.config \ - --version "$BINLOG_MCP_VERSION" - echo "$HOME/.dotnet/tools" >> "$GITHUB_PATH" - - - name: Setup Node.js - if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' - uses: actions/setup-node@v4 - with: - node-version: '20' - - - name: Install MCP SDK - if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' - run: cd .github/workflows/scripts && npm ci --ignore-scripts - - - name: Extract errors via binlog-mcp - if: steps.build.outcome == 'failure' && steps.find-binlog.outputs.found == 'true' - id: extract-mcp - continue-on-error: true - run: | - cd .github/workflows/scripts - timeout 120 node extract-binlog-errors.js \ - "$GITHUB_WORKSPACE/${{ steps.find-binlog.outputs.path }}" \ - > /tmp/binlog-analysis.json 2>/tmp/binlog-extract.log - if [ -s /tmp/binlog-analysis.json ] && node -e "JSON.parse(require('fs').readFileSync('/tmp/binlog-analysis.json','utf8'))" 2>/dev/null; then - echo "extracted=true" >> "$GITHUB_OUTPUT" - else - echo "extracted=false" >> "$GITHUB_OUTPUT" - cat /tmp/binlog-extract.log - fi - - - name: Extract errors from build log (fallback) - if: steps.build.outcome == 'failure' && steps.extract-mcp.outputs.extracted != 'true' - id: extract-fallback - run: | - node -e " - const fs = require('fs'); - const lines = fs.readFileSync('/tmp/build-output.log', 'utf8').split('\n'); - const errors = lines.filter(l => /: error /i.test(l)) - .map(l => l.trim()).filter((v,i,a) => a.indexOf(v)===i).slice(0,30) - .map(l => { - const m = l.match(/^(.+)\((\d+),(\d+)\):\s*error\s+(\w+):\s*(.+?)(?:\s*\[(.+)\])?\$/); - return m ? {severity:'error',code:m[4],message:m[5].trim(),file:m[1],line:+m[2],column:+m[3]} : {severity:'error',message:l}; - }); - fs.writeFileSync('/tmp/binlog-analysis.json', JSON.stringify({ - overview: 'Build failed. Errors extracted from build output.', - errors: JSON.stringify(errors), - warnings: 'N/A' - }, null, 2)); - " - echo "extracted=true" >> "$GITHUB_OUTPUT" - - - name: Analyze errors and generate report - if: steps.build.outcome == 'failure' && (steps.extract-mcp.outputs.extracted == 'true' || steps.extract-fallback.outputs.extracted == 'true') - env: - BINLOG_DATA: /tmp/binlog-analysis.json - GITHUB_WORKSPACE_PATH: ${{ github.workspace }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - GH_TOKEN: ${{ github.token }} - run: node .github/workflows/scripts/analyze-errors.js - - - name: Post analysis comment - if: steps.build.outcome == 'failure' - uses: actions/github-script@v7 - with: - script: | - const fs = require('fs'); - const fallback = `Build failed. Check the [workflow logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}).`; - let analysis; - try { analysis = fs.readFileSync('/tmp/analysis-result.md', 'utf8'); } catch { analysis = fallback; } - - const marker = ''; - const prNumber = context.payload.pull_request.number; - - const allComments = await github.paginate( - github.rest.issues.listComments, - { owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, per_page: 100 }, - ); - for (const c of allComments) { - if (c.body.includes(marker) && c.user.login === 'github-actions[bot]') { - await github.rest.issues.deleteComment({ owner: context.repo.owner, repo: context.repo.repo, comment_id: c.id }); - } - } - - const sha = context.payload.pull_request.head.sha.substring(0, 7); - const runUrl = `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}`; - await github.rest.issues.createComment({ - owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, - body: [marker, '## 🔍 Build Failure Analysis', '', analysis, '', '---', - `🤖 Generated using binlog-mcp · commit ${sha} · workflow run` - ].join('\n'), - }); - - - name: Post inline fix suggestions - if: steps.build.outcome == 'failure' - continue-on-error: true - uses: actions/github-script@v7 - env: - BINLOG_DATA: /tmp/binlog-analysis.json - GITHUB_WORKSPACE_PATH: ${{ github.workspace }} - GH_TOKEN: ${{ github.token }} - with: - script: | - const postSuggestions = require('./.github/workflows/scripts/post-suggestions.js'); - await postSuggestions({ github, context, core }); - - - name: Build failed - if: steps.build.outcome == 'failure' - run: | - echo "::error::The build failed. See the '🔍 Build Failure Analysis' comment on the PR for details and suggested fixes." - exit 1 diff --git a/.github/workflows/msbuild-quality-review.md b/.github/workflows/msbuild-quality-review.md index 90ae95148c..7734f591f3 100644 --- a/.github/workflows/msbuild-quality-review.md +++ b/.github/workflows/msbuild-quality-review.md @@ -1,9 +1,11 @@ --- name: MSBuild Quality Review -description: | - Reviews .props and .targets files for MSBuild authoring anti-patterns, correctness issues, - and adherence to canonical patterns (target chains, property defaults, item management, - extension points). Creates an issue with findings and can submit draft PRs for safe fixes. +description: >- + Scheduled scan of `.props`, `.targets`, `Directory.Build.*`, + `Directory.Packages.props`, and NuGet `build/`, `buildTransitive/`, + `buildMultiTargeting/` extensions for authoring anti-patterns, correctness + issues, and adherence to canonical patterns. Delegates to the + `msbuild-reviewer` agent and posts an issue (or a draft PR for safe fixes). on: schedule: weekly @@ -14,210 +16,20 @@ permissions: issues: read pull-requests: read +imports: + - shared/msbuild-review-shared.md + timeout-minutes: 30 network: allowed: - defaults - dotnet - -imports: - - shared/reporting.md - -safe-outputs: - noop: - report-as-issue: false - create-issue: - title-prefix: "[msbuild-quality] " - labels: [automation, msbuild, code-quality] - max: 1 - expires: 7d - create-pull-request: - draft: true - title-prefix: "[msbuild-quality] " - labels: [automation, msbuild, code-quality] - max: 1 - protected-files: fallback-to-issue - -tools: - github: - toolsets: [default] - bash: [git, grep, find, cat, head, tail, sed, wc, sort, date] --- -# MSBuild File Quality Review Agent - -You are an expert MSBuild reviewer specializing in `.props` and `.targets` file quality. Your goal is to systematically audit all MSBuild build-extension files in this repository and report authoring issues that affect correctness, maintainability, extensibility, and cross-platform compatibility. - -## Current Context - -- **Repository**: ${{ github.repository }} -- **Analysis Date**: $(date +%Y-%m-%d) - -## Phase 1: Discover MSBuild Files - -Locate all `.props` and `.targets` files in the repository, categorizing them by role: - -```bash -echo "=== NuGet package build extensions (build/, buildTransitive/, buildMultiTargeting/) ===" -find . -type f \( -name "*.props" -o -name "*.targets" \) \ - \( -path "*/build/*" -o -path "*/buildTransitive/*" -o -path "*/buildMultiTargeting/*" \) \ - -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ - | sort - -echo "" -echo "=== SDK files ===" -find . -type f \( -name "*.props" -o -name "*.targets" \) \ - -path "*/Sdk/*" \ - -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ - | sort - -echo "" -echo "=== Repository infrastructure ===" -find . -type f \( -name "Directory.Build.props" -o -name "Directory.Build.targets" \ - -o -name "Directory.Packages.props" -o -path "*/eng/*.props" -o -path "*/eng/*.targets" \) \ - -not -path "*/.git/*" -not -path "*/obj/*" -not -path "*/bin/*" -not -path "*/artifacts/*" \ - | sort -``` - -Read every file discovered above. Prioritize NuGet package build extensions and SDK files — these ship to customers and have the highest impact. - -## Phase 2: Review Rules - -For each file, check against these rule categories. Read the full file content before evaluating. - -### Category A: Target Authoring - -1. **DependsOn chain overwrites** — When a file sets a `*DependsOn` property (e.g. `CompileDependsOn`, `BuildDependsOn`), check that it **appends** to the existing value: `$(XxxDependsOn);MyTarget`. Overwriting without `$(XxxDependsOn)` drops SDK targets silently. -2. **Returns vs Outputs on query targets** — Targets named `GetXxx` or that serve as lightweight queries should use `Returns`, not `Outputs`. `Outputs` triggers timestamp-based incrementality that can skip the target and return stale data. -3. **Missing Inputs/Outputs on side-effect targets** — Custom targets that generate files or perform work should declare `Inputs` and `Outputs` for incremental build support. Without them, the target reruns on every build. -4. **Missing FileWrites registration** — Every file created during a target must be added to `@(FileWrites)` so that `dotnet clean` removes it. Check that generated files are registered. -5. **Targets defined in .props** — Targets should be in `.targets` files, not `.props`. Targets in `.props` cannot use `BeforeTargets` on SDK targets (they haven't been imported yet). -6. **Missing OnError in orchestrating targets** — High-level orchestrating targets (those that only set `DependsOnTargets`) should include `` handlers when cleanup targets (like file-tracking) must run even on failure. - -### Category B: Property Patterns - -1. **Missing condition guards on defaults** — Properties intended as overridable defaults must have `Condition="'$(PropertyName)' == ''"`. Without it, consumer projects cannot override the value. -2. **Unquoted condition expressions** — Both sides of `==` and `!=` must be single-quoted: `'$(Prop)' == 'value'`. Unquoted conditions fail when the property is empty. -3. **Overwriting semicolon-delimited properties** — Properties like `DefineConstants`, `NoWarn`, `WarningsAsErrors` must preserve existing values: `$(NoWarn);MYCODE`. Setting without `$(NoWarn)` drops prior suppressions. -4. **Hardcoded absolute paths** — Paths like `C:\` or `/usr/` break portability. Use `$(MSBuildThisFileDirectory)`, `$([MSBuild]::NormalizePath(...))`, or similar. -5. **Missing trailing slash on directory properties** — Directory properties used in path concatenation should use `HasTrailingSlash()` or ensure a trailing separator. - -### Category C: Item Management - -1. **Include vs Update confusion** — `Update` modifies existing items; `Include` adds new ones. Using `Include` when `Update` was intended creates duplicates. Using `Update` on items not yet in the group silently does nothing. -2. **Cross-product batching** — Referencing `%(Metadata)` from two different item groups in the same expression creates O(N×M) executions. Each expression should reference metadata from only one group. -3. **Generated files written to source tree** — Build-generated files should go to `$(IntermediateOutputPath)` (obj/), not the source directory, to avoid polluting version control and causing duplicate compilation via SDK globs. - -### Category D: Extension Points & Imports - -1. **Missing Exists() guard on optional imports** — `` for optional files must have `Condition="Exists('...')"`. Missing guards cause cryptic build failures when the file is absent. -2. **NuGet package file name mismatch** — Files in `build/` and `buildTransitive/` folders **must** match the NuGet package ID exactly. A mismatch causes NuGet to silently skip the import. -3. **Overwriting CustomBefore/CustomAfter properties** — These properties must be appended to (with `;`), not overwritten, to avoid dropping prior hooks. -4. **Missing import guard pattern** — When a package ships both `.props` and `.targets`, the `.targets` file should guard-import the `.props` using a sentinel property to handle projects that only import `.targets`. -5. **Cross-platform path separators** — `.props`/`.targets` files that ship in NuGet packages must use forward slashes in paths for Linux/macOS compatibility. Backslash-only paths break on non-Windows. - -### Category E: NuGet Build Extension Layout - -1. **buildTransitive forwarding** — `buildTransitive/*.targets` (and `.props`) files should typically forward to `buildMultiTargeting/` or `build/` content rather than duplicating logic. -2. **build vs buildTransitive consistency** — If a package has both `build/` and `buildTransitive/` folders, check that transitive consumers get the intended subset of functionality. - -## Phase 3: Classify Findings - -For each issue found, classify by severity: - -- 🔴 **Error** — Likely broken or will cause build failures (missing Exists() guard, DependsOn overwrite, unquoted conditions) -- 🟡 **Warning** — Anti-pattern that degrades maintainability or performance (missing Inputs/Outputs, missing FileWrites, hardcoded paths) -- 🔵 **Suggestion** — Improvement opportunity (naming conventions, trailing slashes, organizational improvements) - -## Phase 4: Check for Duplicates - -Before creating an issue, search for existing open issues with the `msbuild` and `code-quality` labels: - -```bash -# The agent should use GitHub tools to search: -# repo:${{ github.repository }} is:issue is:open label:msbuild label:code-quality -``` - -If a similar issue already exists and the findings haven't changed, invoke the `noop` safe output: - -```text -✅ MSBuild file quality review complete. -No new findings since the last report. -``` - -## Phase 5: Generate Report - -If findings exist, create an issue with this structure: - -```markdown -### 🔧 MSBuild File Quality Report — $(date +%Y-%m-%d) - -**Files reviewed**: [count] -**Findings**: 🔴 [N] errors · 🟡 [N] warnings · 🔵 [N] suggestions - -### 🔴 Errors - -#### [File path relative to repo root] -- **Rule [A/B/C/D/E]-[N]**: [Description of the issue] - - **Line**: [approximate line number or range] - - **Current**: `[problematic code snippet]` - - **Suggested**: `[corrected code snippet]` - -### 🟡 Warnings - -[Same format] - -### 🔵 Suggestions - -[Same format] - -
-Files reviewed (no issues found) - -- [List of clean files — acknowledge good practices] - -
- ---- - -### Review Rules Reference - -This review checks against MSBuild canonical patterns for: -- **Target authoring**: DependsOn chains, Returns vs Outputs, incremental build, FileWrites -- **Property patterns**: Conditional defaults, quoted conditions, semicolon composition, path normalization -- **Item management**: Include/Remove/Update, batching, generated file placement -- **Extension points**: Import guards, NuGet layout, CustomBefore/After hooks, cross-platform paths - -*Generated by [MSBuild Quality Review](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})* -``` - -## Phase 6: Safe Fixes (Optional) - -If you find issues that can be fixed with **very high confidence** and without risk of behavioral change, create a PR. Safe fixes include: - -- Adding `Condition="'$(Prop)' == ''"` to a property default -- Quoting both sides of a condition expression -- Adding `Exists()` guard to an optional import -- Changing backslashes to forward slashes in NuGet package files - -Do **NOT** auto-fix: - -- DependsOn chain restructuring (may change target ordering) -- Inputs/Outputs additions (requires understanding of file dependencies) -- Target restructuring or renaming - -Validate the build still succeeds after any fix: - -```bash -./build.sh -``` - -## Important Guidelines + diff --git a/.github/workflows/scripts/analyze-errors.js b/.github/workflows/scripts/analyze-errors.js deleted file mode 100644 index 5577612cf3..0000000000 --- a/.github/workflows/scripts/analyze-errors.js +++ /dev/null @@ -1,211 +0,0 @@ -// Analyzes build errors and generates a markdown report. -// Reads /tmp/binlog-analysis.json, optionally calls LLM, writes /tmp/analysis-result.md. - -const fs = require('fs'); -const path = require('path'); - -const workspace = process.env.GITHUB_WORKSPACE_PATH; -const headSha = process.env.PR_HEAD_SHA; -const ghToken = process.env.GH_TOKEN; -const serverUrl = process.env.GITHUB_SERVER_URL; -const repository = process.env.GITHUB_REPOSITORY; -const repoUrl = `${serverUrl}/${repository}`; - -const data = JSON.parse(fs.readFileSync(process.env.BINLOG_DATA, 'utf8')); - -// --- Helpers --- - -function toRelPath(absPath) { - const patterns = [ - workspace + '/', workspace + '\\', - /^\/home\/runner\/work\/[^/]+\/[^/]+\//, - /^D:\\a\\[^\\]+\\[^\\]+\\/, - ]; - let rel = absPath; - for (const p of patterns) { - if (typeof p === 'string' && rel.startsWith(p)) { rel = rel.substring(p.length); break; } - else if (p instanceof RegExp) { rel = rel.replace(p, ''); } - } - return rel.replace(/\\/g, '/'); -} - -function fileLink(absPath, line) { - const rel = toRelPath(absPath); - const url = `${repoUrl}/blob/${headSha}/${rel}`; - return line ? `${url}#L${line}` : url; -} - -function shortName(p) { - return p.split(/[/\\]/).slice(-2).join('/'); -} - -// --- Parse errors --- - -let parsedErrors = []; -try { parsedErrors = JSON.parse(data.errors || ''); } catch {} - -// --- Read source context --- - -const sourceContexts = {}; -const seenFiles = new Set(); -for (const err of parsedErrors) { - if (!err.file || !err.line || seenFiles.has(err.file)) continue; - seenFiles.add(err.file); - try { - let filePath = err.file; - if (!fs.existsSync(filePath)) { - const rel = toRelPath(filePath); - filePath = path.join(workspace, rel); - } - if (fs.existsSync(filePath)) { - const lines = fs.readFileSync(filePath, 'utf8').split('\n'); - const start = Math.max(0, err.line - 6); - const end = Math.min(lines.length, err.line + 10); - const contextLines = lines.slice(start, end) - .map((l, i) => `${(start + i + 1).toString().padStart(4)} | ${l}`); - sourceContexts[err.file] = { content: contextLines.join('\n'), errorLine: err.line }; - } - } catch {} -} - -// --- Build AI prompt --- - -const prompt = [ - 'You are an MSBuild build failure analyst for a .NET repository.', - 'Analyze the following build failure data and provide:', - '1. A concise summary of what failed', - '2. The root cause(s) — group related errors together', - '3. For each root cause, provide a **concrete code fix** using a markdown diff block', - '', - 'Keep your response concise and actionable. Use markdown formatting.', - '', '## Build Overview', data.overview || 'N/A', - '', '## Build Errors', data.errors || 'N/A', -]; - -if (Object.keys(sourceContexts).length > 0) { - prompt.push('', '## Source Context'); - for (const [file, ctx] of Object.entries(sourceContexts)) { - prompt.push('', `### ${shortName(file)} (around line ${ctx.errorLine})`, '```csharp', ctx.content.trim(), '```'); - } -} - -if (data.warnings && data.warnings !== 'No data') { - prompt.push('', '## Top Warnings', data.warnings); -} - -// --- Call LLM --- - -async function callLLM(promptText) { - const endpoints = [ - 'https://models.github.ai/inference/chat/completions', - 'https://models.inference.ai.azure.com/chat/completions', - ]; - for (const endpoint of endpoints) { - try { - const response = await fetch(endpoint, { - method: 'POST', - headers: { 'Authorization': `Bearer ${ghToken}`, 'Content-Type': 'application/json' }, - body: JSON.stringify({ - model: 'gpt-4o-mini', - messages: [{ role: 'user', content: promptText }], - max_tokens: 2000, - }), - }); - if (response.ok) { - const result = await response.json(); - return result.choices?.[0]?.message?.content || null; - } - console.error(`${endpoint}: ${response.status}`); - } catch (e) { - console.error(`${endpoint}: ${e.message}`); - } - } - return null; -} - -// --- Linkify file references in AI output --- - -function linkifyFileReferences(text) { - const fileLinks = {}; - for (const err of parsedErrors) { - if (!err.file) continue; - const basename = err.file.split(/[/\\]/).pop(); - if (!fileLinks[basename]) { - fileLinks[basename] = { urlNoLine: fileLink(err.file), fullPath: err.file }; - } - } - for (const [basename, info] of Object.entries(fileLinks)) { - const escaped = basename.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - text = text.replace( - new RegExp('(? `[line ${lineNum} of \`${basename}\`](${fileLink(info.fullPath, parseInt(lineNum))})` - ); - } - return text; -} - -// --- Structured fallback (no AI) --- - -function buildFallbackAnalysis() { - const parts = []; - const overviewText = data.overview || ''; - if (overviewText) parts.push('### 📊 Build Overview', '', overviewText.trim()); - - const realErrors = parsedErrors.filter(e => e.code); - if (realErrors.length > 0) { - const byFile = {}; - for (const e of realErrors) { - const key = e.file || 'unknown'; - if (!byFile[key]) byFile[key] = []; - byFile[key].push(e); - } - parts.push('', '### ❌ Errors', ''); - for (const [fullPath, errors] of Object.entries(byFile)) { - parts.push(`**[\`${shortName(fullPath)}\`](${fileLink(fullPath)})**`); - const unique = [...new Map(errors.map(e => [`${e.code}:${e.message}`, e])).values()]; - for (const e of unique) { - const loc = e.line ? `[\`${e.code}\` (line ${e.line})](${fileLink(fullPath, e.line)})` : `\`${e.code}\``; - parts.push(`- ${loc}: ${e.message}`); - } - parts.push(''); - } - } else { - parts.push('', '### ❌ Errors', '', '```', data.errors || 'No errors captured', '```'); - } - - // Source context with fix hints - if (Object.keys(sourceContexts).length > 0 && realErrors.length > 0) { - parts.push('### 🔧 Source Context', ''); - for (const [fullPath, ctx] of Object.entries(sourceContexts)) { - const fileErrors = realErrors.filter(e => e.file === fullPath); - if (fileErrors.length === 0) continue; - const errLink = fileLink(fullPath, ctx.errorLine); - parts.push(`**[\`${shortName(fullPath)}\`](${errLink})**`); - parts.push('
Click to expand', '', '```csharp', ctx.content.trim(), '```', '
', ''); - } - } - - return parts.join('\n').substring(0, 15000); -} - -// --- Main --- - -async function main() { - let analysis = await callLLM(prompt.join('\n')); - if (analysis) { - analysis = linkifyFileReferences(analysis); - } else { - analysis = buildFallbackAnalysis(); - } - fs.writeFileSync('/tmp/analysis-result.md', analysis); - console.log('Analysis written to /tmp/analysis-result.md'); -} - -main().catch(e => { - console.error(e); - process.exitCode = 1; -}); diff --git a/.github/workflows/scripts/dump-binlog.js b/.github/workflows/scripts/dump-binlog.js new file mode 100644 index 0000000000..6188eee9a5 --- /dev/null +++ b/.github/workflows/scripts/dump-binlog.js @@ -0,0 +1,112 @@ +// Dumps overview / errors / warnings from a `*.binlog` file as JSON, by +// speaking the MCP stdio protocol to the locally-installed `binlog-mcp` +// dotnet global tool. Used by the build-failure-analysis agentic workflows +// as a pre-agent step, because the gh-aw MCP gateway does not support +// non-containerized stdio MCP servers — running this script ahead of the +// agent gets us the same data via plain JSON files the agent can `cat`. +// +// Usage: +// node dump-binlog.js +// +// Writes (best-effort — missing tools or parse failures are tolerated): +// /binlog-overview.json +// /binlog-errors.json +// /binlog-warnings.json +// +// Exit code: +// 0 on success or partial success (agent can still work from what it got) +// 1 if the MCP client could not start at all +// +// Keep this script intentionally small. All interpretation / fix proposal +// lives in the `build-failure-analyst` agent — this is plumbing only. + +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +const binlogPath = process.argv[2]; +const outputDir = process.argv[3] || '/tmp'; + +if (!binlogPath) { + console.error('Usage: node dump-binlog.js []'); + process.exit(1); +} + +if (!fs.existsSync(binlogPath)) { + console.error(`Binlog not found: ${binlogPath}`); + process.exit(1); +} + +const absoluteBinlog = path.resolve(binlogPath); +fs.mkdirSync(outputDir, { recursive: true }); + +function write(file, value) { + const target = path.join(outputDir, file); + fs.writeFileSync(target, JSON.stringify(value, null, 2)); + console.error(`wrote ${target}`); +} + +function extractText(response) { + if (!response?.content) { + return null; + } + return response.content + .filter((c) => c.type === 'text') + .map((c) => c.text) + .join('\n'); +} + +function tryParseJson(text) { + if (text === null || text === undefined) { + return null; + } + try { + return JSON.parse(text); + } catch { + return text; + } +} + +async function callTool(client, name, args) { + try { + const response = await client.callTool({ name, arguments: args }); + return tryParseJson(extractText(response)); + } catch (e) { + console.error(`tool ${name} failed: ${e?.message ?? e}`); + return null; + } +} + +async function main() { + let client; + try { + const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); + const { StdioClientTransport } = await import('@modelcontextprotocol/sdk/client/stdio.js'); + + const transport = new StdioClientTransport({ command: 'binlog-mcp', args: [] }); + client = new Client({ name: 'dump-binlog', version: '1.0.0' }); + await client.connect(transport); + + const overview = await callTool(client, 'binlog_overview', { binlog_file: absoluteBinlog }); + const errors = await callTool(client, 'binlog_errors', { binlog_file: absoluteBinlog }); + const warnings = await callTool(client, 'binlog_warnings', { binlog_file: absoluteBinlog, top: 10 }); + + write('binlog-overview.json', overview ?? { error: 'binlog_overview failed' }); + write('binlog-errors.json', errors ?? { error: 'binlog_errors failed' }); + write('binlog-warnings.json', warnings ?? { error: 'binlog_warnings failed' }); + } catch (e) { + console.error(`fatal: ${e?.stack ?? e?.message ?? e}`); + process.exitCode = 1; + } finally { + try { + if (client) { + await client.close(); + } + } catch { + /* ignore */ + } + } +} + +main(); diff --git a/.github/workflows/scripts/extract-binlog-errors.js b/.github/workflows/scripts/extract-binlog-errors.js deleted file mode 100644 index ea2547beae..0000000000 --- a/.github/workflows/scripts/extract-binlog-errors.js +++ /dev/null @@ -1,90 +0,0 @@ -// Script that communicates with binlog-mcp via MCP stdio protocol to extract build errors. -// Uses the official @modelcontextprotocol/sdk for reliable protocol handling. - -const path = require('path'); -const fs = require('fs'); - -const binlogPath = process.argv[2]; -if (!binlogPath) { - console.error('Usage: node extract-binlog-errors.js '); - process.exit(1); -} - -if (!fs.existsSync(binlogPath)) { - console.error(`Binlog not found: ${binlogPath}`); - process.exit(1); -} - -const absolutePath = path.resolve(binlogPath); - -async function main() { - let client; - let failed = false; - - try { - const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); - const { StdioClientTransport } = await import('@modelcontextprotocol/sdk/client/stdio.js'); - - const transport = new StdioClientTransport({ - command: 'binlog-mcp', - args: [], - }); - - client = new Client({ name: 'binlog-analyzer', version: '1.0.0' }); - await client.connect(transport); - - // Get build overview - const overview = await client.callTool({ - name: 'binlog_overview', - arguments: { binlog_file: absolutePath }, - }); - - // Get build errors - const errors = await client.callTool({ - name: 'binlog_errors', - arguments: { binlog_file: absolutePath }, - }); - - // Get build warnings (limited) - const warnings = await client.callTool({ - name: 'binlog_warnings', - arguments: { binlog_file: absolutePath, top: 10 }, - }); - - const errorsText = extractText(errors); - // Ensure errors is valid JSON array for downstream consumers - let errorsJson = errorsText; - try { JSON.parse(errorsText); } catch { - // If MCP returned plain text, wrap in a simple JSON array - errorsJson = JSON.stringify([{ severity: 'error', message: errorsText }]); - } - - const result = { - overview: extractText(overview), - errors: errorsJson, - warnings: extractText(warnings), - }; - - console.log(JSON.stringify(result, null, 2)); - } catch (e) { - const errorMessage = e instanceof Error ? (e.stack ?? e.message) : String(e); - console.error(`Error: ${errorMessage}`); - failed = true; - } finally { - try { if (client) await client.close(); } catch {} - } - - process.exitCode = failed ? 1 : 0; -} - -function extractText(response) { - if (response?.content) { - return response.content - .filter(c => c.type === 'text') - .map(c => c.text) - .join('\n'); - } - return 'No data'; -} - -main(); diff --git a/.github/workflows/scripts/package.json b/.github/workflows/scripts/package.json index 10a35e1fe9..f51809fac1 100644 --- a/.github/workflows/scripts/package.json +++ b/.github/workflows/scripts/package.json @@ -2,8 +2,8 @@ "name": "binlog-analysis-scripts", "version": "1.0.0", "private": true, - "description": "MCP client scripts for build failure analysis workflow", - "main": "extract-binlog-errors.js", + "description": "MCP client helper used by .github/workflows/build-failure-analysis*.md to dump binlog data as JSON before the agent runs.", + "main": "dump-binlog.js", "scripts": {}, "keywords": [], "author": "", diff --git a/.github/workflows/scripts/post-suggestions.js b/.github/workflows/scripts/post-suggestions.js deleted file mode 100644 index f4ab1c8fed..0000000000 --- a/.github/workflows/scripts/post-suggestions.js +++ /dev/null @@ -1,179 +0,0 @@ -// Posts LLM-generated inline fix suggestions as GitHub PR review comments -// with "Commit suggestion" buttons. - -module.exports = async ({ github, context, core }) => { - const fs = require('fs'); - const path = require('path'); - const workspace = process.env.GITHUB_WORKSPACE_PATH; - const ghToken = process.env.GH_TOKEN; - - let data; - try { - data = JSON.parse(fs.readFileSync(process.env.BINLOG_DATA, 'utf8')); - } catch { return; } - - let parsedErrors = []; - try { parsedErrors = JSON.parse(data.errors || ''); } catch {} - if (parsedErrors.length === 0) return; - - const prNumber = context.payload.pull_request.number; - const headSha = context.payload.pull_request.head.sha; - - // Get PR diff info - const prFiles = await github.paginate( - github.rest.pulls.listFiles, - { owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, per_page: 100 }, - ); - const prFilePaths = new Set(prFiles.map(f => f.filename)); - - // Build set of lines in the PR diff (GitHub rejects suggestions on non-diff lines) - const diffLines = new Set(); - for (const f of prFiles) { - if (!f.patch) continue; - const hunkRegex = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/gm; - let match; - while ((match = hunkRegex.exec(f.patch)) !== null) { - const start = parseInt(match[1]); - const count = parseInt(match[2] || '1'); - for (let i = start; i < start + count; i++) { - diffLines.add(`${f.filename}:${i}`); - } - } - } - - function toRelPath(absPath) { - const patterns = [workspace + '/', workspace + '\\', /^\/home\/runner\/work\/[^/]+\/[^/]+\//, /^D:\\a\\[^\\]+\\[^\\]+\\/]; - let rel = absPath; - for (const p of patterns) { - if (typeof p === 'string' && rel.startsWith(p)) { rel = rel.substring(p.length); break; } - else if (p instanceof RegExp) { rel = rel.replace(p, ''); } - } - return rel.replace(/\\/g, '/'); - } - - // Collect suggestion candidates: errors on diff lines (direct or declaration-side) - const candidates = []; - - for (const err of parsedErrors) { - if (!err.file || !err.line || !err.code) continue; - const relPath = toRelPath(err.file); - if (!prFilePaths.has(relPath) || !diffLines.has(`${relPath}:${err.line}`)) continue; - let contextLines = ''; - try { - let filePath = err.file; - if (!fs.existsSync(filePath)) filePath = path.join(workspace, relPath); - if (fs.existsSync(filePath)) { - const lines = fs.readFileSync(filePath, 'utf8').split('\n'); - const start = Math.max(0, err.line - 4); - const end = Math.min(lines.length, err.line + 4); - contextLines = lines.slice(start, end).map((l, i) => `${start + i + 1}: ${l}`).join('\n'); - } - } catch {} - if (contextLines) candidates.push({ err, relPath, contextLines }); - } - - // For errors on non-PR files, find the declaration in PR files - const nonPrErrors = [...new Map( - parsedErrors.filter(e => e.file && e.line && e.code && !prFilePaths.has(toRelPath(e.file))) - .map(e => [e.message, e]) - ).values()]; - - for (const err of nonPrErrors.slice(0, 3)) { - const nameMatch = err.message.match(/'([^']+)'/); - if (!nameMatch) continue; - for (const prFile of prFiles) { - if (!prFile.filename.endsWith('.cs')) continue; - try { - const fullPath = path.join(workspace, prFile.filename); - if (!fs.existsSync(fullPath)) continue; - const lines = fs.readFileSync(fullPath, 'utf8').split('\n'); - for (let i = 0; i < lines.length; i++) { - if (!lines[i].includes(nameMatch[1]) || !diffLines.has(`${prFile.filename}:${i + 1}`)) continue; - const start = Math.max(0, i - 3); - const end = Math.min(lines.length, i + 4); - const contextLines = lines.slice(start, end).map((l, idx) => `${start + idx + 1}: ${l}`).join('\n'); - candidates.push({ - err: { ...err, file: fullPath, line: i + 1 }, - relPath: prFile.filename, - contextLines, - isDeclaration: true, - }); - break; - } - } catch {} - if (candidates.length >= 10) break; - } - } - - if (candidates.length === 0) { - core.info('No suggestion candidates found in PR diff'); - return; - } - - // Ask LLM for exact replacement lines - const fixPrompt = [ - 'You are a C# code fix assistant. For each build error below, produce the EXACT fixed line(s) to replace the erroring line.', - 'Reply ONLY with a JSON array. Each element: {"index": N, "fixed_lines": "replacement code", "explanation": "one sentence"}', - 'Rules: preserve indentation, set fixed_lines to "" to delete a line, omit index if no fix, no markdown fences.', - '', - ]; - candidates.slice(0, 10).forEach((c, idx) => { - fixPrompt.push(`--- Error ${idx} ---`); - fixPrompt.push(`File: ${c.relPath}, Line: ${c.err.line}`); - fixPrompt.push(`Error: ${c.err.code}: ${c.err.message}`); - if (c.isDeclaration) fixPrompt.push('(Declaration that caused caller errors — make backward-compatible)'); - fixPrompt.push('Context:', c.contextLines, ''); - }); - - let fixes = []; - const endpoints = [ - 'https://models.github.ai/inference/chat/completions', - 'https://models.inference.ai.azure.com/chat/completions', - ]; - for (const endpoint of endpoints) { - if (fixes.length > 0) break; - try { - const resp = await fetch(endpoint, { - method: 'POST', - headers: { 'Authorization': `Bearer ${ghToken}`, 'Content-Type': 'application/json' }, - body: JSON.stringify({ - model: 'gpt-4o-mini', - messages: [{ role: 'user', content: fixPrompt.join('\n') }], - max_tokens: 2000, - }), - }); - if (resp.ok) { - const result = await resp.json(); - let content = result.choices?.[0]?.message?.content || ''; - content = content.replace(/^```(?:json)?\n?/m, '').replace(/\n?```$/m, '').trim(); - fixes = JSON.parse(content); - core.info(`LLM returned ${fixes.length} fix suggestion(s)`); - } else { - core.info(`${endpoint}: ${resp.status}`); - } - } catch (e) { - core.info(`Fix suggestion LLM failed: ${e.message}`); - } - } - - // Post suggestions - let posted = 0; - for (const fix of fixes) { - if (fix.index == null || fix.index >= candidates.length) continue; - const c = candidates[fix.index]; - const body = `🔧 **\`${c.err.code}\`**: ${fix.explanation || ''}\n\`\`\`suggestion\n${fix.fixed_lines ?? ''}\n\`\`\``; - try { - await github.rest.pulls.createReviewComment({ - owner: context.repo.owner, repo: context.repo.repo, - pull_number: prNumber, commit_id: headSha, - path: c.relPath, line: c.err.line, side: 'RIGHT', body, - }); - posted++; - core.info(`Posted suggestion on ${c.relPath}:${c.err.line}`); - } catch (e) { - core.info(`Could not post on ${c.relPath}:${c.err.line}: ${e.message}`); - } - if (posted >= 10) break; - } - core.info(`Posted ${posted} inline suggestion(s)`); -}; diff --git a/.github/workflows/shared/build-failure-analysis-shared.md b/.github/workflows/shared/build-failure-analysis-shared.md new file mode 100644 index 0000000000..101a8d61b3 --- /dev/null +++ b/.github/workflows/shared/build-failure-analysis-shared.md @@ -0,0 +1,55 @@ +--- +# Shared body for the build-failure-analysis workflows. +# +# Imported by build-failure-analysis.md (pull_request trigger) and +# build-failure-analysis-command.md (slash command). Keeps the prompt that +# delegates to the build-failure-analyst agent in one place. Per-trigger +# wiring (steps, env, mcp-servers, permissions) lives in each caller because +# gh-aw merges those fields from imports but each main workflow must still +# re-declare its top-level permissions. + +description: "Shared body for build-failure-analysis workflows" +--- + +# Build Failure Analyst + +Delegate to the `build-failure-analyst` agent defined at +`.github/agents/build-failure-analyst.agent.md` to analyze the binary log of +the build that just ran and post a PR review when (and only when) the build +failed. + +## Instructions + +1. Read the agent-context environment variables: `GH_AW_BUILD_OUTCOME`, + `GH_AW_BINLOG_PATH`, `GH_AW_PR_NUMBER`, `GH_AW_PR_HEAD_SHA`, + `GH_AW_WORKSPACE`. + +2. If `GH_AW_BUILD_OUTCOME == 'success'`, the build did not actually fail — + there is nothing to analyse. Call `noop` with the message + `"Build succeeded — no analysis required."` and stop. + +3. Otherwise, launch the `build-failure-analyst` agent as a **background** + task (`task` tool, `agent_type: "general-purpose"`, + `model: "claude-opus-4.6"`, `mode: "background"`). In the sub-agent prompt + include: + - All five `GH_AW_*` environment values verbatim so the sub-agent knows + which binlog metadata to read and where to post. + - A reminder that the pre-agent steps already dumped overview / errors / + warnings to `/tmp/binlog-data/*.json` and that the sub-agent should + start by `cat`ing those files via the `bash` tool. + - A reminder that the parent workflow `noop`s immediately and that the + sub-agent itself is responsible for calling `add_comment` (summary) and + `create_pull_request_review_comment` (inline ```suggestion blocks). + - A reminder that `submit_pull_request_review` is **not** a safe output + for this workflow — inline comments stand alone. + +4. **Immediately after launching the background task** — do NOT wait for it + to finish and do NOT read its result — call `noop` with a brief status + message such as + `"Build-failure analyst launched in background for PR #N. It will post the analysis directly."`. + Then stop. + +> **Important**: Reading the background agent result would pull its entire +> conversation (including every binlog query and every source file it +> inspected) into your context. Do not call `read_agent` or any equivalent +> after calling `noop`. diff --git a/.github/workflows/shared/msbuild-review-shared.md b/.github/workflows/shared/msbuild-review-shared.md new file mode 100644 index 0000000000..926922017f --- /dev/null +++ b/.github/workflows/shared/msbuild-review-shared.md @@ -0,0 +1,63 @@ +--- +# Shared configuration for MSBuild authoring review workflows. +# +# Imported by msbuild-quality-review.md. Keeps permissions, tools and +# safe-outputs in one place so any future trigger (slash command, +# `/review-msbuild`, etc.) can reuse the same contract. + +description: "Shared configuration for MSBuild authoring review workflows" + +permissions: + contents: read + issues: read + pull-requests: read + +tools: + github: + toolsets: [issues, repos] + bash: ["git", "grep", "find", "cat", "head", "tail", "sed", "wc", "sort", "date"] + +safe-outputs: + # The scan-mode msbuild-reviewer agent self-posts. Provide a generous-enough + # budget for a single weekly report plus an optional draft PR with safe fixes. + create-issue: + title-prefix: "[msbuild-quality] " + labels: [automation, msbuild, code-quality] + max: 1 + expires: 7d + create-pull-request: + draft: true + title-prefix: "[msbuild-quality] " + labels: [automation, msbuild, code-quality] + max: 1 + protected-files: fallback-to-issue + # NOTE: Consumers must also define this explicitly until workflow import/merge + # preserves `report-as-issue: false` in compiled lock files. + noop: + report-as-issue: false +--- + +# MSBuild Quality Review + +Review the MSBuild authoring quality of this repository using the +`msbuild-reviewer` agent defined at `.github/agents/msbuild-reviewer.agent.md`. + +## Instructions + +1. Launch the `msbuild-reviewer` agent in **`scan` mode** as a **background** task + (`task` tool, `agent_type: "general-purpose"`, `model: "claude-opus-4.6"`, + `mode: "background"`). In the sub-agent prompt include: + - The string `MODE: scan` on the first line so the agent picks the correct + operating mode. + - The repository owner/name and the current date. + - A reminder that the parent workflow `noop`s immediately and therefore the + sub-agent itself is responsible for calling `create_issue` (or + `create_pull_request` for safe auto-fixes only). +2. **Immediately after launching the background task** — do NOT wait for it to + finish and do NOT read its result — call `noop` with a brief status message + such as `"MSBuild reviewer launched in background. It will post the report directly."`. + Then stop. + +> **Important**: Reading the background agent result would pull its entire +> conversation (including every file it inspected) into your context. Do not +> call `read_agent` or any equivalent after calling `noop`.