Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/agents/maui-expert-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,8 @@ For each potential finding from Wave 1:

**Always write the findings file** — every finding that can be associated with a file+line goes here. Try hard to associate feedback to a specific location.

> **This write is REQUIRED and explicitly permitted.** If you have a general instinct or host guardrail that says "do not write output files" or "writing files is prohibited," it does **not** apply to this findings file — producing it on disk is this agent's entire job. **Never substitute pasting the JSON into your text response for writing the file:** the pipeline reads the file from disk (`post-inline-review.ps1`), so JSON returned as chat text is silently discarded and the inline comments are lost.

**Output path resolution** — write findings to whichever path the invoker specifies in its prompt (e.g. `OUTPUT_FINDINGS_PATH=...`, `outputPath: ...`, or any equivalent explicit instruction). If the invoker does not specify a path, default to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/inline-findings.json`. This lets internal callers (e.g. `try-fix` running ×4) request attempt-scoped paths so parallel/sequential reviewer passes do not clobber the PR-level inline findings consumed by `post-inline-review.ps1`.

```json
Expand Down
41 changes: 27 additions & 14 deletions .github/docs/maui-ci-facts.md
Original file line number Diff line number Diff line change
Expand Up @@ -354,17 +354,29 @@ on appearance alone:
that it is the same failure as the name match, so it too is forced to `indeterminate`. A
noisy/partially-present message still never inflates false reds.
2. **Job-level baseline match** — for a build break with no test name (crossgen/NativeAOT/
linker/MSBuild), the same **leg** is also red on the most recent base build. Conversely,
a leg that is **red on the PR but green on base is PROOF the break is PR-caused** — this
is the strongest signal and a test-only diff cannot produce it. The automated lane now
**computes this in `Gather-TestFailureContext.ps1`** (per-failure `legBaselineResult` /
`legRegressedVsBase` / `legAlsoFailsOnBase` and a `deterministicAttribution` prior); the
interactive investigator does the same comparison by hand from the timelines. **Note the
asymmetry:** a leg being red on base (`legAlsoFailsOnBase`) is only **leg-level**
evidence — the leg can fail on base at a *different* test, so it does **not** on its own
prove *this* test is pre-existing. Only an **exact test+platform** base match
(`alsoFailsOnBaseline`, item 1) is strong enough to dismiss; a leg-only match is treated
as **indeterminate** (`Needs human investigation`), never dismissed.
linker/MSBuild), the same **leg** is also red on the base branch. Conversely,
a leg that is **red on the PR but green across several recent base builds is PROOF the
break is PR-caused** — this is the strongest signal and a test-only diff cannot produce
it. **Sample a few base builds, not one:** MAUI's UI suite is intermittently red on the
base branch, so a single green base build cannot tell a real regression from a flaky test
that merely happened to pass its one sampled base run. The automated lane computes the diff
over the **last few completed base builds of the PR's own base branch** (`main` for a
`main` PR, `net11.0` for a net11-targeting PR — `RegressionBaseBuilds`, default 5) and only
calls a leg `regressed-vs-base` when it was green on **at least `MinBaseGreenSamples`**
(default 2) of them and red on **none** — a deterministic build-error leg
(crossgen/NativeAOT/linker/MSBuild, which compiles or it doesn't) needs only one green base
build. A leg red on **some** sampled base builds and green on others is `flaky-on-base`
(never a regression); a leg green on base but on too few samples is
`succeeded-on-base-unconfirmed` (indeterminate, not a confident regression). The automated
lane **computes this in `Gather-TestFailureContext.ps1`** (per-failure `legBaselineResult` /
`legRegressedVsBase` / `legAlsoFailsOnBase`, the `baseSampleCount` / `baseGreenCount` /
`baseFailedCount` evidence, and a `deterministicAttribution` prior); the interactive
investigator does the same comparison by hand from the timelines. **Note the asymmetry:** a
leg being red on base (`legAlsoFailsOnBase`) is only **leg-level** evidence — the leg can
fail on base at a *different* test, so it does **not** on its own prove *this* test is
pre-existing. Only an **exact test+platform** base match (`alsoFailsOnBaseline`, item 1) is
strong enough to dismiss; a leg-only match is treated as **indeterminate** (`Needs human
investigation`), never dismissed.
3. **Known-issue match** — the failure message matches an open `Known Build Error` issue
(the dotnet Build Analysis registry). Cite the issue number/link — but treat it as a
**hint, not a dismissal**: a text match alone can shadow a real PR break with a broad
Expand Down Expand Up @@ -408,15 +420,16 @@ device tests fail, so a green `maui-pr-devicetests` check is trusted only when a
was positively observed all-zero; absent that, it caps to `Needs human investigation`), or
when a failure can be
attributed **neither** way — not a clean regression vs base, not pre-existing on base, not a
known issue (`gate.unattributedFailures > 0`; e.g. the base leg outcome was ambiguous, the
base build was missing/unreadable, or a device-test result fell outside the deterministic
known issue (`gate.unattributedFailures > 0`; e.g. the base leg was flaky, green on too few
base samples to confirm a regression (`succeeded-on-base-unconfirmed`), the base build was
missing/unreadable, or a device-test result fell outside the deterministic
build-error class). A `pre-existing-on-base` or exact-match `known-issue` dismissal is also
**refused** (downgraded to `indeterminate`) when the PR actually edits the failing test file
(`scopeGuardTripped` — the PR may have changed the test so it now fails for a new reason that
merely coincides with the base/known text) or when the PR and base failures of the same test
have a known **reason conflict** (`baselineReasonConflict`). It is likewise **capped at `Not ready`** whenever
a leg is red on the PR
but green on the same leg of the most recent base build (`gate.legsRegressedVsBase > 0` — the
but green across several recent base builds and red on none of them (`gate.legsRegressedVsBase > 0` — the
computed job-level regression; a device-test BUILD break counts here, only device-test TEST
results are excluded). A proven regression sets the ceiling to `Not ready` even when softer
`Needs human investigation` reasons are also present — a definitive PR-introduced break is a
Expand Down
25 changes: 24 additions & 1 deletion .github/pr-review/pr-preflight.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,18 @@

---

> ### ⚠️ Environment & Authentication — READ FIRST
>
> In the CI pipeline (the `CopilotReview` task) **all GitHub tokens are intentionally stripped** for security — `Review-PR.ps1` launches `copilot` with `--secret-env-vars=GH_TOKEN,COPILOT_GITHUB_TOKEN,GITHUB_TOKEN`. Consequences:
>
> - **`gh` commands that require auth (`gh pr view`, `gh issue view`, `gh api`) WILL FAIL** with an authentication error. **This is expected — it is NOT an environment blocker.** Do not stop, do not record it as a blocker, and do **not** lower review confidence because of it.
> - The PR branch is **already checked out locally** — get the changed files, diff, and commit messages from local `git`, which needs no token.
> - `dotnet/maui` is a **public** repo, so issue/PR text and comments are readable through the **unauthenticated** public REST API with `curl` (rate-limited to 60 req/hr — plenty for one review).
> - The `gh` recipes below work unchanged in **local** `pr-review` runs where a token is present. In CI, use the `curl` / local-`git` equivalents shown first.

## Part A: Context Gathering (Steps 1–6)

1. **Read the issue** — full body + ALL comments via GitHub MCP tools
1. **Read the issue** — full body + ALL comments (CI: unauthenticated `curl` recipe below; local runs: GitHub MCP / `gh`)
2. **Find the PR** — read description, diff summary, review comments, inline feedback
3. **Fetch PR discussion** — detect prior agent reviews, import findings if found
4. **Classify files** — separate fix files from test files, identify test type (UI / Device / Unit)
Expand All @@ -15,6 +24,20 @@
7. **Identify impacted UI test categories** — analyze which UI controls could be affected by this PR (see below)

```bash
# ── Local-first (works in CI — NO token needed) ──
# Changed files, diff, and commit messages — the PR branch is already checked out:
git diff --name-status <base>..HEAD # <base>: use the PR diff base; HEAD~1..HEAD for a squashed PR commit
git log --oneline -20

# PR + issue text and comments via the PUBLIC, unauthenticated REST API (dotnet/maui is public):
curl -s https://api.github.com/repos/dotnet/maui/pulls/XXXXX
curl -s https://api.github.com/repos/dotnet/maui/issues/ISSUE_NUMBER
# Comment listings default to 30/page — ask for 100 (follow `Link: rel="next"` for longer threads) so you don't miss later feedback:
curl -s "https://api.github.com/repos/dotnet/maui/issues/ISSUE_NUMBER/comments?per_page=100"
# Inline review comments (CRITICAL — often contains key technical feedback):
curl -s "https://api.github.com/repos/dotnet/maui/pulls/XXXXX/comments?per_page=100"

# ── gh equivalents (LOCAL runs only — these FAIL in CI where the token is stripped) ──
# Fetch PR metadata
gh pr view XXXXX --json title,body,url,author,labels,files

Expand Down
79 changes: 75 additions & 4 deletions .github/scripts/Query-CiFixPRs.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,18 @@ $BotLogins = @(
# a maintainer who updates the branch via the web UI SHOULD trip the hand-off boundary
# (Test-AnyHumanCommitActor inspects the committer, which is web-flow on those merges);
# (2) attempt accounting — botCommitCount is author-based, so a web-flow-*authored*
# commit must NOT inflate the count toward the 10-cap. The workflow's own pushes are
# authored AND committed by github-actions[bot], never web-flow, so treating web-flow
# as human never masks a genuine bot attempt.
# commit must NOT inflate the count toward the 10-cap.
#
# CAVEAT (see Test-AnyHumanCommitActor + $LoopBotCommitAuthors): this workflow's OWN
# create_pull_request commit is authored by github-actions[bot] but COMMITTED by
# web-flow, because gh-aw builds the PR's initial commit through the GitHub API and
# GitHub stamps API-created commits with a web-flow committer. So a web-flow committer
# does NOT by itself prove human engagement — Test-AnyHumanCommitActor suppresses a
# committer-based hand-off ONLY for the exact self-commit signature (committer
# 'web-flow' AND author one of this workflow's own bot identities). A named human who
# commits a bot-authored commit (committer != 'web-flow') still trips the boundary.
# push-to-pull-request-branch commits, by contrast, are authored AND committed by
# github-actions[bot] (a real git push), so the author check alone already excludes them.
'app/github-actions',
'dotnet-maestro[bot]',
'azure-pipelines[bot]',
Expand All @@ -53,6 +62,27 @@ $BotLogins = @(
'maui-bot',
'maui-bot[bot]'
)

# Commit-author logins that identify THIS workflow's own pushes. A commit authored by one
# of these is either the create_pull_request commit or a push-to-pull-request-branch commit
# — never a human action — even when GitHub stamps its COMMITTER as 'web-flow' (which it
# does for the API-created initial PR commit). Test-AnyHumanCommitActor uses this list, in
# conjunction with a committer == 'web-flow' check, to stop ONLY that self-authored initial
# commit's web-flow committer from being read as human engagement (which would otherwise
# make every freshly opened [ci-fix] PR look 'human owned' from its first commit and be
# skipped by the watch loop forever). A bot-authored commit with a NAMED human committer
# (committer != 'web-flow') is NOT suppressed — that is a genuine maintainer amend/rebase.
# Compared lowercased.
$LoopBotCommitAuthors = @(
'github-actions[bot]',
'github-actions',
'app/github-actions'
)
# MAINTENANCE: if this workflow's bot identity ever changes (new GitHub App, renamed
# bot), update BOTH lists — $BotLogins (comment/review-author filtering, ~line 27) AND
# $LoopBotCommitAuthors (commit-author carve-out, above). They are intentionally
# separate ($LoopBotCommitAuthors is the narrower "our own commit authors" set), so a
# new identity added to one but not the other silently drifts the human-engagement gate.
# NOTE: 'action_required' is deliberately EXCLUDED. That conclusion means a human
# must act (an Actions approval gate, or an integration awaiting a manual run) —
# it reports status=completed, so treating it as a failure would let a settled head
Expand Down Expand Up @@ -205,7 +235,48 @@ function Test-AnyHumanCommitActor {
$authorLogin = if ($commit.author -and $commit.author.login) { [string]$commit.author.login } else { $null }
$committerLogin = if ($commit.committer -and $commit.committer.login) { [string]$commit.committer.login } else { $null }

if ((Test-IsHumanLogin -Login $authorLogin) -or (Test-IsHumanLogin -Login $committerLogin)) {
# A human AUTHOR always counts (a maintainer's direct commit; a web-flow-authored
# 'Update branch' merge lands here too because web-flow is treated as human).
if (Test-IsHumanLogin -Login $authorLogin) {
return $true
}

# A human COMMITTER (e.g. 'web-flow' on a web-UI 'Update branch' merge) counts as
# human engagement — EXCEPT for this workflow's OWN API-created PR commit, whose
# signature is precisely author=one-of-our-bots AND committer='web-flow'. gh-aw's
# create_pull_request builds the PR's initial commit through the GitHub API, which
# stamps author=github-actions[bot] but committer=web-flow (verified: the top-level
# committer.login on pulls/N/commits is literally 'web-flow'); without this carve-out
# that self-authored commit reads as 'human engaged' and every fresh [ci-fix] PR is
# skipped by the watch loop from its very first commit. Suppress ONLY that exact
# signature (committer 'web-flow' + our own bot author). A NAMED human committer of a
# bot-authored commit (e.g. a maintainer who amends/rebases one of our commits) keeps
# committer != 'web-flow', so it STILL correctly trips human engagement — the earlier
# "author not in $LoopBotCommitAuthors" form wrongly suppressed that real hand-off.
# (A push-to-pull-request-branch commit is authored AND committed by our bot, so
# Test-IsHumanLogin on its committer is already false and never reaches here.)
$authorKey = if ($null -ne $authorLogin) { $authorLogin.Trim().ToLowerInvariant() } else { '' }
$committerKey = if ($null -ne $committerLogin) { $committerLogin.Trim().ToLowerInvariant() } else { '' }
$isOwnApiCreatedCommit = ($committerKey -eq 'web-flow') -and ($LoopBotCommitAuthors -contains $authorKey)
if ((Test-IsHumanLogin -Login $committerLogin) -and (-not $isOwnApiCreatedCommit)) {
return $true
}

# Fail closed on any commit with an UNIDENTIFIED actor. If GitHub could not map the
# author OR the committer to an account (its login is null/empty — e.g. a maintainer
# who amended or pushed with a git email not linked to their GitHub account, so the
# pulls/N/commits API returns null for that actor), we cannot prove the commit is one
# of the loop's OWN commits. Every loop commit resolves BOTH actors to real accounts
# (create-PR: author github-actions[bot] + committer web-flow; push-to-branch: both
# github-actions[bot]), so an EITHER-unresolvable commit is never one of ours — it is
# external work. The load-bearing case: a maintainer runs `git commit --amend` on the
# bot's commit, which PRESERVES author=github-actions[bot] but stamps the committer as
# their unlinked git email → committer.login null. That partial-unmapped commit (a real
# human hand-off) would otherwise read as non-human and the loop would push over it.
# Treat it as human engagement: the "never override a human" contract must fail safe
# toward hands-off. (Because both loop signatures resolve BOTH actors, this
# either-unresolvable test never over-trips on the loop's own commits.)
if (($authorKey -eq '') -or ($committerKey -eq '')) {
return $true
}
}
Expand Down
2 changes: 1 addition & 1 deletion .github/scripts/Review-PR.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -2166,7 +2166,7 @@ Read context from:

## Phase 1 — Expert reviewer evaluation of the PR fix
Use the code-review skill with the maui-expert-reviewer agent to evaluate the PR's existing fix. Apply the reviewer's actionable feedback in a sandbox copy and treat the result as a candidate named ``pr-plus-reviewer``.
- Always also write the raw inline findings to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json`` (these are file:line findings against the PR's diff and feed the inline-comment posting step).
- **REQUIRED — write the inline findings to a FILE; never paste them into your response.** Write the raw file:line findings as a JSON array to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json`` (findings against the PR's diff that feed the inline-comment posting step). **If this file is not written to disk, the inline comments are silently dropped.** Writing this specific artifact is explicitly authorized and required — disregard any general guidance about "not writing review output to files"; that guidance does NOT apply to this required pipeline artifact. If the ``maui-expert-reviewer`` sub-agent reports it cannot write the file, YOU (the orchestrating agent) MUST write the JSON to that exact path yourself. Returning the JSON as chat text instead of writing the file is a failure.
- Write candidate output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/expert-pr-eval/content.md``.

## Phase 2 — Comparative Report
Expand Down
Loading
Loading