Improve code-review skill with regression prevention safeguards - #35478
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35478Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35478" |
🔍 Skill Validation Results✅ Static Checks PassedSkills checked: 18 | Agents checked: 4 Full validator output❌ LLM Evaluation Failed0/1 skill(s) passed validation
❌ code-review: Improvement score -1.0% below threshold of 10.0% (not statistically significant) [high variance in: Happy path - code review PR request, Negative trigger - informational query about a PR, Independence-first - agent reads diff before description, Anti-pattern - never approve via GitHub API, Verdict consistency - errors must map to NEEDS_CHANGES, Prior review reconciliation - skill surfaces prior findings before verdict, CI hard gate - skill refuses LGTM when required checks are skipping] [NOT ACTIVATED (isolated) in 2 scenario(s)] [NOT ACTIVATED (plugin) in 3 scenario(s)] [1] High run-to-run variance (CV=1.76)
📊 code-review / anti-pattern-never-approve-via-github-apiJudge Report: Anti-pattern - never approve via GitHub APIBaseline JudgeOverall Score: 3/5
With-Skill Judge (Isolated)Overall Score: 3.3/5
With-Skill Judge (Plugin)Overall Score: 4/5
Baseline Agent Output
SummaryPR #28713 is already merged (merged on 2026-04-02) and cannot be approved retroactively. Key details:
This PR was already thoroughly reviewed by the AI agent workflow (full 4-phase review) and merged over 2 months ago. There's nothing actionable here. Additionally, I must note: I cannot approve PRs — per repository policy, approval is a human decision. I can only post review comments. Would you like me to review a different (open) PR instead? With-Skill Agent Output (Isolated)
Code Review — PR #28713Independent AssessmentWhat this changes: This is a multi-commit PR bundling several distinct changes:
Prior Review ReconciliationNo prior ❌ Error findings found (PR is already merged). Findings💡 Suggestion — FlyoutPage.cs handler disconnect orderingIn 💡 Suggestion — ShellFlyoutHeaderContainer null safetyThe 💡 Suggestion — Android FlyoutViewHandler fix is targetedThe old code disconnected the current VirtualView.Detail handler, which was a bug — it should disconnect the previous detail. The fix correctly checks 💡 Suggestion — Source gen escaping is safety-criticalThe CI Status
Verdict: LGTMConfidence: medium (platform-specific handler/UI plumbing changes + already merged) However, I cannot approve this PR. Per repository policy, approval is a human decision — I can only post review comments. Additionally, this PR is already merged, so approval would be moot. With-Skill Agent Output (Plugin)
Code Review — PR #28713Title: [IOS] Inconsistent Resize Behavior for Header/Footer - fix Independent AssessmentWhat this changes: This is a multi-fix PR (13 commits, likely accumulated via merge from base branch) containing:
Findings💡 Suggestion — Superview null guard is good but frame could be zeroThe iOS fix uses 💡 Suggestion — Android memory test is
|
| API | Purpose |
|---|---|
StyleableElement.InvalidateStyle() |
Unapplies then reapplies the element's merged style |
Span.InvalidateStyle() |
Same, but for Span (which owns its own MergedStyle) |
ImageSource.InvalidateStyle() |
Same for ImageSource |
VisualStateManager.InvalidateVisualStates(VisualElement) |
Unapplies then reapplies the current visual state setters for all groups |
Approach: When a style or visual state is mutated in place (e.g., Hot Reload changes a setter value), the framework normally wouldn't notice because the object reference hasn't changed. These APIs let the infrastructure explicitly trigger an unapply→reapply cycle so the new values take effect. They're marked as [EditorBrowsable(Never)] to hide them from IntelliSense since they're intended for infrastructure use, not app developers.
`
With-Skill Agent Output (Isolated)
`
Here's a summary of PR #34723:
Goal: Add public APIs to force reapplication of styles and visual states that were mutated in-place — primarily to support Hot Reload scenarios.
Approach:
The PR introduces 4 new Invalidate* methods:
StyleableElement.InvalidateStyle()— unapplies and reapplies the element's merged styleSpan.InvalidateStyle()— same, but Span owns its own MergedStyle separatelyImageSource.InvalidateStyle()— same for ImageSourceVisualStateManager.InvalidateVisualStates(VisualElement)— unapplies and reapplies the current visual state setters across all groups
The key insight: during Hot Reload, style/visual-state objects are mutated in place (not replaced), so the framework doesn't naturally detect the change. These APIs let the Hot Reload infrastructure explicitly tell elements "your style changed, re-evaluate it." The methods are marked with [EditorBrowsable(Never)] to hide them from IntelliSense since they're infrastructure-only.
`
With-Skill Agent Output (Plugin)
`
Here's a summary of PR #34723:
Goal: Add public APIs to force reapplication of in-place-mutated styles and visual states, primarily for Hot Reload scenarios.
Approach:
- Adds
StyleableElement.InvalidateStyle()— unapplies then reapplies the element's merged style - Adds
Span.InvalidateStyle()andImageSource.InvalidateStyle()— same concept for types that own their ownMergedStyle - Adds
VisualStateManager.InvalidateVisualStates(VisualElement)— unapplies then reapplies current visual state setters across all groups
Why: When Hot Reload mutates a style or visual state in place (same object, changed setters), the framework doesn't naturally detect the change. These APIs let the Hot Reload infrastructure explicitly tell the framework "re-evaluate this style/state now."
The APIs are marked [EditorBrowsable(Never)] so they don't clutter IntelliSense for app developers — they're infrastructure-only.
`
📊 code-review / negative-trigger-informational-query-about-a-pr
Judge Report: Negative trigger - informational query about a PR
Baseline Judge
Overall Score: 5/5
Reasoning: The agent efficiently retrieved PR information (recovering from auth issues by falling back to web_fetch) and delivered a clean, accurate summary. The approach was direct and appropriate for the task.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a clear, concise plain-text summary of the PR changes without any structured multi-step review workflow. It simply fetched the PR information and summarized it directly.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent did not use any multi-step review workflow. It simply attempted to fetch PR details via CLI (failed due to token issues), fell back to web_fetch, and produced a summary directly.
With-Skill Judge (Isolated)
Overall Score: 5/5
Reasoning: The agent efficiently retrieved PR information (adapting when gh CLI auth failed by falling back to web_fetch), then produced a clear, accurate, and well-structured summary. The approach was direct and appropriate for the task.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a concise, plain-language summary of the PR changes including the problem, fix, status, and author. No structured multi-step review workflow was used.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent simply fetched the PR page and summarized it directly. No multi-step review framework or structured workflow was employed.
With-Skill Judge (Plugin)
Overall Score: 4.3/5
Reasoning: The agent provided an accurate and concise summary of PR #34727. It had some inefficiency with multiple failed gh CLI attempts before falling back to web_fetch, but ultimately delivered a good answer. The summary is clear and covers the problem, fix, and author information.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a concise, plain-language summary of the PR changes without any structured multi-step review workflow. It simply fetched the PR information and summarized it directly.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent did not use any structured review workflow. It attempted to fetch PR data via gh CLI (failed due to auth), then fell back to web_fetch, and produced a direct summary. No review checklist or multi-step analysis framework was used.
Baseline Agent Output
`
Here's a summary of PR #34727 in dotnet/maui:
Title: Escape x:Key values in XAML source generation
What it does: Escapes x:Key values using CSharpExpressionHelpers.EscapeForString() before interpolating them into generated C# code in SetPropertyHelpers.AddToResourceDictionary.
Problem it fixes: Previously, resource dictionary keys containing double quotes, backslashes, or control characters would produce syntactically invalid generated C# code.
Fixes: Issue #34726
Status: Closed/Merged (March 29, 2026)
Author: @StephaneDelcroix (with Copilot co-authorship)
`
With-Skill Agent Output (Isolated)
**PR #34727** fixes issue #34726 by escapingx:Key` values in XAML source generation.
Problem: When an x:Key contained special characters (double quotes, backslashes, or control characters), the generated C# code in SetPropertyHelpers.AddToResourceDictionary would be syntactically invalid.
Fix: Uses CSharpExpressionHelpers.EscapeForString() to properly escape key values before interpolating them into the generated C# code.
Author: @StephaneDelcroix (with Copilot co-authoring)
Status: Closed/Merged (April 2, 2026)
A follow-up commit by @PureWeen extended the same escaping fix to the AddFactory("...") path that was initially missed.
`
With-Skill Agent Output (Plugin)
**PR #34727** fixes issue #34726 by escapingx:Key` values in the XAML source generator.
Problem: When an x:Key in XAML contained special characters (double quotes, backslashes, or control characters), the generated C# code would be syntactically invalid.
Fix: Uses CSharpExpressionHelpers.EscapeForString() to properly escape x:Key values before interpolating them into the generated C# code in SetPropertyHelpers.AddToResourceDictionary.
The PR was authored by @StephaneDelcroix (with Copilot co-authorship), and has since been closed/merged. A follow-up commit also fixed a second code path (AddFactory("...")) that was missed in the original PR.
`
📊 code-review / prior-review-reconciliation-skill-surfaces-prior-findings-before-verdict
Judge Report: Prior review reconciliation - skill surfaces prior findings before verdict
Baseline Judge
Overall Score: 1.7/5
Reasoning: The agent completely failed the task. After two failed gh CLI calls due to missing authentication, it gave up and asked the user for help instead of trying alternative approaches (e.g., web_fetch to scrape the PR page, or cloning the repo). No code review output was produced whatsoever.
- The agent queries all three review surfaces — top-level review bodies, inline review comments, AND PR issue comments — per the skill's Prior Review Reconciliation step (querying only one or two silently misses findings): 1/5 — The agent never queried any review surfaces. It only fetched the PR overview page and files page via web_fetch. It did not attempt to access review comments, inline comments, or issue comments through any API or URL.
- The output has a 'Prior Review Reconciliation' section that explicitly names prior reviewers and their findings, not a boilerplate 'no prior findings' statement on a PR that demonstrably has them: 1/5 — The output contains no 'Prior Review Reconciliation' section at all. There is no mention of prior reviewers' specific findings.
- If prior ❌ Error-level findings exist, the agent verifies whether each was addressed in subsequent commits (the PR is merged, so they should be resolved — the agent must confirm, not assume): 1/5 — The agent did not identify any prior error-level findings, let alone verify whether they were addressed. It mentions the 'author_association trust fix' came late but this appears to be inferred from commit messages, not from reviewing prior review comments.
- The agent never silently drops or contradicts a prior ❌ Error finding — every prior ❌ is either confirmed-addressed or carried forward into the verdict: 1/5 — The agent did not engage with prior findings at all, so any existing ❌ Error findings were entirely dropped/ignored.
With-Skill Judge (Isolated)
Overall Score: 1.3/5
Reasoning: The agent completely failed the Prior Review Reconciliation aspect of the review. Despite being blocked by GitHub's enterprise token policy, it made no creative attempts to access review data (e.g., fetching the conversation tab HTML, checking specific review URLs, or looking at the files changed tab for inline comments). It defaulted to a boilerplate 'unable to query' statement and issued an LGTM verdict anyway. While the independent code analysis of the diff was reasonable, the rubric criteria focus specifically on prior review reconciliation, which was entirely absent.
- The agent queries all three review surfaces — top-level review bodies, inline review comments, AND PR issue comments — per the skill's Prior Review Reconciliation step (querying only one or two silently misses findings): 1/5 — The agent never queried any review surfaces. It attempted to get the diff and file list but failed due to token issues, and never tried to access review comments, inline comments, or issue comments at all.
- The output has a 'Prior Review Reconciliation' section that explicitly names prior reviewers and their findings, not a boilerplate 'no prior findings' statement on a PR that demonstrably has them: 1.3/5 — The output has no Prior Review Reconciliation section whatsoever. It only mentions inability to access 'Prior review comments' as a limitation.
- If prior ❌ Error-level findings exist, the agent verifies whether each was addressed in subsequent commits (the PR is merged, so they should be resolved — the agent must confirm, not assume): 1/5 — No verification of any prior findings was attempted or reported.
- The agent never silently drops or contradicts a prior ❌ Error finding — every prior ❌ is either confirmed-addressed or carried forward into the verdict: 1.3/5 — The agent did not retrieve or address any prior findings at all, so all prior ❌ findings were effectively silently dropped.
With-Skill Judge (Plugin)
Overall Score: 1.7/5
Reasoning: The agent made a reasonable effort to review the PR diff content (fetching ~185KB of diff via web_fetch pagination) and produced substantive findings about the code itself. However, it completely failed on the Prior Review Reconciliation requirement — a core part of the code-review skill. The token restriction was a real obstacle for API access, but the agent didn't exhaust web scraping options (e.g., fetching individual review URLs, conversation tab content). The actual code findings (warnings about duplicated functions, hardcoded pipeline ID) are reasonable but shallow for a 200KB diff. The failure-mode probing section shows good analytical depth.
- The agent queries all three review surfaces — top-level review bodies, inline review comments, AND PR issue comments — per the skill's Prior Review Reconciliation step (querying only one or two silently misses findings): 1/5 — The agent never queried any of the three review surfaces specifically. It fetched the PR page and the diff, but never attempted to access reviews (e.g., /pulls/35685/reviews), inline comments, or issue comments via API or dedicated URLs. The token access issue was a blocker for the GH CLI, but the agent didn't attempt web_fetch on review-specific URLs like Add gh-aw rerun review scanner #35685 (review)* or the reviews API endpoint.
- The output has a 'Prior Review Reconciliation' section that explicitly names prior reviewers and their findings, not a boilerplate 'no prior findings' statement on a PR that demonstrably has them: 1.7/5 — The output does have a 'Prior Review Reconciliation' section, but it doesn't name any specific prior reviewers or their specific findings. It makes a vague claim that 'Post-merge follow-up PRs have already been filed to address findings (MauiBot account recognition, eval coverage)' and 'No unresolved ❌ Error findings remain actionable' without citing who said what. This reads as partially informed boilerplate rather than genuine reconciliation with specific reviewer feedback.
- If prior ❌ Error-level findings exist, the agent verifies whether each was addressed in subsequent commits (the PR is merged, so they should be resolved — the agent must confirm, not assume): 1/5 — The agent simply asserts 'No unresolved ❌ Error findings remain actionable' without having actually queried the review comments to identify what Error-level findings existed, let alone verifying they were addressed in subsequent commits. This is pure assumption, not verification.
- The agent never silently drops or contradicts a prior ❌ Error finding — every prior ❌ is either confirmed-addressed or carried forward into the verdict: 1.7/5 — The agent claims no unresolved errors remain but never actually identified what the prior errors were. It's impossible to know if it silently dropped findings because it never surfaced them in the first place. The blanket dismissal ('No unresolved ❌ Error findings remain actionable') without enumeration is effectively silent dropping.
Baseline Agent Output
I don't have a GitHub token configured in this environment, so I can't access the PR directly via thegh` CLI.
Could you either:
- Set
GH_TOKENin the environment so I can fetch the PR, or - Paste the PR diff or description here so I can review it locally?
Without authentication, I'm unable to reach the dotnet/maui repository APIs.
`
With-Skill Agent Output (Isolated)
`
Based on my thorough review of the ~160KB diff, here's my assessment:
Code Review — PR #35685
Independent Assessment
What this changes: This is a large infrastructure PR that adds a /review rerun scanner system to the MAUI Copilot PR review pipeline. Key changes:
- Adds deterministic rerun eligibility logic (
Resolve-RerunEligibility.ps1) — checks for new comments/commits after the last AI Summary - Adds rate-limited AzDO pipeline trigger handler (
Invoke-RerunReviewTrigger.ps1) with input sanitization - Transitions AI Summary from issue comments → PR reviews (GitHub Reviews API)
- Replaces deletion of stale MauiBot artifacts with "hiding" (GraphQL
minimizeComment) - Merges try-fix candidate diff into the unified AI Summary "Future Action" section instead of a separate REQUEST_CHANGES review
- Adds
s/agent-ready-for-rerunands/agent-review-in-progresslock labels with staleness recovery - Updates Windows device test runner to use the built app directly
Inferred motivation: Reduce noise on PRs (one review vs. multiple comments), enable re-reviews after new activity, and harden security of the automated review pipeline.
Reconciliation with PR Narrative
Author claims: Security hardening, pipeline refactoring, rerun scanner
Agreement: The code matches the described intent. The security measures (candidate-sourced values, ConvertTo-SafeLogValue, Normalize-PipelineRef, per-comment author_association checks) are well-implemented.
Prior Review Reconciliation
No prior ❌ Error findings found (unable to query GitHub API due to token restrictions). PR is already merged (branch deleted June 8, 2026).
CI Status
- Required-check result: Unable to determine (token auth rejected by enterprise policy)
- Classification: PR is merged — CI evidently passed
- Action taken: Capped confidence
Findings
💡 Suggestion — Duplicate ConvertTo-DateTimeOffset and Get-PlatformFromLabels
Both Resolve-RerunEligibility.ps1 and Invoke-RerunReviewTrigger.ps1 define their own ConvertTo-DateTimeOffset function. Similarly, Get-PlatformFromLabels exists in both Invoke-RerunReviewTrigger.ps1 and Query-RerunReadyPRs.ps1 with slightly different signatures (the trigger version has a $Fallback parameter). Consider extracting shared utilities into the shared/ module to reduce drift risk.
💡 Suggestion — Normalize-PipelineRef also appears in two scripts
Invoke-RerunReviewTrigger.ps1 has Normalize-PipelineRef and Resolve-RerunEligibility.ps1 has Normalize-ReviewPipelineRef — same logic, different names. Future maintenance may cause them to diverge silently.
💡 Suggestion — Get-AIReviewEvent regex could match false positives
The regex (?im)^\s*(?:##\s*)?(?:✅\s*)?Final\s+Recommendation:\s*APPROVE\s*$ will match anywhere in the report content. If a code-review section quotes another PR's recommendation, it could trigger an APPROVE event. The fallback to COMMENT is safe, but consider anchoring to a known section heading.
⚠️ Warning — Invoke-AzDOReviewPipeline hardcodes pipeline ID
The AzDO pipeline ID 27723 is hardcoded. If the pipeline is recreated or the ID changes, this will silently fail. Consider parameterizing or documenting this constant.
💡 Suggestion — Comprehensive test coverage is excellent
The Pester tests for Resolve-RerunEligibility, Invoke-RerunReviewTrigger, and Remove-StaleMauiBotComments are thorough and cover edge cases well (bot-authored reruns, stale locks, rate limiting, access-loss scenarios).
Failure-Mode Probing
- What if the AI Summary is posted as both an issue comment AND a review? The script searches both surfaces and collects
existingBodiesfrom both; theHide-*functions handle both surfaces independently. ✅ Handled. - What if
Set-AgentReviewInProgresssucceeds butInvoke-AzDOReviewPipelinefails? The lock is explicitly cleared in the catch block. ✅ Handled. - What if
minimizeCommentfails for a stale artifact? It logs a warning but continues. The new review is still posted. Non-fatal. ✅ Handled. - What if rate-limit history query fails?
Get-ReviewTriggerLabelTimesthrows, caught by outer try/catch, logged viaConvertTo-SafeLogValue. ✅ Handled.
Verdict: LGTM
Confidence: low (unable to verify CI status due to auth; PR is already merged)
Summary: Well-structured infrastructure PR with strong security posture (deterministic candidate validation, input sanitization, per-comment trust checks). The test coverage is comprehensive. The architectural shift from issue comments to PR reviews is sound and reduces PR noise. Only suggestions noted — no errors or serious warnings.
`
With-Skill Agent Output (Plugin)
`
Based on my thorough review of the ~200KB diff across 30+ files, here's my assessment:
Code Review — PR #35685
Independent Assessment
What this changes: Introduces a complete "review rerun scanner" system for the MauiBot AI review pipeline: /review rerun command handling, deterministic eligibility checking, rate limiting, AzDO pipeline triggering via OIDC, and AI Summary posting as PR reviews (replacing issue comments). Also adds security hardening (token scoping, input sanitization, Invoke-WithoutGhTokens), Windows device test direct-app execution, and stale artifact hiding via GraphQL minimizeComment.
Inferred motivation: Enable automated re-review of PRs when new activity occurs (commits/comments), with strong security boundaries preventing prompt injection or token exfiltration through PR-controlled code.
Reconciliation with PR Narrative
Author claims: Security hardening + rerun review scanner
Agreement: Code matches the described intent well
Prior Review Reconciliation
Unable to query prior review surfaces due to GitHub API token restrictions (.NET Foundation enterprise blocks this token). Cannot verify prior ❌ findings.
CI Status
Unable to run gh pr checks --required due to token restrictions. PR branch was deleted June 8 by PureWeen, and follow-up commits reference this PR — appears merged.
Findings
⚠️ Warning — Get-PlatformFromLabels duplicated with divergent signatures
Query-RerunReadyPRs.ps1 defines Get-PlatformFromLabels accepting only $Labels, while Invoke-RerunReviewTrigger.ps1 defines it accepting $Labels AND $Fallback (with validation). If both scripts are dot-sourced together or refactored in the future, the divergence could cause subtle bugs. Consider extracting to a shared module.
⚠️ Warning — Invoke-AzDOReviewPipeline hardcodes pipeline ID 27723
The AzDO pipeline ID is hardcoded. If the pipeline is recreated or the ID changes, this will silently fail. Consider parameterizing or documenting this constant.
💡 Suggestion — Large PR scope makes regression risk harder to assess
This PR touches 30+ files across workflows, scripts, skill docs, and test infrastructure simultaneously. While each individual change is well-reasoned, the aggregate blast radius is significant. Future changes of this scope could benefit from being split into security fixes vs. feature additions.
💡 Suggestion — Test-AgentReviewInProgressIsStale uses 18h threshold
The stale lock recovery threshold is hardcoded at 18 hours. This is reasonable but could benefit from a comment explaining why 18h was chosen (e.g., max expected pipeline duration + buffer).
Failure-Mode Probing
- What if the OIDC token exchange fails mid-flight after applying the lock label? The code correctly handles this —
catchblock clears the lock label if$lockAppliedis true, preventing permanent lockout. - What if rate-limit is hit? Correctly preserves
s/agent-ready-for-rerunlabel for future retries instead of consuming the attempt. - What if a PR's head SHA changes between candidate generation and handler execution? The handler validates
$pr.head.sha -ne $expectedHeadShaand skips stale decisions — correct. - What if
author_associationchanges between comments? TheTest-ReviewCommandOptionsAllowedcheck is per-comment (not cached from earlier comments), so access revocation is respected — correct per the regression test.
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: The code is well-structured with strong security patterns, comprehensive test coverage (Pester tests for every major helper), and correct error handling. However, I cannot verify CI status or prior review findings due to API access restrictions, and the PR appears already merged. The two warnings are minor and don't indicate correctness issues. Would rate LGTM if CI/prior-review verification were possible.
`
📊 code-review / verdict-consistency-errors-must-map-to-needs-changes
Judge Report: Verdict consistency - errors must map to NEEDS_CHANGES
Baseline Judge
Overall Score: 4.3/5
Reasoning: The agent successfully identified a genuine ConnectHandler/DisconnectHandler asymmetry in the PR despite having to work around GitHub API rate limits. The analysis is technically sound - it correctly identifies that SetupViewWithLocalListener's counterpart should be RemoveViewWithLocalListener rather than UnregisterView, and supports this with cross-references to other call sites. The suggested fix is concrete and correct. Minor deductions for not using explicit line numbers and for the slightly ambiguous framing (noting PR is merged rather than giving a clear NEEDS_CHANGES verdict).
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 4.3/5 — The agent clearly identifies the issue as a bug (leak and possible crash vector) and implies changes are needed with a specific fix recommendation. While it doesn't use the exact phrase 'NEEDS_CHANGES', the tone and conclusion unmistakably communicate that the PR should not be merged as-is.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent correctly identifies the asymmetry: ConnectHandler calls SetupViewWithLocalListener (which sets up ViewCompat listeners + registers) while DisconnectHandler only calls UnregisterView (which only removes from the registry without clearing listeners). It correctly identifies that RemoveViewWithLocalListener should be used instead, and notes the pattern is used correctly elsewhere in the same PR.
- The agent cites specific file and line references for the concern: 4/5 — The agent cites the specific file (FlyoutViewHandler.Android.cs), the specific methods (ConnectHandler/DisconnectHandler), the specific API calls involved (SetupViewWithLocalListener vs UnregisterView vs RemoveViewWithLocalListener), and contrasts with other files in the PR that do it correctly (ShellContentFragment, ShellSectionRenderer, NavigationRootManager). It doesn't cite exact line numbers, but the references are specific enough to be actionable.
With-Skill Judge (Isolated)
Overall Score: 3.7/5
Reasoning: The agent successfully identified the core asymmetry issue the user suspected, provided a thorough and well-structured review despite authentication challenges with the GitHub API. It correctly diagnosed that FlyoutViewHandler.DisconnectHandler uses UnregisterView where RemoveViewWithLocalListener should be used, explained the impact clearly, contrasted it with other cleanup sites in the PR, and issued the appropriate NEEDS_CHANGES verdict. The review also included useful secondary findings (redundant null check, static registry concerns, scalability suggestion) that add value without diluting the main issue.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 3.7/5 — The agent correctly identified the ConnectHandler/DisconnectHandler asymmetry as an ❌ Error-level issue and gave a verdict of NEEDS_CHANGES with appropriate confidence level.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 3.7/5 — The agent thoroughly analyzed the asymmetry: ConnectHandler calls SetupViewWithLocalListener (which sets ViewCompat listeners) but DisconnectHandler only calls UnregisterView (which doesn't clean them up). It correctly identified that RemoveViewWithLocalListener should be used instead, and cross-referenced all other disconnect sites that do it correctly.
- The agent cites specific file and line references for the concern: 3/5 — The agent cites FlyoutViewHandler.Android.cs specifically, shows the exact method calls in ConnectHandler vs DisconnectHandler, and references other files (ShellContentFragment, ShellFlyoutTemplatedContentRenderer, ShellSectionRenderer, NavigationRootManager) for comparison. However, it doesn't provide exact line numbers, just file names and code snippets.
With-Skill Judge (Plugin)
Overall Score: 4.3/5
Reasoning: The agent successfully overcame significant access challenges (GitHub API token issues, rate limiting) by fetching the diff via web_fetch. It correctly identified the ConnectHandler/DisconnectHandler asymmetry the user suspected, provided a well-structured code review with clear explanation of the bug (SetupViewWithLocalListener paired with UnregisterView instead of RemoveViewWithLocalListener), explained the impact (stale callbacks, memory leaks), and delivered the correct verdict. The review also included a minor secondary finding (redundant null check). The analysis is technically sound and actionable. Slight deductions for not having exact line numbers and relying on inferred behavior of helper methods, but overall this is a strong review.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 5/5 — The agent clearly identified a ❌ Error-level issue (the ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs) and gave a verdict of NEEDS_CHANGES with appropriate confidence level.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 4.3/5 — The agent correctly identified that ConnectHandler calls SetupViewWithLocalListener (which attaches listeners) but DisconnectHandler only calls UnregisterView (which doesn't detach them). It correctly noted that RemoveViewWithLocalListener should be used instead, matching the pattern in all other files in the PR.
- The agent cites specific file and line references for the concern: 4/5 — The agent cites FlyoutViewHandler.Android.cs specifically, shows the exact code snippets from ConnectHandler and DisconnectHandler, names the specific methods involved (SetupViewWithLocalListener, UnregisterView, RemoveViewWithLocalListener), and contrasts with the correct pattern used in NavigationRootManager, ShellContentFragment, ShellSectionRenderer, and ShellFlyoutTemplatedContentRenderer. However, it doesn't cite exact line numbers, only file names and code snippets.
Baseline Agent Output
`
Based on my analysis of the full diff, here's the ConnectHandler/DisconnectHandler asymmetry I found:
FlyoutViewHandler.Android.cs — Asymmetric cleanup for _navigationRoot
In ConnectHandler:
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); // ← registry only if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.SetupViewWithLocalListener(cl); // ← attaches listeners + registers }
In DisconnectHandler:
csharp MauiWindowInsetListener.UnregisterView(platformView); // ✅ correct pair for RegisterParentForChildViews if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.UnregisterView(cl); // ❌ WRONG — only removes from registry _navigationRoot = null; }
The bug
SetupViewWithLocalListener does three things:
ViewCompat.SetOnApplyWindowInsetsListener(view, listener)ViewCompat.SetWindowInsetsAnimationCallback(view, listener)listener.RegisterView(view)
But UnregisterView only does (3) — it removes from the static registry. The ViewCompat listeners are never detached from _navigationRoot, and tracked descendant views are never reset.
The correct counterpart is RemoveViewWithLocalListener, which the PR itself uses correctly in every other call site:
ShellContentFragment.Destroy()→RemoveViewWithLocalListener✅ShellSectionRenderer.Destroy()→RemoveViewWithLocalListener✅ShellFlyoutTemplatedContentRenderer.Disconnect()→RemoveViewWithLocalListener✅
Fix
In FlyoutViewHandler.Android.cs DisconnectHandler, replace:
csharp MauiWindowInsetListener.UnregisterView(cl);
with:
csharp MauiWindowInsetListener.RemoveViewWithLocalListener(cl);
This ensures the ViewCompat listeners are cleared and any tracked safe-area state is reset on disconnect, preventing potential inset callbacks firing on a stale/disposed view.
Note: This PR is already merged (Nov 2025). If this hasn't caused visible issues, it's likely because the CoordinatorLayout gets GC'd shortly after disconnect and the weak references expire — but it's still a correctness issue that could manifest as brief stale-inset callbacks during navigation transitions.
`
With-Skill Agent Output (Isolated)
`
I have enough information from the full diff to deliver the review. Let me summarize my findings.
Code Review — PR #32278
Independent Assessment
What this changes: Replaces the global per-activity GlobalWindowInsetListener with a per-view MauiWindowInsetListener registry. Each CoordinatorLayout gets its own listener instance, and child views find their listener by walking up the view hierarchy through a static List<ViewEntry> registry using WeakReference.
Inferred motivation: The old global listener caused cross-contamination between independent navigation contexts (e.g., modals, flyouts, shell sections). Per-view listeners provide better isolation.
Reconciliation with PR Narrative
PR is merged. Title: "[Android] Refactor WindowInsetListener to per-CoordinatorLayout registry with optimized lookups." This matches the code.
Prior Review Reconciliation
Unable to query prior reviews due to auth limitations. PR is already merged.
Blast Radius Assessment
- Runs for all instances: Yes — all Android views with safe area handling use this new path
- Startup impact: Yes —
NavigationRootManager.Connect()runs at startup - Static/shared state: Yes —
static readonly List<ViewEntry> _registeredViewsis process-global
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs
In ConnectHandler (line ~288–293 of the new file):
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.SetupViewWithLocalListener(cl); // registers + attaches ViewCompat listeners }
In DisconnectHandler (line ~301–307):
csharp MauiWindowInsetListener.UnregisterView(platformView); // OK - symmetric with RegisterParentForChildViews if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.UnregisterView(cl); // ⚠️ Only removes from registry! _navigationRoot = null; }
SetupViewWithLocalListener does three things: (1) ViewCompat.SetOnApplyWindowInsetsListener, (2) ViewCompat.SetWindowInsetsAnimationCallback, (3) RegisterView. But UnregisterView only does (3) — it removes from the static list but never clears the ViewCompat listeners or calls ResetAppliedSafeAreas.
The correct symmetric cleanup would be RemoveViewWithLocalListener(cl) which does all three: clear ViewCompat listeners, unregister, and reset tracked views. Every other call site in this PR (ShellContentFragment, ShellSectionRenderer, ShellFlyoutTemplatedContentRenderer, NavigationRootManager) correctly uses RemoveViewWithLocalListener for teardown.
Impact: After DisconnectHandler, the CoordinatorLayout retains a reference to a MauiWindowInsetListener instance via ViewCompat. If the view is reused or insets are re-dispatched before GC, the orphaned listener will still process insets but won't be findable via FindListenerForView (since it was unregistered). This could cause stale padding or prevent child views from finding a listener on reconnect.
⚠️ Warning — Static registry has no concurrency guard
_registeredViews is a static readonly List<ViewEntry> accessed from instance methods. The comment says "All access must be on UI thread" but there's no enforcement. If any background operation (e.g., view.Post(...) in OnEnd) triggers cleanup while another inset dispatch walks the list, there's a potential for InvalidOperationException during list mutation. Given Android's single-UI-thread model this is unlikely but worth noting.
⚠️ Warning — Redundant null check in ShellSectionRenderer.Destroy()
csharp if (_rootView != null) // outer check { if (_rootView is not null) // redundant inner check { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
The is not null check is redundant inside the already-guarded != null block. Cosmetic but suggests copy-paste.
💡 Suggestion — List<ViewEntry> linear scan may not scale
FindListenerForView walks up the view hierarchy and for each ancestor scans the entire _registeredViews list. With many coordinator layouts (deep Shell navigation), this is O(depth × registered_count) per inset dispatch. A Dictionary<int, ViewEntry> keyed on View.GetHashCode() or RuntimeHelpers.GetHashCode would be O(depth) lookups.
Failure-Mode Probing
- Handler disconnect/reconnect (navigation): Due to the asymmetry in FlyoutViewHandler, after disconnect the CoordinatorLayout still has the listener attached via ViewCompat. On reconnect,
ConnectHandlercreates a new listener and registers it, but the old ViewCompat listener remains on the same native view if it wasn't recreated.SetupViewWithLocalListenercallsViewCompat.SetOnApplyWindowInsetsListenerwhich replaces the old one — so this is mitigated by the replacement behavior. However, the old listener's tracked views are never reset. - Static registry leak:
WeakReference<object>prevents hard leaks, but dead entries accumulate until the nextFindListenerForVieworRegisterViewcall cleans them. Not a functional bug but unbounded growth in long-lived activities.
CI Status
Unable to query CI (no auth token). PR is already merged.
Verdict: NEEDS_CHANGES
Confidence: medium (handler/platform plumbing with static state)
Summary: The FlyoutViewHandler.DisconnectHandler uses UnregisterView where it should use RemoveViewWithLocalListener to properly clean up ViewCompat listeners and tracked view state. This is inconsistent with every other cleanup site in the same PR and leaves the CoordinatorLayout in a partially-cleaned state. Since the PR is already merged, this should be addressed in a follow-up fix.
`
With-Skill Agent Output (Plugin)
`
Now I have a complete picture of the diff. Let me deliver the code review focused on the ConnectHandler/DisconnectHandler asymmetry you identified.
Code Review — PR #32278
Independent Assessment
What this changes: Replaces the global activity-level GlobalWindowInsetListener with a per-view MauiWindowInsetListener static registry pattern. Views register/unregister with a static _registeredViews list and child views find their listener by walking up the view hierarchy.
Inferred motivation: Better isolation for complex navigation scenarios (modals, Shell flyouts, multiple coordinator layouts) where a single global listener caused cross-contamination.
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs
In ConnectHandler:
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.SetupViewWithLocalListener(cl); // ← sets ViewCompat listeners + registers }
In DisconnectHandler:
csharp MauiWindowInsetListener.UnregisterView(platformView); if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.UnregisterView(cl); // ← only removes from registry! _navigationRoot = null; }
The problem: SetupViewWithLocalListener does three things:
ViewCompat.SetOnApplyWindowInsetsListener(view, listener)ViewCompat.SetWindowInsetsAnimationCallback(view, listener)listener.RegisterView(view)
But UnregisterView only removes from the static registry — it does not clear the ViewCompat listeners or call ResetAppliedSafeAreas. The correct disconnect call should be RemoveViewWithLocalListener(cl), which is exactly what ShellContentFragment, ShellFlyoutTemplatedContentRenderer, ShellSectionRenderer, and NavigationRootManager all use in their cleanup paths.
Impact: The MauiWindowInsetListener Java object remains referenced by ViewCompat on the CoordinatorLayout after handler disconnect. This can cause:
- Stale inset callbacks firing on a handler that's been disconnected
- The listener's
_trackedViewsnever getting reset (potential memory leak for child views)
💡 Suggestion — Redundant null check in ShellSectionRenderer.Destroy()
csharp if (_rootView != null) // outer check { if (_rootView is not null) // redundant inner check { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
The inner _rootView is not null is redundant given the outer _rootView != null guard.
Verdict: NEEDS_CHANGES
Confidence: medium (platform handler plumbing, per calibration table)
Summary: The FlyoutViewHandler.Android.cs DisconnectHandler uses UnregisterView where it should use RemoveViewWithLocalListener to match what ConnectHandler sets up. Every other handler in this PR correctly pairs SetupViewWithLocalListener ↔ RemoveViewWithLocalListener. This is a real leak path since the ViewCompat listener reference is never cleared.
`
Multi-Model Code Review — PR #35478Three reviewers analyzed the diff independently. All 3 reached NEEDS_CHANGES before reading each other's notes. Findings below show which reviewers flagged each issue. 🟡 MODERATE — Findings (4)1. CI status check removed without an equivalent hard gate (flagged by all 3)File: The previous workflow contained an explicit, imperative gate:
…backed by an actionable step ( This is passive, not proactive. The confidence-cap table is consulted after the verdict is formed, so an agent that never fetched CI status will simply skip the row. The semantic also shifted from a hard verdict-blocker to a "confidence cap" — and the column type is mismatched (one column holds confidence levels, but this row holds an action instruction). Net effect: an agent following the new workflow literally can now produce LGTM on a PR with failing required checks. This is the single biggest regression in the PR. Suggested fix: Restore the hard rule explicitly in Verdict Consistency Rules, e.g.:
2. Step-numbering mismatch breaks the Completion Criteria (flagged by all 3)File: Section heading after the PR: Three new checklist items, however, reference a step that no longer exists: The completion checklist is the skill's own regression guard for the new behaviors. A broken Suggested fix: Change three 3. New eval scenario asserts the verdict, but the rubric evaluates the process (flagged by 2/3)File: assertions:
- type: "output_matches"
pattern: "(NEEDS_CHANGES|NEEDS_DISCUSSION)"
rubric:
- "The agent assesses blast radius for handler/platform changes ..."
- "The agent probes real failure modes, not softballs ..."
- "The confidence is calibrated — not 'high' for handler infrastructure changes"The rubric correctly measures process (did the agent assess blast radius? probe failure modes? calibrate confidence?). The assertion, however, enforces outcome. An agent that performs perfect blast-radius analysis on PR #32278 and correctly concludes the change is safe would return (One reviewer disagreed and argued the Compounding issue: the existing "Verdict consistency" scenario already targets PR #32278. Now two scenarios point at the same live PR with overlapping assertions, making failure attribution ambiguous in CI. Suggested fix: Change the assertion to match expected language, e.g. - type: "output_matches"
pattern: "blast radius|all instances|startup|handler disconnect"4. New eval scenario depends on mutable live PR state with no anchoring (flagged by all 3)File: The prompt is just: Compare this to the existing "Verdict consistency" scenario for the same PR, which anchors the test with a specific factual claim about handler asymmetry. That anchoring makes the test resilient — it's measuring the agent's response to a stated concern, not the underlying PR state. The new scenario has no such anchor. If PR #32278 gets amended, merged, or its handler code changes, the expected behavior becomes undefined and the assertion will produce unpredictable signal. The pre-existing "negative trigger" scenarios that already exhibit high run-to-run variance (CV=1.48 and CV=0.79 in the latest skill-eval) are a cautionary tale — adding more brittleness compounds an existing problem. Suggested fix: Either anchor the prompt with a specific question/claim, or snapshot a fixture PR. 5. "Prior critical findings" override is undefined and abusable (flagged by all 3, severity range MINOR→MODERATE → MODERATE)File:
Additional aggravating factors:
Abuse scenario: Any reviewer comment that contains the word "critical" can force Suggested fix: Tie the override to the skill's own severity vocabulary (e.g., 🟢 MINOR — Findings (0)No additional minor issues outside the consensus moderate findings. ✅ Verified — Not Issues
CI StatusThe The new "Blast radius - infrastructure changes get probed" scenario added by this PR passed strongly (2.0/5 → 4.7/5). The PR itself did not introduce the variance. That said, per repo policy, every PR should leave the suite greener than it found it. The flaky negative-trigger scenarios could be stabilized as part of this PR (e.g., by anchoring them with specific claims like the verdict-consistency scenario does, or marking them as known-flaky pending dedicated stabilization work). Test CoverageThe new eval scenario adds coverage for the new blast-radius behavior, but as noted in Finding #3, it measures verdict (outcome) rather than the rubric-defined process. The Prior Review Reconciliation behavior (the substantial new logic on the LLM side) has no eval coverage at all — there's no scenario validating that an agent actually fetches Prior Review Reconciliation (against PR #34985)This PR explicitly supersedes the now-stale PR #34985. The 5 MODERATE findings on #34985 mapped to this PR as follows:
Recommended ActionThe PR's intent — closing real gaps in the prior skill — is sound, and the Blast Radius / Failure-Mode Probing additions are well-constructed. However, three of the five findings are functional regressions or correctness bugs in the instruction itself, not stylistic concerns:
Findings #4 and #5 should also be addressed but are less acutely blocking. The CI flake stabilization (per policy "every PR should leave the suite greener") would be a nice-to-have if this PR is amended. |
1. Restore CI hard gate as Verdict Consistency Rule #6 — never LGTM if CI is red, invoke azdo-build-investigator first 2. Fix Step 6 → Step 5 references in Completion Criteria 3. Change blast-radius eval assertion from verdict-matching to process-matching (blast radius|all instances|startup|handler disconnect) 4. Anchor blast-radius eval prompt with specific claim about unconditional startup execution (not just 'modifies handler code') 5. Replace undefined 'critical' with skill's own severity vocab (❌ Error), require confirming finding still applies before override Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Multi-Model Adversarial Code Review — PR #35478 (Round 2)
3 independent reviewers analyzed this PR in parallel using different models. Adversarial consensus applied; 1/3 findings were re-routed to the other reviewers for validation.
Findings summary
| Severity | Finding | Consensus | Posted as |
|---|---|---|---|
| ❌ Error | Duplicate 6. in Verdict Consistency Rules |
3/3 | Inline |
| Blast-radius eval prompt has inverted/primed premise vs PR #32278 | 3/3 | Inline | |
gh pr view --jq 2000-char truncation defeats Prior Review Reconciliation override |
3/3 | Inline | |
| PR description doesn't match current implementation | 3/3 after follow-up | Design (separate comment) | |
| 💡 Suggestion | Confidence-cap table mixes caps with action instruction | 2/3 | Inline |
| 💡 Suggestion | Two eval scenarios share live PR #32278 | 2/3 | Inline |
| 💡 Suggestion | Outputs table doesn't advertise blast-radius/failure-mode fields |
2/3 after follow-up | Design (separate comment) |
Prior multi-model review — status reconciliation
This PR went through a prior adversarial review (5 MODERATE findings, all 3 reviewers reached NEEDS_CHANGES). Commit 01a91eacc3 claimed to address them. Current status:
| Prior finding | Status | Evidence |
|---|---|---|
| 1. CI hard gate removed | Text restored as Rule #6, but renumbering of subsequent rules was not completed → new defect: duplicate Rule #6 (Finding 1 above) | |
| 2. Step-numbering mismatch in Completion Criteria | ✅ Fixed | Completion Criteria now references Step 4 / Step 5 correctly |
| 3. Eval asserts verdict not process | Assertion changed to keyword pattern, but prompt is now primed with the exact assertion keywords → new defect (Finding 2 above) | |
| 4. Mutable live PR with no anchoring | Specific claim added, but PR #32278 is still live + shared with another scenario (Finding 6 above) | |
| 5. Prior-finding override undefined and abusable | Tied to ❌ Error taxonomy + "confirm finding still applies" — good — but 2000-char truncation can hide the very findings the override exists to catch (Finding 3 above) |
CI status
Skill Validation check is failing — overall skill-improvement score -2.2% (below the 10% threshold).
The new "Blast radius - infrastructure changes get probed" scenario added by this PR passed strongly (3.0 → 4.7/5). Failures come from pre-existing high-variance "negative trigger" scenarios (CV=4.60 and 16.87, with 300s timeouts) — the PR author acknowledged this in an earlier comment as pre-existing infra variance, not PR-caused. The new scenario itself does not regress CI.
That said, the new "Blast radius" scenario has the prompt/assertion issues called out in Finding 2 — its current passing score may not be a reliable signal.
Test coverage
- Eval coverage added for the new blast-radius behavior (one scenario).
- No eval coverage for the Prior Review Reconciliation behavior — the most substantial new logic on the LLM side. No scenario validates that an agent actually fetches
--json reviews, identifies prior❌ Errorfindings, or applies the override rule correctly. This is consistent with the prior review's note about gap-coverage.
Prior review thread
No prior human PR reviews on this thread. The PR author posted a multi-model adversarial review as a top-level comment (referenced as "prior review" above). All 5 findings from that review map to current-code status above.
Methodology
3 independent reviewers (Reviewer 1 / Reviewer 2 / Reviewer 3) using different models ran in parallel. Findings flagged by only one reviewer were routed to the other two for adversarial validation. Findings on shared/config files were impact-checked against the consumer skill (pr-review). Only findings reaching ≥2/3 consensus survived. Reviewer identities are not exposed.
Verdict
NEEDS_CHANGES — one ❌ Error finding (duplicate Rule #6 numbering) is a concrete structural defect in an LLM-consumed instruction file that was introduced by the fix commit and not present in the original PR. Three ⚠️ Warning findings are concrete and actionable. Three 💡 Suggestions are nice-to-haves.
The PR's intent and most of its content are sound — the regressions are localized and fixable in 1–2 small commits.
| 6. **Write findings to disk, do not post directly.** The agent does not have the GitHub comment token. Write findings to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/` — the CI pipeline or posting scripts handle GitHub interaction. | ||
| 5. **Prior ❌ Error findings override.** If any prior review flagged an ❌ Error-level issue (using this skill's severity taxonomy) that remains unresolved in the current code, verdict must be `NEEDS_CHANGES` regardless of your own assessment. Confirm the finding still applies to the current diff before applying the override. | ||
| 6. **Never LGTM if CI is red.** If required CI checks are failing, invoke `azdo-build-investigator` to determine whether failures are PR-caused. Do not post `LGTM` until CI passes or failures are confirmed unrelated. | ||
| 6. **🚨 NEVER use `--approve` or `--request-changes` on GitHub.** Only post comments. Approval is a human decision. |
There was a problem hiding this comment.
❌ Logic — Duplicate 6. in Verdict Consistency Rules
Flagged by: 3/3 reviewers
Lines 233 and 234 both start with 6.:
6. **Never LGTM if CI is red.** ...
6. **🚨 NEVER use `--approve` or `--request-changes` on GitHub.** ...
7. **Write findings to disk, do not post directly.** ...
This skill is an LLM-consumed instruction file where numbered rules are parsed sequentially. The duplicate creates ambiguity — the safety-critical "NEVER --approve" rule shares a number with the CI-gate rule. Any subsequent text that says "see Rule 6" becomes ambiguous, and an LLM reasoning over the list may dedupe or deprioritize one of the two 6. items.
This is a new defect introduced by commit 01a91eacc3 when restoring the CI gate as Rule #6 without renumbering the subsequent rules. The prior multi-model review's Finding #1 ("CI hard gate removed") is therefore only partially fixed.
Suggested fix: Renumber to 6 (CI gate), 7 (NEVER --approve), 8 (Write to disk).
| timeout: 120 | ||
|
|
||
| - name: "Blast radius - infrastructure changes get probed" | ||
| prompt: "code review PR #32278 in dotnet/maui — I believe the handler changes affect platform extensions that run unconditionally at startup, even for controls that don't use the new feature" |
There was a problem hiding this comment.
Flagged by: 3/3 reviewers
Two compounding issues with this scenario:
1. Premise contradicts the underlying PR. PR #32278 is [Android] Refactor WindowInsetListener to per-view registry with MauiWindowInsetListener — it removes the global unconditional listener and replaces it with per-view registration. The prompt premise ("handler changes affect platform extensions that run unconditionally at startup, even for controls that don't use the new feature") is the opposite of what the PR does. A correct reviewer should refute the premise — but the rubric and assertion don't credit refutation. They reward agents that go along with a false claim.
2. Primed assertion. The prompt injects the exact keywords (startup, unconditional) that the assertion regex tests for: (blast radius|all instances|startup|handler disconnect|unconditional). Even a degraded agent that just echoes the prompt's words back will pass. Worse, bare-keyword matching also matches negative phrasings: an agent that correctly says "does NOT run at startup, NOT unconditional" passes the same assertion, providing zero discrimination between correct refutation and confused echo.
Prior review Finding #3 (assertion-vs-rubric mismatch) was partially addressed by switching the assertion to keywords, but this introduced these two new defects.
Suggested fix: Either (a) pick a PR that genuinely has unconditional startup code, or (b) reframe the prompt as a hypothesis to verify/refute and add rubric language requiring evidence-backed acceptance or rejection. Consider also adding an assertion on the confidence level — e.g. (medium|low) — which cannot be primed from the prompt.
|
|
||
| ```bash | ||
| gh pr checks <PR_NUMBER> | ||
| gh pr view <PR_NUMBER> --json reviews --jq '.reviews[] | "Reviewer: \(.author.login) | State: \(.state)\n\(.body[0:2000])\n---"' |
There was a problem hiding this comment.
[0:2000] truncation can hide the very findings the Prior Review Reconciliation override is designed to catch
Flagged by: 3/3 reviewers
gh pr view <PR_NUMBER> --json reviews --jq '.reviews[] | "Reviewer: \(.author.login) | State: \(.state)\n\(.body[0:2000])\n---"'Two issues:
1. Truncation defeats the override. Multi-model reviews and the Copilot reviewer bot routinely produce 5–15 KB review bodies where the ❌ Error markers sit deep in the "Findings" section. If the ❌ is at char 2200, the agent sees a truncated body, finds no markers, concludes "No prior ❌ Error findings found," and proceeds to LGTM — silently violating Rule 5. This is the exact behavior the override was added to prevent. The fix from Prior Finding #5 ("confirm the finding still applies before applying the override") doesn't help if the finding is never visible in the first place.
2. .body can be null for reviews submitted without a body (e.g., approval-only). null[0:2000] returns null in jq, which interpolates as the literal string "null" — noisy but not crashing.
Suggested fix: Greatly increase the cap (e.g., [0:10000]), OR filter specifically for ❌ markers and only then surface body text:
.reviews[] | select((.body // "") | contains("❌")) | "Reviewer: \(.author.login) | State: \(.state)\n\(.body)\n---"Also add // "" null coalescing for the truncated-display path.
|
|
||
| | Evidence | Confidence Cap | | ||
| |----------|---------------| | ||
| | CI red or pending | Defer to `azdo-build-investigator` skill for CI analysis | |
There was a problem hiding this comment.
💡 Process — Confidence-cap table row mixes an action instruction with cap values
Flagged by: 2/3 reviewers
| Evidence | Confidence Cap |
|----------|---------------|
| CI red or pending | Defer to `azdo-build-investigator` skill for CI analysis | ← action, not a cap
| No relevant tests run (UITests skip PR builds) | Max **low** |
| Prior critical findings unresolved | **NEEDS_CHANGES** (no LGTM) |
Every other row in the "Confidence Cap" column holds an explicit cap value (Max low, NEEDS_CHANGES). The "CI red or pending" row instead holds a delegation instruction. An LLM consuming this for confidence calibration may interpret "Defer" as "this row doesn't constrain confidence" — resulting in e.g. confidence: high, verdict: NEEDS_DISCUSSION while CI is red. (Rule #6 only blocks the LGTM verdict, not the confidence label.)
Suggested fix: Either split into two columns (Confidence Cap + Required Action), OR change the cell to an explicit cap (Max low) and move "defer to azdo-build-investigator" to a sentence beneath the table.
| - "No severity markers (❌/⚠️/💡) or verdicts appear in the output" | ||
| timeout: 120 | ||
|
|
||
| - name: "Blast radius - infrastructure changes get probed" |
There was a problem hiding this comment.
💡 Testing — This scenario reuses the same live PR (#32278) as the existing "Verdict consistency" scenario
Flagged by: 2/3 reviewers
PR #32278 is referenced by both this new scenario and the existing "Verdict consistency - errors must map to NEEDS_CHANGES" scenario (line 59). Consequences:
- Attribution ambiguity in CI. If PR [Android] Refactor WindowInsetListener to per-view registry with MauiWindowInsetListener #32278's state changes (force-push, branch retargeting, etc.) both scenarios fail at once and the CI signal can't isolate which invariant broke.
- Compounded brittleness. Prior multi-model review Finding Aloha System.Maui! #4 was "depends on mutable live PR state with no anchoring." The fix-commit added a specific claim to the prompt, but the underlying instability — live PR + same anchor PR across multiple scenarios — remains.
Suggested fix: Pick a different stable, merged PR for one of the two scenarios so failures localize cleanly.
Design-level findings (outside the diff)These two findings target PR metadata and a documentation contract — they can't be attached to specific diff lines.
|
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Multi-Model Adversarial Code Review — PR #35478 (Round 3)
3 independent reviewers analyzed the round-3 commit (b82bdba49f) in parallel. One initial reviewer's findings were superseded after a worktree fast-forward picked up the round-3 changes; a tiebreaker reviewer validated the remaining 1/3 findings.
Round-3 fix verification — round-2 findings revisited
| Round-2 finding | Status in b82bdba49f |
|---|---|
❌ Duplicate 6. in Verdict Consistency Rules |
✅ Fixed — rules now 6/7/8 sequentially |
✅ Fixed — PR #35223 (genuinely matches), hypothesis framing, (medium|low) assertion, evidence-backed rubric |
|
gh pr view --jq [0:2000] truncation |
✅ Fixed — [0:10000] + select((.body // "") != "") null guard |
| ❌ Still unresolved — see below | |
| 💡 Confidence-cap table mixes caps with action | ✅ Fixed — "Max low — invoke azdo-build-investigator..." |
| 💡 Two eval scenarios share PR #32278 | ✅ Fixed — new scenario uses PR #35223; existing scenario still uses #32278 |
| 💡 Outputs table missing blast-radius/failure-mode fields | Not addressed (was disputed 2/3; acceptable to leave) |
5 of 6 substantive findings addressed correctly. The fix to the jq command (select((.body // "") != "") followed by .body[0:10000]) is correct: it filters null/empty bodies before slicing.
Remaining issue — PR description still stale
The commit message for b82bdba49f item 7 says "Update PR description to match current implementation", but the PR description on GitHub is unchanged. It still reads:
- "CI verification hard gate (Step 5)" — CI verification was removed as a step in commit
ef6b6c49c2and is now Verdict Consistency Rule #6 - "Blast radius assessment (Step 6)" — actually Step 5
- "Failure-mode probing (Step 6)" — actually Step 5
Commits modify files, not PR descriptions. This needs gh pr edit 35478 --repo dotnet/maui --body "..." or a web-UI edit. MAUI's pr-finalize policy requires the description to match implementation before merge.
3/3 consensus across rounds 2 and 3.
New findings (round-3 self-introduced and eval design)
Two minor 💡 observations on the new eval scenario (see inline comments at eval.yaml:86 and eval.yaml:91):
- The hypothesis is ambiguous about pre-PR vs post-PR state; the rubric credits both correct refutation and incorrect acceptance.
- The new
(medium|low)assertion is a bare substring match that can be triggered by phrases like "medium-sized change" — anchor it to a confidence field.
Both are 2/3 (initial dispute + tiebreaker). Non-blocking — eval design refinements that can land in a follow-up.
CI status
evaluate (code-review)— pending (re-running afterb82bdba49f)Skill Validation— last completed run was -2.2% improvement (below the 10% threshold), but the failing scenarios are pre-existing high-variance "negative trigger" tests (CV=4.60, 16.87, 300s timeouts) acknowledged by the author as infra variance, not PR-caused. The new "Blast radius" scenario passed strongly (3.0 → 4.7/5) — and now with the round-3 prompt/assertion improvements, its signal should also be slightly more rigorous (modulo the inline observations).- Cannot LGTM until the eval check resolves (Verdict Consistency Rule #6).
Test coverage
- Eval coverage added for blast-radius behavior (one scenario, now better-framed).
- Still no eval coverage for the Prior Review Reconciliation behavior — no scenario validates that an agent actually fetches
--json reviews, parses prior❌ Errorfindings, or applies the override. Carried forward from round 2.
Prior review reconciliation
This is round 3 of multi-model review on this PR. Round-2 findings (review #4311858863 + design comment #4479590465) are tracked in the table above. 5/6 substantive findings are now resolved.
Methodology
3 independent reviewers (Reviewer 1 / Reviewer 2 / Reviewer 3) using different models analyzed the round-3 commit in parallel. One reviewer ran against a slightly-stale worktree (1 commit behind), so its findings were discarded as out-of-date. A tiebreaker reviewer validated the 1/3 findings from the two fresh reviewers. Only ≥2/3-consensus findings survived.
Verdict
NEEDS_CHANGES — one gh pr edit). Two 💡 Suggestions on eval design are non-blocking. Round-3 code fixes are otherwise sound. CI must also resolve before merge.
| timeout: 120 | ||
|
|
||
| - name: "Blast radius - infrastructure changes get probed" | ||
| prompt: "code review PR #35223 in dotnet/maui — this PR changes back-navigation callback registration on Android. Hypothesis: the registration may run unconditionally for all activities, not just those using predictive back. Please verify or refute." |
There was a problem hiding this comment.
💡 Testing — Hypothesis is ambiguous about temporal state (pre-PR vs post-PR)
Flagged by: 2/3 reviewers
PR #35223 is the FIX that removes unconditional registration — its root cause description literally says "MauiAppCompatActivity registering a predictive-back callback unconditionally on Android 13+." The hypothesis "the registration may run unconditionally for all activities" is therefore:
- TRUE for pre-PR code (the bug being fixed)
- FALSE for post-PR code (this PR makes it conditional)
The prompt doesn't tell the agent which state to evaluate, and the rubric line "The agent provides evidence-backed acceptance or refutation of the hypothesis — not just echoing it" credits either outcome as long as evidence is provided. A correct refutation and an incorrect acceptance both pass — the test is non-discriminating.
Suggested fix: Reframe so refutation is the unique correct answer, e.g.: "Hypothesis: even after this PR, the registration still runs unconditionally for all activities. Please verify or refute." This makes a wrong "accept" fail the rubric on factual grounds.
Non-blocking — eval design refinement.
| - type: "output_matches" | ||
| pattern: "(blast radius|all instances|unconditional|callback registration)" | ||
| - type: "output_matches" | ||
| pattern: "(medium|low)" |
There was a problem hiding this comment.
💡 Testing — (medium|low) substring match is too permissive (round-3 self-introduced)
Flagged by: 2/3 reviewers
The output_matches assertion uses a bare substring regex, so any output containing "medium" or "low" anywhere passes — including innocuous phrases like "a medium-sized change", "low risk of regression", or "medium-priority bug". The skill's own output format at SKILL.md:219 declares confidence as **Confidence:** high | medium | low, so the assertion can be made much more discriminating without losing valid matches.
Suggested fix:
- type: "output_matches"
pattern: "[Cc]onfidence[:*]*\\s*(medium|low)"or pin to the exact field name used in the skill's structured output.
Non-blocking — eval design refinement.
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
|
|
||
| | Evidence | Confidence Cap | | ||
| |----------|---------------| | ||
| | CI red or pending | Max **low** — invoke `azdo-build-investigator` skill for CI analysis before finalizing | |
There was a problem hiding this comment.
[major] Regression Prevention - This keeps the CI verdict rule, but the workflow no longer has a required step that fetches CI status before finalizing. The previous explicit gh pr checks <PR_NUMBER> step was removed, so an agent following the new workflow can reach verdict delivery without collecting the evidence needed to know whether CI is red or pending. Concrete scenario: required checks are failing, but the reviewer never runs a checks command and still emits LGTM. Add a mandatory CI-status collection step/output field before confidence/verdict calibration.
MauiBot
added
s/agent-review-incomplete
s/agent-fix-win
This comment has been minimized.
This comment has been minimized.
PureWeen
added
the
area-ai-agents
b82bdba to
fcc0d57
Compare
Round-4 adversarial review identified 6 issues. Applying all of them.
1. (3/3 \xE2\x9D\x8C critical) Step 4 Prior Review Reconciliation only queried
top-level review bodies via 'gh pr view --json reviews'. MauiBot, Copilot,
and this skill's own adversarial reviewer post stubs at the top level
('Expert Review \xE2\x80\x94 N findings, see inline comments') and put the
actual severity-tagged findings in inline review comments. An agent
following the documented command would conclude 'No prior \xE2\x9D\x8C Error
findings found' even when 3/3-consensus errors were sitting in inline
comments. Empirically verified on PR #35478: top-level query returns
stub bodies; inline query returns the real findings.
Fix: instructions now require querying BOTH 'gh pr view --json reviews'
AND 'gh api .../pulls/<PR>/comments --paginate' and explain why.
2. (3/3 \xE2\x9D\x8C critical) Step 5 fallback ambiguity: 'gh pr checks --required'
exits with code 1 when checks are failing and 8 when pending. An LLM
agent could misinterpret non-zero exit as 'tool unavailable' and apply
the fallback (cap confidence at low) instead of the 'PR-caused \xE2\x9D\x8C
NEEDS_CHANGES' classification \xE2\x80\x94 silently downgrading a red CI into a
confidence cap.
Fix: explicit exit-code semantics note added before classification.
Tool-unavailable fallback restricted to actual 'command not found' /
auth-error scenarios.
3. (2/3 \xE2\x9A\xA0\xEF\xB8\x8F) Step 5 didn't classify skipped/pending/empty required
checks. An agent could treat a draft PR with no required checks
configured as 'clean build' and LGTM.
Fix: new bullet covers 'skipped / pending / empty result' \xE2\x86\x92
undetermined coverage, cap confidence at low.
4. (2/3 \xE2\x9A\xA0\xEF\xB8\x8F) Output constraint 'Don't flag what CI catches' could be
read to contradict Step 5's 'flag PR-caused failures as \xE2\x9D\x8C Error'. A
scope-confused agent could suppress CI-driven verdict signals.
Fix: constraint reworded to 'Don't duplicate CI output as inline
comments' \xE2\x80\x94 makes clear the rule governs inline-comment surface only,
not verdict/summary surface.
5. (2/3 \xF0\x9F\x92\xA1) Rule #6 and Step 6 confidence-cap table were
reconcilable but written in three places with diverging phrasing.
Fix: Step 6 table row now cross-references Rule #6. Rule #6 now
explicitly notes the Step 6 confidence cap still applies even when red
failures are confirmed PR-unrelated.
6. (3/3 \xE2\x9A\xA0\xEF\xB8\x8F) Blast-radius eval scenario's first assertion
'(blast radius|all instances|unconditional|callback registration)' was
satisfiable by parroting the prompt (which contains 'unconditionally'
and 'callback registration'). The deterministic layer added near-zero
signal for the behavior the scenario claims to test.
Fix: first assertion now requires analytical vocabulary NOT in the
prompt (blast radius, all instances, every/each instance). Third
assertion added requiring refutation evidence: '\bconditional' (word
boundary prevents matching inside 'unconditional'), 'guarded', 'gated',
'opt-in', 'refut', 'hypothesis is false', 'now scoped', 'no longer'.
Empirically verified: 8/9 realistic refutations PASS, all 4 parroting
attempts FAIL.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Multi-Model Adversarial Code Review — PR #35478 (Round 4)
3 independent reviewers analyzed fcc0d571e5 in parallel. After adversarial consensus (including 2 follow-up reviewers for disputed 1/3 findings), 6 findings reached consensus and have been applied in 7bfcf35b5e rather than left as comments to action.
Findings & Resolutions
| # | Severity | Consensus | Finding | Resolution |
|---|---|---|---|---|
| 1 | ❌ | 3/3 (after follow-up; both validators AGREE) | Step 4 Prior Review Reconciliation only queried top-level review bodies. Most automated reviewers (this skill's own reviewer, the Copilot bot, etc.) post stubs at the top level and the actual ❌/⚠️/💡 findings in inline comments. Empirically verified — there's a 3/3-consensus ❌ Logic finding sitting in inline comments on this very PR that the documented command misses. |
Instructions now require querying BOTH gh pr view --json reviews AND gh api .../pulls/<PR>/comments --paginate. |
| 2 | ❌ | 3/3 (after follow-up; both validators AGREE) | gh pr checks --required exits with code 1 when checks are failing. An LLM agent could misinterpret non-zero exit as "tool unavailable" and apply the cap-confidence fallback instead of flagging ❌ NEEDS_CHANGES. |
Explicit exit-code semantics note added before classification (1=failing, 8=pending, 0=passing). Tool-unavailable fallback restricted to command not found / auth errors. |
| 3 | 2/3 | Step 5 didn't handle skipped/pending/empty required-check rows. A draft PR with no required checks could be classified as "clean build". | New bullet for "skipped, pending, or empty result" → undetermined coverage, cap confidence at low. | |
| 4 | 2/3 (after follow-up) | "Don't flag what CI catches" output constraint could be read to contradict Step 5's "flag PR-caused failures as ❌ Error". | Constraint reworded to "Don't duplicate CI output as inline comments" with explicit carve-out that CI failures still drive the verdict/summary. | |
| 5 | 💡 | 2/3 | Step 6 confidence-cap table, Rule #6, and Step 5 wording were reconcilable but in three places with diverging phrasing — drift risk. | Step 6 table row now cross-references Rule #6. Rule #6 now explicitly notes the Step 6 confidence cap still applies when red failures are confirmed PR-unrelated. |
| 6 | 3/3 | The new blast-radius eval scenario's first assertion (blast radius|all instances|unconditional|callback registration) was satisfiable by parroting prompt terms — the deterministic layer added near-zero signal. |
First assertion now requires vocabulary NOT in the prompt (blast radius|all instances|every instance|each instance). Third assertion added requiring refutation evidence (\bconditional|guarded|gated|opt-in|refut|hypothesis is false|now scoped|no longer), with \b anchor to prevent matching conditional inside unconditional. Empirically validated: 8/9 realistic refutations PASS, 4/4 parroting attempts FAIL. |
Discarded (single-reviewer, not validated)
- 💡 Hardcoded step-number references in completion criteria could drift — minor doc-debt, not actionable.
- 💡 Blast-radius rubric hard-codes a factual premise about PR #35223 — true but external-PR brittleness is inherent to eval scenarios that target real PRs.
- 💡 Negative-trigger eval (
Failure-Mode Probing) is case-sensitive — defensible: the SKILL.md output-format heading is### Failure-Mode Probing(TitleCase), which is exactly what real review output emits. ⚠️ Revert Step 5 reintroduction (scope: code review ≠ CI gating) — 1/3 from one reviewer, but another reviewer explicitly disagreed with detailed reasoning. Even with multi-round self-correction threshold lowering, the explicit counter-vote prevents 2/3 consensus.
Methodology
3 independent reviewers with adversarial consensus. 1/3 disputed findings (E, F, G) sent through cross-validation with the other 2 reviewers. Findings E and G were validated 2/2 (becoming 3/3); finding F was validated 1/1 + 1/1-mild-disagree (becoming 2/3). All consensus thresholds documented per skill rules.
This is Round 4 of multi-round review. Round-4 changes (Step 5 reintroduction + others) were subject to multi-round self-correction: each Round-4 change was re-examined for evidence of regression, and the reintroduction of Step 5 was confirmed as materially different from the Round-1 form (adds failure classification, fallback semantics, and tool-existence checks).
Prior Review Status
5 prior reviews on this PR (Rounds 2 & 3, MauiBot, kubaflo CHANGES_REQUESTED). All prior [major]/[moderate]/❌ findings from MauiBot and the prior rounds were already addressed in fcc0d571e5. This commit adds the Round-4 adversarial findings.
Round 5 dispatched 3 fresh reviewers (different model families). Nine findings reached consensus (1 \xE2\x9D\x8C, 7 \xE2\x9A\xA0\xEF\xB8\x8F, 1 \xF0\x9F\x92\xA1). Several were orchestrator-verified empirically against PR #35478's actual API surface. 1. (\xE2\x9D\x8C Logic, R3) Rule 8 contradicted Step 2. Step 2 explicitly tells the agent to post inline findings directly via 'gh api ... reviews --method POST' when COMMENTS_VIA_FILE is unset (local mode). Rule 8 said 'Write findings to disk, do not post directly. The agent does not have the GitHub comment token.' An LLM following both gets a hard contradiction. Rule 8 now distinguishes CI mode (write to disk) from local mode (Step 2's direct-post path is allowed). 2. (\xE2\x9A\xA0\xEF\xB8\x8F Regression, R2 + orchestrator-verified) Prior Review Reconciliation only queried 2 of 3 reviewer surfaces. MauiBot's AI Summary comments, PureWeen's multi-model review summary comments, and other wall-of-text summary reviews are posted as PR ISSUE comments (gh api repos/<o>/<r>/issues/<PR>/comments) \xE2\x80\x94 which the review API does not return. Verified on this PR: those exact comments contain \xE2\x9D\x8C/\xE2\x9A\xA0\xEF\xB8\x8F findings that the documented commands would miss. Step 4 now queries all three surfaces. 3. (\xE2\x9A\xA0\xEF\xB8\x8F Logic, R2 + orchestrator-verified) Step 5 exit-code claim said '0 if all required checks pass', but empirically 'gh pr checks --required' on this PR returns exit 0 with 'maui-pr skipping' \xE2\x80\x94 a skipped required check is not 'passing'. PRs with zero required checks also exit 0. Step 5 now clarifies exit 0 is not a clean-pass signal and you must always inspect stdout rows. 4. (\xE2\x9A\xA0\xEF\xB8\x8F LLM-Prompt-Ambiguity, R1) Pending CI had only a soft anti-LGTM guard. Step 5 said 'prefer NEEDS_DISCUSSION over LGTM' and Rule #6 only hard-prohibited LGTM for RED (failing) CI, not pending. An agent could LGTM with low confidence while required checks were still running \xE2\x80\x94 the exact 'silently approve with confidence' class this PR is meant to eliminate. Step 5 now says 'do NOT post LGTM \xE2\x80\x94 use NEEDS_DISCUSSION' and Rule #6 extends to red/pending/undetermined. 5. (\xF0\x9F\x92\xA1 Eval-Assertion, R1 + R3 = 2/3) Eval regex assertions were case-sensitive. A compliant agent that capitalizes 'Medium' (e.g., '**Confidence:** Medium') or starts a sentence with 'Conditional...' would falsely fail the assertion. Used [Mm] / [Cc] character classes throughout for case tolerance without resorting to (?i) (more portable). 6. (\xE2\x9A\xA0\xEF\xB8\x8F Cross-Reference-Drift, R3) Step 5 instructs the agent to surface CI results in 'the CI Status / Verdict sections', but the output template never defined a CI Status section. Agents would either invent one or cram CI logs into Findings. Added '### CI Status' section to the template between Blast Radius Assessment and Findings. 7. (\xE2\x9A\xA0\xEF\xB8\x8F LLM-Prompt-Ambiguity, R3) Step 4 used 'repos/<OWNER>/<REPO>/...' placeholders while Step 2 hardcoded 'repos/dotnet/maui/...'. LLMs sometimes execute code blocks literally without placeholder substitution, returning 404 and falsely concluding no prior reviews exist. Standardized all gh api calls to hardcoded 'dotnet/maui'. 8. (\xE2\x9A\xA0\xEF\xB8\x8F Shell-Edge-Case, R3) Step 5's claim 'non-zero exit means the tool succeeded and is reporting check status' was too absolute. Tool unavailability (auth failure, network error, missing gh) also exits non-zero with messages that should trigger the tool-unavailable fallback. Scoped the 'tool succeeded' claim to exit codes 1 and 8 specifically, and explicitly carved out other non-zero codes. 9. (\xE2\x9A\xA0\xEF\xB8\x8F Eval-Assertion, R2) Refutation regex contained 'refut' substring, which matched the prompt's literal phrase 'verify or refute' \xE2\x80\x94 a parroting agent that just echoed the prompt would pass without demonstrating analysis. Replaced 'refut' with 'refuted|refutes| refutation' (past-tense / noun forms that demonstrate completed analysis vs the bare infinitive). Empirically validated: prompt echoes FAIL, realistic refutations PASS. Discarded as 1/3 with no orchestrator confirmation: - (\xF0\x9F\x92\xA1 R1) First blast-radius assertion satisfiable by template echo. R1 itself said the scenario is not broken overall \xE2\x80\x94 assertions 2+3 still catch parrots; minor signal reduction is acceptable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Multi-Model Adversarial Code Review — PR #35478 (Round 5)
3 independent reviewers analyzed 7bfcf35b5e (the prior Round-4 commit) in parallel. Nine findings reached consensus or were orchestrator-verified empirically against this PR's actual GitHub-API surface. All 9 have been applied directly in commit 9440744c2c rather than left as comments to action.
Findings & Resolutions
| # | Severity | Source | Finding | Resolution |
|---|---|---|---|---|
| 1 | ❌ | R3 | Rule 8 vs Step 2 contradiction. Step 2 explicitly tells the agent to gh api ... reviews --method POST directly in local mode, but Rule 8 said "Write findings to disk, do not post directly. The agent does not have the GitHub comment token." A hard contradiction. |
Rule 8 now distinguishes CI mode (write to disk, pipeline posts) from local mode (Step 2's direct-post path is allowed). |
| 2 | R2 + orchestrator-verified | Prior Review Reconciliation missed the 3rd surface. MauiBot AI Summary comments + prior multi-model review wall-of-text summaries are PR-issue comments, not review-API comments. Verified on this very PR: those issue comments contain real ❌/ |
Step 4 now queries all three surfaces: review bodies + inline review comments + gh api repos/dotnet/maui/issues/<PR>/comments. |
|
| 3 | R2 + orchestrator-verified | gh pr checks --required exit 0 ≠ all pass. Verified on this PR: 1 skipped returned exit 0 with "All checks were successful". A PR with no required checks configured also exits 0. Round-4's exit-code semantics overstated what 0 means. |
Clarified: exit 0 means no failing/pending checks (but skipped checks also exit 0 — always inspect stdout rows). |
|
| 4 | R1 | Pending CI had only a soft anti-LGTM guard. Step 5 said "prefer NEEDS_DISCUSSION over LGTM" while Rule #6 hard-prohibited LGTM only for red CI. Same silent-approval class this PR targets, one state over. | Step 5 now reads "do NOT post LGTM — use NEEDS_DISCUSSION". Rule #6 extended: "Never LGTM if CI is red, pending, or undetermined." |
|
| 5 | 💡 | R1 + R3 (2/3) | Eval regex assertions case-sensitive. A compliant agent capitalizing **Confidence:** Medium or Conditional registration... would falsely fail. |
Used [Mm]/[Cc] character classes throughout eval.yaml assertions for case tolerance (more portable than (?i)). |
| 6 | R3 | Output template missing the "CI Status" section Step 5 referenced. Agents would either invent the section or cram CI logs into Findings. | Added ### CI Status to the template between Blast Radius Assessment and Findings. |
|
| 7 | R3 | <OWNER>/<REPO> placeholder vs hardcoded dotnet/maui. Step 4 used placeholders that LLMs sometimes execute literally → HTTP 404 → false "no prior reviews". |
Standardized all gh api repos/... references to hardcoded dotnet/maui. |
|
| 8 | R3 | "Non-zero exit means tool succeeded" was too absolute. Auth failure, network error, or invalid PR also exit non-zero. | Scoped the "tool succeeded" claim to exit codes 1 and 8 specifically; carved out other non-zero codes as tool-unavailable. |
|
| 9 | R2 | Refutation regex refut matched the prompt's literal "verify or refute". A parroting agent that echoes the prompt without analysis would pass. |
Replaced bare refut with refuted|refutes|refutation (forms that demonstrate completed analysis vs the prompt's infinitive). Empirically validated: 5/5 realistic refutations pass, 2/2 prompt parrots fail. |
Discarded (single-reviewer, not orchestrator-verified)
- 💡 (R1) First blast-radius assertion satisfiable by SKILL.md template echo. R1 explicitly noted the scenario is not broken overall — assertions 2 and 3 still catch parrots. Minor signal reduction is acceptable.
Methodology
3 independent reviewers with adversarial consensus. For findings the orchestrator could verify empirically (against the PR's actual GitHub-API surface or with hand-traced regex), the verification supplemented cross-validation; for others, the multi-round self-correction rule was applied (lower threshold for reverts of self-introduced changes). All findings are textually evidenced and the regex changes were validated against both parrot inputs (must fail) and realistic refutations (must pass).
This is Round 5. Round-4 fixes from 7bfcf35b5e were retained where reviewers agreed they were materially correct (e.g., the 3-surface query design from Round 4 stands — Round 5 just extended it to add a missing 3rd surface that Round 4 didn't catch).
|
/review -b feature/regression-check -p ios |
Round 6 dispatched 3 fresh reviewers (different model families). Seven findings reached consensus or were orchestrator-verified empirically. Two of them caught factual errors in the Round-5 commit message itself. CORRECTIONS to Round-5 claims: - Round-5 said exit 1 from 'gh pr checks --required' uniquely means 'failing checks'. Empirically WRONG: gh exits 1 for invalid PR / GraphQL error too (verified: 'gh pr checks 99999999' returns exit=1 with 'GraphQL: Could not resolve to a PullRequest'). Round-5's empirical claim that this case exits 0 was a measurement error. - Round-5 commit message said '[Bb]last radius' was made case-tolerant for the SKILL.md heading 'Blast Radius'. Empirically WRONG: only the leading 'B' was tolerated; lowercase 'r' in 'radius' was hardcoded, so the regex doesn't match 'Blast Radius' at all. Round-5 saved by the '[Aa]ll instances' alternative because the template happens to use that phrase, but the heading match was dead code. Round-6 findings (2 \xE2\x9D\x8C, 4 \xE2\x9A\xA0\xEF\xB8\x8F, 1 \xF0\x9F\x92\xA1): 1. (\xE2\x9D\x8C Logic, R2 + empirically verified) Step 5 exit-code 1 claim was wrong. 'gh pr checks --required' exit code 1 is OVERLOADED — it can mean failing checks OR a tool/API error (invalid PR, auth failure, network error, GraphQL error). Rewrote Step 5 to acknowledge the overload, require stdout/stderr inspection to distinguish, and route tool errors to the fallback rather than the failing-checks branch. 2. (\xE2\x9D\x8C Internal-Contradiction, R3) Round-5 Rule #8 ambiguously said 'the agent ... write findings to inline-findings.json' \xE2\x80\x94 but Step 2 says the maui-expert-reviewer (a different sub-agent) owns that file. An LLM following Rule #8 literally would have the code-review agent overwrite the expert reviewer's structured JSON with its own markdown, breaking post-inline-review.ps1. Rewrote Rule #8 to: (a) name which agent owns inline-findings.json (expert reviewer, in Step 2), (b) explicitly prohibit the code-review agent from emitting/overwriting it, (c) note the code-review agent's wall-of-text is posted by post-ai-summary-comment.ps1 separately. 3. (\xE2\x9A\xA0\xEF\xB8\x8F Eval-Assertion, R1 + R3 = 2/3, empirically verified) eval.yaml regex '[Bb]last radius' did not match 'Blast Radius' (only B was case-tolerant; r was hardcoded lowercase). Fixed to '[Bb]last [Rr]adius' and tightened the other alternatives similarly. Verified all 7 styles ('Blast Radius', 'Blast radius', 'blast radius', 'All Instances', etc.) now match. 4. (\xE2\x9A\xA0\xEF\xB8\x8F Eval-Assertion, R2 + empirically verified) The blast-radius eval was bypassable by a 3-line parrot template: ### Blast Radius Assessment **Confidence:** low Hypothesis is false. With the regex fix in #3 (which makes assertion 1 match the TitleCase heading), this parrot would satisfy all 3 existing assertions without the agent doing any code analysis. Added a 4th assertion requiring at least one concrete symbol from PR #35223's actual diff (MauiOnBackPressedCallback, ShouldRegisterPredictiveBackCallback, OnBackPressedDispatcher, OnBackPressedCallback, IBackNavigationState, HandleOnBackPressed, MauiAppCompatActivity). Empirically validated: parrot fails the 4th assertion; realistic competent output (which names at least one of these symbols when explaining the PR's fix) passes all 4. 5. (\xE2\x9A\xA0\xEF\xB8\x8F Internal-Contradiction, R2 + R1 = 2/3) Round-5 Rule #8 permits local direct posting, but 'Posting the Review' section unconditionally said 'The agent writes findings to disk. Posting is done separately by Review-PR.ps1.' Qualified the section's intro with the CI vs local distinction so an agent reading both gets consistent guidance. 6. (\xE2\x9A\xA0\xEF\xB8\x8F Cross-Reference-Drift, R2) After Round 5 added a third prior-review surface (issue comments), line 128 still instructed 'Scan both outputs for ... markers'. Updated to 'Scan all three outputs'. 7. (\xE2\x9A\xA0\xEF\xB8\x8F Shell-Edge-Case, R3) 'gh pr view' and 'gh pr checks' calls in Steps 2, 4, and 5 lacked '--repo dotnet/maui'. If the skill is invoked from a forked clone, those commands would resolve the PR number against the fork (which doesn't have it) and return wrong data while gh api calls (already hardcoded) targeted upstream correctly. Added '--repo dotnet/maui' to all three. Discarded (1/3, no orchestrator confirmation): - (\xF0\x9F\x92\xA1 R1) Cosmetic note that the Round-5 commit message's claim about case tolerance was misleading. Already addressed implicitly by #3 above and by this commit message's corrections section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Multi-Model Adversarial Code Review — PR #35478 (Round 6)
3 independent reviewers analyzed 9440744c2c (the prior Round-5 commit) in parallel. Seven findings reached consensus or were orchestrator-verified empirically. Two of them caught factual errors in the Round-5 commit message itself — empirical re-tests showed Round-5 had measurement errors in two claims that I had marked "empirically verified".
All 7 findings have been applied directly in commit a90c71b0b4.
Round-5 Corrections
| Round-5 claim | Truth (Round-6 verified) |
|---|---|
gh pr checks 99999999 exits 0 with GraphQL error → exit 1 uniquely means "failing checks" |
Actually exits 1 with GraphQL error. Exit 1 is overloaded: failing checks or tool/API error. |
Regex [Bb]last radius made case-tolerant for the SKILL.md heading Blast Radius |
Only the leading B was tolerated; lowercase r in radius was hardcoded. Heading match was dead code; eval saved only by the [Aa]ll instances alternative. |
Findings & Resolutions
| # | Severity | Source | Finding | Resolution |
|---|---|---|---|---|
| 1 | ❌ | R2 + empirically verified | Step 5 exit-code 1 claim was wrong. gh pr checks --required exit 1 is overloaded — failing checks OR tool/API error. Rounds 4 & 5 both incorrectly claimed exit 1 was uniquely "failing checks". |
Rewrote Step 5: acknowledge the overload, require stdout/stderr inspection (failure rows in stdout vs. GraphQL:/Could not resolve/error: in stderr), route tool errors to the fallback. |
| 2 | ❌ | R3 | Round-5 Rule #8 ambiguously instructed "the agent" to write to inline-findings.json — but Step 2 says the maui-expert-reviewer sub-agent owns that file. An LLM following Rule #8 literally would overwrite the expert reviewer's structured JSON with markdown findings, breaking post-inline-review.ps1. |
Rewrote Rule #8: name which agent owns the file (expert reviewer, Step 2), explicitly prohibit the code-review agent from emitting/overwriting it, note its wall-of-text is posted separately by post-ai-summary-comment.ps1. |
| 3 | R1 + R3 (2/3) + empirically verified | [Bb]last radius doesn't match Blast Radius — only B was case-tolerant; r was hardcoded lowercase. |
Fixed to [Bb]last [Rr]adius and tightened other alternatives. Verified all 7 phrasings (Blast Radius, Blast radius, blast radius, All Instances, etc.) now match. |
|
| 4 | R2 + empirically verified | Eval bypassable by 3-line parrot template. Combined with fix #3, the parrot ### Blast Radius Assessment\n**Confidence:** low\nHypothesis is false. would satisfy all 3 existing assertions without code analysis. |
Added a 4th assertion requiring a concrete symbol from PR #35223's actual diff (MauiOnBackPressedCallback, ShouldRegisterPredictiveBackCallback, etc.). Validated: parrot fails the 4th; realistic competent output passes all 4. |
|
| 5 | R2 + R1 (2/3) | Round-5 Rule #8 vs "Posting the Review" drift. Rule #8 now permits local direct posting; the section's intro still unconditionally said "The agent writes findings to disk." | Qualified the section's intro with the CI vs local distinction. | |
| 6 | R2 | "Scan both outputs" stale after Round-5 added the 3rd surface to Step 4. | Updated to "Scan all three outputs". | |
| 7 | R3 | gh pr view and gh pr checks lacked --repo dotnet/maui in Steps 2, 4, and 5. If invoked from a forked clone, they'd resolve the PR number against the fork (which doesn't have it) while the (already hardcoded) gh api calls correctly target upstream — leading to mixed/hallucinated CI state. |
Added --repo dotnet/maui to all three. |
Discarded (single-reviewer, not material)
- 💡 (R1) Cosmetic observation that the Round-5 commit message's case-tolerance claim was misleading. Addressed implicitly by #3 and by this commit's corrections section.
Methodology
3 independent reviewers with adversarial consensus. Two findings (#1, #4) re-tested the Round-5 commit's "empirically verified" claims and demonstrated they were wrong — exactly the kind of cross-round self-correction the rule was designed for. All regex changes were validated against parrot inputs (must fail) and realistic outputs (must pass). All shell-edge-case claims were empirically re-tested.
Net change: +25 / -13 across both files.
Round-7 adversarial review (3 reviewers; R1=clean, R2+R3 = 1 consensus issue, R3 found 3 additional 1/3 findings — orchestrator hand-verified each empirically). Applied (4): 1. eval.yaml assertion #4 (R2 + R3 = 2/3 consensus, empirically verified): tighten the symbol regex to MAUI-internal symbols only (MauiOnBackPressedCallback, ShouldRegisterPredictiveBackCallback, IBackNavigationState, HandleOnBackPressed). Drops the generic AndroidX types (OnBackPressedDispatcher, OnBackPressedCallback) and the well-known public base class (MauiAppCompatActivity) — all three were guessable from the prompt's back-navigation-callback hint without reading the diff, so a parrot agent that appended one of those names could pass all 4 assertions without doing analysis. Empirically validated: 3 parrot constructions now fail #4; realistic competent output still passes. 2. SKILL.md Step 5 exit-code paragraph (R3, empirically refuted my Round-6 claim): PRs with zero required checks actually exit 1 with stderr "no required checks reported on the '<branch>' branch" — NOT exit 0 as Round 6 incorrectly stated. Moved this case from the Exit 0 list to a new sub-bullet under Exit 1 alongside (a) failing checks and (c) gh tool errors, with explicit routing. Updated the Skipped/pending/empty bullet to mention the exact stderr message so agents recognize it. 3. SKILL.md Step 1 gh pr diff invocations (R3, same class of fix as Round-6 #7): add --repo dotnet/maui to both gh pr diff <PR_NUMBER> and gh pr diff <PR_NUMBER> --name-only so the commands work from any working directory, not just inside a dotnet/maui clone. 4. SKILL.md Rule #8 (R3, internal-contradiction with Review-PR.ps1:1459 which instructs the orchestrator agent to "always also write the raw inline findings to inline-findings.json"): clarify that the prohibition targets the wall-of-text-producing code-review agent specifically — when the orchestrator pipeline says "write inline findings", it means "ensure the Step 2 expert reviewer ran and produced the file", NOT "have the wall-of-text agent author the JSON". Distinguishes the orchestrator's verification responsibility from the prohibited prose-into-JSON pathway. Discarded (1): - R3 Finding 3 ("writes findings to disk" wording could cause hallucinated filename): speculative; no concrete failure scenario; R1 explicitly verified the Posting-the-Review section reads coherently. Methodology note: this round saw R1 declare no findings — the first clean reviewer result across 7 rounds. Combined with R2 finding only the consensus issue, we appear to be approaching diminishing returns. Future rounds should require stronger evidence before applying single-reviewer findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2ee15f1 to
c9f1b0e
Compare
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
🔍 Adversarial Review — Round 7
Three independent reviewers (multi-model). R1 declared no findings for the first time across 7 rounds — convergence signal. R2 and R3 found one consensus issue; R3 found three additional single-reviewer items that I hand-verified empirically before applying.
HEAD after this round: c9f1b0e605
Applied (4)
| # | Source | Severity | Change |
|---|---|---|---|
| 1 | R2 + R3 (2/3) | eval.yaml assertion #4 tightened — Dropped generic AndroidX types (OnBackPressedDispatcher, OnBackPressedCallback) and the well-known public base MauiAppCompatActivity from the symbol-name regex. All three were guessable from the prompt's "back-navigation callback" hint without reading the diff, allowing parrot agents to pass all 4 assertions without analysis. Now restricted to 4 MAUI-internal-only symbols (MauiOnBackPressedCallback, ShouldRegisterPredictiveBackCallback, IBackNavigationState, HandleOnBackPressed). Empirically validated: 3 parrot constructions now fail #4; realistic competent output still passes. |
|
| 2 | R3 (1/3, hand-verified) | ❌ | SKILL.md Step 5 exit-code prose — Round 6 incorrectly claimed PRs with zero required checks exit 0. Empirical test (PR #35804): they actually exit 1 with stderr no required checks reported on the '<branch>' branch. Moved the case from Exit-0 to a new sub-bullet (b) under Exit-1, alongside (a) failing checks and (c) gh tool errors, with explicit routing. Updated the "Skipped/pending/empty" bullet to recognize the exact stderr message. |
| 3 | R3 (1/3, hand-verified) | SKILL.md lines 57/62 — gh pr diff <PR_NUMBER> and gh pr diff <PR_NUMBER> --name-only now include --repo dotnet/maui. Same class of fix as Round-6 #7 (which I missed in those two lines). Validated: works from non-dotnet/maui working directories. |
|
| 4 | R3 (1/3, hand-verified) | SKILL.md Rule #8 clarified — Contradiction with Review-PR.ps1:1459 ("always also write the raw inline findings to inline-findings.json") resolved by distinguishing the wall-of-text-producing code-review agent (must NOT author the JSON) from the orchestrator pipeline (whose "write inline findings" instruction means "ensure the Step 2 expert reviewer ran and produced the file"). Schema-protection intent preserved; pipeline behavior no longer reads as contradictory. |
Discarded (1)
- R3: "writes findings to disk" wording in Posting the Review could cause hallucinated filenames. Speculative — R1 explicitly verified that section reads coherently and there's no concrete failure scenario.
Methodology evolution
- 7 rounds in; convergence is occurring. R1's clean signal is a meaningful first.
- Future rounds should raise the evidence bar for single-reviewer findings: orchestrator must empirically reproduce the failure mode, not just confirm the claim is plausible.
- One Round-6 claim of mine (zero-required-checks exit code) was empirically wrong. Round 7 corrected it. This is exactly what adversarial review is supposed to catch — bias toward "my prior work was right" is a real failure mode.
Methodology: 3 independent reviewers (mixed model families) → adversarial consensus → orchestrator empirical hand-verification of each 1/3 finding before apply.
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
🔍 Adversarial Review — Round 8
Three independent reviewers (multi-model). R1 (opus) clean for the second consecutive round — strong convergence signal. R2 and R3 each found one concrete, empirically-verifiable ❌ — both applied after orchestrator verification.
HEAD after this round: b2ba0ffe7c · Files changed: SKILL.md only · Diff: +6 / −6
Applied (2)
| # | Source | Severity | Change |
|---|---|---|---|
| 1 | R2 (1/3, hand-verified) | ❌ | Prior Review Reconciliation block (lines ~117, 125) — Removed .body[0:10000] and .body[0:5000] slices on the three gh queries used to scan prior reviews/comments. Empirically verified on this PR itself: MauiBot's most recent review body is 26,256 chars with 11 ❌ markers — 5 visible in first 10K and 6 hidden past the cutoff (>50% miss rate on the most recent automated review of this PR). Following pre-R8 instructions verbatim would silently miss those 6 markers and break the "Prior ❌ Error override" rule. Fix: use full .body; added explicit "do not truncate" callout with the empirical evidence. |
| 2 | R3 (1/3, hand-verified against cli/cli source) | ❌ | Step 5 Exit-1 sub-case (b) (lines 148/153/159) — gh pr checks --required emits two distinct "empty" stderr messages (verified at pkg/cmd/pr/checks/checks.go:303 and :308): no checks reported on the '<branch>' branch (zero checks of any kind) and no required checks reported on the '<branch>' branch (checks exist but none required). Pre-R8 prose only handled the latter; the former fell through to the tool-unavailable fallback path. Fix: match the shared substring checks reported and explicitly enumerate both spellings. Note: R3's proposed fix used substring no checks reported which would have inadvertently broken the common case (substring isn't contained in the longer message — "required" intervenes between "no" and "checks"); orchestrator corrected to the shared-substring fix before applying. |
Discarded (0)
R1 declared clean (second consecutive round). No 1/3 findings rejected — both R2 and R3 produced quantitatively verifiable issues, not interpretive nits.
Methodology evolution
- Convergence confirmed: R1 has now declared clean for two consecutive rounds. R2 and R3 each found exactly one concrete issue rather than several speculative ones. The skill is approaching steady state.
- Quantitative verification: Both Round-8 issues were verified with hard evidence (
26,256 chars / 11 ❌ / 6 hiddenfor #1;cli/clisource-line references for #2). No "this could be ambiguous" findings made it through this round. - Reviewer-fix correction: R3's proposed substring fix would have broken the common case while patching the rare one. The orchestrator's empirical verification step (rerun-and-validate) caught it before apply. This is a useful pattern to keep — single-reviewer findings need orchestrator validation not just of the problem but also of the proposed fix.
Methodology: 3 independent reviewers (mixed model families) → adversarial consensus → orchestrator empirical verification of both the finding and the proposed fix before apply.
…ks messages Round-8 adversarial review (3 reviewers; R1=clean, R2 and R3 each found one new ❌ issue, both empirically orchestrator-confirmed). Applied (2): 1. Prior Review Reconciliation block (R2, orchestrator-confirmed): remove the .body[0:10000] / .body[0:5000] slices on the three gh queries used to scan prior reviews and PR comments. Empirically verified on PR #35478 itself: MauiBot's most recent review body is 26,256 chars and contains 11 ❌ markers — 5 inside the first 10K and 6 hidden past the slice cutoff. An agent following the pre-R8 instructions verbatim would see 5 ❌ findings and silently miss 6 of them, materially defeating the "Prior ❌ Error findings override" rule (#5 in the verdict-rules section) and producing false LGTM verdicts. Fix: use full .body in all three jq queries and add an explicit "do not truncate" callout citing the empirical evidence. 2. Step 5 Exit-1 sub-case (b) (R3, orchestrator-confirmed against cli/cli source): gh pr checks --required emits TWO distinct "empty" stderr messages, not one. The pre-R8 prose only handled "no required checks reported on the '<branch>' branch", but the cli/cli source (pkg/cmd/pr/checks/checks.go:303 and :308) shows that "no checks reported on the '<branch>' branch" (without the word "required") fires when the PR has zero checks of any kind. Both cases share substring "checks reported" but neither is a substring of the other (the word "required" intervenes), so a strict literal match on "no required checks reported" misses the zero-checks case and that case falls through to the tool-unavailable fallback path — wrong classification. Fix: match the shared substring "checks reported" and explicitly enumerate both spellings, with routing to the same "Skipped, pending, or empty result" bullet for both. (R3 proposed substring "no checks reported" which would have BROKEN the common no-required-checks case while fixing the rare zero-checks case — orchestrator corrected the fix before applying.) Discarded (0): - All R1 findings (none — opus declared clean for the second consecutive round). - No 1/3 findings were rejected this round. Methodology note: R1's continued "no new findings" signal, plus R2 and R3 each finding a single concrete-empirically-verifiable issue (rather than several speculative ones), reinforces the Round-7 convergence observation. The Round-8 issues are quantitatively verifiable bugs (6 ❌ markers hidden; 2 stderr messages vs 1 documented), not interpretive nits. After this round the skill should be in a defensible state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🛠️ Round 8 correction — my earlier Round-8 review summary in this PR cited HEAD |
Multi-model code review — PR #35478Reviewed current HEAD Independent assessmentWhat this changes: The PR hardens the Reconciliation with PR narrative: The implementation matches the stated intent, and the latest commits address many earlier round findings. The remaining concerns are around validation coverage, ambiguous fallback wording, and the local posting path. CI / validation status
Findings❌ Error — Skill validation is still red, and the new hard gates lack direct eval coverageThe PR adds safety-critical behavior for prior Suggested fix: add targeted eval scenarios for prior-review reconciliation and CI-status handling, or otherwise update the validation strategy so these new hard gates are exercised directly.
|
…ix blast-radius rubric ground truth Two changes addressing kubaflo's review request that the skill's new safety-critical behaviors (prior-review reconciliation and the CI hard gate) have no eval coverage, plus a one-line factual correction to the blast-radius rubric. 1. New scenario: "Prior review reconciliation - skill surfaces prior findings before verdict" - Anchor: PR #35685 (gh-aw rerun review scanner, merged) - Has substantive review history across all three surfaces: 4 human reviewers (PureWeen, JanKrivanek, T-Gro, kubaflo), top-level reviews with ❌ findings, inline review comments with ❌ markers at specific files, AND MauiBot AI Summary in issue comments with 17 ❌ markers - Assertions verify the section heading exists, that at least one real reviewer is named (defeats "no prior findings" boilerplate), and that a verdict is delivered - Rubric verifies all three review surfaces are queried, prior ❌ findings are reconciled against current code state, and no finding is silently dropped 2. New scenario: "CI hard gate - skill refuses LGTM when required checks are skipping" - Anchor: PR #35820 (Fix /review tests trigger, merged) - Has license/cla=pass and maui-pr=skipping — `gh pr checks --required` exits 0 with "All checks were successful" plus "1 skipped". This is precisely the exit-0-with-skipping trap the skill's Step 5 calls out as NOT a clean pass. - Assertions verify the CI Status section exists, evidence of required-check inspection (named check or skipping/undetermined classification), and that the verdict is NEEDS_DISCUSSION rather than LGTM (per Rule #6: no LGTM on pending/skipping/undetermined CI). - Rubric verifies the agent ran gh pr checks --required, classified per the skill's exit-code semantics, and did not interpret the "All checks were successful" gh summary line as license to LGTM when a required check actually skipped. 3. Rubric fix: blast-radius scenario line 121 - The previous rubric stated "the PR makes the registration conditional, so the post-PR claim is false". This is factually incorrect about MAUI PR #35223: AddCallback registration remains unconditional (Activity.cs:3372); only the callback's Enabled state is conditional (lines 3405/3408). - The judge had been penalizing the more-precise nuanced answer ("registration unconditional, Enabled conditional") at 3.3/5 because it didn't match the rubric's binary "refute". - New rubric awards credit for the correct technical distinction and removes the false expectation that the hypothesis must be binary-refuted. The eval scenarios anchor on PRs not used elsewhere in eval.yaml, addressing PureWeen's anchor-reuse concern in a different direction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3 participants
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!
Description
Adds four process and structural invariants to the code-review skill, informed by systematic analysis of 372+ historical regressions and the lessons from PR #34669 (badge feature startup crash). Supersedes PR #34985 with changes fitted to the current codebase architecture.
Changes
SKILL.md — 4 new safeguards:
Prior review reconciliation (Step 4) — Check for and reconcile ❌ Error findings from prior reviews before delivering a verdict. Never silently drop or contradict an earlier ❌ Error finding.
CI verification hard gate (Step 5) — Mandatory
gh pr checks <PR_NUMBER> --requiredstep before verdict formation. Classify failing checks as PR-caused / pre-existing flake / ambiguous (delegating toazdo-build-investigatorwhen uncertain). Never claim "clean build" without evidence.Blast radius assessment (Step 6) — Mandatory for handler/platform/infrastructure changes. Probe whether code runs for ALL instances or only feature users, startup impact, static state scope.
Failure-mode probing (Step 6) — Replaces "Devil's Advocate" with rigorous failure scenario analysis. Probes handler disconnect, null PlatformView, missing unsubscribe, stale static state.
Also adds:
**Confidence:** medium|lowfieldContext
Analysis of 372+
regressed-in-*issues revealed that most reviewer-missed regressions share structural patterns (feature code running unconditionally, unverified CI claims, contradicted prior findings). These safeguards encode the generalizable invariants — not incident-specific rules.Relationship to PR #34985
PR #34985 addressed the same problem but targeted a codebase state that has since changed (
review-rules.mdno longer exists, expert reviewer restructured). This PR takes the 4 strongest ideas from #34985 and integrates them into the current architecture.