From e16614d49ef0ffea0d2e6cc2c83eafc79cb8989c Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 3 Jun 2026 08:17:55 +0200 Subject: [PATCH 01/16] bootstrap arcade-services for agentic coding --- .github/agents/planner.agent.md | 27 ++++++++++++ .github/agents/test-specialist.agent.md | 25 +++++++++++ .github/copilot-instructions.md | 4 +- .github/copilot-setup-steps.yml | 15 ------- .github/hooks/security.json | 10 +++++ .../ef-migrations.instructions.md | 8 ++++ .github/instructions/testing.instructions.md | 14 ++++++ .github/prompts/generate-tests.prompt.md | 15 +++++++ .github/skills/add-darc-command/SKILL.md | 43 +++++++++++++++++++ .github/workflows/copilot-setup-steps.yml | 19 ++++++++ .gitignore | 1 + AGENTS.md | 35 +++++++++++++++ scripts/security-check.ps1 | 10 +++++ scripts/security-check.sh | 10 +++++ scripts/verify | 5 +++ scripts/verify.ps1 | 6 +++ src/AGENTS.md | 27 ++++++++++++ src/ProductConstructionService/AGENTS.md | 14 ++++++ 18 files changed, 271 insertions(+), 17 deletions(-) create mode 100644 .github/agents/planner.agent.md create mode 100644 .github/agents/test-specialist.agent.md delete mode 100644 .github/copilot-setup-steps.yml create mode 100644 .github/hooks/security.json create mode 100644 .github/instructions/ef-migrations.instructions.md create mode 100644 .github/instructions/testing.instructions.md create mode 100644 .github/prompts/generate-tests.prompt.md create mode 100644 .github/skills/add-darc-command/SKILL.md create mode 100644 .github/workflows/copilot-setup-steps.yml create mode 100644 AGENTS.md create mode 100644 scripts/security-check.ps1 create mode 100644 scripts/security-check.sh create mode 100644 scripts/verify create mode 100644 scripts/verify.ps1 create mode 100644 src/AGENTS.md create mode 100644 src/ProductConstructionService/AGENTS.md diff --git a/.github/agents/planner.agent.md b/.github/agents/planner.agent.md new file mode 100644 index 0000000000..7d09983f39 --- /dev/null +++ b/.github/agents/planner.agent.md @@ -0,0 +1,27 @@ +--- +name: planner +description: Plans cross-cutting changes spanning the Maestro, DARC, and Product Construction Service components. Use for architecture/design work before implementation; hands off to implementers. +tools: ['read', 'search'] +handoffs: ['test-specialist'] +--- + +# Planner + +Produces an implementation plan for changes that touch multiple components without editing code itself. + +## Scope +- `src/Maestro/` (legacy shared libraries) +- `src/Microsoft.DotNet.Darc/` (DARC CLI + DarcLib) +- `src/ProductConstructionService/` (PCS service, Aspire/Docker) + +## Process + + + +## Output +- A step-by-step plan identifying affected projects, contracts, and tests. +- Explicit hand-off notes for the implementer / test-specialist. + +## Constraints +- Read-only — do not modify code. +- Flag any change that requires regenerating the `Microsoft.DotNet.ProductConstructionService.Client`. diff --git a/.github/agents/test-specialist.agent.md b/.github/agents/test-specialist.agent.md new file mode 100644 index 0000000000..442e959b37 --- /dev/null +++ b/.github/agents/test-specialist.agent.md @@ -0,0 +1,25 @@ +--- +name: test-specialist +description: Writes and expands unit tests for arcade-services using the repo's NUnit + Moq + AwesomeAssertions conventions. Use when asked to add tests, improve coverage, or test a specific class. +tools: ['read', 'edit', 'search', 'runTerminalCommand'] +--- + +# Test Specialist + +Focused on producing high-value unit tests that follow this repo's testing conventions. + +## Conventions +- NUnit (`[Test]`, `[TestFixture]`), Moq for mocks, AwesomeAssertions for fluent assertions. +- Arrange-Act-Assert (AAA) structure. +- Mirror the existing `test/` project layout (Darc, Maestro, ProductConstructionService). + +## Process + + + +## Constraints +- Do NOT add tests to `test/ProductConstructionService.ScenarioTests` — they require a deployed service. +- Never weaken assertions just to make a test pass. + +## Validation +- `dotnet test --no-build` passes for the affected test project. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index eaf14b2eb6..e41b370710 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -5,7 +5,7 @@ The .NET Arcade Services repository contains the Product Construction Service (p ## Working Effectively ### Prerequisites and Environment Setup -- Install .NET 8 SDK (the build script will download the specific required version automatically) +- Install .NET 10 SDK (the build script will download the specific required version automatically) - For full local development: Install Docker Desktop (required for Product Construction Service) - For Windows development: Install Visual Studio with Azure Development and ASP.NET workloads - Configure git for long paths: `git config --global core.longpaths true` @@ -89,7 +89,7 @@ After making changes, validate by testing these workflows: - `.github/` - GitHub workflows and templates ## Key Technologies -- .NET 8 (see global.json for exact version) +- .NET 10 (see global.json for exact version) - Azure DevOps APIs - ASP.NET Core for web APIs - Entity Framework Core for data access diff --git a/.github/copilot-setup-steps.yml b/.github/copilot-setup-steps.yml deleted file mode 100644 index 8e70e2979f..0000000000 --- a/.github/copilot-setup-steps.yml +++ /dev/null @@ -1,15 +0,0 @@ -name: Copilot Setup Steps - -# These steps will be executed before GitHub Copilot assists with code -# This helps ensure Copilot has access to properly restored dependencies - -steps: -- name: Checkout repository - uses: actions/checkout@v3 - with: - fetch-depth: 0 - -- name: Restore dependencies - run: | - ./eng/common/build.sh -restore - shell: bash diff --git a/.github/hooks/security.json b/.github/hooks/security.json new file mode 100644 index 0000000000..9f075b750b --- /dev/null +++ b/.github/hooks/security.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "hooks": { + "preToolUse": [{ + "type": "command", + "bash": "./scripts/security-check.sh", + "powershell": "pwsh -File scripts/security-check.ps1" + }] + } +} diff --git a/.github/instructions/ef-migrations.instructions.md b/.github/instructions/ef-migrations.instructions.md new file mode 100644 index 0000000000..8dc9410411 --- /dev/null +++ b/.github/instructions/ef-migrations.instructions.md @@ -0,0 +1,8 @@ +--- +applyTo: 'src/Maestro/Maestro.Data/Migrations/**' +--- +# EF Core Migrations +**When to read:** Changing the data model or migrations. + +- These files are generated by `dotnet ef migrations add` — do not hand-edit the generated `*.Designer.cs` or snapshot files. +- Create a new migration for model changes rather than editing existing ones. See `docs/DevGuide.md` for the exact EF commands. diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md new file mode 100644 index 0000000000..cbdd3aab7b --- /dev/null +++ b/.github/instructions/testing.instructions.md @@ -0,0 +1,14 @@ +--- +applyTo: 'test/**/*.cs' +--- +# Testing Conventions +**When to read:** Writing or modifying test files. + +- Use NUnit (`[Test]`, `[TestFixture]`), Moq for mocks, AwesomeAssertions for fluent assertions. +- Follow the Arrange-Act-Assert (AAA) pattern. +- Do NOT add tests to `test/ProductConstructionService.ScenarioTests` unless intentionally writing deployed-service scenarios — they require a full service deployment and are excluded from local/agent verification. + +## Codeflow tests +- `test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests` are local end-to-end tests: they create real on-disk git repositories and exercise the real codeflow classes through a real `ServiceProvider` (no mocks except the BAR/API client). They require `git` on PATH. +- Extend `CodeFlowTestsBase` for these; reuse its repo/VMR setup and `GitOperations` helper rather than rolling your own. +- They are slower than ordinary unit tests — keep them deterministic and clean up their temp directories. diff --git a/.github/prompts/generate-tests.prompt.md b/.github/prompts/generate-tests.prompt.md new file mode 100644 index 0000000000..f0de1986e6 --- /dev/null +++ b/.github/prompts/generate-tests.prompt.md @@ -0,0 +1,15 @@ +--- +description: Generate NUnit unit tests for the active file following repo conventions. +--- + +# Generate Tests + +Generate unit tests for `${file}` using this repo's conventions. + +- Framework: NUnit (`[Test]`, `[TestFixture]`). +- Mocking: Moq. Assertions: AwesomeAssertions (fluent). +- Structure each test with the Arrange-Act-Assert (AAA) pattern. +- Place the test in the matching `test/` project; reuse existing fixtures/helpers where available. +- Do NOT target `test/ProductConstructionService.ScenarioTests` (requires a deployed service). + + diff --git a/.github/skills/add-darc-command/SKILL.md b/.github/skills/add-darc-command/SKILL.md new file mode 100644 index 0000000000..a4a15b250b --- /dev/null +++ b/.github/skills/add-darc-command/SKILL.md @@ -0,0 +1,43 @@ +--- +name: add-darc-command +description: 'Add a new command (verb) to the DARC CLI. Use when asked to "add a darc command", "create a new darc verb", "add a darc operation", or extend the darc tool with a new subcommand.' +--- + +# Add a DARC CLI Command + +Adds a new verb to the DARC CLI by creating a paired CommandLineOptions class and an Operation class. The `CommandLine` library auto-discovers verbs via the `[Verb]` attribute. + +## When to Use + +- Adding a new `darc ` subcommand. +- Exposing a new BAR / configuration operation through the CLI. + +## Process + +### Step 1: Create the options class +Create `src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs`. +- Decorate with `[Verb("my-verb", HelpText = "...")]`. +- Inherit the appropriate base (e.g. `CommandLineOptions`, `ConfigurationManagementCommandLineOptions`) — pick the base used by sibling commands with the same auth/context needs. +- Add `[Option(...)]` properties for each argument; mark required ones `Required = true`. + + +### Step 2: Create the operation class +Create `src/Microsoft.DotNet.Darc/Darc/Operations/Operation.cs`. +- Inherit the matching operation base used by the options' generic parameter. +- Inject services via the constructor (e.g. `IBarApiClient`, `ILogger`). +- Implement `protected override async Task ExecuteInternalAsync()` and return a process exit code. + + +### Step 3: Wire up dependencies + + +## Constraints +- Async methods must end in `Async`; never block with `.Result`/`.Wait()`. +- Use `ILogger` and structural logging; each method logs its own actions. +- Never throw generic `Exception`. +- Match the existing options/operation base-class conventions — do not invent a new pattern. + +## Validation +- `dotnet build` succeeds (warnings are errors in this repo). +- `dotnet run --project src/Microsoft.DotNet.Darc/Darc -- my-verb --help` shows the new command and its options. +- Add/extend unit tests under `test/` (NUnit + Moq + AwesomeAssertions, AAA pattern). diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml new file mode 100644 index 0000000000..3e31605e56 --- /dev/null +++ b/.github/workflows/copilot-setup-steps.yml @@ -0,0 +1,19 @@ +name: "Copilot Setup Steps" + +on: workflow_dispatch + +jobs: + copilot-setup-steps: + runs-on: ubuntu-latest + environment: copilot + timeout-minutes: 30 + permissions: + contents: read + id-token: write + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - name: Restore dependencies + run: ./eng/common/build.sh -restore + shell: bash diff --git a/.gitignore b/.gitignore index 3b7e679024..d559fd4b55 100644 --- a/.gitignore +++ b/.gitignore @@ -236,3 +236,4 @@ node_modules/ /eng/git-commit-diagram.mmd + diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..1e8ae16f89 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,35 @@ +# AGENTS.md + +## Verification +- Build & test: `dotnet build` then `dotnet test --no-build` +- The required .NET SDK is pinned in `global.json` — assume it is installed. +- If verification fails, fix the root cause and re-run. + +## Environment +- .NET 10 (see `global.json`). `Directory.Build.props` sets `TreatWarningsAsErrors=true` — unused usings and warnings break the build. +- Build via `dotnet build`, `Build.cmd` (Windows), or `./build.sh` (Linux/macOS); the repo uses the Arcade SDK. + +## Guardrails +- Async methods must have an `Async` suffix; prefer async/await over `.Result`/`.Wait()`. +- Never throw generic `Exception`; use structural logging and `ILogger` (not `ILogger`). +- Prefer immutable types (records / readonly); annotate nullable reference types. +- Tests: NUnit + Moq + AwesomeAssertions, AAA pattern. +- Do NOT run `test/ProductConstructionService.ScenarioTests` — they require a deployed service. + +## Constraints +- Keep diffs minimal and scoped to the request. +- Update or add tests for any behavior change. +- Do not modify CI, dependency versions, or security settings unless asked. +- Never print, log, or commit secrets. + +## Learning from corrections +- When the user corrects you, rejects an approach, or states a durable preference or convention, store it with Copilot Memory (the `store_memory` tool) so it persists across sessions. +- Only store durable, generally-applicable facts — not ephemeral, task-specific instructions ("for this PR…", "just this once…"). +- Before storing, check the surfaced memories; if a similar fact exists, upvote/refine it instead of adding a near-duplicate. +- Keep each memory atomic and short, and don't store anything already covered by AGENTS.md, `.github/instructions/`, or inferable from code — prefer in-repo docs for team conventions, reserving memory for cross-session preferences. +- If a correction contradicts an existing memory, update it: downvote the outdated memory and store the corrected fact. + +## Where to find more +- Detailed conventions & build guide: `.github/copilot-instructions.md` and `docs/DevGuide.md` +- Path-specific rules: `.github/instructions/` +- Multi-step workflows: `.github/skills/*/SKILL.md` diff --git a/scripts/security-check.ps1 b/scripts/security-check.ps1 new file mode 100644 index 0000000000..f0e1d96242 --- /dev/null +++ b/scripts/security-check.ps1 @@ -0,0 +1,10 @@ +$ErrorActionPreference = 'Stop' +# Block destructive commands — customize this blocklist for your repo +$blockedPatterns = @('rm -rf /', 'DROP DATABASE', 'format C:', 'mkfs', 'git push --force') +$commandText = $args -join ' ' +foreach ($pattern in $blockedPatterns) { + if ($commandText -match [regex]::Escape($pattern)) { + Write-Error "Blocked: destructive pattern detected ($pattern)" + exit 1 + } +} diff --git a/scripts/security-check.sh b/scripts/security-check.sh new file mode 100644 index 0000000000..0f04603864 --- /dev/null +++ b/scripts/security-check.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash +set -euo pipefail +# Block destructive commands — customize this blocklist for your repo +BLOCKED_PATTERNS=("rm -rf /" "DROP DATABASE" "format C:" "mkfs" "git push --force") +for pattern in "${BLOCKED_PATTERNS[@]}"; do + if echo "$*" | grep -qi "$pattern"; then + echo "❌ Blocked: destructive pattern detected ($pattern)" >&2 + exit 1 + fi +done diff --git a/scripts/verify b/scripts/verify new file mode 100644 index 0000000000..d0a61d3fd4 --- /dev/null +++ b/scripts/verify @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +set -euo pipefail +# Verify: repeatable health check. Assumes the required .NET SDK (see global.json) is installed. +dotnet build +dotnet test --no-build diff --git a/scripts/verify.ps1 b/scripts/verify.ps1 new file mode 100644 index 0000000000..9b4cf102e6 --- /dev/null +++ b/scripts/verify.ps1 @@ -0,0 +1,6 @@ +$ErrorActionPreference = 'Stop' +# Verify: repeatable health check. Assumes the required .NET SDK (see global.json) is installed. +dotnet build +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } +dotnet test --no-build +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } diff --git a/src/AGENTS.md b/src/AGENTS.md new file mode 100644 index 0000000000..6a3e260ce9 --- /dev/null +++ b/src/AGENTS.md @@ -0,0 +1,27 @@ +# AGENTS.md — `src/` project map + +Scope: `src/`. Extends the root AGENTS.md. A brief orientation of what lives where; much dependency-flow work spans several of these projects. + +## DARC CLI — `Microsoft.DotNet.Darc/` +- `Darc` — the DARC command-line tool (verbs = Operations + CommandLineOptions pairs). +- `DarcLib` — the core dependency-flow library (VMR / codeflow, git operations, version files) shared by DARC and PCS. + +## Maestro libraries — `Maestro/` +(Former Maestro service; now shared libraries consumed by PCS.) +- `Maestro.Data` — EF Core data layer and the Build Asset Registry (BAR) `DbContext` + migrations. +- `Maestro.DataProviders` — BAR data-access implementations (SQL BAR client, remote/token factories). +- `Maestro.Common` — shared utilities (git URL helpers, caching, logging, version constants). +- `Maestro.Services.Common` — shared host/service wiring (database, Key Vault, data protection, service defaults). +- `Maestro.WorkItems` — background work-item and reminder infrastructure. +- `Maestro.MergePolicies` / `Maestro.MergePolicyEvaluation` — merge policy definitions and their evaluation logic/models. +- `Microsoft.DotNet.Maestro.Tasks` — MSBuild tasks used by builds to publish build/asset metadata to the BAR. + +## Product Construction Service — `ProductConstructionService/` +See `ProductConstructionService/AGENTS.md` for run/convention details. +- `ProductConstructionService.Api` — main service host (ASP.NET Core API + background workers, Dockerized). +- `ProductConstructionService.AppHost` — .NET Aspire orchestration for running the service locally. +- `ProductConstructionService.DependencyFlow` — core dependency-flow logic: subscription triggering, PR updaters/targets, merge policy evaluation. +- `ProductConstructionService.SubscriptionTriggerer` — job that triggers subscriptions on a schedule. +- `ProductConstructionService.FeedCleaner` — job that cleans up stale package feeds. +- `ProductConstructionService.BarViz` — Blazor web UI for visualizing the Build Asset Registry. +- `Microsoft.DotNet.ProductConstructionService.Client` — generated client for the PCS API; regenerate when API contracts change. diff --git a/src/ProductConstructionService/AGENTS.md b/src/ProductConstructionService/AGENTS.md new file mode 100644 index 0000000000..0e4be9b6a2 --- /dev/null +++ b/src/ProductConstructionService/AGENTS.md @@ -0,0 +1,14 @@ +# AGENTS.md — Product Construction Service (PCS) + +Scope: `src/ProductConstructionService/`. Extends the root AGENTS.md. See `src/AGENTS.md` for the project map. + +## Running locally +- Run the service via the Aspire AppHost: `dotnet run --project ProductConstructionService.AppHost`. +- Requires Docker Desktop running. Do not assume a plain `dotnet run` on the Api project is sufficient. + +## Conventions +- The API uses Newtonsoft.Json with camelCase properties and camelCase string enums; dates are ISO-8601 UTC (`yyyy-MM-ddTHH:mm:ssZ`). Match this when adding contracts. +- API controllers live in `ProductConstructionService.Api/Api/`. + +## Safety +- `test/ProductConstructionService.ScenarioTests` require a fully deployed service — never run them locally or in agent verification. From 6b3f3ccf6c92016d59ae2ba3e1653183d47f6208 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 10 Jun 2026 17:38:04 +0200 Subject: [PATCH 02/16] First draft, still no review --- docs/CodeflowSourceDiffVerification.md | 240 ++++++++++++++++++ .../VirtualMonoRepo/CodeflowChangeAnalyzer.cs | 236 +++++++++++++++++ .../CodeFlowPullRequestUpdater.cs | 80 ++++++ .../CodeflowChangeAnalyzerTests.cs | 10 + .../Operations/Operation.cs | 4 +- 5 files changed, 568 insertions(+), 2 deletions(-) create mode 100644 docs/CodeflowSourceDiffVerification.md diff --git a/docs/CodeflowSourceDiffVerification.md b/docs/CodeflowSourceDiffVerification.md new file mode 100644 index 0000000000..1964d75f58 --- /dev/null +++ b/docs/CodeflowSourceDiffVerification.md @@ -0,0 +1,240 @@ +# Codeflow Source-Diff Verification + +## Status + +Proposed / design. Not yet implemented. + +## Goal (high level) + +When PCS opens a **forward-flow** codeflow PR (changes flowing from a source repo +into the VMR, e.g. `dotnet/efcore` -> `dotnet/dotnet`), reviewers currently have +no automated confirmation that the PR actually contains the changes from the +source build. The PR description only prints a `darc vmr diff` command they could +run by hand. + +The goal of this feature is to give reviewers a **confidence signal**: after the +codeflow commit is pushed, automatically verify that the PR faithfully reflects +the source repo's commit diff (`oldSha..newSha`), and post the result as a +**follow-up PR comment**. + +This is a *reporting / confidence* feature, **not** a correctness gate: + +- It tells reviewers "this PR == the source diff, minus the expected exclusions + and no-ops". +- It does **not** prove the VMR mapping configuration itself is correct, because + it reuses the same `source-mappings.json` exclusions the codeflow used. A bug + in that config would be invisible to this check. The comment should state this + limitation so reviewers don't over-trust it. + +## Why this is not a trivial 1:1 diff + +A correct forward-flow PR is intentionally **not** a byte-for-byte copy of the +source diff. Comparing the two diffs naively produces false mismatches. The known, +legitimate sources of divergence are: + +1. **Path remap** — source paths (`test/Foo.cs`) live under `src//` in + the VMR (`src/efcore/test/Foo.cs`). +2. **Cloaked / excluded paths** — `source-mappings.json` `exclude` rules and + submodules. Source-side changes to those are expected to be absent from the PR. +3. **`eng/common/**`** — Arcade-managed and not flowed per-repo (except for the + `arcade` mapping itself). +4. **Version / metadata files** — `src/source-manifest.json`, `Version.Details.xml`, + `Version.Details.props`, `Versions.props`, `global.json`, etc. get + codeflow-specific content (BAR id, SHAs) that won't match the source diff. +5. **Already-at-target ("no-op") files** — the source diff changes a file, but the + VMR copy was already at the new content (brought there by a prior flow or other + means). Codeflow correctly produces no change for it. A name-only comparison + would wrongly report this as "missing". +6. **Cosmetic diff-render differences** — hunk-header line offsets (`@@ -27` vs + `@@ -24`) and trailing-newline markers (`\ No newline at end of file`). The same + logical change renders differently because surrounding lines differ between the + source file and its VMR copy. + +The verification must therefore be *semantic* (compare the actual changed lines, +after normalizing away the items above), not textual. + +## Inputs + +Per forward-flow PR: + +| Value | Meaning | Source | +|-----------------|----------------------------------------------------------------------|--------| +| source repo | e.g. `https://github.com/dotnet/efcore` | subscription | +| `newSha` | source commit being flowed | `build.Commit` | +| `oldSha` | previously-flowed source commit | mapping's commit in the **merge-base** `source-manifest.json` | +| `mapping` | VMR target directory, e.g. `efcore` | subscription `TargetDirectory` | +| VMR repo | target repo URL | PR target repo | +| `headBranch` | the codeflow PR branch | PR head | +| `targetBranch` | branch the PR targets, e.g. `main` | subscription `TargetBranch` | + +`oldSha...newSha` is exactly the compare range shown in the PR description's +"Commit Diff" link. + +## Algorithm + +Two phases. **Phase 1** cheaply enumerates and partitions the touched files; +**Phase 2** compares the actual changes only where needed. + +### Phase 1 — name-only partition + +Build two file sets in the same coordinate space (mapping-relative paths): + +Version/metadata files that codeflow rewrites with codeflow-specific content +(BAR id, SHAs) change in **both** the source repo and the VMR but with +**different** content. They can never match in Phase 2 and would false-flag, so +they must be dropped from **both** R and A. The canonical set is +`DependencyFileManager.CodeflowDependencyFiles` = +`{ eng/Version.Details.xml, eng/Version.Details.props, global.json, +.config/dotnet-tools.json }`. + +Note: `Versions.props` is deliberately **not** in that set ("In VMR repos, +Versions.props doesn't contain dependency versions maintained by automation, so +every change is meaningful"). It flows as a normal file, so we do **not** drop it +— if it was a no-op (VMR already at target) it is correctly cleared by the no-op +check in `R \ A`. The root `src/source-manifest.json` lives outside +`src//`, so scoping A to `src//` already excludes it. + +- **R** ("reference", what the source diff should contribute): + `git diff --name-only oldSha...newSha` in the **source repo**, then + - drop paths matching the mapping's `exclude`/submodule filters + (`GetDiffFilters` already computes these from `source-mappings.json`), + - drop `eng/common/**` for non-arcade mappings, + - drop the version/metadata files listed above + (`DependencyFileManager.CodeflowDependencyFiles`). +- **A** ("actual", what the PR changed under the mapping): + `git diff --name-only targetBranch...headBranch -- src//` in the + **VMR**, then strip the `src//` prefix, and + - drop `eng/common/**` for non-arcade mappings, + - drop the same version/metadata files. + +Partition: + +| Bucket | Meaning | Action | +|-----------|--------------------------------------|--------| +| `R ∩ A` | touched in both | Phase 2 content compare | +| `R \ A` | source changed it, PR did not | no-op file-state check (below) | +| `A \ R` | changed in PR only | **must be empty** — else flag (codeflow wrote changes not traceable to the source diff) | + +> **Why `A \ R` must be empty.** The VMR diff is three-dot +> (`targetBranch...headBranch`), so it contains only the changes the PR +> introduced — not unrelated edits already on the target branch. The comment is +> posted right after PCS pushes the branch, before any human edits the PR. So +> after the same exclusions are applied to `A` (version/metadata files and, for +> non-arcade mappings, `eng/common/**`), any remaining file the PR changed under +> `src//` that is absent from the source diff means the codeflow +> produced a change that does not come from the source — a correctness red flag, +> not an expected divergence. (This is what lets the signal *guarantee* the +> codeflow reproduced exactly the source diff, rather than merely "contains" it.) + +### Phase 2 — per-file content compare (`R ∩ A`) + +For each file, scope a zero-context diff to that single file in each repo: + +```bash +git -C diff -U0 oldSha...newSha -- +git -C diff -U0 targetBranch...headBranch -- src// +``` + +Normalize each by removing the lines that are *expected* to differ, then compare +what remains: + +- Strip `diff --git`, `index `, `@@ `, `--- `, `+++ ` (this is exactly + `CodeflowChangeAnalyzer.IgnoredDiffLines`). With `-U0` there are no context + lines, so only `+`/`-` change lines remain. +- Compare the remaining `+`/`-` lines. + - equal -> **match** (change applied faithfully). + - different -> **flag**: "changes don't match". + +Decision to record: whether to strip trailing-newline markers +(`\ No newline at end of file`). They are a genuine byte difference; for mapped +source files we keep them (only observed on excluded version files so far). + +### No-op check (`R \ A`) + +A file in `R \ A` is only legitimate if the VMR is **already at the source's new +state**. Diff content can't distinguish "correctly no-op'd" from "wrongly +dropped", so compare the resulting **file state**: + +``` +content(VMR_head : src//) vs content(source@newSha : ) +``` + +- equal, **or both absent** (deletion already reconciled) -> legitimate no-op, + ignore. +- different -> **flag**: "changes don't match". + +## Output + +The goal is simple: run the check and post a short PR comment expressing our +confidence. It is a positive/negative signal, not a detailed report. + +- Empty flag set (`R ∩ A` all match, `R \ A` all no-ops, `A \ R` empty) => post a + short confirmation, e.g.: + + ``` + ### Source diff verification + ✅ This PR matches the source diff 60e46e1...5932bea of dotnet/wpf. + ``` + +- Anything flagged (mismatching content or unexpected files) => post a short + "couldn't verify, please review manually" comment, e.g.: + + ``` + ### Source diff verification + ⚠️ Couldn't verify that this PR matches the source diff 60e46e1...5932bea of + dotnet/wpf. Please review the changes manually. + ``` + +Both comments carry a footnote that this is a best-effort signal validated under +the current source-mappings config. + +## Reuse / where it plugs in + +- **`CodeflowChangeAnalyzer`** (`DarcLib/VirtualMonoRepo`) already computes the + VMR-side diff against the merge base, classifies source vs version/metadata + files, handles the non-arcade `eng/common` exclusion, and defines + `IgnoredDiffLines`. This feature lives **inside** `CodeflowChangeAnalyzer` as + `VerifyForwardFlowAsync` and reuses these helpers/constants directly. +- **`GetDiffFilters`** (`Darc/Operations/VirtualMonoRepo/VmrDiffOperation.cs`) + already turns `source-mappings.json` excludes + submodules into git pathspec + exclusion rules; reuse it to filter **R**. +- **`ForwardFlowMergePolicy`** already obtains `mergeBase`, head/merge-base + `SourceManifest`s, and validates them; the SHAs and manifests this feature + needs are already available at evaluation time. +- PCS already has the VMR cloned locally during flow + (`ILocalGitRepoFactory`); the source `oldSha...newSha` is available either from + the patch codeflow already built or via a remote diff call. + +The analyzer returns the bucketed result (list of mismatching files; empty = +match). PCS renders it into a follow-up PR comment, separate from PR creation so +it never slows down or fails the codeflow push. + +## Scope / limitations + +- Trusts `R ∩ A` on presence + content-line equality; it does not re-run the + 3-way merge, so it validates "PR reflects the source diff under the current + mapping config", not "the config is correct". +- `A \ R` must be empty after the standard exclusions: a non-empty set means the + codeflow changed files under `src//` that don't trace back to the + source diff. This assumes the check runs right after PCS pushes, before a human + adds conflict-resolution or unrelated commits to the PR branch (which would also + show up in `A`); that is the only intended call site. +- Large flows (e.g. `runtime`) can touch thousands of files; Phase 1 is name-only + and cheap, but Phase 2/no-op content fetches should be bounded (only `R ∩ A` + and `R \ A`). Consider a size cap or running fully asynchronously as a comment. + +## Validation done so far + +Manually verified the algorithm by hand against: + +- **dotnet/dotnet#7107** (efcore, small): R = A = 5 files, `R \ A` empty, all 5 + files match content 1:1. +- **dotnet/dotnet#7142** (wpf, larger): applying the full algorithm — + `R∩A` = 40 files (all content-match), `R\A` = 2 files + (`.github/policies/resourceManagement.yml`, `eng/restore-toolset.ps1`), both + confirmed genuine no-ops (VMR head equals source@newSha), and `A\R` = 0 after + exclusions — the 17 PR-only files were all `eng/common/**`, which is dropped + from `A` for non-arcade mappings before the `A\R` check, so nothing remained to + flag. No mismatches flagged. Version files + (`Versions.props`, `Version.Details.xml`, `global.json`, ...) were excluded from + both sides as designed. diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs index 390de55ba4..a25fdfe7eb 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs @@ -4,9 +4,11 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.Models; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using Microsoft.Extensions.Logging; @@ -16,6 +18,20 @@ namespace Microsoft.DotNet.DarcLib.VirtualMonoRepo; public interface ICodeflowChangeAnalyzer { Task ForwardFlowHasMeaningfulChangesAsync(string mappingName, string headBranch, string targetBranch); + + /// + /// Verifies that a forward-flow codeflow PR (source repo -> VMR) faithfully contains the source + /// repo's commit diff (oldSha...newSha), accounting for the expected divergences (path remap, + /// excludes, eng/common, version files, no-ops). + /// + Task VerifyForwardFlowAsync( + string mappingName, + string sourceRepoUri, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default); } /// @@ -39,6 +55,9 @@ public class CodeflowChangeAnalyzer : ICodeflowChangeAnalyzer private readonly ILocalGitRepoFactory _localGitRepoFactory; private readonly IVersionDetailsParser _versionDetailsParser; private readonly IBasicBarClient _barClient; + private readonly IRepositoryCloneManager _cloneManager; + private readonly IVmrDependencyTracker _dependencyTracker; + private readonly ISourceManifest _sourceManifest; private readonly IVmrInfo _vmrInfo; private readonly ILogger _logger; @@ -46,12 +65,18 @@ public CodeflowChangeAnalyzer( ILocalGitRepoFactory localGitRepoFactory, IVersionDetailsParser versionDetailsParser, IBasicBarClient barClient, + IRepositoryCloneManager cloneManager, + IVmrDependencyTracker dependencyTracker, + ISourceManifest sourceManifest, IVmrInfo vmrInfo, ILogger logger) { _localGitRepoFactory = localGitRepoFactory; _versionDetailsParser = versionDetailsParser; _barClient = barClient; + _cloneManager = cloneManager; + _dependencyTracker = dependencyTracker; + _sourceManifest = sourceManifest; _vmrInfo = vmrInfo; _logger = logger; } @@ -233,4 +258,215 @@ private static IEnumerable GetExpectedContentsForBuild(Build? b) ..darcFeeds, ]; } + + public async Task VerifyForwardFlowAsync( + string mappingName, + string sourceRepoUri, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default) + { + _logger.LogInformation( + "Verifying forward flow PR for {mappingName} against source diff {oldSha}...{newSha}", + mappingName, + oldSha, + newSha); + + ILocalGitRepo vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath); + var srcMappingPath = VmrInfo.GetRelativeRepoSourcesPath(mappingName); + + await vmr.CheckoutAsync(vmrHeadBranch); + await _dependencyTracker.RefreshMetadataAsync(); + SourceMapping mapping = _dependencyTracker.GetMapping(mappingName); + + var exclusionPathspecs = GetDiffFilters(mapping, _sourceManifest); + + // Obtain a local clone of the source repo containing both SHAs. + ILocalGitRepo sourceRepo = await _cloneManager.PrepareCloneAsync( + mapping, + [sourceRepoUri], + [oldSha, newSha], + newSha, + resetToRemote: false, + cancellationToken); + + var srcMappingPrefix = srcMappingPath + "/"; + HashSet sourceRepoChanges = await GetChangedMappingFilesAsync( + sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); + HashSet targetRepoChanges = await GetChangedMappingFilesAsync( + vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); + + var intersection = sourceRepoChanges.Where(targetRepoChanges.Contains).ToList(); + var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !targetRepoChanges.Contains(f)).ToList(); + var unexpectedFiles = targetRepoChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); + + // The PR changed files that the source diff didn't - the codeflow can't be trusted. + if (unexpectedFiles.Count > 0) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {unexpected} file(s) changed in the PR but not in the source diff", + mappingName, + unexpectedFiles.Count); + return false; + } + + // Per-file content compare on the intersection. + foreach (var file in intersection) + { + if (!await ChangedLinesMatchAsync(sourceRepo, vmr, file, srcMappingPath, oldSha, newSha, vmrTargetBranch, vmrHeadBranch, cancellationToken)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: changes to {file} don't match the source diff", + mappingName, + file); + return false; + } + } + + // No-op check on files the source changed but the PR did not. + foreach (var file in sourceRepoOnlyChanges) + { + if (!await IsLegitimateNoOpAsync(sourceRepo, vmr, file, srcMappingPath, newSha, vmrHeadBranch)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {file} changed in the source diff but is not reflected in the PR", + mappingName, + file); + return false; + } + } + + _logger.LogInformation("Source diff verification for {mappingName} passed", mappingName); + return true; + } + + /// + /// Builds the git pathspec exclusion rules the same way VmrDiffOperation.GetDiffFilters does: + /// the mapping's excludes plus submodule paths under the mapping, turned into git exclusion rules. + /// + private static IReadOnlyCollection GetDiffFilters(SourceMapping mapping, ISourceManifest manifest) + { + var submodules = manifest.Submodules + .Where(s => s.Path.StartsWith(mapping.Name + '/', StringComparison.OrdinalIgnoreCase)) + .Select(s => s.Path.Substring(mapping.Name.Length + 1)); + + return (mapping.Exclude ?? []) + .Concat(submodules) + .Select(VmrPatchHandler.GetExclusionRule) + .ToList(); + } + + /// + /// Runs a three-dot name-only diff (...) scoped to the + /// mapping's location in the repo and returns the mapping-relative paths, after dropping eng/common + /// (non-arcade) and version/metadata files. Used for both the source repo diff (mapping lives at the repo + /// root, so is empty) and the target repo (VMR) diff (mapping lives + /// under src/<mapping>/, which is stripped from the resulting paths). + /// + private async Task> GetChangedMappingFilesAsync( + ILocalGitRepo repo, + string mappingName, + string fromRef, + string toRef, + string? relativePath = null, + IReadOnlyCollection? exclusionPathspecs = null, + CancellationToken cancellationToken = default) + { + IReadOnlyCollection pathspecs = string.IsNullOrEmpty(relativePath) + ? [".", .. exclusionPathspecs ?? []] + : [relativePath, .. exclusionPathspecs ?? []]; + + var result = await repo.ExecuteGitCommand( + ["diff", "--name-only", $"{fromRef}...{toRef}", "--", .. pathspecs], + cancellationToken); + result.ThrowIfFailed($"Failed to get the diff between {fromRef} and {toRef}"); + + IEnumerable files = result.GetOutputLines(); + if (!string.IsNullOrEmpty(relativePath)) + { + files = files + .Where(f => f.StartsWith(relativePath, StringComparison.OrdinalIgnoreCase)) + .Select(f => f.Substring(relativePath.Length)); + } + + return FilterMappingFiles(files, mappingName); + } + + /// + /// Drops eng/common (for non-arcade mappings) and codeflow version/metadata files, which are + /// expected to legitimately diverge between the source repo and the VMR. + /// + private static HashSet FilterMappingFiles(IEnumerable files, string mappingName) + { + var engCommonPrefix = Constants.CommonScriptFilesPath + "/"; + var dropEngCommon = mappingName != VmrInfo.ArcadeMappingName; + + return files + .Where(f => !DependencyFileManager.CodeflowDependencyFiles.Contains(f, StringComparer.OrdinalIgnoreCase)) + .Where(f => !dropEngCommon || !f.StartsWith(engCommonPrefix, StringComparison.OrdinalIgnoreCase)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + } + + /// + /// Compares the zero-context change lines of a single file between the source diff and the PR. + /// Lines that belong to the diff format (and are expected to differ) are ignored before comparing. + /// + private async Task ChangedLinesMatchAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + string srcMappingPath, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken) + { + var sourceResult = await sourceRepo.ExecuteGitCommand(["diff", "-U0", $"{oldSha}...{newSha}", "--", file], cancellationToken); + sourceResult.ThrowIfFailed($"Failed to get the source diff of {file} between {oldSha} and {newSha}"); + + var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", $"{srcMappingPath}/{file}"], cancellationToken); + vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); + + var sourceChanges = GetChangeLines(sourceResult); + var vmrChanges = GetChangeLines(vmrResult); + + return sourceChanges.SequenceEqual(vmrChanges); + } + + /// + /// Keeps only the +/- change lines from a zero-context diff, dropping the diff-format lines that + /// are expected to differ between the source repo and its VMR copy. + /// + private static List GetChangeLines(ProcessExecutionResult diffResult) + { + return diffResult.GetOutputLines() + .Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith)) + .ToList(); + } + + /// + /// A file the source changed but the PR did not is only legitimate when the VMR copy is already + /// at the source's new state (equal content, or both absent for an already-reconciled deletion). + /// + private async Task IsLegitimateNoOpAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + string srcMappingPath, + string newSha, + string vmrHeadBranch) + { + var sourceContent = await sourceRepo.GetFileFromGitAsync(file, newSha); + var vmrContent = await vmr.GetFileFromGitAsync($"{srcMappingPath}/{file}", vmrHeadBranch); + + if (sourceContent == null && vmrContent == null) + { + return true; + } + + return string.Equals(sourceContent, vmrContent, StringComparison.Ordinal); + } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 811d5851e6..009be27d88 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Text; using Maestro.Common; using Maestro.Common.Telemetry; using Maestro.DataProviders; @@ -34,6 +35,7 @@ internal class CodeFlowPullRequestUpdater : PullRequestUpdater private readonly ICommentCollector _commentCollector; private readonly IPullRequestStateManager _stateManager; private readonly ISubscriptionEventRecorder _subscriptionEventRecorder; + private readonly ICodeflowChangeAnalyzer _codeflowChangeAnalyzer; private readonly IPullRequestTarget _target; private readonly ILogger _logger; @@ -52,6 +54,7 @@ public CodeFlowPullRequestUpdater( IPullRequestCommenter pullRequestCommenter, IPullRequestStateManager stateManager, ISubscriptionEventRecorder subscriptionEventRecorder, + ICodeflowChangeAnalyzer codeflowChangeAnalyzer, ILogger logger) : base(target, mergePolicyEvaluator, remoteFactory, sqlClient, pullRequestCommenter, stateManager, subscriptionEventRecorder, logger) { @@ -67,6 +70,7 @@ public CodeFlowPullRequestUpdater( _logger = logger; _stateManager = stateManager; _subscriptionEventRecorder = subscriptionEventRecorder; + _codeflowChangeAnalyzer = codeflowChangeAnalyzer; _target = target; } @@ -210,6 +214,11 @@ protected override async Task ProcessSubscriptionUpdat await ClosePullRequestAfterUnsafeFlowAsync(oldPrUrl, subscription, pr.Url); } + if (isForwardFlow) + { + await PostSourceDiffVerificationCommentAsync(remote, subscription, build, previousSourceSha, prHeadBranch, pr.Url); + } + return new SubscriptionUpdateResult( $"New codeflow PR created: {GitRepoUrlUtils.TurnApiUrlToWebsite(pr.Url)}.", Maestro.Data.Models.SubscriptionOutcomeType.Updated); @@ -230,6 +239,11 @@ await UpdateCodeFlowPullRequestAsync( codeFlowRes.DependencyUpdates, upstreamRepoDiffs); + if (isForwardFlow) + { + await PostSourceDiffVerificationCommentAsync(remote, subscription, build, previousSourceSha, prHeadBranch, pr.Url); + } + return new SubscriptionUpdateResult( $"Existing codeflow PR has been updated: {GitRepoUrlUtils.TurnApiUrlToWebsite(pr.Url)}.", Maestro.Data.Models.SubscriptionOutcomeType.Updated); @@ -785,6 +799,72 @@ private async Task UpdateCodeFlowPullRequestAsync( } } + private async Task PostSourceDiffVerificationCommentAsync( + IRemote remote, + SubscriptionDTO subscription, + BuildDTO build, + string? previousSourceSha, + string? prHeadBranch, + string prUrl) + { + // Best-effort confidence signal: never fail or slow down the codeflow push. + try + { + if (string.IsNullOrEmpty(previousSourceSha) || string.IsNullOrEmpty(prHeadBranch)) + { + // Without a previous source SHA there is no compare range, so we skip silently + // (matching how the PR description's diff link is skipped). + return; + } + + bool matches = await _codeflowChangeAnalyzer.VerifyForwardFlowAsync( + subscription.TargetDirectory, + subscription.SourceRepository, + previousSourceSha, + build.Commit, + subscription.TargetBranch, + prHeadBranch); + + var comment = BuildSourceDiffVerificationComment(matches, subscription.SourceRepository, previousSourceSha, build.Commit); + await remote.CommentPullRequestAsync(prUrl, comment); + } + catch (Exception e) + { + _logger.LogWarning(e, + "Failed to post source diff verification comment for PR {prUrl} of subscription {subscriptionId}", + prUrl, + subscription.Id); + } + } + + private static string BuildSourceDiffVerificationComment( + bool matches, + string sourceRepo, + string oldSha, + string newSha) + { + var shortOld = Commit.GetShortSha(oldSha); + var shortNew = Commit.GetShortSha(newSha); + + var builder = new StringBuilder(); + builder.AppendLine("### Source diff verification"); + builder.AppendLine(); + + if (matches) + { + builder.AppendLine($"✅ This PR matches the source diff `{shortOld}...{shortNew}` of `{sourceRepo}`."); + } + else + { + builder.AppendLine($"⚠️ Couldn't verify that this PR matches the source diff `{shortOld}...{shortNew}` of `{sourceRepo}`. Please review the changes manually."); + } + + builder.AppendLine(); + builder.AppendLine("> This is a best-effort confidence signal. It reuses the same source-mappings configuration the codeflow used, so it validates that the PR reflects the source diff under the current config — not that the config itself is correct."); + + return builder.ToString(); + } + private async Task HandleBlockingCodeflowException(InProgressPullRequest pr) { _logger.LogInformation("PR with url {prUrl} is blocked from receiving future codeflows.", pr.Url); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs index 8f1fa362f2..109563e7a5 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs @@ -7,6 +7,7 @@ using AwesomeAssertions; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.Models; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using Microsoft.Extensions.Logging.Abstractions; @@ -29,6 +30,9 @@ public class CodeflowChangeAnalyzerTests private Mock _localGitRepo = null!; private Mock _versionDetailsParser = null!; private Mock _barClient = null!; + private Mock _cloneManager = null!; + private Mock _dependencyTracker = null!; + private Mock _sourceManifest = null!; private IVmrInfo _vmrInfo = null!; private CodeflowChangeAnalyzer _analyzer = null!; @@ -39,6 +43,9 @@ public void SetUp() _localGitRepo = new Mock(); _versionDetailsParser = new Mock(); _barClient = new Mock(); + _cloneManager = new Mock(); + _dependencyTracker = new Mock(); + _sourceManifest = new Mock(); _vmrInfo = Mock.Of(x => x.VmrPath == TestVmrPath); _localGitRepoFactory @@ -53,6 +60,9 @@ public void SetUp() _localGitRepoFactory.Object, _versionDetailsParser.Object, _barClient.Object, + _cloneManager.Object, + _dependencyTracker.Object, + _sourceManifest.Object, _vmrInfo, NullLogger.Instance); } diff --git a/tools/ProductConstructionService.ReproTool/Operations/Operation.cs b/tools/ProductConstructionService.ReproTool/Operations/Operation.cs index 1254f416b6..84b1e0f10b 100644 --- a/tools/ProductConstructionService.ReproTool/Operations/Operation.cs +++ b/tools/ProductConstructionService.ReproTool/Operations/Operation.cs @@ -134,7 +134,7 @@ protected async Task> PrepareVmrForkAsync(string br { logger.LogInformation("Preparing VMR fork"); // Sync the VMR fork branch - await SyncForkAsync("dotnet", "dotnet", branch); + //await SyncForkAsync("dotnet", "dotnet", branch); return await CreateTmpBranchAsync(VmrForkRepoName, branch, skipCleanup); } @@ -214,7 +214,7 @@ protected async Task> PrepareProductRepoForkAsync( if (allRepos.Any(repo => repo.HtmlUrl.Equals(productRepoForkUri, StringComparison.OrdinalIgnoreCase))) { logger.LogInformation("Product repo fork {fork} already exists, syncing branch {branch} with source", productRepoForkUri, productRepoBranch); - await SyncForkAsync(org, name, productRepoBranch); + //await SyncForkAsync(org, name, productRepoBranch); } // If we don't, create a fork else From a85a6217ac8eb3cc2aa420d84250439764724efb Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Tue, 23 Jun 2026 09:30:49 +0200 Subject: [PATCH 03/16] small improvements --- .../VirtualMonoRepo/CodeflowChangeAnalyzer.cs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs index a25fdfe7eb..08aca55862 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs @@ -295,12 +295,12 @@ public async Task VerifyForwardFlowAsync( var srcMappingPrefix = srcMappingPath + "/"; HashSet sourceRepoChanges = await GetChangedMappingFilesAsync( sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); - HashSet targetRepoChanges = await GetChangedMappingFilesAsync( + HashSet vmrPrChanges = await GetChangedMappingFilesAsync( vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); - var intersection = sourceRepoChanges.Where(targetRepoChanges.Contains).ToList(); - var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !targetRepoChanges.Contains(f)).ToList(); - var unexpectedFiles = targetRepoChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); + var intersection = sourceRepoChanges.Where(vmrPrChanges.Contains).ToList(); + var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !vmrPrChanges.Contains(f)).ToList(); + var unexpectedFiles = vmrPrChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); // The PR changed files that the source diff didn't - the codeflow can't be trusted. if (unexpectedFiles.Count > 0) @@ -430,8 +430,8 @@ private async Task ChangedLinesMatchAsync( var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", $"{srcMappingPath}/{file}"], cancellationToken); vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); - var sourceChanges = GetChangeLines(sourceResult); - var vmrChanges = GetChangeLines(vmrResult); + var sourceChanges = GetChangeLines(sourceResult.GetOutputLines()); + var vmrChanges = GetChangeLines(vmrResult.GetOutputLines()); return sourceChanges.SequenceEqual(vmrChanges); } @@ -440,18 +440,14 @@ private async Task ChangedLinesMatchAsync( /// Keeps only the +/- change lines from a zero-context diff, dropping the diff-format lines that /// are expected to differ between the source repo and its VMR copy. /// - private static List GetChangeLines(ProcessExecutionResult diffResult) - { - return diffResult.GetOutputLines() - .Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith)) - .ToList(); - } + private static List GetChangeLines(IReadOnlyCollection lines) => + [.. lines.Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith))]; /// /// A file the source changed but the PR did not is only legitimate when the VMR copy is already /// at the source's new state (equal content, or both absent for an already-reconciled deletion). /// - private async Task IsLegitimateNoOpAsync( + private static async Task IsLegitimateNoOpAsync( ILocalGitRepo sourceRepo, ILocalGitRepo vmr, string file, From f9111da812540c2b9a1d388d47b365d18b867f38 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Tue, 23 Jun 2026 16:23:10 +0200 Subject: [PATCH 04/16] Run the codeflow code check as it's own job --- .../VirtualMonoRepo/CodeflowChangeAnalyzer.cs | 232 -------------- .../VirtualMonoRepo/CodeflowExtensions.cs | 1 + .../CodeflowSourceDiffVerifier.cs | 286 ++++++++++++++++++ .../VirtualMonoRepo/VmrPatchHandler.cs | 2 +- .../DependencyFlowConfiguration.cs | 1 + .../IPullRequestStateManager.cs | 6 + .../PullRequestStateManager.cs | 6 + .../CodeFlowPullRequestUpdater.cs | 121 ++++++-- .../CodeflowUpdateCheckProcessor.cs | 53 ++++ .../WorkItems/CodeflowUpdateCheck.cs | 11 + .../CodeflowChangeAnalyzerTests.cs | 9 - 11 files changed, 456 insertions(+), 272 deletions(-) create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs create mode 100644 src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs create mode 100644 src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs index 08aca55862..390de55ba4 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowChangeAnalyzer.cs @@ -4,11 +4,9 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.Models; -using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using Microsoft.Extensions.Logging; @@ -18,20 +16,6 @@ namespace Microsoft.DotNet.DarcLib.VirtualMonoRepo; public interface ICodeflowChangeAnalyzer { Task ForwardFlowHasMeaningfulChangesAsync(string mappingName, string headBranch, string targetBranch); - - /// - /// Verifies that a forward-flow codeflow PR (source repo -> VMR) faithfully contains the source - /// repo's commit diff (oldSha...newSha), accounting for the expected divergences (path remap, - /// excludes, eng/common, version files, no-ops). - /// - Task VerifyForwardFlowAsync( - string mappingName, - string sourceRepoUri, - string oldSha, - string newSha, - string vmrTargetBranch, - string vmrHeadBranch, - CancellationToken cancellationToken = default); } /// @@ -55,9 +39,6 @@ public class CodeflowChangeAnalyzer : ICodeflowChangeAnalyzer private readonly ILocalGitRepoFactory _localGitRepoFactory; private readonly IVersionDetailsParser _versionDetailsParser; private readonly IBasicBarClient _barClient; - private readonly IRepositoryCloneManager _cloneManager; - private readonly IVmrDependencyTracker _dependencyTracker; - private readonly ISourceManifest _sourceManifest; private readonly IVmrInfo _vmrInfo; private readonly ILogger _logger; @@ -65,18 +46,12 @@ public CodeflowChangeAnalyzer( ILocalGitRepoFactory localGitRepoFactory, IVersionDetailsParser versionDetailsParser, IBasicBarClient barClient, - IRepositoryCloneManager cloneManager, - IVmrDependencyTracker dependencyTracker, - ISourceManifest sourceManifest, IVmrInfo vmrInfo, ILogger logger) { _localGitRepoFactory = localGitRepoFactory; _versionDetailsParser = versionDetailsParser; _barClient = barClient; - _cloneManager = cloneManager; - _dependencyTracker = dependencyTracker; - _sourceManifest = sourceManifest; _vmrInfo = vmrInfo; _logger = logger; } @@ -258,211 +233,4 @@ private static IEnumerable GetExpectedContentsForBuild(Build? b) ..darcFeeds, ]; } - - public async Task VerifyForwardFlowAsync( - string mappingName, - string sourceRepoUri, - string oldSha, - string newSha, - string vmrTargetBranch, - string vmrHeadBranch, - CancellationToken cancellationToken = default) - { - _logger.LogInformation( - "Verifying forward flow PR for {mappingName} against source diff {oldSha}...{newSha}", - mappingName, - oldSha, - newSha); - - ILocalGitRepo vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath); - var srcMappingPath = VmrInfo.GetRelativeRepoSourcesPath(mappingName); - - await vmr.CheckoutAsync(vmrHeadBranch); - await _dependencyTracker.RefreshMetadataAsync(); - SourceMapping mapping = _dependencyTracker.GetMapping(mappingName); - - var exclusionPathspecs = GetDiffFilters(mapping, _sourceManifest); - - // Obtain a local clone of the source repo containing both SHAs. - ILocalGitRepo sourceRepo = await _cloneManager.PrepareCloneAsync( - mapping, - [sourceRepoUri], - [oldSha, newSha], - newSha, - resetToRemote: false, - cancellationToken); - - var srcMappingPrefix = srcMappingPath + "/"; - HashSet sourceRepoChanges = await GetChangedMappingFilesAsync( - sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); - HashSet vmrPrChanges = await GetChangedMappingFilesAsync( - vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); - - var intersection = sourceRepoChanges.Where(vmrPrChanges.Contains).ToList(); - var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !vmrPrChanges.Contains(f)).ToList(); - var unexpectedFiles = vmrPrChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); - - // The PR changed files that the source diff didn't - the codeflow can't be trusted. - if (unexpectedFiles.Count > 0) - { - _logger.LogInformation( - "Source diff verification for {mappingName} failed: {unexpected} file(s) changed in the PR but not in the source diff", - mappingName, - unexpectedFiles.Count); - return false; - } - - // Per-file content compare on the intersection. - foreach (var file in intersection) - { - if (!await ChangedLinesMatchAsync(sourceRepo, vmr, file, srcMappingPath, oldSha, newSha, vmrTargetBranch, vmrHeadBranch, cancellationToken)) - { - _logger.LogInformation( - "Source diff verification for {mappingName} failed: changes to {file} don't match the source diff", - mappingName, - file); - return false; - } - } - - // No-op check on files the source changed but the PR did not. - foreach (var file in sourceRepoOnlyChanges) - { - if (!await IsLegitimateNoOpAsync(sourceRepo, vmr, file, srcMappingPath, newSha, vmrHeadBranch)) - { - _logger.LogInformation( - "Source diff verification for {mappingName} failed: {file} changed in the source diff but is not reflected in the PR", - mappingName, - file); - return false; - } - } - - _logger.LogInformation("Source diff verification for {mappingName} passed", mappingName); - return true; - } - - /// - /// Builds the git pathspec exclusion rules the same way VmrDiffOperation.GetDiffFilters does: - /// the mapping's excludes plus submodule paths under the mapping, turned into git exclusion rules. - /// - private static IReadOnlyCollection GetDiffFilters(SourceMapping mapping, ISourceManifest manifest) - { - var submodules = manifest.Submodules - .Where(s => s.Path.StartsWith(mapping.Name + '/', StringComparison.OrdinalIgnoreCase)) - .Select(s => s.Path.Substring(mapping.Name.Length + 1)); - - return (mapping.Exclude ?? []) - .Concat(submodules) - .Select(VmrPatchHandler.GetExclusionRule) - .ToList(); - } - - /// - /// Runs a three-dot name-only diff (...) scoped to the - /// mapping's location in the repo and returns the mapping-relative paths, after dropping eng/common - /// (non-arcade) and version/metadata files. Used for both the source repo diff (mapping lives at the repo - /// root, so is empty) and the target repo (VMR) diff (mapping lives - /// under src/<mapping>/, which is stripped from the resulting paths). - /// - private async Task> GetChangedMappingFilesAsync( - ILocalGitRepo repo, - string mappingName, - string fromRef, - string toRef, - string? relativePath = null, - IReadOnlyCollection? exclusionPathspecs = null, - CancellationToken cancellationToken = default) - { - IReadOnlyCollection pathspecs = string.IsNullOrEmpty(relativePath) - ? [".", .. exclusionPathspecs ?? []] - : [relativePath, .. exclusionPathspecs ?? []]; - - var result = await repo.ExecuteGitCommand( - ["diff", "--name-only", $"{fromRef}...{toRef}", "--", .. pathspecs], - cancellationToken); - result.ThrowIfFailed($"Failed to get the diff between {fromRef} and {toRef}"); - - IEnumerable files = result.GetOutputLines(); - if (!string.IsNullOrEmpty(relativePath)) - { - files = files - .Where(f => f.StartsWith(relativePath, StringComparison.OrdinalIgnoreCase)) - .Select(f => f.Substring(relativePath.Length)); - } - - return FilterMappingFiles(files, mappingName); - } - - /// - /// Drops eng/common (for non-arcade mappings) and codeflow version/metadata files, which are - /// expected to legitimately diverge between the source repo and the VMR. - /// - private static HashSet FilterMappingFiles(IEnumerable files, string mappingName) - { - var engCommonPrefix = Constants.CommonScriptFilesPath + "/"; - var dropEngCommon = mappingName != VmrInfo.ArcadeMappingName; - - return files - .Where(f => !DependencyFileManager.CodeflowDependencyFiles.Contains(f, StringComparer.OrdinalIgnoreCase)) - .Where(f => !dropEngCommon || !f.StartsWith(engCommonPrefix, StringComparison.OrdinalIgnoreCase)) - .ToHashSet(StringComparer.OrdinalIgnoreCase); - } - - /// - /// Compares the zero-context change lines of a single file between the source diff and the PR. - /// Lines that belong to the diff format (and are expected to differ) are ignored before comparing. - /// - private async Task ChangedLinesMatchAsync( - ILocalGitRepo sourceRepo, - ILocalGitRepo vmr, - string file, - string srcMappingPath, - string oldSha, - string newSha, - string vmrTargetBranch, - string vmrHeadBranch, - CancellationToken cancellationToken) - { - var sourceResult = await sourceRepo.ExecuteGitCommand(["diff", "-U0", $"{oldSha}...{newSha}", "--", file], cancellationToken); - sourceResult.ThrowIfFailed($"Failed to get the source diff of {file} between {oldSha} and {newSha}"); - - var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", $"{srcMappingPath}/{file}"], cancellationToken); - vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); - - var sourceChanges = GetChangeLines(sourceResult.GetOutputLines()); - var vmrChanges = GetChangeLines(vmrResult.GetOutputLines()); - - return sourceChanges.SequenceEqual(vmrChanges); - } - - /// - /// Keeps only the +/- change lines from a zero-context diff, dropping the diff-format lines that - /// are expected to differ between the source repo and its VMR copy. - /// - private static List GetChangeLines(IReadOnlyCollection lines) => - [.. lines.Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith))]; - - /// - /// A file the source changed but the PR did not is only legitimate when the VMR copy is already - /// at the source's new state (equal content, or both absent for an already-reconciled deletion). - /// - private static async Task IsLegitimateNoOpAsync( - ILocalGitRepo sourceRepo, - ILocalGitRepo vmr, - string file, - string srcMappingPath, - string newSha, - string vmrHeadBranch) - { - var sourceContent = await sourceRepo.GetFileFromGitAsync(file, newSha); - var vmrContent = await vmr.GetFileFromGitAsync($"{srcMappingPath}/{file}", vmrHeadBranch); - - if (sourceContent == null && vmrContent == null) - { - return true; - } - - return string.Equals(sourceContent, vmrContent, StringComparison.Ordinal); - } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs index a897d6531b..71b757b9b8 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs @@ -69,6 +69,7 @@ public static IServiceCollection AddCodeflow( services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); + services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs new file mode 100644 index 0000000000..968cc6c1a4 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -0,0 +1,286 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; +using Microsoft.Extensions.Logging; + +#nullable enable +namespace Microsoft.DotNet.DarcLib.VirtualMonoRepo; + +public interface ICodeflowSourceDiffVerifier +{ + /// + /// Verifies that a forward-flow codeflow PR (source repo -> VMR) faithfully contains the source + /// repo's commit diff (oldSha...newSha), accounting for the expected divergences (path remap, + /// excludes, eng/common, version files, no-ops). + /// + Task VerifyForwardFlowAsync( + string sourceRepoUri, + string vmrUri, + string mappingName, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default); +} + +/// +/// Verifies that a forward-flow codeflow PR faithfully reflects the source repo's commit diff, +/// accounting for the expected, legitimate divergences between a source repo and its VMR copy +/// (path remap, cloaked/excluded paths, eng/common, version/metadata files and no-ops). +/// +public class CodeflowSourceDiffVerifier : ICodeflowSourceDiffVerifier +{ + // List of characters that belong to the git diff format. + // Example diff output: + // diff --git a/src/product-repo1/eng/Versions.props b/src/product-repo1/eng/Versions.props + // index fb13f6d..76d73de 100644 + // --- a/src/product-repo1/eng/Versions.props + // +++ b/src/product-repo1/eng/Versions.props + // @@ -10,2 +10,2 @@ + // - 1.0.0 + // - 2.0.0 + // + 2.0.1 + // + 2.0.1 + private static readonly IReadOnlyCollection IgnoredDiffLines = ["diff --git", "index ", "@@ ", "--- ", "+++ "]; + + private readonly IVmrCloneManager _vmrCloneManager; + private readonly IRepositoryCloneManager _cloneManager; + private readonly IVmrDependencyTracker _dependencyTracker; + private readonly ISourceManifest _sourceManifest; + private readonly ILogger _logger; + + public CodeflowSourceDiffVerifier( + IVmrCloneManager vmrCloneManager, + IRepositoryCloneManager cloneManager, + IVmrDependencyTracker dependencyTracker, + ISourceManifest sourceManifest, + ILogger logger) + { + _vmrCloneManager = vmrCloneManager; + _cloneManager = cloneManager; + _dependencyTracker = dependencyTracker; + _sourceManifest = sourceManifest; + _logger = logger; + } + + public async Task VerifyForwardFlowAsync( + string sourceRepoUri, + string vmrUri, + string mappingName, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default) + { + _logger.LogInformation( + "Verifying forward flow PR for {mappingName} against source diff {oldSha}...{newSha}", + mappingName, + oldSha, + newSha); + + var srcMappingPath = VmrInfo.GetRelativeRepoSourcesPath(mappingName); + + // Make sure both the target and the PR head branches are available locally in the VMR. + ILocalGitRepo vmr = await _vmrCloneManager.PrepareVmrAsync( + [vmrUri], + [vmrTargetBranch, vmrHeadBranch], + vmrHeadBranch, + resetToRemote: true, + cancellationToken); + + SourceMapping mapping = _dependencyTracker.GetMapping(mappingName); + + var exclusionPathspecs = GetDiffFilters(mapping, _sourceManifest); + + // Obtain a local clone of the source repo containing both SHAs. + ILocalGitRepo sourceRepo = await _cloneManager.PrepareCloneAsync( + mapping, + [sourceRepoUri], + [oldSha, newSha], + newSha, + resetToRemote: false, + cancellationToken); + + var srcMappingPrefix = srcMappingPath + "/"; + HashSet sourceRepoChanges = await GetChangedMappingFilesAsync( + sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); + HashSet vmrPrChanges = await GetChangedMappingFilesAsync( + vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); + + var intersection = sourceRepoChanges.Where(vmrPrChanges.Contains).ToList(); + var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !vmrPrChanges.Contains(f)).ToList(); + var unexpectedFiles = vmrPrChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); + + // The PR changed files that the source diff didn't - the codeflow can't be trusted. + if (unexpectedFiles.Count > 0) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {unexpected} file(s) changed in the PR but not in the source diff", + mappingName, + unexpectedFiles.Count); + return false; + } + + // Per-file content compare on the intersection. + foreach (var file in intersection) + { + if (!await ChangedLinesMatchAsync(sourceRepo, vmr, file, srcMappingPath, oldSha, newSha, vmrTargetBranch, vmrHeadBranch, cancellationToken)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: changes to {file} don't match the source diff", + mappingName, + file); + return false; + } + } + + // No-op check on files the source changed but the PR did not. + foreach (var file in sourceRepoOnlyChanges) + { + if (!await IsLegitimateNoOpAsync(sourceRepo, vmr, file, srcMappingPath, newSha, vmrHeadBranch)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {file} changed in the source diff but is not reflected in the PR", + mappingName, + file); + return false; + } + } + + _logger.LogInformation("Source diff verification for {mappingName} passed", mappingName); + return true; + } + + /// + /// Builds the git pathspec exclusion rules the same way VmrDiffOperation.GetDiffFilters does: + /// the mapping's excludes plus submodule paths under the mapping, turned into git exclusion rules. + /// + private static List GetDiffFilters(SourceMapping mapping, ISourceManifest manifest) + { + var submodules = manifest.Submodules + .Where(s => s.Path.StartsWith(mapping.Name + '/', StringComparison.OrdinalIgnoreCase)) + .Select(s => s.Path.Substring(mapping.Name.Length + 1)); + + return (mapping.Exclude ?? []) + .Concat(submodules) + .Select(VmrPatchHandler.GetExclusionRule) + .ToList(); + } + + /// + /// Runs a three-dot name-only diff (...) scoped to the + /// mapping's location in the repo and returns the mapping-relative paths, after dropping eng/common + /// (non-arcade) and version/metadata files. Used for both the source repo diff (mapping lives at the repo + /// root, so is empty) and the target repo (VMR) diff (mapping lives + /// under src/<mapping>/, which is stripped from the resulting paths). + /// + private static async Task> GetChangedMappingFilesAsync( + ILocalGitRepo repo, + string mappingName, + string fromRef, + string toRef, + string? relativePath = null, + IReadOnlyCollection? exclusionPathspecs = null, + CancellationToken cancellationToken = default) + { + IReadOnlyCollection pathspecs = string.IsNullOrEmpty(relativePath) + ? [".", .. exclusionPathspecs ?? []] + : [relativePath, .. exclusionPathspecs ?? []]; + + var result = await repo.ExecuteGitCommand( + ["diff", "--name-only", $"{fromRef}...{toRef}", "--", .. pathspecs], + cancellationToken); + result.ThrowIfFailed($"Failed to get the diff between {fromRef} and {toRef}"); + + IEnumerable files = result.GetOutputLines(); + if (!string.IsNullOrEmpty(relativePath)) + { + files = files + .Where(f => f.StartsWith(relativePath, StringComparison.OrdinalIgnoreCase)) + .Select(f => f.Substring(relativePath.Length)); + } + + return FilterMappingFiles(files, mappingName); + } + + /// + /// Drops eng/common (for non-arcade mappings) and codeflow version/metadata files, which are + /// expected to legitimately diverge between the source repo and the VMR. + /// + private static HashSet FilterMappingFiles(IEnumerable files, string mappingName) + { + var engCommonPrefix = Constants.CommonScriptFilesPath + "/"; + var dropEngCommon = mappingName != VmrInfo.ArcadeMappingName; + + return files + .Where(f => !DependencyFileManager.CodeflowDependencyFiles.Contains(f, StringComparer.OrdinalIgnoreCase)) + .Where(f => !dropEngCommon || !f.StartsWith(engCommonPrefix, StringComparison.OrdinalIgnoreCase)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + } + + /// + /// Compares the zero-context change lines of a single file between the source diff and the PR. + /// Lines that belong to the diff format (and are expected to differ) are ignored before comparing. + /// + private static async Task ChangedLinesMatchAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + string srcMappingPath, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken) + { + var sourceResult = await sourceRepo.ExecuteGitCommand(["diff", "-U0", $"{oldSha}...{newSha}", "--", file], cancellationToken); + sourceResult.ThrowIfFailed($"Failed to get the source diff of {file} between {oldSha} and {newSha}"); + + var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", $"{srcMappingPath}/{file}"], cancellationToken); + vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); + + var sourceChanges = GetChangeLines(sourceResult.GetOutputLines()); + var vmrChanges = GetChangeLines(vmrResult.GetOutputLines()); + + return sourceChanges.SequenceEqual(vmrChanges); + } + + /// + /// Keeps only the +/- change lines from a zero-context diff, dropping the diff-format lines that + /// are expected to differ between the source repo and its VMR copy. + /// + private static List GetChangeLines(IReadOnlyCollection lines) => + [.. lines.Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith))]; + + /// + /// A file the source changed but the PR did not is only legitimate when the VMR copy is already + /// at the source's new state (equal content, or both absent for an already-reconciled deletion). + /// + private static async Task IsLegitimateNoOpAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + string srcMappingPath, + string newSha, + string vmrHeadBranch) + { + var sourceContent = await sourceRepo.GetFileFromGitAsync(file, newSha); + var vmrContent = await vmr.GetFileFromGitAsync($"{srcMappingPath}/{file}", vmrHeadBranch); + + if (sourceContent == null && vmrContent == null) + { + return true; + } + + return string.Equals(sourceContent, vmrContent, StringComparison.Ordinal); + } +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs index 812fec9a73..72bbc30f73 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs @@ -348,7 +348,7 @@ public async Task> ApplyPatch( } } - _logger.LogDebug("{output}", result.ToString()); + //_logger.LogDebug("{output}", result.ToString()); if (removePatchAfter) { diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs index ea8895063b..65d39733c8 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs @@ -37,5 +37,6 @@ public static void AddDependencyFlowProcessors(this IServiceCollection services) services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); + services.AddWorkItemProcessor(); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs index 8346a16d23..c47e638a6f 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs @@ -58,6 +58,12 @@ internal interface IPullRequestStateManager #endregion + #region Codeflow update check + + Task SetCodeflowUpdateCheck(CodeflowUpdateCheck check); + + #endregion + #region Bulk cleanup /// diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs index 647d0e205c..9374504b09 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs @@ -21,6 +21,7 @@ internal class PullRequestStateManager : IPullRequestStateManager private readonly IRedisCache _mergePolicyEvaluationState; private readonly IReminderManager _pullRequestUpdateReminders; private readonly IReminderManager _pullRequestCheckReminders; + private readonly IReminderManager _codeflowUpdateCheckReminders; public PullRequestStateManager( IPullRequestTarget pullRequestTarget, @@ -33,6 +34,7 @@ public PullRequestStateManager( _mergePolicyEvaluationState = cacheFactory.Create(cacheKey); _pullRequestUpdateReminders = reminderManagerFactory.CreateReminderManager(cacheKey); _pullRequestCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); + _codeflowUpdateCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); } public Task GetInProgressPullRequestAsync() => @@ -93,6 +95,7 @@ public async Task ClearAllStateAsync(bool isCodeFlow, bool clearPendingUpdates) { await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); + await _codeflowUpdateCheckReminders.UnsetReminderAsync(isCodeFlow); if (clearPendingUpdates) { await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); @@ -106,4 +109,7 @@ public async Task ScheduleUpdateForLater(InProgressPullRequest pr, SubscriptionU pr.NextBuildsToProcess[update.SubscriptionId] = update.BuildId; await SetInProgressPullRequestAsync(pr); } + + public async Task SetCodeflowUpdateCheck(CodeflowUpdateCheck check) => + await _codeflowUpdateCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, true); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 9ef48abb4a..2b70c6cdb3 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -2,8 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Text; -using Maestro.Common; using Maestro.Common.Telemetry; +using Maestro.Data.Models; using Maestro.DataProviders; using Maestro.MergePolicies; using Maestro.WorkItems; @@ -35,7 +35,7 @@ internal class CodeFlowPullRequestUpdater : PullRequestUpdater private readonly ICommentCollector _commentCollector; private readonly IPullRequestStateManager _stateManager; private readonly ISubscriptionEventRecorder _subscriptionEventRecorder; - private readonly ICodeflowChangeAnalyzer _codeflowChangeAnalyzer; + private readonly ICodeflowSourceDiffVerifier _codeflowSourceDiffVerifier; private readonly ISubscriptionUpdateOutcomeRecorder _outcomeRecorder; private readonly IPullRequestTarget _target; private readonly ILogger _logger; @@ -55,7 +55,7 @@ public CodeFlowPullRequestUpdater( IPullRequestCommenter pullRequestCommenter, IPullRequestStateManager stateManager, ISubscriptionEventRecorder subscriptionEventRecorder, - ICodeflowChangeAnalyzer codeflowChangeAnalyzer, + ICodeflowSourceDiffVerifier codeflowSourceDiffVerifier, ISubscriptionUpdateOutcomeRecorder outcomeRecorder, ILogger logger) : base(target, mergePolicyEvaluator, remoteFactory, sqlClient, pullRequestCommenter, stateManager, subscriptionEventRecorder, outcomeRecorder, logger) @@ -72,7 +72,7 @@ public CodeFlowPullRequestUpdater( _logger = logger; _stateManager = stateManager; _subscriptionEventRecorder = subscriptionEventRecorder; - _codeflowChangeAnalyzer = codeflowChangeAnalyzer; + _codeflowSourceDiffVerifier = codeflowSourceDiffVerifier; _outcomeRecorder = outcomeRecorder; _target = target; } @@ -95,7 +95,7 @@ protected override async Task ProcessSubscriptionUpdat await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); return new SubscriptionUpdateResult( $"The existing PR is already up to date with source repo (commit {update.SourceSha})", - Maestro.Data.Models.SubscriptionOutcomeType.NoUpdate); + SubscriptionOutcomeType.NoUpdate); } if (pr?.BlockedFromFutureUpdates == true && !forceUpdate) @@ -108,7 +108,7 @@ protected override async Task ProcessSubscriptionUpdat await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); return new SubscriptionUpdateResult( "The existing codeflow PR is currently blocked from future updates", - Maestro.Data.Models.SubscriptionOutcomeType.NotUpdatable); + SubscriptionOutcomeType.NotUpdatable); } var subscription = await _sqlClient.GetSubscriptionAsync(update.SubscriptionId); @@ -143,7 +143,7 @@ protected override async Task ProcessSubscriptionUpdat await HandleBlockingCodeflowException(pr); return new SubscriptionUpdateResult( e.Message, - Maestro.Data.Models.SubscriptionOutcomeType.NotUpdatable); + SubscriptionOutcomeType.NotUpdatable); } if (!codeFlowRes.HadUpdates) @@ -151,7 +151,7 @@ protected override async Task ProcessSubscriptionUpdat var msg = pr != null ? "No source code updates detected" : "Codeflow PR not created: no source code updates detected"; - return new SubscriptionUpdateResult(msg, Maestro.Data.Models.SubscriptionOutcomeType.NoUpdate); + return new SubscriptionUpdateResult(msg, SubscriptionOutcomeType.NoUpdate); } if (isForwardFlow) @@ -189,7 +189,7 @@ protected override async Task ProcessSubscriptionUpdat isUnsafeFlow); return new SubscriptionUpdateResult( "Conflict resolution is required by user", - Maestro.Data.Models.SubscriptionOutcomeType.HasConflict); + SubscriptionOutcomeType.HasConflict); } string? oldPrUrl = null; @@ -218,14 +218,9 @@ protected override async Task ProcessSubscriptionUpdat await ClosePullRequestAfterUnsafeFlowAsync(oldPrUrl, subscription, pr.Url); } - if (isForwardFlow) - { - await PostSourceDiffVerificationCommentAsync(remote, subscription, build, previousSourceSha, prHeadBranch, pr.Url); - } - return new SubscriptionUpdateResult( "New codeflow PR created", - Maestro.Data.Models.SubscriptionOutcomeType.Updated); + SubscriptionOutcomeType.Updated); } else { @@ -243,14 +238,9 @@ await UpdateCodeFlowPullRequestAsync( codeFlowRes.DependencyUpdates, upstreamRepoDiffs); - if (isForwardFlow) - { - await PostSourceDiffVerificationCommentAsync(remote, subscription, build, previousSourceSha, prHeadBranch, pr.Url); - } - return new SubscriptionUpdateResult( string.Empty, - Maestro.Data.Models.SubscriptionOutcomeType.Updated); + SubscriptionOutcomeType.Updated); } } @@ -279,7 +269,7 @@ await UpdateCodeFlowPullRequestAsync( try { - codeFlowRes = await InvokeFlowAsync(subscription, build, pr, prHeadBranch, forceUpdate, unsafeFlow: false); + codeFlowRes = await InvokeFlowAsync(subscription, build, prHeadBranch, forceUpdate, unsafeFlow: false); } catch (NonLinearCodeflowException e) { @@ -304,7 +294,7 @@ await UpdateCodeFlowPullRequestAsync( subscription.TargetBranch, prHeadBranch); - codeFlowRes = await InvokeFlowAsync(subscription, build, pr, prHeadBranch, forceUpdate, unsafeFlow: true); + codeFlowRes = await InvokeFlowAsync(subscription, build, prHeadBranch, forceUpdate, unsafeFlow: true); } if (codeFlowRes.HadConflicts) @@ -343,7 +333,6 @@ await UpdateCodeFlowPullRequestAsync( private async Task InvokeFlowAsync( SubscriptionDTO subscription, BuildDTO build, - InProgressPullRequest? pr, string branch, bool forceUpdate, bool unsafeFlow) @@ -541,7 +530,8 @@ private async Task RequestManualConflictResolutionAsync( prHeadBranch, codeFlowResult.DependencyUpdates, upstreamRepoDiffs, - unsafeFlown); + unsafeFlown, + skipCodeflowUpdateCheck: true); } else { @@ -571,7 +561,8 @@ await UpdateCodeFlowPullRequestAsync( previousSourceSha, subscription, codeFlowResult.DependencyUpdates, - upstreamRepoDiffs); + upstreamRepoDiffs, + skipCodeflowUpdateCheck: true); // Since we changed the PR state in cache but no commit was pushed, // we need to delete non-transient check results so that they can be re-evaluated @@ -643,7 +634,8 @@ private async Task CreateEmptyPrBranch( string prBranch, List dependencyUpdates, IReadOnlyCollection? upstreamRepoDiffs, - bool unsafeFlow) + bool unsafeFlow, + bool skipCodeflowUpdateCheck = false) { IRemote darcRemote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -706,6 +698,16 @@ await _subscriptionEventRecorder.AddDependencyFlowEventsAsync( inProgressPr.LastUpdate = DateTime.UtcNow; await _stateManager.SetCheckReminderAsync(inProgressPr, pr, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); + if (!skipCodeflowUpdateCheck && !string.IsNullOrEmpty(previousSourceSha)) + { + await _stateManager.SetCodeflowUpdateCheck(new CodeflowUpdateCheck + { + UpdaterId = _target.UpdaterId, + SubscriptionId = update.SubscriptionId, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = update.SourceSha + }); + } _outcomeRecorder.SetPullRequestUrl(inProgressPr.Url); _logger.LogInformation("Code flow pull request created: {prUrl}", pr.Url); @@ -738,7 +740,8 @@ private async Task UpdateCodeFlowPullRequestAsync( string? previousSourceSha, SubscriptionDTO subscription, List newDependencyUpdates, - IReadOnlyCollection? upstreamRepoDiffs) + IReadOnlyCollection? upstreamRepoDiffs, + bool skipCodeflowUpdateCheck = false) { IRemote remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -802,9 +805,66 @@ private async Task UpdateCodeFlowPullRequestAsync( pullRequest.BlockedFromFutureUpdates = false; // if a sub is blocked, and someone force triggers it, we can continue flowing afterwards await _stateManager.SetCheckReminderAsync(pullRequest, prInfo!, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); + if (!skipCodeflowUpdateCheck && !string.IsNullOrEmpty(previousSourceSha)) + { + await _stateManager.SetCodeflowUpdateCheck(new CodeflowUpdateCheck + { + UpdaterId = _target.UpdaterId, + SubscriptionId = update.SubscriptionId, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = update.SourceSha + }); + } } } + public async Task RunCodeflowUpdateCodeCheck( + SubscriptionDTO subscription, + CodeflowUpdateCheck codeflowUpdateCheck, + CancellationToken cancellationToken) + { + if (subscription.IsBackflow()) + { + _logger.LogError("Can't run codeflow code check on backflow subscriptions"); + return; + } + + var pr = await _stateManager.GetInProgressPullRequestAsync(); + + if (pr == null) + { + _logger.LogError("No in-progress PR found for codeflow update code check"); + return; + } + + var remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); + var prInfo = await remote.GetPullRequestAsync(pr.Url); + + if (prInfo.Status != PrStatus.Open) + { + _logger.LogInformation( + "Skipping codeflow update code check for PR {prUrl} because it is no longer open (status: {status})", + pr.Url, + prInfo.Status); + return; + } + + var matches = await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( + subscription.SourceRepository, + subscription.TargetRepository, + subscription.TargetDirectory, + codeflowUpdateCheck.PreviousSourceSha, + codeflowUpdateCheck.CurrentSourceSha, + subscription.TargetBranch, + pr.HeadBranch, + cancellationToken); + + if (matches) + { + await remote.CommentPullRequestAsync(pr.Url, "Looks good"); + } + } + private async Task PostSourceDiffVerificationCommentAsync( IRemote remote, SubscriptionDTO subscription, @@ -823,9 +883,10 @@ private async Task PostSourceDiffVerificationCommentAsync( return; } - bool matches = await _codeflowChangeAnalyzer.VerifyForwardFlowAsync( - subscription.TargetDirectory, + bool matches = await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( subscription.SourceRepository, + subscription.TargetRepository, + subscription.TargetDirectory, previousSourceSha, build.Commit, subscription.TargetBranch, diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs new file mode 100644 index 0000000000..cc44c6a5c4 --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Maestro.Data; +using Maestro.DataProviders; +using Maestro.WorkItems; +using Microsoft.EntityFrameworkCore; +using ProductConstructionService.DependencyFlow.Model; +using ProductConstructionService.DependencyFlow.PullRequestUpdaters; +using ProductConstructionService.DependencyFlow.WorkItems; + +namespace ProductConstructionService.DependencyFlow.WorkItemProcessors; + +internal class CodeflowUpdateCheckProcessor : WorkItemProcessor +{ + private readonly BuildAssetRegistryContext _context; + private readonly IPullRequestUpdaterFactory _updaterFactory; + + public CodeflowUpdateCheckProcessor( + BuildAssetRegistryContext context, + IPullRequestUpdaterFactory updaterFactory) + { + _context = context; + _updaterFactory = updaterFactory; + } + + public override async Task ProcessWorkItemAsync(CodeflowUpdateCheck workItem, CancellationToken cancellationToken) + { + var subscription = _context.Subscriptions + .Include(s => s.Channel) + .FirstOrDefault(s => s.Id == workItem.SubscriptionId); + + if (subscription == null) + { + throw new InvalidOperationException($"Subscription with ID {workItem.SubscriptionId} not found."); + } + + var pullRequestUpdaterId = PullRequestUpdaterId.CreateUpdaterId(subscription); + var pullRequestUpdater = _updaterFactory.CreatePullRequestUpdater(pullRequestUpdaterId); + + if (pullRequestUpdater is not CodeFlowPullRequestUpdater codeFlowUpdater) + { + throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow update check"); + } + + await codeFlowUpdater.RunCodeflowUpdateCodeCheck( + SqlBarClient.ToClientModelSubscription(subscription), + workItem, + cancellationToken); + + return true; + } +} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs new file mode 100644 index 0000000000..7eea3d3c08 --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs @@ -0,0 +1,11 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace ProductConstructionService.DependencyFlow.WorkItems; + +internal class CodeflowUpdateCheck : DependencyFlowWorkItem +{ + public required Guid SubscriptionId { get; init; } + public required string PreviousSourceSha { get; init; } + public required string CurrentSourceSha { get; init; } +} diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs index 109563e7a5..9827aabe06 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs @@ -30,9 +30,6 @@ public class CodeflowChangeAnalyzerTests private Mock _localGitRepo = null!; private Mock _versionDetailsParser = null!; private Mock _barClient = null!; - private Mock _cloneManager = null!; - private Mock _dependencyTracker = null!; - private Mock _sourceManifest = null!; private IVmrInfo _vmrInfo = null!; private CodeflowChangeAnalyzer _analyzer = null!; @@ -43,9 +40,6 @@ public void SetUp() _localGitRepo = new Mock(); _versionDetailsParser = new Mock(); _barClient = new Mock(); - _cloneManager = new Mock(); - _dependencyTracker = new Mock(); - _sourceManifest = new Mock(); _vmrInfo = Mock.Of(x => x.VmrPath == TestVmrPath); _localGitRepoFactory @@ -60,9 +54,6 @@ public void SetUp() _localGitRepoFactory.Object, _versionDetailsParser.Object, _barClient.Object, - _cloneManager.Object, - _dependencyTracker.Object, - _sourceManifest.Object, _vmrInfo, NullLogger.Instance); } From aa86859934f3a3bd22be8b6a2e82fa392089792c Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Tue, 23 Jun 2026 16:54:56 +0200 Subject: [PATCH 05/16] Add tests --- .../CodeFlowTestsBase.cs | 24 +++++++++++++++++++ .../CodeFlowUpdatingPRsTests.cs | 12 ++++++++++ .../ForwardFlowTests.cs | 12 ++++++++++ 3 files changed, 48 insertions(+) diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs index ce5c228c2c..e370d07cc1 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs @@ -287,6 +287,30 @@ protected async Task CallForwardflow( protected IReadOnlyList GetLastFlowCollectedComments() => _lastFlowCollectedComments; + /// + /// Runs the source-diff verification (the same check PCS runs after a forward-flow push) + /// against the current state of the VMR PR branch and returns whether the PR faithfully + /// reflects the source repo's diff (...). + /// The PR branch must be committed before calling this. + /// + protected async Task VerifyForwardFlowSourceDiff(string branchName, string oldSha, string newSha) + { + using var scope = ServiceProvider.CreateScope(); + var cloneManager = scope.ServiceProvider.GetRequiredService(); + await cloneManager.RegisterCloneAsync(VmrPath); + + var verifier = scope.ServiceProvider.GetRequiredService(); + return await verifier.VerifyForwardFlowAsync( + sourceRepoUri: ProductRepoPath, + vmrUri: VmrPath, + mappingName: Constants.ProductRepoName, + oldSha: oldSha, + newSha: newSha, + vmrTargetBranch: "main", + vmrHeadBranch: branchName, + cancellationToken: _cancellationToken.Token); + } + protected async Task> CallDarcCloakedFileScan(string baselinesFilePath) { using var scope = ServiceProvider.CreateScope(); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs index 556fe91e7f..e6326873d8 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs @@ -26,10 +26,14 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() const string backflowBranch = nameof(ForwardFlowWithChangesInThePrBranchTest) + "-bf"; const string forwardFlowBranch = nameof(ForwardFlowWithChangesInThePrBranchTest) + "-ff"; + // The source commit currently reflected in the VMR's main branch + var previousSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + // Make changes in the product repo await GitOperations.Checkout(ProductRepoPath, "main"); await File.WriteAllTextAsync(ProductRepoPath / "repo.txt", "New file in the repo"); await GitOperations.CommitAll(ProductRepoPath, "New file in the repo"); + var firstFlowSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); // Make changes in the VMR await GitOperations.Checkout(VmrPath, "main"); @@ -49,6 +53,9 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() await GitOperations.CommitAll(VmrPath, "Forward flow commit"); + // The freshly opened FF PR should faithfully reflect the source diff + (await VerifyForwardFlowSourceDiff(forwardFlowBranch, previousSourceSha, firstFlowSourceSha)).Should().BeTrue(); + // 3. Make some changes in the forward flow PR branch await GitOperations.Checkout(VmrPath, forwardFlowBranch); await File.WriteAllTextAsync(_productRepoVmrPath / "repo.txt", "Updated file in the PR"); @@ -58,11 +65,16 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() await GitOperations.MergePrBranch(ProductRepoPath, backflowBranch); // 5. Flow the changes from the repo into the VMR PR + var secondFlowSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); result = await CallForwardflow(Constants.ProductRepoName, ProductRepoPath, forwardFlowBranch); result.ShouldHaveUpdates(); await GitOperations.CommitAll(VmrPath, "Forward flow update", allowEmpty: true); + // The PR now carries an extra manual change (repo.txt) that the source diff doesn't contain, + // so the verification should fail + (await VerifyForwardFlowSourceDiff(forwardFlowBranch, firstFlowSourceSha, secondFlowSourceSha)).Should().BeFalse(); + // Check that the changes in the PR branch are preserved var prFileContent = await File.ReadAllTextAsync(_productRepoVmrPath / "repo.txt"); prFileContent.Should().Be("Updated file in the PR", "The changes in the PR branch should be preserved after the backflow merge."); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs index f870ac6166..dd2ac1eb52 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs @@ -25,8 +25,12 @@ public async Task OnlyForwardflowsTest() const string branchName = nameof(OnlyForwardflowsTest); + var oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); var codeFlowResult = await ChangeRepoFileAndFlowIt("New content in the individual repo", branchName); codeFlowResult.ShouldHaveUpdates(); + var newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); await FinalizeForwardFlow(branchName); // Flow again - should be a no-op @@ -37,9 +41,13 @@ public async Task OnlyForwardflowsTest() CheckFileContents(_productRepoVmrFilePath, "New content in the individual repo"); // Make a change in the repo again + oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); codeFlowResult = await ChangeRepoFileAndFlowIt("New content in the individual repo again", branchName); codeFlowResult.ShouldHaveUpdates(); CheckFileContents(_productRepoVmrFilePath, "New content in the individual repo again"); + newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); // Make an additional change in the PR branch before merging await File.WriteAllTextAsync(_productRepoVmrFilePath, "Change that happened in the PR"); @@ -64,8 +72,12 @@ await GitOperations.VerifyMergeConflict(VmrPath, branchName, // The last forward flow will have to recreate all of the flows to be able to apply the changes // Make another flow to VMR to have flows both ways ready + oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); codeFlowResult = await ChangeRepoFileAndFlowIt("Again some content in the individual repo", branchName); codeFlowResult.ShouldHaveUpdates(); + newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); await FinalizeForwardFlow(branchName); // The file.txt will keep getting changed and conflicting in each flow From 026ef42dbc08e54f31ce69820c91c22f7de5cc7d Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 10:42:06 +0200 Subject: [PATCH 06/16] cleanup --- .github/workflows/copilot-setup-steps.yml | 19 -- docs/CodeflowSourceDiffVerification.md | 240 ---------------------- 2 files changed, 259 deletions(-) delete mode 100644 .github/workflows/copilot-setup-steps.yml delete mode 100644 docs/CodeflowSourceDiffVerification.md diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml deleted file mode 100644 index 3e31605e56..0000000000 --- a/.github/workflows/copilot-setup-steps.yml +++ /dev/null @@ -1,19 +0,0 @@ -name: "Copilot Setup Steps" - -on: workflow_dispatch - -jobs: - copilot-setup-steps: - runs-on: ubuntu-latest - environment: copilot - timeout-minutes: 30 - permissions: - contents: read - id-token: write - steps: - - uses: actions/checkout@v5 - with: - fetch-depth: 0 - - name: Restore dependencies - run: ./eng/common/build.sh -restore - shell: bash diff --git a/docs/CodeflowSourceDiffVerification.md b/docs/CodeflowSourceDiffVerification.md deleted file mode 100644 index 1964d75f58..0000000000 --- a/docs/CodeflowSourceDiffVerification.md +++ /dev/null @@ -1,240 +0,0 @@ -# Codeflow Source-Diff Verification - -## Status - -Proposed / design. Not yet implemented. - -## Goal (high level) - -When PCS opens a **forward-flow** codeflow PR (changes flowing from a source repo -into the VMR, e.g. `dotnet/efcore` -> `dotnet/dotnet`), reviewers currently have -no automated confirmation that the PR actually contains the changes from the -source build. The PR description only prints a `darc vmr diff` command they could -run by hand. - -The goal of this feature is to give reviewers a **confidence signal**: after the -codeflow commit is pushed, automatically verify that the PR faithfully reflects -the source repo's commit diff (`oldSha..newSha`), and post the result as a -**follow-up PR comment**. - -This is a *reporting / confidence* feature, **not** a correctness gate: - -- It tells reviewers "this PR == the source diff, minus the expected exclusions - and no-ops". -- It does **not** prove the VMR mapping configuration itself is correct, because - it reuses the same `source-mappings.json` exclusions the codeflow used. A bug - in that config would be invisible to this check. The comment should state this - limitation so reviewers don't over-trust it. - -## Why this is not a trivial 1:1 diff - -A correct forward-flow PR is intentionally **not** a byte-for-byte copy of the -source diff. Comparing the two diffs naively produces false mismatches. The known, -legitimate sources of divergence are: - -1. **Path remap** — source paths (`test/Foo.cs`) live under `src//` in - the VMR (`src/efcore/test/Foo.cs`). -2. **Cloaked / excluded paths** — `source-mappings.json` `exclude` rules and - submodules. Source-side changes to those are expected to be absent from the PR. -3. **`eng/common/**`** — Arcade-managed and not flowed per-repo (except for the - `arcade` mapping itself). -4. **Version / metadata files** — `src/source-manifest.json`, `Version.Details.xml`, - `Version.Details.props`, `Versions.props`, `global.json`, etc. get - codeflow-specific content (BAR id, SHAs) that won't match the source diff. -5. **Already-at-target ("no-op") files** — the source diff changes a file, but the - VMR copy was already at the new content (brought there by a prior flow or other - means). Codeflow correctly produces no change for it. A name-only comparison - would wrongly report this as "missing". -6. **Cosmetic diff-render differences** — hunk-header line offsets (`@@ -27` vs - `@@ -24`) and trailing-newline markers (`\ No newline at end of file`). The same - logical change renders differently because surrounding lines differ between the - source file and its VMR copy. - -The verification must therefore be *semantic* (compare the actual changed lines, -after normalizing away the items above), not textual. - -## Inputs - -Per forward-flow PR: - -| Value | Meaning | Source | -|-----------------|----------------------------------------------------------------------|--------| -| source repo | e.g. `https://github.com/dotnet/efcore` | subscription | -| `newSha` | source commit being flowed | `build.Commit` | -| `oldSha` | previously-flowed source commit | mapping's commit in the **merge-base** `source-manifest.json` | -| `mapping` | VMR target directory, e.g. `efcore` | subscription `TargetDirectory` | -| VMR repo | target repo URL | PR target repo | -| `headBranch` | the codeflow PR branch | PR head | -| `targetBranch` | branch the PR targets, e.g. `main` | subscription `TargetBranch` | - -`oldSha...newSha` is exactly the compare range shown in the PR description's -"Commit Diff" link. - -## Algorithm - -Two phases. **Phase 1** cheaply enumerates and partitions the touched files; -**Phase 2** compares the actual changes only where needed. - -### Phase 1 — name-only partition - -Build two file sets in the same coordinate space (mapping-relative paths): - -Version/metadata files that codeflow rewrites with codeflow-specific content -(BAR id, SHAs) change in **both** the source repo and the VMR but with -**different** content. They can never match in Phase 2 and would false-flag, so -they must be dropped from **both** R and A. The canonical set is -`DependencyFileManager.CodeflowDependencyFiles` = -`{ eng/Version.Details.xml, eng/Version.Details.props, global.json, -.config/dotnet-tools.json }`. - -Note: `Versions.props` is deliberately **not** in that set ("In VMR repos, -Versions.props doesn't contain dependency versions maintained by automation, so -every change is meaningful"). It flows as a normal file, so we do **not** drop it -— if it was a no-op (VMR already at target) it is correctly cleared by the no-op -check in `R \ A`. The root `src/source-manifest.json` lives outside -`src//`, so scoping A to `src//` already excludes it. - -- **R** ("reference", what the source diff should contribute): - `git diff --name-only oldSha...newSha` in the **source repo**, then - - drop paths matching the mapping's `exclude`/submodule filters - (`GetDiffFilters` already computes these from `source-mappings.json`), - - drop `eng/common/**` for non-arcade mappings, - - drop the version/metadata files listed above - (`DependencyFileManager.CodeflowDependencyFiles`). -- **A** ("actual", what the PR changed under the mapping): - `git diff --name-only targetBranch...headBranch -- src//` in the - **VMR**, then strip the `src//` prefix, and - - drop `eng/common/**` for non-arcade mappings, - - drop the same version/metadata files. - -Partition: - -| Bucket | Meaning | Action | -|-----------|--------------------------------------|--------| -| `R ∩ A` | touched in both | Phase 2 content compare | -| `R \ A` | source changed it, PR did not | no-op file-state check (below) | -| `A \ R` | changed in PR only | **must be empty** — else flag (codeflow wrote changes not traceable to the source diff) | - -> **Why `A \ R` must be empty.** The VMR diff is three-dot -> (`targetBranch...headBranch`), so it contains only the changes the PR -> introduced — not unrelated edits already on the target branch. The comment is -> posted right after PCS pushes the branch, before any human edits the PR. So -> after the same exclusions are applied to `A` (version/metadata files and, for -> non-arcade mappings, `eng/common/**`), any remaining file the PR changed under -> `src//` that is absent from the source diff means the codeflow -> produced a change that does not come from the source — a correctness red flag, -> not an expected divergence. (This is what lets the signal *guarantee* the -> codeflow reproduced exactly the source diff, rather than merely "contains" it.) - -### Phase 2 — per-file content compare (`R ∩ A`) - -For each file, scope a zero-context diff to that single file in each repo: - -```bash -git -C diff -U0 oldSha...newSha -- -git -C diff -U0 targetBranch...headBranch -- src// -``` - -Normalize each by removing the lines that are *expected* to differ, then compare -what remains: - -- Strip `diff --git`, `index `, `@@ `, `--- `, `+++ ` (this is exactly - `CodeflowChangeAnalyzer.IgnoredDiffLines`). With `-U0` there are no context - lines, so only `+`/`-` change lines remain. -- Compare the remaining `+`/`-` lines. - - equal -> **match** (change applied faithfully). - - different -> **flag**: "changes don't match". - -Decision to record: whether to strip trailing-newline markers -(`\ No newline at end of file`). They are a genuine byte difference; for mapped -source files we keep them (only observed on excluded version files so far). - -### No-op check (`R \ A`) - -A file in `R \ A` is only legitimate if the VMR is **already at the source's new -state**. Diff content can't distinguish "correctly no-op'd" from "wrongly -dropped", so compare the resulting **file state**: - -``` -content(VMR_head : src//) vs content(source@newSha : ) -``` - -- equal, **or both absent** (deletion already reconciled) -> legitimate no-op, - ignore. -- different -> **flag**: "changes don't match". - -## Output - -The goal is simple: run the check and post a short PR comment expressing our -confidence. It is a positive/negative signal, not a detailed report. - -- Empty flag set (`R ∩ A` all match, `R \ A` all no-ops, `A \ R` empty) => post a - short confirmation, e.g.: - - ``` - ### Source diff verification - ✅ This PR matches the source diff 60e46e1...5932bea of dotnet/wpf. - ``` - -- Anything flagged (mismatching content or unexpected files) => post a short - "couldn't verify, please review manually" comment, e.g.: - - ``` - ### Source diff verification - ⚠️ Couldn't verify that this PR matches the source diff 60e46e1...5932bea of - dotnet/wpf. Please review the changes manually. - ``` - -Both comments carry a footnote that this is a best-effort signal validated under -the current source-mappings config. - -## Reuse / where it plugs in - -- **`CodeflowChangeAnalyzer`** (`DarcLib/VirtualMonoRepo`) already computes the - VMR-side diff against the merge base, classifies source vs version/metadata - files, handles the non-arcade `eng/common` exclusion, and defines - `IgnoredDiffLines`. This feature lives **inside** `CodeflowChangeAnalyzer` as - `VerifyForwardFlowAsync` and reuses these helpers/constants directly. -- **`GetDiffFilters`** (`Darc/Operations/VirtualMonoRepo/VmrDiffOperation.cs`) - already turns `source-mappings.json` excludes + submodules into git pathspec - exclusion rules; reuse it to filter **R**. -- **`ForwardFlowMergePolicy`** already obtains `mergeBase`, head/merge-base - `SourceManifest`s, and validates them; the SHAs and manifests this feature - needs are already available at evaluation time. -- PCS already has the VMR cloned locally during flow - (`ILocalGitRepoFactory`); the source `oldSha...newSha` is available either from - the patch codeflow already built or via a remote diff call. - -The analyzer returns the bucketed result (list of mismatching files; empty = -match). PCS renders it into a follow-up PR comment, separate from PR creation so -it never slows down or fails the codeflow push. - -## Scope / limitations - -- Trusts `R ∩ A` on presence + content-line equality; it does not re-run the - 3-way merge, so it validates "PR reflects the source diff under the current - mapping config", not "the config is correct". -- `A \ R` must be empty after the standard exclusions: a non-empty set means the - codeflow changed files under `src//` that don't trace back to the - source diff. This assumes the check runs right after PCS pushes, before a human - adds conflict-resolution or unrelated commits to the PR branch (which would also - show up in `A`); that is the only intended call site. -- Large flows (e.g. `runtime`) can touch thousands of files; Phase 1 is name-only - and cheap, but Phase 2/no-op content fetches should be bounded (only `R ∩ A` - and `R \ A`). Consider a size cap or running fully asynchronously as a comment. - -## Validation done so far - -Manually verified the algorithm by hand against: - -- **dotnet/dotnet#7107** (efcore, small): R = A = 5 files, `R \ A` empty, all 5 - files match content 1:1. -- **dotnet/dotnet#7142** (wpf, larger): applying the full algorithm — - `R∩A` = 40 files (all content-match), `R\A` = 2 files - (`.github/policies/resourceManagement.yml`, `eng/restore-toolset.ps1`), both - confirmed genuine no-ops (VMR head equals source@newSha), and `A\R` = 0 after - exclusions — the 17 PR-only files were all `eng/common/**`, which is dropped - from `A` for non-arcade mappings before the `A\R` check, so nothing remained to - flag. No mismatches flagged. Version files - (`Versions.props`, `Version.Details.xml`, `global.json`, ...) were excluded from - both sides as designed. From 10fb18cab43b6d035840122685ec1fd79d7f62cb Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 11:29:14 +0200 Subject: [PATCH 07/16] test --- arcade-services.slnx | 1 + .../PullRequestUpdaterTests.cs | 14 ++++++++++++++ .../UpdateAssetsForCodeFlowTests.cs | 3 +++ .../TestHelpersTest.cs | 1 + 4 files changed, 19 insertions(+) diff --git a/arcade-services.slnx b/arcade-services.slnx index 3b9292f533..296f673fcf 100644 --- a/arcade-services.slnx +++ b/arcade-services.slnx @@ -59,6 +59,7 @@ + diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index d89483887a..1040e6e695 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -39,6 +39,7 @@ internal abstract class PullRequestUpdaterTests : SubscriptionOrPullRequestUpdat private Mock _backFlower = null!; private Mock _forwardFlower = null!; private Mock _gitClient = null!; + private Mock _codeflowSourceDiffVerifier = null!; [SetUp] public void PullRequestUpdaterTests_SetUp() @@ -46,6 +47,7 @@ public void PullRequestUpdaterTests_SetUp() _backFlower = new(); _forwardFlower = new(); _gitClient = new(); + _codeflowSourceDiffVerifier = new(); } protected override void RegisterServices(IServiceCollection services) @@ -55,6 +57,7 @@ protected override void RegisterServices(IServiceCollection services) services.AddSingleton(_backFlower.Object); services.AddSingleton(_forwardFlower.Object); services.AddSingleton(_gitClient.Object); + services.AddSingleton(_codeflowSourceDiffVerifier.Object); CodeFlowResult codeFlowRes = new(true, [], new NativePath(VmrPath), []); _forwardFlower.SetReturnsDefault(Task.FromResult(codeFlowRes)); @@ -678,6 +681,17 @@ protected void AndShouldNotHavePullRequestCheckReminder() RemoveExpectedReminder(Subscription); } + protected void AndShouldHaveCodeflowUpdateCheckReminder(string previousSourceSha, string currentSourceSha) + { + SetExpectedReminder(Subscription, new CodeflowUpdateCheck() + { + UpdaterId = GetPullRequestUpdaterId().ToString(), + SubscriptionId = Subscription.Id, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = currentSourceSha, + }); + } + protected void AndShouldHaveInProgressPullRequestState( Build forBuild, int nextBuildToProcess = 0, diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 66bc51773b..b24afba917 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -268,6 +268,7 @@ public async Task UpdateWithManuallyMergedPrAndNewBuild() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowUpdateCheckReminder(build.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -354,6 +355,7 @@ public async Task UpdateWithManuallyMergedPrAndLatestFlowNotApplied() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowUpdateCheckReminder(build1.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -522,6 +524,7 @@ public async Task UpdateWithExistingOppositeSubscription() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowUpdateCheckReminder(build.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } diff --git a/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs b/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs index 5c28293a81..90ab0dacf3 100644 --- a/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs +++ b/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using AwesomeAssertions; +using ProductConstructionService.ScenarioTests.Helpers; namespace ProductConstructionService.ScenarioTests.Tests; From ff31e682b8fcbacb3d0adfc9b8826446fdf387c6 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 11:42:06 +0200 Subject: [PATCH 08/16] Rename to Codeflow Approval Check --- .../DependencyFlowConfiguration.cs | 2 +- .../IPullRequestStateManager.cs | 4 +-- .../PullRequestStateManager.cs | 10 +++---- .../CodeFlowPullRequestUpdater.cs | 30 +++++++++---------- ...r.cs => CodeflowApprovalCheckProcessor.cs} | 10 +++---- ...pdateCheck.cs => CodeflowApprovalCheck.cs} | 2 +- .../PullRequestUpdaterTests.cs | 4 +-- .../UpdateAssetsForCodeFlowTests.cs | 6 ++-- 8 files changed, 34 insertions(+), 34 deletions(-) rename src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/{CodeflowUpdateCheckProcessor.cs => CodeflowApprovalCheckProcessor.cs} (86%) rename src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/{CodeflowUpdateCheck.cs => CodeflowApprovalCheck.cs} (86%) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs index 65d39733c8..3693117cae 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs @@ -37,6 +37,6 @@ public static void AddDependencyFlowProcessors(this IServiceCollection services) services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); - services.AddWorkItemProcessor(); + services.AddWorkItemProcessor(); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs index c47e638a6f..9087e41055 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs @@ -58,9 +58,9 @@ internal interface IPullRequestStateManager #endregion - #region Codeflow update check + #region Codeflow approval check - Task SetCodeflowUpdateCheck(CodeflowUpdateCheck check); + Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check); #endregion diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs index 9374504b09..5b72c63fb9 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs @@ -21,7 +21,7 @@ internal class PullRequestStateManager : IPullRequestStateManager private readonly IRedisCache _mergePolicyEvaluationState; private readonly IReminderManager _pullRequestUpdateReminders; private readonly IReminderManager _pullRequestCheckReminders; - private readonly IReminderManager _codeflowUpdateCheckReminders; + private readonly IReminderManager _codeflowApprovalCheckReminders; public PullRequestStateManager( IPullRequestTarget pullRequestTarget, @@ -34,7 +34,7 @@ public PullRequestStateManager( _mergePolicyEvaluationState = cacheFactory.Create(cacheKey); _pullRequestUpdateReminders = reminderManagerFactory.CreateReminderManager(cacheKey); _pullRequestCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); - _codeflowUpdateCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); + _codeflowApprovalCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); } public Task GetInProgressPullRequestAsync() => @@ -95,7 +95,7 @@ public async Task ClearAllStateAsync(bool isCodeFlow, bool clearPendingUpdates) { await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - await _codeflowUpdateCheckReminders.UnsetReminderAsync(isCodeFlow); + await _codeflowApprovalCheckReminders.UnsetReminderAsync(isCodeFlow); if (clearPendingUpdates) { await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); @@ -110,6 +110,6 @@ public async Task ScheduleUpdateForLater(InProgressPullRequest pr, SubscriptionU await SetInProgressPullRequestAsync(pr); } - public async Task SetCodeflowUpdateCheck(CodeflowUpdateCheck check) => - await _codeflowUpdateCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, true); + public async Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check) => + await _codeflowApprovalCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, true); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 977a7d9b1f..254a939875 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -531,7 +531,7 @@ private async Task RequestManualConflictResolutionAsync( codeFlowResult.DependencyUpdates, upstreamRepoDiffs, unsafeFlown, - skipCodeflowUpdateCheck: true); + skipCodeflowApprovalCheck: true); } else { @@ -562,7 +562,7 @@ await UpdateCodeFlowPullRequestAsync( subscription, codeFlowResult.DependencyUpdates, upstreamRepoDiffs, - skipCodeflowUpdateCheck: true); + skipCodeflowApprovalCheck: true); // Since we changed the PR state in cache but no commit was pushed, // we need to delete non-transient check results so that they can be re-evaluated @@ -635,7 +635,7 @@ private async Task CreateEmptyPrBranch( List dependencyUpdates, IReadOnlyCollection? upstreamRepoDiffs, bool unsafeFlow, - bool skipCodeflowUpdateCheck = false) + bool skipCodeflowApprovalCheck = false) { IRemote darcRemote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -698,9 +698,9 @@ await _subscriptionEventRecorder.AddDependencyFlowEventsAsync( inProgressPr.LastUpdate = DateTime.UtcNow; await _stateManager.SetCheckReminderAsync(inProgressPr, pr, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); - if (!skipCodeflowUpdateCheck && !string.IsNullOrEmpty(previousSourceSha)) + if (!skipCodeflowApprovalCheck && !string.IsNullOrEmpty(previousSourceSha)) { - await _stateManager.SetCodeflowUpdateCheck(new CodeflowUpdateCheck + await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck { UpdaterId = _target.UpdaterId, SubscriptionId = update.SubscriptionId, @@ -743,7 +743,7 @@ private async Task UpdateCodeFlowPullRequestAsync( SubscriptionDTO subscription, List newDependencyUpdates, IReadOnlyCollection? upstreamRepoDiffs, - bool skipCodeflowUpdateCheck = false) + bool skipCodeflowApprovalCheck = false) { IRemote remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -807,9 +807,9 @@ private async Task UpdateCodeFlowPullRequestAsync( pullRequest.BlockedFromFutureUpdates = false; // if a sub is blocked, and someone force triggers it, we can continue flowing afterwards await _stateManager.SetCheckReminderAsync(pullRequest, prInfo!, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); - if (!skipCodeflowUpdateCheck && !string.IsNullOrEmpty(previousSourceSha)) + if (!skipCodeflowApprovalCheck && !string.IsNullOrEmpty(previousSourceSha)) { - await _stateManager.SetCodeflowUpdateCheck(new CodeflowUpdateCheck + await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck { UpdaterId = _target.UpdaterId, SubscriptionId = update.SubscriptionId, @@ -820,14 +820,14 @@ await _stateManager.SetCodeflowUpdateCheck(new CodeflowUpdateCheck } } - public async Task RunCodeflowUpdateCodeCheck( + public async Task RunCodeflowApprovalCheck( SubscriptionDTO subscription, - CodeflowUpdateCheck codeflowUpdateCheck, + CodeflowApprovalCheck codeflowApprovalCheck, CancellationToken cancellationToken) { if (subscription.IsBackflow()) { - _logger.LogError("Can't run codeflow code check on backflow subscriptions"); + _logger.LogError("Can't run codeflow approval check on backflow subscriptions"); return; } @@ -835,7 +835,7 @@ public async Task RunCodeflowUpdateCodeCheck( if (pr == null) { - _logger.LogError("No in-progress PR found for codeflow update code check"); + _logger.LogError("No in-progress PR found for codeflow approval check"); return; } @@ -845,7 +845,7 @@ public async Task RunCodeflowUpdateCodeCheck( if (prInfo.Status != PrStatus.Open) { _logger.LogInformation( - "Skipping codeflow update code check for PR {prUrl} because it is no longer open (status: {status})", + "Skipping codeflow approval check for PR {prUrl} because it is no longer open (status: {status})", pr.Url, prInfo.Status); return; @@ -855,8 +855,8 @@ public async Task RunCodeflowUpdateCodeCheck( subscription.SourceRepository, subscription.TargetRepository, subscription.TargetDirectory, - codeflowUpdateCheck.PreviousSourceSha, - codeflowUpdateCheck.CurrentSourceSha, + codeflowApprovalCheck.PreviousSourceSha, + codeflowApprovalCheck.CurrentSourceSha, subscription.TargetBranch, pr.HeadBranch, cancellationToken); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs similarity index 86% rename from src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs rename to src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs index cc44c6a5c4..74ede84b73 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowUpdateCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs @@ -11,12 +11,12 @@ namespace ProductConstructionService.DependencyFlow.WorkItemProcessors; -internal class CodeflowUpdateCheckProcessor : WorkItemProcessor +internal class CodeflowApprovalCheckProcessor : WorkItemProcessor { private readonly BuildAssetRegistryContext _context; private readonly IPullRequestUpdaterFactory _updaterFactory; - public CodeflowUpdateCheckProcessor( + public CodeflowApprovalCheckProcessor( BuildAssetRegistryContext context, IPullRequestUpdaterFactory updaterFactory) { @@ -24,7 +24,7 @@ public CodeflowUpdateCheckProcessor( _updaterFactory = updaterFactory; } - public override async Task ProcessWorkItemAsync(CodeflowUpdateCheck workItem, CancellationToken cancellationToken) + public override async Task ProcessWorkItemAsync(CodeflowApprovalCheck workItem, CancellationToken cancellationToken) { var subscription = _context.Subscriptions .Include(s => s.Channel) @@ -40,10 +40,10 @@ public override async Task ProcessWorkItemAsync(CodeflowUpdateCheck workIt if (pullRequestUpdater is not CodeFlowPullRequestUpdater codeFlowUpdater) { - throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow update check"); + throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow approval check"); } - await codeFlowUpdater.RunCodeflowUpdateCodeCheck( + await codeFlowUpdater.RunCodeflowApprovalCheck( SqlBarClient.ToClientModelSubscription(subscription), workItem, cancellationToken); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs similarity index 86% rename from src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs rename to src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs index 7eea3d3c08..1f5b8f919d 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowUpdateCheck.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs @@ -3,7 +3,7 @@ namespace ProductConstructionService.DependencyFlow.WorkItems; -internal class CodeflowUpdateCheck : DependencyFlowWorkItem +internal class CodeflowApprovalCheck : DependencyFlowWorkItem { public required Guid SubscriptionId { get; init; } public required string PreviousSourceSha { get; init; } diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index 1040e6e695..6a754eee49 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -681,9 +681,9 @@ protected void AndShouldNotHavePullRequestCheckReminder() RemoveExpectedReminder(Subscription); } - protected void AndShouldHaveCodeflowUpdateCheckReminder(string previousSourceSha, string currentSourceSha) + protected void AndShouldHaveCodeflowApprovalCheckReminder(string previousSourceSha, string currentSourceSha) { - SetExpectedReminder(Subscription, new CodeflowUpdateCheck() + SetExpectedReminder(Subscription, new CodeflowApprovalCheck() { UpdaterId = GetPullRequestUpdaterId().ToString(), SubscriptionId = Subscription.Id, diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index b24afba917..0e54192a19 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -268,7 +268,7 @@ public async Task UpdateWithManuallyMergedPrAndNewBuild() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowUpdateCheckReminder(build.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -355,7 +355,7 @@ public async Task UpdateWithManuallyMergedPrAndLatestFlowNotApplied() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowUpdateCheckReminder(build1.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build1.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -524,7 +524,7 @@ public async Task UpdateWithExistingOppositeSubscription() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowUpdateCheckReminder(build.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } From a47dee720f09bc12af0bf8ab57fe5379f23060d8 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 11:47:50 +0200 Subject: [PATCH 09/16] Revert test changes --- .../DarcLib/VirtualMonoRepo/VmrPatchHandler.cs | 2 +- .../Operations/Operation.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs index 72bbc30f73..812fec9a73 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs @@ -348,7 +348,7 @@ public async Task> ApplyPatch( } } - //_logger.LogDebug("{output}", result.ToString()); + _logger.LogDebug("{output}", result.ToString()); if (removePatchAfter) { diff --git a/tools/ProductConstructionService.ReproTool/Operations/Operation.cs b/tools/ProductConstructionService.ReproTool/Operations/Operation.cs index 84b1e0f10b..1254f416b6 100644 --- a/tools/ProductConstructionService.ReproTool/Operations/Operation.cs +++ b/tools/ProductConstructionService.ReproTool/Operations/Operation.cs @@ -134,7 +134,7 @@ protected async Task> PrepareVmrForkAsync(string br { logger.LogInformation("Preparing VMR fork"); // Sync the VMR fork branch - //await SyncForkAsync("dotnet", "dotnet", branch); + await SyncForkAsync("dotnet", "dotnet", branch); return await CreateTmpBranchAsync(VmrForkRepoName, branch, skipCleanup); } @@ -214,7 +214,7 @@ protected async Task> PrepareProductRepoForkAsync( if (allRepos.Any(repo => repo.HtmlUrl.Equals(productRepoForkUri, StringComparison.OrdinalIgnoreCase))) { logger.LogInformation("Product repo fork {fork} already exists, syncing branch {branch} with source", productRepoForkUri, productRepoBranch); - //await SyncForkAsync(org, name, productRepoBranch); + await SyncForkAsync(org, name, productRepoBranch); } // If we don't, create a fork else From 8e9d7e88967f25e8036504d5927899d71b30d1ba Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 14:13:11 +0200 Subject: [PATCH 10/16] Add verifier tests --- .../CodeflowSourceDiffVerifier.cs | 46 +-- .../CodeFlowPullRequestUpdater.cs | 69 +---- .../CodeflowApprovalCheckProcessor.cs | 2 +- .../CodeflowSourceDiffVerifierTests.cs | 289 ++++++++++++++++++ 4 files changed, 320 insertions(+), 86 deletions(-) create mode 100644 test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs index 968cc6c1a4..959be84d06 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -38,19 +38,6 @@ Task VerifyForwardFlowAsync( /// public class CodeflowSourceDiffVerifier : ICodeflowSourceDiffVerifier { - // List of characters that belong to the git diff format. - // Example diff output: - // diff --git a/src/product-repo1/eng/Versions.props b/src/product-repo1/eng/Versions.props - // index fb13f6d..76d73de 100644 - // --- a/src/product-repo1/eng/Versions.props - // +++ b/src/product-repo1/eng/Versions.props - // @@ -10,2 +10,2 @@ - // - 1.0.0 - // - 2.0.0 - // + 2.0.1 - // + 2.0.1 - private static readonly IReadOnlyCollection IgnoredDiffLines = ["diff --git", "index ", "@@ ", "--- ", "+++ "]; - private readonly IVmrCloneManager _vmrCloneManager; private readonly IRepositoryCloneManager _cloneManager; private readonly IVmrDependencyTracker _dependencyTracker; @@ -255,11 +242,36 @@ private static async Task ChangedLinesMatchAsync( } /// - /// Keeps only the +/- change lines from a zero-context diff, dropping the diff-format lines that - /// are expected to differ between the source repo and its VMR copy. + /// Keeps only the +/- change lines from a zero-context diff. Only lines inside a hunk (after an + /// "@@" header) are collected, so the per-file "--- a/file" / "+++ b/file" headers - which share a + /// prefix with genuine content lines such as a removed "-- comment" (rendered as "--- comment") - are + /// excluded structurally rather than by an ambiguous textual prefix match. /// - private static List GetChangeLines(IReadOnlyCollection lines) => - [.. lines.Where(line => (line.StartsWith('+') || line.StartsWith('-')) && !IgnoredDiffLines.Any(line.StartsWith))]; + private static List GetChangeLines(IReadOnlyCollection lines) + { + var changeLines = new List(); + var insideHunk = false; + + foreach (var line in lines) + { + if (line.StartsWith("@@")) + { + // Start of a hunk; the lines that follow (until the next hunk or file) are the changes. + insideHunk = true; + } + else if (line.StartsWith("diff --git")) + { + // Start of a new file's header section, whose "--- "/"+++ " headers precede its first hunk. + insideHunk = false; + } + else if (insideHunk && (line.StartsWith('+') || line.StartsWith('-'))) + { + changeLines.Add(line); + } + } + + return changeLines; + } /// /// A file the source changed but the PR did not is only legitimate when the VMR copy is already diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 254a939875..440ee524b9 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -820,7 +820,7 @@ await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck } } - public async Task RunCodeflowApprovalCheck( + public async Task RunCodeflowApprovalCheckAsync( SubscriptionDTO subscription, CodeflowApprovalCheck codeflowApprovalCheck, CancellationToken cancellationToken) @@ -867,73 +867,6 @@ public async Task RunCodeflowApprovalCheck( } } - private async Task PostSourceDiffVerificationCommentAsync( - IRemote remote, - SubscriptionDTO subscription, - BuildDTO build, - string? previousSourceSha, - string? prHeadBranch, - string prUrl) - { - // Best-effort confidence signal: never fail or slow down the codeflow push. - try - { - if (string.IsNullOrEmpty(previousSourceSha) || string.IsNullOrEmpty(prHeadBranch)) - { - // Without a previous source SHA there is no compare range, so we skip silently - // (matching how the PR description's diff link is skipped). - return; - } - - bool matches = await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( - subscription.SourceRepository, - subscription.TargetRepository, - subscription.TargetDirectory, - previousSourceSha, - build.Commit, - subscription.TargetBranch, - prHeadBranch); - - var comment = BuildSourceDiffVerificationComment(matches, subscription.SourceRepository, previousSourceSha, build.Commit); - await remote.CommentPullRequestAsync(prUrl, comment); - } - catch (Exception e) - { - _logger.LogWarning(e, - "Failed to post source diff verification comment for PR {prUrl} of subscription {subscriptionId}", - prUrl, - subscription.Id); - } - } - - private static string BuildSourceDiffVerificationComment( - bool matches, - string sourceRepo, - string oldSha, - string newSha) - { - var shortOld = Commit.GetShortSha(oldSha); - var shortNew = Commit.GetShortSha(newSha); - - var builder = new StringBuilder(); - builder.AppendLine("### Source diff verification"); - builder.AppendLine(); - - if (matches) - { - builder.AppendLine($"✅ This PR matches the source diff `{shortOld}...{shortNew}` of `{sourceRepo}`."); - } - else - { - builder.AppendLine($"⚠️ Couldn't verify that this PR matches the source diff `{shortOld}...{shortNew}` of `{sourceRepo}`. Please review the changes manually."); - } - - builder.AppendLine(); - builder.AppendLine("> This is a best-effort confidence signal. It reuses the same source-mappings configuration the codeflow used, so it validates that the PR reflects the source diff under the current config — not that the config itself is correct."); - - return builder.ToString(); - } - private async Task HandleBlockingCodeflowException(InProgressPullRequest pr) { _logger.LogInformation("PR with url {prUrl} is blocked from receiving future codeflows.", pr.Url); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs index 74ede84b73..50598e232c 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs @@ -43,7 +43,7 @@ public override async Task ProcessWorkItemAsync(CodeflowApprovalCheck work throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow approval check"); } - await codeFlowUpdater.RunCodeflowApprovalCheck( + await codeFlowUpdater.RunCodeflowApprovalCheckAsync( SqlBarClient.ToClientModelSubscription(subscription), workItem, cancellationToken); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs new file mode 100644 index 0000000000..4f83d9748a --- /dev/null +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs @@ -0,0 +1,289 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using AwesomeAssertions; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using NUnit.Framework; + +namespace Microsoft.DotNet.DarcLib.Codeflow.Tests; + +/// +/// Unit tests for . The git/VMR +/// interactions are mocked so each test drives the public verifier with hand-crafted +/// "source repo" and "VMR PR" diffs and asserts the verdict. +/// +[TestFixture] +internal class CodeflowSourceDiffVerifierTests +{ + private const string MappingName = "product-repo1"; + private const string SourceRepoUri = "https://github.com/dotnet/product-repo1"; + private const string VmrUri = "https://github.com/dotnet/dotnet"; + private const string OldSha = "aaaaaaa"; + private const string NewSha = "bbbbbbb"; + private const string VmrTargetBranch = "main"; + private const string VmrHeadBranch = "pr-branch"; + + // VmrInfo.GetRelativeRepoSourcesPath(MappingName) == "src/product-repo1". + private const string SrcMappingPath = "src/" + MappingName; + private const string VmrPrefix = SrcMappingPath + "/"; + + private Mock _vmrCloneManager = null!; + private Mock _cloneManager = null!; + private Mock _dependencyTracker = null!; + private Mock _sourceManifest = null!; + private Mock _sourceRepo = null!; + private Mock _vmr = null!; + private CodeflowSourceDiffVerifier _verifier = null!; + + [SetUp] + public void SetUp() + { + _sourceRepo = new Mock(); + _vmr = new Mock(); + + var mapping = new SourceMapping( + Name: MappingName, + DefaultRemote: SourceRepoUri, + DefaultRef: "main", + Include: [], + Exclude: [], + DisableSynchronization: false); + + _dependencyTracker = new Mock(); + _dependencyTracker.Setup(t => t.GetMapping(MappingName)).Returns(mapping); + + _sourceManifest = new Mock(); + _sourceManifest.Setup(m => m.Submodules).Returns([]); + + _vmrCloneManager = new Mock(); + _vmrCloneManager + .Setup(m => m.PrepareVmrAsync( + It.IsAny>(), + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(() => _vmr.Object); + + _cloneManager = new Mock(); + _cloneManager + .Setup(m => m.PrepareCloneAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(() => _sourceRepo.Object); + + _verifier = new CodeflowSourceDiffVerifier( + _vmrCloneManager.Object, + _cloneManager.Object, + _dependencyTracker.Object, + _sourceManifest.Object, + NullLogger.Instance); + } + + private Task VerifyAsync() => _verifier.VerifyForwardFlowAsync( + SourceRepoUri, + VmrUri, + MappingName, + OldSha, + NewSha, + VmrTargetBranch, + VmrHeadBranch, + CancellationToken.None); + + /// + /// Configures a mocked repo's "git diff" calls. is returned for the + /// "diff --name-only" call (the changed file listing); maps the file argument + /// of a "diff -U0 ... -- <file>" call to that file's zero-context diff text. + /// + private static void SetupDiffs( + Mock repo, + string nameOnlyOutput, + IReadOnlyDictionary fileDiffs) + { + repo.Setup(r => r.ExecuteGitCommand(It.IsAny(), It.IsAny())) + .Returns((string[] args, CancellationToken _) => + { + var output = args.Contains("--name-only") + ? nameOnlyOutput + : fileDiffs.GetValueOrDefault(args[^1], string.Empty); + + return Task.FromResult(new ProcessExecutionResult { ExitCode = 0, StandardOutput = output }); + }); + } + + /// + /// Builds a realistic "git diff -U0" output for a single-line change of . + /// The removed/added content lines are passed verbatim (already including any leading characters of the + /// actual file content, e.g. "-- old" for a SQL comment). + /// + private static string SingleLineDiff(string path, string removedLine, string addedLine) => string.Join('\n', + [ + $"diff --git a/{path} b/{path}", + "index 1111111..2222222 100644", + $"--- a/{path}", + $"+++ b/{path}", + "@@ -1 +1 @@", + "-" + removedLine, + "+" + addedLine, + ]); + + /// + /// Builds a "git diff -U0" output for a file changed in two separate places. With zero context each + /// change region is its own hunk, so this exercises multi-hunk parsing. + /// + private static string TwoHunkDiff( + string path, + (string Removed, string Added) firstHunk, + (string Removed, string Added) secondHunk) => string.Join('\n', + [ + $"diff --git a/{path} b/{path}", + "index 1111111..3333333 100644", + $"--- a/{path}", + $"+++ b/{path}", + "@@ -1 +1 @@", + "-" + firstHunk.Removed, + "+" + firstHunk.Added, + "@@ -5 +5 @@", + "-" + secondHunk.Removed, + "+" + secondHunk.Added, + ]); + + [Test] + public async Task VerifyForwardFlowAsync_PrMatchesSourceDiff_ReturnsTrue() + { + // Arrange: the source repo and the VMR PR change the same file in the same way. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "new content"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } + + [Test] + public async Task VerifyForwardFlowAsync_PrChangesFileNotInSourceDiff_ReturnsFalse() + { + // Arrange: the PR touches an extra file the source diff never changed. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, string.Join('\n', VmrPrefix + "data.txt", VmrPrefix + "extra.txt"), new Dictionary()); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse(); + } + + [Test] + public async Task VerifyForwardFlowAsync_PrChangesFileDifferently_ReturnsFalse() + { + // Arrange: the same file is changed in both, but to different content. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "tampered content"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse(); + } + + [Test] + public async Task VerifyForwardFlowAsync_SourceOnlyChangeAlreadyReconciledInVmr_ReturnsTrue() + { + // Arrange: the source changed two files but the PR only changed one. The other (readme.md) is a + // legitimate no-op because the VMR copy already holds the source's new content. + SetupDiffs(_sourceRepo, string.Join('\n', "data.txt", "readme.md"), new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "new content"), + }); + + _sourceRepo.Setup(r => r.GetFileFromGitAsync("readme.md", NewSha, null)).ReturnsAsync("identical readme"); + _vmr.Setup(r => r.GetFileFromGitAsync(VmrPrefix + "readme.md", VmrHeadBranch, null)).ReturnsAsync("identical readme"); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } + + [Test] + public async Task VerifyForwardFlowAsync_PrTampersWithDashDashCommentLine_ReturnsFalse() + { + // Arrange: a SQL file whose comment line is changed. + // Source repo: "-- old" -> "-- new" + // VMR PR: "-- DIFFERENT" -> "-- new" + // The PR did NOT faithfully reproduce the source change (it removed a different line), so the + // verifier must reject the PR. In a -U0 diff the removed content lines render as "--- old" / + // "--- DIFFERENT", which share a prefix with the "--- " file header; GetChangeLines must still + // tell them apart (it parses hunks structurally) so the tampering is detected. + SetupDiffs(_sourceRepo, "schema.sql", new Dictionary + { + ["schema.sql"] = SingleLineDiff("schema.sql", "-- old", "-- new"), + }); + SetupDiffs(_vmr, VmrPrefix + "schema.sql", new Dictionary + { + [VmrPrefix + "schema.sql"] = SingleLineDiff(VmrPrefix + "schema.sql", "-- DIFFERENT", "-- new"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse("the PR removed a different comment line than the source diff and must be rejected"); + } + + [Test] + public async Task VerifyForwardFlowAsync_MultiHunkFileWithMatchingChanges_ReturnsTrue() + { + // Arrange: a file changed in two separate places (two hunks) identically in source and PR. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = TwoHunkDiff("data.txt", ("first old", "first new"), ("second old", "second new")), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = TwoHunkDiff(VmrPrefix + "data.txt", ("first old", "first new"), ("second old", "second new")), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } +} From 87e83fc51fc3de3287663599f31a0025619ae491 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 14:25:20 +0200 Subject: [PATCH 11/16] cleanup --- .../CodeflowSourceDiffVerifier.cs | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs index 959be84d06..0e0c9c81db 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -76,7 +76,6 @@ public async Task VerifyForwardFlowAsync( var srcMappingPath = VmrInfo.GetRelativeRepoSourcesPath(mappingName); - // Make sure both the target and the PR head branches are available locally in the VMR. ILocalGitRepo vmr = await _vmrCloneManager.PrepareVmrAsync( [vmrUri], [vmrTargetBranch, vmrHeadBranch], @@ -88,7 +87,6 @@ public async Task VerifyForwardFlowAsync( var exclusionPathspecs = GetDiffFilters(mapping, _sourceManifest); - // Obtain a local clone of the source repo containing both SHAs. ILocalGitRepo sourceRepo = await _cloneManager.PrepareCloneAsync( mapping, [sourceRepoUri], @@ -98,14 +96,14 @@ public async Task VerifyForwardFlowAsync( cancellationToken); var srcMappingPrefix = srcMappingPath + "/"; - HashSet sourceRepoChanges = await GetChangedMappingFilesAsync( + HashSet sourceRepoChangedFiles = await GetChangedMappingFilesAsync( sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); - HashSet vmrPrChanges = await GetChangedMappingFilesAsync( + HashSet vmrPrChangedFiles = await GetChangedMappingFilesAsync( vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); - var intersection = sourceRepoChanges.Where(vmrPrChanges.Contains).ToList(); - var sourceRepoOnlyChanges = sourceRepoChanges.Where(f => !vmrPrChanges.Contains(f)).ToList(); - var unexpectedFiles = vmrPrChanges.Where(f => !sourceRepoChanges.Contains(f)).ToList(); + var intersection = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList(); + var sourceRepoOnlyChanges = sourceRepoChangedFiles.Where(f => !vmrPrChangedFiles.Contains(f)).ToList(); + var unexpectedFiles = vmrPrChangedFiles.Where(f => !sourceRepoChangedFiles.Contains(f)).ToList(); // The PR changed files that the source diff didn't - the codeflow can't be trusted. if (unexpectedFiles.Count > 0) @@ -166,9 +164,7 @@ private static List GetDiffFilters(SourceMapping mapping, ISourceManifes /// /// Runs a three-dot name-only diff (...) scoped to the /// mapping's location in the repo and returns the mapping-relative paths, after dropping eng/common - /// (non-arcade) and version/metadata files. Used for both the source repo diff (mapping lives at the repo - /// root, so is empty) and the target repo (VMR) diff (mapping lives - /// under src/<mapping>/, which is stripped from the resulting paths). + /// (non-arcade) and version/metadata files. /// private static async Task> GetChangedMappingFilesAsync( ILocalGitRepo repo, @@ -192,17 +188,12 @@ private static async Task> GetChangedMappingFilesAsync( if (!string.IsNullOrEmpty(relativePath)) { files = files - .Where(f => f.StartsWith(relativePath, StringComparison.OrdinalIgnoreCase)) .Select(f => f.Substring(relativePath.Length)); } return FilterMappingFiles(files, mappingName); } - /// - /// Drops eng/common (for non-arcade mappings) and codeflow version/metadata files, which are - /// expected to legitimately diverge between the source repo and the VMR. - /// private static HashSet FilterMappingFiles(IEnumerable files, string mappingName) { var engCommonPrefix = Constants.CommonScriptFilesPath + "/"; @@ -256,14 +247,9 @@ private static List GetChangeLines(IReadOnlyCollection lines) { if (line.StartsWith("@@")) { - // Start of a hunk; the lines that follow (until the next hunk or file) are the changes. + // start of code changes. we want to start adding lines after this, but not the hunk header itself insideHunk = true; } - else if (line.StartsWith("diff --git")) - { - // Start of a new file's header section, whose "--- "/"+++ " headers precede its first hunk. - insideHunk = false; - } else if (insideHunk && (line.StartsWith('+') || line.StartsWith('-'))) { changeLines.Add(line); From e9a8e997ee945d50889ec8496b47921f1d04dab7 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Wed, 24 Jun 2026 15:27:02 +0200 Subject: [PATCH 12/16] copilot cr changes --- .../CodeFlowPullRequestUpdater.cs | 21 ++++++++----------- .../CodeflowApprovalCheckProcessor.cs | 6 +++--- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 440ee524b9..127028ebee 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Text; using Maestro.Common.Telemetry; using Maestro.Data.Models; using Maestro.DataProviders; @@ -851,17 +850,15 @@ public async Task RunCodeflowApprovalCheckAsync( return; } - var matches = await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( - subscription.SourceRepository, - subscription.TargetRepository, - subscription.TargetDirectory, - codeflowApprovalCheck.PreviousSourceSha, - codeflowApprovalCheck.CurrentSourceSha, - subscription.TargetBranch, - pr.HeadBranch, - cancellationToken); - - if (matches) + if (await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( + subscription.SourceRepository, + subscription.TargetRepository, + subscription.TargetDirectory, + codeflowApprovalCheck.PreviousSourceSha, + codeflowApprovalCheck.CurrentSourceSha, + subscription.TargetBranch, + pr.HeadBranch, + cancellationToken)) { await remote.CommentPullRequestAsync(pr.Url, "Looks good"); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs index 50598e232c..ac4ff0aa7e 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs @@ -26,10 +26,10 @@ public CodeflowApprovalCheckProcessor( public override async Task ProcessWorkItemAsync(CodeflowApprovalCheck workItem, CancellationToken cancellationToken) { - var subscription = _context.Subscriptions + var subscription = await _context.Subscriptions .Include(s => s.Channel) - .FirstOrDefault(s => s.Id == workItem.SubscriptionId); - + .FirstOrDefaultAsync(s => s.Id == workItem.SubscriptionId, cancellationToken); + if (subscription == null) { throw new InvalidOperationException($"Subscription with ID {workItem.SubscriptionId} not found."); From 54cce800eaedb4f0d38a41303fa36562110ec1b4 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Thu, 25 Jun 2026 15:32:57 +0200 Subject: [PATCH 13/16] Check commit authors, rename things, add logging --- src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs | 7 ++++ src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 5 +++ .../CodeflowSourceDiffVerifier.cs | 4 +-- .../CodeFlowPullRequestUpdater.cs | 35 ++++++++++++++++--- .../CodeFlowTestsBase.cs | 2 +- .../CodeflowSourceDiffVerifierTests.cs | 16 ++++----- 6 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs index 1fbaaead92..54cf7f0b88 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs @@ -66,6 +66,13 @@ public interface IRemote /// Pull request information. Task GetPullRequestAsync(string pullRequestUri); + /// + /// Retrieve the commits of a pull request. + /// + /// URI of pull request. + /// The commits contained in the pull request. + Task> GetPullRequestCommitsAsync(string pullRequestUri); + /// /// Create a new pull request. /// diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index c5dbeb6765..3f3866d187 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -339,6 +339,11 @@ public Task GetPullRequestAsync(string pullRequestUri) return _remoteGitClient.GetPullRequestAsync(pullRequestUri); } + public Task> GetPullRequestCommitsAsync(string pullRequestUri) + { + return _remoteGitClient.GetPullRequestCommitsAsync(pullRequestUri); + } + public Task GetPullRequestUrlAsync(string repoUri, string headBranch, string targetBranch) { return _remoteGitClient.GetPullRequestUrlAsync(repoUri, headBranch, targetBranch); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs index 0e0c9c81db..5838907d66 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -20,7 +20,7 @@ public interface ICodeflowSourceDiffVerifier /// repo's commit diff (oldSha...newSha), accounting for the expected divergences (path remap, /// excludes, eng/common, version files, no-ops). /// - Task VerifyForwardFlowAsync( + Task ForwardFlowMatchesSourceDiffAsync( string sourceRepoUri, string vmrUri, string mappingName, @@ -58,7 +58,7 @@ public CodeflowSourceDiffVerifier( _logger = logger; } - public async Task VerifyForwardFlowAsync( + public async Task ForwardFlowMatchesSourceDiffAsync( string sourceRepoUri, string vmrUri, string mappingName, diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 127028ebee..3a2e1c0759 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -697,7 +697,9 @@ await _subscriptionEventRecorder.AddDependencyFlowEventsAsync( inProgressPr.LastUpdate = DateTime.UtcNow; await _stateManager.SetCheckReminderAsync(inProgressPr, pr, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); - if (!skipCodeflowApprovalCheck && !string.IsNullOrEmpty(previousSourceSha)) + if (!skipCodeflowApprovalCheck + && !string.IsNullOrEmpty(previousSourceSha) + && subscription.IsForwardFlow()) { await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck { @@ -806,7 +808,9 @@ private async Task UpdateCodeFlowPullRequestAsync( pullRequest.BlockedFromFutureUpdates = false; // if a sub is blocked, and someone force triggers it, we can continue flowing afterwards await _stateManager.SetCheckReminderAsync(pullRequest, prInfo!, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); - if (!skipCodeflowApprovalCheck && !string.IsNullOrEmpty(previousSourceSha)) + if (!skipCodeflowApprovalCheck + && !string.IsNullOrEmpty(previousSourceSha) + && subscription.IsForwardFlow()) { await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck { @@ -850,7 +854,21 @@ public async Task RunCodeflowApprovalCheckAsync( return; } - if (await _codeflowSourceDiffVerifier.VerifyForwardFlowAsync( + var commits = await remote.GetPullRequestCommitsAsync(pr.Url); + var nonBotCommits = commits + .Where(commit => !string.Equals(commit.Author, Constants.DarcBotName, StringComparison.Ordinal)) + .ToList(); + if (nonBotCommits.Count > 0) + { + _logger.LogInformation( + "Skipping codeflow approval check for PR {prUrl} because it contains {count} commit(s) not authored by {bot}", + pr.Url, + nonBotCommits.Count, + Constants.DarcBotName); + return; + } + + if (await _codeflowSourceDiffVerifier.ForwardFlowMatchesSourceDiffAsync( subscription.SourceRepository, subscription.TargetRepository, subscription.TargetDirectory, @@ -860,8 +878,17 @@ public async Task RunCodeflowApprovalCheckAsync( pr.HeadBranch, cancellationToken)) { + _logger.LogInformation( + "Codeflow approval check for PR {prUrl} passed; posting approval comment", + pr.Url); await remote.CommentPullRequestAsync(pr.Url, "Looks good"); - } + } + else + { + _logger.LogInformation( + "Forward flow PR {prUrl} contains unexpected changes, not auto approving", + pr.Url); + } } private async Task HandleBlockingCodeflowException(InProgressPullRequest pr) diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs index e370d07cc1..da6922824c 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs @@ -300,7 +300,7 @@ protected async Task VerifyForwardFlowSourceDiff(string branchName, string await cloneManager.RegisterCloneAsync(VmrPath); var verifier = scope.ServiceProvider.GetRequiredService(); - return await verifier.VerifyForwardFlowAsync( + return await verifier.ForwardFlowMatchesSourceDiffAsync( sourceRepoUri: ProductRepoPath, vmrUri: VmrPath, mappingName: Constants.ProductRepoName, diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs index 4f83d9748a..e81af8dccf 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs @@ -16,7 +16,7 @@ namespace Microsoft.DotNet.DarcLib.Codeflow.Tests; /// -/// Unit tests for . The git/VMR +/// Unit tests for . The git/VMR /// interactions are mocked so each test drives the public verifier with hand-crafted /// "source repo" and "VMR PR" diffs and asserts the verdict. /// @@ -92,7 +92,7 @@ public void SetUp() NullLogger.Instance); } - private Task VerifyAsync() => _verifier.VerifyForwardFlowAsync( + private Task VerifyAsync() => _verifier.ForwardFlowMatchesSourceDiffAsync( SourceRepoUri, VmrUri, MappingName, @@ -161,7 +161,7 @@ private static string TwoHunkDiff( ]); [Test] - public async Task VerifyForwardFlowAsync_PrMatchesSourceDiff_ReturnsTrue() + public async Task ForwardFlowMatchesSourceDiffAsync_PrMatchesSourceDiff_ReturnsTrue() { // Arrange: the source repo and the VMR PR change the same file in the same way. SetupDiffs(_sourceRepo, "data.txt", new Dictionary @@ -181,7 +181,7 @@ public async Task VerifyForwardFlowAsync_PrMatchesSourceDiff_ReturnsTrue() } [Test] - public async Task VerifyForwardFlowAsync_PrChangesFileNotInSourceDiff_ReturnsFalse() + public async Task ForwardFlowMatchesSourceDiffAsync_PrChangesFileNotInSourceDiff_ReturnsFalse() { // Arrange: the PR touches an extra file the source diff never changed. SetupDiffs(_sourceRepo, "data.txt", new Dictionary @@ -198,7 +198,7 @@ public async Task VerifyForwardFlowAsync_PrChangesFileNotInSourceDiff_ReturnsFal } [Test] - public async Task VerifyForwardFlowAsync_PrChangesFileDifferently_ReturnsFalse() + public async Task ForwardFlowMatchesSourceDiffAsync_PrChangesFileDifferently_ReturnsFalse() { // Arrange: the same file is changed in both, but to different content. SetupDiffs(_sourceRepo, "data.txt", new Dictionary @@ -218,7 +218,7 @@ public async Task VerifyForwardFlowAsync_PrChangesFileDifferently_ReturnsFalse() } [Test] - public async Task VerifyForwardFlowAsync_SourceOnlyChangeAlreadyReconciledInVmr_ReturnsTrue() + public async Task ForwardFlowMatchesSourceDiffAsync_SourceOnlyChangeAlreadyReconciledInVmr_ReturnsTrue() { // Arrange: the source changed two files but the PR only changed one. The other (readme.md) is a // legitimate no-op because the VMR copy already holds the source's new content. @@ -242,7 +242,7 @@ public async Task VerifyForwardFlowAsync_SourceOnlyChangeAlreadyReconciledInVmr_ } [Test] - public async Task VerifyForwardFlowAsync_PrTampersWithDashDashCommentLine_ReturnsFalse() + public async Task ForwardFlowMatchesSourceDiffAsync_PrTampersWithDashDashCommentLine_ReturnsFalse() { // Arrange: a SQL file whose comment line is changed. // Source repo: "-- old" -> "-- new" @@ -268,7 +268,7 @@ public async Task VerifyForwardFlowAsync_PrTampersWithDashDashCommentLine_Return } [Test] - public async Task VerifyForwardFlowAsync_MultiHunkFileWithMatchingChanges_ReturnsTrue() + public async Task ForwardFlowMatchesSourceDiffAsync_MultiHunkFileWithMatchingChanges_ReturnsTrue() { // Arrange: a file changed in two separate places (two hunks) identically in source and PR. SetupDiffs(_sourceRepo, "data.txt", new Dictionary From c7b447d2868f96067dd081cb6e4ed94fa58a5d0e Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Fri, 26 Jun 2026 09:21:42 +0200 Subject: [PATCH 14/16] cr changes --- .../CodeflowSourceDiffVerifier.cs | 19 +++++++++--------- .../PullRequestStateManager.cs | 2 +- .../CodeFlowPullRequestUpdater.cs | 20 +++++++++++++++---- .../WorkItems/CodeflowApprovalCheck.cs | 1 + .../PullRequestUpdaterTests.cs | 6 +++++- .../UpdateAssetsForCodeFlowTests.cs | 6 +++--- 6 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs index 5838907d66..97c8d9d771 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -101,22 +101,21 @@ public async Task ForwardFlowMatchesSourceDiffAsync( HashSet vmrPrChangedFiles = await GetChangedMappingFilesAsync( vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); - var intersection = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList(); + var filesChangedInBoth = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList(); var sourceRepoOnlyChanges = sourceRepoChangedFiles.Where(f => !vmrPrChangedFiles.Contains(f)).ToList(); - var unexpectedFiles = vmrPrChangedFiles.Where(f => !sourceRepoChangedFiles.Contains(f)).ToList(); + var filesChangedInPrOnly = vmrPrChangedFiles.Where(f => !sourceRepoChangedFiles.Contains(f)).ToList(); - // The PR changed files that the source diff didn't - the codeflow can't be trusted. - if (unexpectedFiles.Count > 0) + if (filesChangedInPrOnly.Count > 0) { _logger.LogInformation( "Source diff verification for {mappingName} failed: {unexpected} file(s) changed in the PR but not in the source diff", mappingName, - unexpectedFiles.Count); + filesChangedInPrOnly.Count); return false; } // Per-file content compare on the intersection. - foreach (var file in intersection) + foreach (var file in filesChangedInBoth) { if (!await ChangedLinesMatchAsync(sourceRepo, vmr, file, srcMappingPath, oldSha, newSha, vmrTargetBranch, vmrHeadBranch, cancellationToken)) { @@ -213,7 +212,7 @@ private static async Task ChangedLinesMatchAsync( ILocalGitRepo sourceRepo, ILocalGitRepo vmr, string file, - string srcMappingPath, + UnixPath srcMappingPath, string oldSha, string newSha, string vmrTargetBranch, @@ -223,7 +222,7 @@ private static async Task ChangedLinesMatchAsync( var sourceResult = await sourceRepo.ExecuteGitCommand(["diff", "-U0", $"{oldSha}...{newSha}", "--", file], cancellationToken); sourceResult.ThrowIfFailed($"Failed to get the source diff of {file} between {oldSha} and {newSha}"); - var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", $"{srcMappingPath}/{file}"], cancellationToken); + var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", srcMappingPath / file], cancellationToken); vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); var sourceChanges = GetChangeLines(sourceResult.GetOutputLines()); @@ -267,12 +266,12 @@ private static async Task IsLegitimateNoOpAsync( ILocalGitRepo sourceRepo, ILocalGitRepo vmr, string file, - string srcMappingPath, + UnixPath srcMappingPath, string newSha, string vmrHeadBranch) { var sourceContent = await sourceRepo.GetFileFromGitAsync(file, newSha); - var vmrContent = await vmr.GetFileFromGitAsync($"{srcMappingPath}/{file}", vmrHeadBranch); + var vmrContent = await vmr.GetFileFromGitAsync(srcMappingPath / file, vmrHeadBranch); if (sourceContent == null && vmrContent == null) { diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs index 5b72c63fb9..d7497af100 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs @@ -111,5 +111,5 @@ public async Task ScheduleUpdateForLater(InProgressPullRequest pr, SubscriptionU } public async Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check) => - await _codeflowApprovalCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, true); + await _codeflowApprovalCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, isCodeFlow: true); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 3a2e1c0759..ed0b75bcaa 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -706,7 +706,8 @@ await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck UpdaterId = _target.UpdaterId, SubscriptionId = update.SubscriptionId, PreviousSourceSha = previousSourceSha, - CurrentSourceSha = update.SourceSha + CurrentSourceSha = update.SourceSha, + PullRequestUrl = pr.Url }); } _outcomeRecorder.SetPullRequestUrl(inProgressPr.Url); @@ -817,7 +818,8 @@ await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck UpdaterId = _target.UpdaterId, SubscriptionId = update.SubscriptionId, PreviousSourceSha = previousSourceSha, - CurrentSourceSha = update.SourceSha + CurrentSourceSha = update.SourceSha, + PullRequestUrl = pullRequest.Url }); } } @@ -842,6 +844,15 @@ public async Task RunCodeflowApprovalCheckAsync( return; } + if (pr.Url != codeflowApprovalCheck.PullRequestUrl) + { + _logger.LogInformation( + "Codeflow approval check PR URL {codeflowPrUrl} does not match in-progress PR URL {inProgressPrUrl}", + codeflowApprovalCheck.PullRequestUrl, + pr.Url); + return; + } + var remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var prInfo = await remote.GetPullRequestAsync(pr.Url); @@ -868,7 +879,7 @@ public async Task RunCodeflowApprovalCheckAsync( return; } - if (await _codeflowSourceDiffVerifier.ForwardFlowMatchesSourceDiffAsync( + bool match = await _codeflowSourceDiffVerifier.ForwardFlowMatchesSourceDiffAsync( subscription.SourceRepository, subscription.TargetRepository, subscription.TargetDirectory, @@ -876,7 +887,8 @@ public async Task RunCodeflowApprovalCheckAsync( codeflowApprovalCheck.CurrentSourceSha, subscription.TargetBranch, pr.HeadBranch, - cancellationToken)) + cancellationToken); + if (match) { _logger.LogInformation( "Codeflow approval check for PR {prUrl} passed; posting approval comment", diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs index 1f5b8f919d..923124c9ec 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs @@ -6,6 +6,7 @@ namespace ProductConstructionService.DependencyFlow.WorkItems; internal class CodeflowApprovalCheck : DependencyFlowWorkItem { public required Guid SubscriptionId { get; init; } + public required string PullRequestUrl { get; init; } public required string PreviousSourceSha { get; init; } public required string CurrentSourceSha { get; init; } } diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index 6a754eee49..b58a2616f3 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -681,7 +681,10 @@ protected void AndShouldNotHavePullRequestCheckReminder() RemoveExpectedReminder(Subscription); } - protected void AndShouldHaveCodeflowApprovalCheckReminder(string previousSourceSha, string currentSourceSha) + protected void AndShouldHaveCodeflowApprovalCheckReminder( + string previousSourceSha, + string currentSourceSha, + string pullRequestUrl) { SetExpectedReminder(Subscription, new CodeflowApprovalCheck() { @@ -689,6 +692,7 @@ protected void AndShouldHaveCodeflowApprovalCheckReminder(string previousSourceS SubscriptionId = Subscription.Id, PreviousSourceSha = previousSourceSha, CurrentSourceSha = currentSourceSha, + PullRequestUrl = pullRequestUrl }); } diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 0e54192a19..a06176c445 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -268,7 +268,7 @@ public async Task UpdateWithManuallyMergedPrAndNewBuild() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -355,7 +355,7 @@ public async Task UpdateWithManuallyMergedPrAndLatestFlowNotApplied() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowApprovalCheckReminder(build1.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build1.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -524,7 +524,7 @@ public async Task UpdateWithExistingOppositeSubscription() }; AndShouldHavePullRequestCheckReminder(); - AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } From 16438882ba4dc288330766a1807aeb4c9c85c17b Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Fri, 26 Jun 2026 09:26:09 +0200 Subject: [PATCH 15/16] Add distributed redis lock around codeflow approval task --- .../CodeflowApprovalCheckProcessor.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs index ac4ff0aa7e..95954611b4 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs @@ -3,6 +3,7 @@ using Maestro.Data; using Maestro.DataProviders; +using Maestro.Services.Common.Cache; using Maestro.WorkItems; using Microsoft.EntityFrameworkCore; using ProductConstructionService.DependencyFlow.Model; @@ -15,13 +16,16 @@ internal class CodeflowApprovalCheckProcessor : WorkItemProcessor ProcessWorkItemAsync(CodeflowApprovalCheck workItem, CancellationToken cancellationToken) @@ -43,10 +47,14 @@ public override async Task ProcessWorkItemAsync(CodeflowApprovalCheck work throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow approval check"); } - await codeFlowUpdater.RunCodeflowApprovalCheckAsync( - SqlBarClient.ToClientModelSubscription(subscription), - workItem, - cancellationToken); + var mutexKey = pullRequestUpdaterId.Id; + + await _distributedLock.ExecuteWithLockAsync(mutexKey, + async () => await codeFlowUpdater.RunCodeflowApprovalCheckAsync( + SqlBarClient.ToClientModelSubscription(subscription), + workItem, + cancellationToken), + cancellationToken: cancellationToken); return true; } From 1aa40d985941e30362316771064b01c13db9aae4 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa Date: Fri, 26 Jun 2026 11:41:51 +0200 Subject: [PATCH 16/16] Improve message --- .../PullRequestUpdaters/CodeFlowPullRequestUpdater.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index ed0b75bcaa..0555410552 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -893,7 +893,10 @@ public async Task RunCodeflowApprovalCheckAsync( _logger.LogInformation( "Codeflow approval check for PR {prUrl} passed; posting approval comment", pr.Url); - await remote.CommentPullRequestAsync(pr.Url, "Looks good"); + await remote.CommentPullRequestAsync( + pr.Url, + $"This pull request only contains source updates from {subscription.SourceRepository} " + + $"between commits {codeflowApprovalCheck.PreviousSourceSha} and {codeflowApprovalCheck.CurrentSourceSha}."); } else {