Add producer-trace, over-restriction & coverage-adequacy review CHECKs (retro from #36185) - #36210
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36210Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36210" |
Skill Validation Results
✅ Skill Validation Results —
|
| Suite | Score | Threshold | Verdict |
|---|---|---|---|
| code-review-capabilities | 0.96 | 0.60 | ✅ |
| code-review-inline-findings-write | 1.00 | 0.70 | ✅ |
| code-review-producer-trace | 1.00 | 0.85 | ✅ |
| code-review-trim-aot | 0.94 | 0.90 | ✅ |
| code-review-regressions | 0.88 | 0.60 | ✅ |
Harness hermeticity (negative control)
✅ Hermetic — the negative-control stimulus correctly came back unauthenticated (anonymous core rate limit; no GitHub token leaked into the agent env).
📊 code-review — eval report
Eval Results
Timestamp: 2026-07-22T03:23:57.176Z
code-review-capabilities [claude-opus-4.6] (/home/runner/work/maui/maui/.github/skills/code-review/tests/eval.capability.vally.yaml)
Capability suite for the code-review skill — behaviorial properties measured against real PRs (tool-call ordering, structured-output shape, API-misuse refusal, blast-radius reasoning, prior-review surfacing, CI-status interpretation).
| Stimulus | Skills | Graders | Pass Rate | pass@k | pass^k | Duration (median) | Tokens (median) | Turns (median) | Tool Calls (median) | Verdict |
|---|---|---|---|---|---|---|---|---|---|---|
| anti-pattern-never-approve-via-api | — | ✅ output-not-contains 3/3 ✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 2m 39s | 1,129,043 | 28 | 28 calls (median)total across 3 trials: bash: 50, web_fetch: 22</details> |
✅ 1 |
| blast-radius-infra-changes-get-probed | — | ✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 1m 53s | 408,021 | 16 | 18 calls (median)total across 3 trials: bash: 31, web_fetch: 17</details> |
✅ |
| ci-hard-gate-skipping-required-checks-block-lgtm | — | ✅ output-not-contains 3/3 ✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 1m 43s | 647,606 | 15 | 16 calls (median)total across 3 trials: web_fetch: 37, bash: 8</details> |
✅ |
| happy-path-code-review-request | code-review (3×) |
✅ output-matches 3/3 ✅ output-not-contains 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 2m 09s | 536,025 | 13 | 14 calls (median)total across 3 trials: web_fetch: 27, bash: 12, skill: 3</details> |
✅ |
| independence-first-diff-before-description | code-review (3×) |
✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 2m 38s | 1,013,270 | 25 | 26 calls (median)total across 3 trials: bash: 34, web_fetch: 31, view: 7, skill: 3, glob: 1</details> |
✅ |
| negative-trigger-describe-changes-query | — | ✅ output-not-contains 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 27.9s | 87,488 | 4 | 3 calls (median)total across 3 trials: web_fetch: 6, bash: 3</details> |
✅ |
| negative-trigger-informational-query | — | ✅ output-not-contains 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 26.7s | 86,522 | 4 | 3 calls (median)total across 3 trials: web_fetch: 6, bash: 3</details> |
✅ |
| prior-review-reconciliation-surfaces-prior-findings | code-review (3×) |
✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 3m 18s | 1,328,713 | 25 | 30 calls (median)total across 3 trials: bash: 77, web_fetch: 20, skill: 3, view: 1</details> |
✅ |
| verdict-consistency-errors-block-lgtm | code-review (3×) |
✅ output-not-contains 3/3 ✅ output-matches 3/3 ✅ prompt 3/3 |
3/3 | 100.0% | 100.0% | 1m 46s | 474,416 | 11 | 12 calls (median)total across 3 trials: web_fetch: 23, bash: 9, skill: 3</details> |
✅ |
Model: claude-opus-4.6 | Judge: claude-opus-4.6 | Executor: copilot-sdk
code-review-inline-findings-write [claude-opus-4.6] (/home/runner/work/maui/maui/.github/skills/code-review/tests/eval.inline-findings.vally.yaml)
Regression guard for the expert-review phase: after producing file:line findings on a diff, the agent must WRITE them as a JSON array to the inline-findings.json path the pipeline reads from disk — it must NOT declare the write prohibited or substitute a chat-only dump. Missing the file silently drops every inline comment.
| Stimulus | Environment | Graders | Pass Rate | pass@k | pass^k | Duration (median) | Tokens (median) | Turns (median) | Tool Calls (median) | Verdict |
|---|---|---|---|---|---|---|---|---|---|---|
| regression-writes-inline-findings-to-disk | git: Git: worktree @ |
✅ output-matches 5/5 ✅ inline-findings-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 28s | 359,134 | 12 | 16 calls (median)total across 5 trials: bash: 78, view: 4, create: 2</details> |
✅ |
Model: claude-opus-4.6 | Judge: claude-opus-4.6 | Executor: copilot-sdk
code-review-producer-trace [claude-opus-4.6] (/home/runner/work/maui/maui/.github/skills/code-review/tests/eval.producer-trace.vally.yaml)
Verifies that code-review independently traces a token matched from external output to its out-of-diff producer and catches an over-restrictive gate.
| Stimulus | Environment | Skills | Graders | Pass Rate | pass@k | pass^k | Duration (median) | Tokens (median) | Turns (median) | Tool Calls (median) | Verdict |
|---|---|---|---|---|---|---|---|---|---|---|---|
| producer-trace-console-token-over-cap | 3 files · 1 skillFiles:fixtures/producer-trace/change.diff → review-input/change.diff, fixtures/producer-trace/src/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1 → review-input/src/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1, fixtures/producer-trace/src/eng/devices/run-windows-devicetests.cmd → review-input/src/eng/devices/run-windows-devicetests.cmdSkills: ..</details> |
code-review (5×) |
✅ output-matches 5/5 ✅ tool-calls 5/5 ✅ producer-trace-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 35s | 239,526 | 9 | 10 calls (median)total across 5 trials: bash: 21, view: 19, skill: 5, glob: 5</details> |
✅ |
Model: claude-opus-4.6 | Judge: claude-opus-4.6 | Executor: copilot-sdk
code-review-trim-aot [claude-opus-4.6] (/home/runner/work/maui/maui/.github/skills/code-review/tests/eval.trim-aot.vally.yaml)
Hermetic capability suite for code-review Trim and NativeAOT guidance. The reviewer must trace annotation chains and distinguish a documented analyzer limitation from a reachable dynamic-code path.
Environment: 3 files
| Stimulus | Environment | Graders | Pass Rate | pass@k | pass^k | Duration (median) | Tokens (median) | Turns (median) | Tool Calls (median) | Verdict |
|---|---|---|---|---|---|---|---|---|---|---|
| annotated-helper-structurally-isolates-dynamic-registration | 5 filesFiles:fixtures/trim-aot/shared/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs → review-input/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs, fixtures/trim-aot/shared/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs → review-input/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs, fixtures/trim-aot/shared/src/Core/src/RuntimeFeature.cs → review-input/src/Core/src/RuntimeFeature.cs, fixtures/trim-aot/case-b/change.diff → review-input/change.diff, fixtures/trim-aot/case-b/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs → review-input/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs</details> |
✅ output-matches 5/5 ✅ tool-calls 5/5 ✅ annotated-helper-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 08s | 136,195 | 6 | 9 calls (median)total across 5 trials: bash: 24, view: 18</details> |
✅ |
| reachable-suppression-must-be-rejected | 5 filesFiles:fixtures/trim-aot/shared/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs → review-input/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs, fixtures/trim-aot/shared/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs → review-input/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs, fixtures/trim-aot/shared/src/Core/src/RuntimeFeature.cs → review-input/src/Core/src/RuntimeFeature.cs, fixtures/trim-aot/case-c/change.diff → review-input/change.diff, fixtures/trim-aot/case-c/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs → review-input/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs</details> |
✅ output-matches 5/5 ✅ tool-calls 5/5 ✅ reachable-suppression-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 10s | 137,046 | 6 | 10 calls (median)total across 5 trials: bash: 27, view: 21</details> |
✅ |
| scoped-suppression-requires-annotation-chain-proof | 5 filesFiles:fixtures/trim-aot/shared/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs → review-input/src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs, fixtures/trim-aot/shared/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs → review-input/src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs, fixtures/trim-aot/shared/src/Core/src/RuntimeFeature.cs → review-input/src/Core/src/RuntimeFeature.cs, fixtures/trim-aot/case-a/change.diff → review-input/change.diff, fixtures/trim-aot/case-a/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs → review-input/src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs</details> |
✅ output-matches 5/5 ❌ tool-calls 3/5 ✅ scoped-suppression-judge 5/5 |
3/5 | 100.0% | 7.8% | 1m 00s | 135,090 | 6 | 8 calls (median)total across 5 trials: bash: 22, view: 20</details> |
🟡 2 |
Model: claude-opus-4.6 | Judge: claude-opus-4.6 | Executor: copilot-sdk
code-review-regressions [claude-opus-4.6] (/home/runner/work/maui/maui/.github/skills/code-review/tests/eval.vally.yaml)
Regression-detection corpus for the code-review skill. Each stimulus presents the diff of a PR that was later confirmed to have introduced a real, p/0-class regression in a shipping MAUI release. The eval asserts the reviewer would have surfaced the regression risk had they reviewed the PR pre-merge.
| Stimulus | Environment | Graders | Pass Rate | pass@k | pass^k | Duration (median) | Tokens (median) | Turns (median) | Tool Calls (median) | Verdict |
|---|---|---|---|---|---|---|---|---|---|---|
| gradient-alpha-forced-opaque | git: Git: worktree @ |
✅ output-matches 5/5 ✅ regression-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 42s | 404,598 | 15 | 20 calls (median)total across 5 trials: bash: 85, view: 21, grep: 4, glob: 2</details> |
✅ |
| native-collection-null-overlays | git: Git: worktree @ |
✅ output-matches 5/5 ✅ regression-judge 5/5 |
5/5 | 100.0% | 100.0% | 1m 24s | 231,490 | 10 | 13 calls (median)total across 5 trials: bash: 49, view: 12, glob: 4</details> |
✅ 3 |
Model: claude-opus-4.6 | Judge: claude-opus-4.6 | Executor: copilot-sdk
Footnotes
There was a problem hiding this comment.
Pull request overview
This PR strengthens the repo’s agentic PR-review guidance and regression evaluation corpus to prevent “producer-trace” misses where a regex/string is validated only against the consumer (parser) and not the out-of-diff producer (scripts/tools emitting the token).
Changes:
- Extend the always-active
[critical]review CHECKs to require producer-tracing of external-tool output tokens and to treat over-restriction as a correctness risk (not “safe by fail-closed”). - Add a hermetic
vallyevaluation stimulus pinned to the introducing commit of the #36185 over-cap bug, asserting the reviewer must trace the token to its unconditional producer and flag the regression.
Show a summary per file
| File | Description |
|---|---|
.github/agents/maui-expert-reviewer.md |
Adds producer-trace, over-restriction, and coverage-adequacy CHECKs to the always-on critical dimensions. |
.github/skills/code-review/tests/eval.vally.yaml |
Adds a regression scenario ensuring reviewers must trace console tokens back to their out-of-diff producer and avoid “fail-closed so it’s fine” reasoning. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
github-actions
Bot
added
the
area-infrastructure
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — multi-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), independent reviews + cross-pollination.
✅ LGTM — unanimous (high confidence)
A genuinely valuable retro. It codifies the exact lesson from #36185 — where GitHub's Copilot reviewer caught an over-cap bug (exit code:[1-9] keyed as "incomplete" when the Windows runner prints that line unconditionally) that an adversarial multi-model panel missed by not tracing the matched console string to its producer. The three new CHECKs (producer-trace, over-restriction, coverage-adequacy) directly target that class of miss.
I verified the eval scenario is factually accurate (and all three models independently confirmed):
- Commit
90edd581introduces the over-match (test execution completed with exit code:\s*[1-9]→isIncomplete→ always-emitted capping record). - The out-of-diff producer
eng/devices/run-windows-devicetests.cmd:481echoes that line unconditionally at its:uploadtail withEXIT_CODE=1on any failure. - The later fix commit
6efaf3bab0confirms the diagnosis (splits the reason-regex from a narrower incomplete-regex, citingcmd:481) — which is exactly why the scenario correctly pins to the introducing commit, not the merge. - The 3 CHECKs land in the right always-active
[critical]dimensions (5 Logic & Correctness, 6 Regression Prevention) — appropriate, since #36185 was a non-C# tooling PR that triggers no routed dimensions. - No conflict with #36211: this PR only appends CHECK bullets to existing dimensions and doesn't touch the
30→31count, Dimension 31, or the routing table.
| Model | Verdict | Confidence |
|---|---|---|
| Claude Opus 4.8 | LGTM | high |
| GPT-5.5 | LGTM | high |
| Gemini 3.1 Pro | LGTM | high |
💡 Non-blocking (watch, don't block)
- The over-restriction CHECK ("a guard made more restrictive") describes a very common PR shape; placed in an always-active
[critical]dimension it could prompt scrutiny on many PRs. The verify-then-flag framing ("name the previously-passing input it now rejects and confirm the rejection is intended") keeps false-findings low — but worth watching corpus eval pass-rates/latency for over-flagging. - The producer-trace "file-format text" clause is broad (XML/JSON/csproj parsing is common in build tasks); naturally limited to tooling/build PRs, but tighten if eval noise appears.
- The fixture pins to an intermediate (fetch-by-SHA) commit
90edd581— the established corpus pattern (the other scenarios are also off-main), correctly justified here; consider documenting thegit fetch <sha>retrieval requirement so a future ref-GC is diagnosable.
Reviewed at head bdc1b390. CI: license/cla ✅ pass; maui-pr skipping (by-design .github/** path-exclusion). Eval/prompt change — reviewed for correctness + factual accuracy of the eval writeup. (Full disclosure: this PR is a retro on a miss by this very review system — it's a good fix and I'm glad to see it.)
|
/evaluate-skills |
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
|
/evaluate-skills |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
LGTM — re-approving after 12b97b83f6fe ("Invoke code review in producer eval"), which dismissed my prior approval. Clean eval-fixture tightening that completes the External-Output-Contract arc:
- Prompt now explicitly "Invoke the
code-reviewskill to review a candidate patch" — the eval exercises the real skill path (including the new Step 1.5) rather than a generic review prompt. - Structural grader tightened from
(❌|⚠️|🔴|NEEDS_CHANGES|NEEDS_DISCUSSION)toExternal Output Contract[\s\S]*(NEEDS_CHANGES|NEEDS_DISCUSSION)— now requires the actual External Output Contract artifact to appear before a non-LGTM verdict. Far more decisive and producer-coupled than matching any warning emoji; a producer-blind review that skips the contract table no longer earns the structural point. - Scoring comment updated consistently (
missing the contract + non-LGTM verdict … caps at 2/3).
Pure eval-fixture (eval.producer-trace.vally.yaml) — zero framework/runtime blast radius.
CI: required checks clean (license/cla pass, maui-pr skipping); evaluate (code-review) continues to hang (~2h, same transient LLM-infra, non-required). No live unresolved threads.
Add a candidate-patch input mode to code-review and invoke it explicitly from the producer-trace eval so the harness executes the changed reviewer instructions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4ad7de2c-4346-4c93-8823-8477b8763107
|
/evaluate-skills |
GitHub Actions
Inactive
GitHub Actions
Inactive
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Multi-model consensus: LGTM (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro — independent → cross-pollinated). Re-review of new commit 087bc111 "Support materialized code reviews" on top of the previously-approved r8.
What changed (vs r8 12b97b83): pure documentation + eval-harness enhancement, zero runtime blast radius.
SKILL.md(+15-3): formalizes a second input mode —pr_number(live PR) orreview_input(materialized candidate diff + supporting source snapshots), "exactly one required". Documents the fixture-consumption contract for thereview_inputpath: readchange.difffirst, read supporting source in full, trace callers/consumers/producers in the snapshot, don't fetch PR narrative/history the fixture doesn't provide, then continue at Step 1.5. Consistent with the hermetic-fixture pattern the eval already uses.eval.producer-trace.vally.yaml(+5-4): switches the eval stimulus from prose "Invoke thecode-reviewskill…" to the canonical/code-reviewslash-invocation — more deterministic skill dispatch. Rest of the prompt (materialize underreview-input, inspectchange.difffirst, readsrcsnapshots) preserved.
CI: license/cla pass; maui-pr skipping (correct — the PR touches only .github/skills/ markdown + eval config, nothing buildable). Consistent with rounds 6-8.
Open threads: the 4 unresolved copilot-pull-request-reviewer threads are all on files under tests/fixtures/producer-trace/src/… — i.e. the intentionally-materialized producer snapshots that are this eval's input (the code the skill is meant to flag), not part of the PR's own contribution. They're false-positives w.r.t. this PR and non-blocking. PureWeen has no open threads of his own.
No ❌/
Use Vally environment.skills to load only code-review for the producer-trace stimulus, eliminating nondeterministic selection among all repository skills. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4ad7de2c-4346-4c93-8823-8477b8763107
|
/evaluate-skills |
GitHub Actions
Inactive
GitHub Actions
Inactive
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Multi-model consensus: LGTM (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro). Re-review of new commit 34c6ef7c "Isolate producer eval skill".
What changed (vs r9 087bc111): +4 lines to eval.producer-trace.vally.yaml only — adds environment.skills: [..] to the producer-trace stimulus so the eval loads only the code-review skill (.. resolves to the skill root, per the fixture-relative-path convention) instead of every repo skill via --skill-dir. As the inline comment notes, this prevents Copilot from answering without activating the unit under test — a determinism/isolation fix for the eval harness. Zero runtime blast radius.
CI: license/cla pass; maui-pr skipping (eval-config only). Threads: unchanged — the 4 open copilot-pull-request-reviewer threads are all on the materialized producer-trace fixtures (eval input, not this PR's contribution); non-blocking. No author threads.
No ❌/
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Problem
A retro on #36185 found a reviewer-recall gap. GitHub's built-in Copilot reviewer caught a real bug at
Gather-TestFailureContext.ps1:928, while a multi-model adversarial panel returned LGTM.The bug required correlating an in-diff regex with an out-of-diff producer:
run-windows-devicetests.cmdunconditionally emitted an exit-code line that the parser treated as proof of incompleteness. Every ordinary failed Windows work item was therefore over-capped as incomplete. The behavior remained fail-closed, but defeated attribution of cleanly completed named failures.Changes
Canonical MAUI reviewer rules —
.github/agents/maui-expert-reviewer.mdadds always-active critical CHECKs that require reviewers to:Direct code-review execution —
.github/skills/code-review/SKILL.mdnow supports either a livepr_numberor a hermetic materializedreview_input. When external output is classified, it requires an External Output Contract table covering the consumer token, producer location and emission condition, consumer assumption, ordinary negative case, and downstream effect. This is the fallback for environments that cannot delegate tomaui-expert-reviewer.Hermetic regression eval —
eval.producer-trace.vally.yamlandtests/fixtures/producer-trace/:[FAIL]branch cannot satisfy the scenario through a different valid finding.environment.skillsto load only thecode-reviewskill; baseline and after runs therefore exercise their respectiveSKILL.mdinstead of nondeterministically selecting among every repository skill.0.85suite threshold.The fixture source files are intentionally faulty, inert review inputs; they are not executed or shipped product code.
Validation