Fix /review tests screenshot publishing - #36785
Conversation
Publish new visual assets from an orphan asset-only branch so the Actions token does not require Workflows permission to advance the ref. Preserve the legacy branch for existing commit-pinned image URLs and validate branch contents before every publication. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a280b482-e102-4ca0-9ff9-1cfe1946e21f
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36785Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36785" |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Skill Validation Results
✅ Skill Validation Results —
|
There was a problem hiding this comment.
Pull request overview
This PR fixes /review tests visual screenshot publishing by switching publication to a new orphan, asset-only branch (review-tests-assets-v2) and validating that the branch tip contains only approved top-level paths, avoiding GitHub’s workflow-file permission restrictions that block GITHUB_TOKEN from advancing refs on trees containing .github/workflows.
Changes:
- Default publishing branch changed from
review-tests-assetstoreview-tests-assets-v2. - Asset branch initialization now creates an orphan root commit containing only a marker blob, rather than inheriting the default branch tree.
- Added branch-tip validation logic and expanded Pester coverage for branch initialization and concurrent creation scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 | Switches default branch to review-tests-assets-v2, adds asset-branch initialization via orphan root commit, and validates branch tree contents before publishing. |
| .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 | Adds tests covering asset-only initialization, reuse of valid asset branch, rejection of unsafe trees, and concurrent creation handling. |
| .github/docs/maui-ci-facts.md | Updates CI documentation to reflect new asset branch and explains why the legacy branch is no longer used for new publications. |
github-actions
Bot
added
the
area-infrastructure
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial multi-model review
Three independent reviewers analyzed this PR in parallel, followed by a dispute round on contested findings. No blocking (❌) or should-fix (
Verification highlights
The root-cause claim was independently confirmed rather than taken on trust. One reviewer queried the live repo's Git Data API and found review-tests-assets does carry .github/src at its root (confirming the inherited-tree → workflow-permission 403), while review-tests-assets-v2 already exists with root tree exactly .review-tests-assets + pr-33007 — so the fix has already round-tripped in production and branch-protection rulesets don't block ref creation.
Several plausible-sounding concerns were investigated and rejected as false positives (recorded here so they don't get re-litigated):
- Stale
$branchState→ silent data loss on retry. Rejected 3/3. Every path out of the ref-PATCHcatcheitherthrows or sets$branchState = $nullbefore backing off. The PATCH isforce = $false, so git's fast-forward check makes silently dropping a concurrent publisher's entries impossible — the worst case is one wasted round-trip under contention, bounded byMaxAttempts = 5. parents = @()mangled byConvertTo-Json, breaking the orphan-commit premise. Rejected 2/3 by direct execution:Invoke-GhApiJsonuses-Depth 20, and both"parents":[]and the nested single-elementtreearray serialize correctly.^pr-[1-9][0-9]*$rejecting real asset paths. Rejected.$PrNumberis[int]and validated against a real PR upstream, sopr-0/leading-zero directories aren't reachable.- Legacy links breaking. Rejected. Raw URLs are built from the asset commit SHA, never the branch name, so the branch-name default change can't affect pinned links.
-notmatch 'HTTP 422|Reference already exists'masking unrelated 422s. Rejected — that guard is unchanged by this PR, andthrow $creationError.Exceptionnow preserves the original message verbatim instead of the previous generic one.- Command injection / path traversal. Rejected.
$Repository/$Branch/$EndpointreachghviaArgumentList(argument array, no shell), and the pre-existing$AssetBranchdeny-list is unchanged.
Findings (both non-blocking)
💡 Logic — validation over-blocks beyond what the 403 defense requires (2/3 after dispute; inline on Publish-TestVisualAssets.ps1)
The guard runs on every publish and rejects any foreign top-level entry, including plain blobs. A blob can't carry a workflow, so rejecting README.md/LICENSE/.gitattributes is broader than the defense needs — and there's no recovery path in code. Reviewers split
💡 Testing — two permissive validation edge cases are untested (2/3 after dispute; inline on Publish-TestVisualAssets.Tests.ps1)
See inline note — with a caveat about which of the two is actually worth pinning.
On the existing Copilot comment (marker not enforced)
All three reviewers independently reproduced this: Get-ValidatedAssetBranchState rejects only unexpected entries, so an empty tree — or one with pr-N dirs and no marker — passes, contradicting the thrown message and the doc. Calibration: real, but benign. None of the three could construct a failure from it, because a marker-less tree still excludes top-level repo directories, which is the actual 403 trigger. Worth noting it points the opposite direction from the finding above — one says the guard is too loose, the other too strict. Deciding whether the marker is a required invariant would resolve both at once.
Test coverage
The four new Pester tests cover the changed paths meaningfully, and the 5→4 deadline-call-count change is legitimate (3/3) — the removed per-attempt GET /git/commits/* is now served from the cached validated state, not a lost call. On the repo's "bug fix needs a regression test for the original scenario" rule: no test reproduces the literal 403, but initializes a missing branch from an asset-only root... asserts 0 GETs of repos/dotnet/maui and no base_tree, so it fails the moment anyone reverts to seeding from the default branch. That covers the causal mechanism, which is the more durable assertion.
What looks right
Threading the validated {ref, commit} state through the retry loop removes a per-attempt GET /git/commits/* while adding validation — the deadline-budget test's 5→4 assertion is a genuine reduction in API calls, not a dropped safety check.
Prior reviews
One prior COMMENTED review from copilot-pull-request-reviewer with a single inline comment (marker-not-enforced, still unresolved) — addressed above rather than duplicated.
Methodology: 3 independent reviewers with adversarial consensus; contested findings sent through a follow-up dispute round. Findings surviving on a single reviewer without corroboration were discarded.
kubaflo
added
the
area-ai-agents
… review) Get-ValidatedAssetBranchState rejected ANY foreign top-level entry, including plain blobs (README.md, LICENSE, .gitattributes). Only a top-level *directory* can carry .github/workflows content -- the inherited-repo-tree case that trips the workflow-scope 403 this guard defends against -- so a blob can never be a threat. Rejecting blobs risked a repo-wide screenshot-publish lockout with no in-code recovery path if a maintainer added a README to the long-lived asset branch. Narrow the predicate to reject only unexpected `tree` entries and tolerate arbitrary top-level blobs, keeping the 403 defense fully intact (PureWeen review). Tests: add a top-level-blob-tolerance test (README/LICENSE/marker + pr-42 dir now pass) and an empty-root-tree test (pins that JSON [] -> @() -> zero entries passes, guarding the dense predicate against a future refactor). Per the review, the marker-invariant edge case is intentionally left untested pending a decision on whether the marker is required. Publish suite 25/0; verified fail-without-fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen addressed both non-blocking findings in
Publish suite 25/0; verified fail-without-fix. Ready for re-review — thanks! |
…6785 review) The prior narrowing rejected only unexpected `tree` entries, which opened a fail-open gap: a top-level submodule gitlink (tree entry type 'commit') or a malformed/$null entry would pass validation, and a 'commit'-typed `.github` could re-introduce the workflow-scope 403 the guard defends against (Copilot follow-up finding). Flip the predicate to a fail-CLOSED allowlist: an entry is allowed only if it is a top-level blob (README/LICENSE/.gitattributes/marker -- none can carry a workflow) or a pr-<number> directory. Everything else -- unexpected trees, 'commit' gitlinks, and $null entries -- is rejected. This preserves PureWeen's blob tolerance while restoring fail-closed behavior for every other shape. Tests: add a submodule-gitlink rejection test and a malformed-null-entry rejection test; both verified fail-without-fix. Publish suite 27/0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen / Copilot — addressed the fail-open gap from the last round in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1:556
- The allowlist comment implies only a small set of blob filenames are permitted, but the implementation intentionally allows any top-level blob. Tweaking the comment to say “any plain file” avoids future refactors accidentally tightening behavior based on this wording (the tests explicitly rely on arbitrary blobs being allowed).
# Fail CLOSED: an entry is allowed ONLY IF it is a top-level blob (README.md, LICENSE,
# .gitattributes, or the .review-tests-assets marker -- none can carry a workflow) or a
# pr-<number> directory. Everything else is rejected: an unexpected `tree` (.github) is
When the GitHub trees API returns truncated=true for the asset branch root, the guard correctly fails closed (it can't validate what it can't see) but the error was a dead end. Expand the message to explain the cause (too many top-level entries, typically accumulated pr-<number> directories) and the recovery (prune old pr-<number> dirs or rotate to a fresh marker-only asset branch; publishing resumes automatically once the root tree fits a single non-truncated response). Add a truncated-tree test asserting the fail-closed throw carries actionable recovery guidance (Copilot review). Publish suite 28/0; fail-without-fix verified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Round 5 — re-review of cc8bb167 (depth-aware bound)
Re-reviewed the single new commit (the round-4 follow-up that implements the depth-aware bound I suggested). Three independent reviewers, adversarial consensus, plus a dispute round on the one contested finding. Every claim was verified empirically in pwsh against the real functions (dot-sourced via AST) and A/B'd against the parent b1ba3194 — not reasoned about abstractly. CI status is out of scope.
The core round-4 fix is real — verified
The bare-unfenced self-quote case is genuinely fixed: 3 of the 4 new tests are non-vacuous mutation-killers (fail on the parent, pass on head), the full suite is 27/27 on head, and earliest-wins + the round-2/3 protections still hold (re-verified against the live function, including CRLF / no-trailing-newline / <details open> / the stray-leading-</details> decrement guard).
❌ But the round-4 dispute caveat is NOT closed — the bound can still borrow a later report's structure (2/3, + independent A/B verification)
See inline on Review-Tests.ps1:307. My round-4 caveat ("a naive 'bound only at depth 0' is not automatically safe — an unclosed/malformed earlier report at depth > 0 could borrow a later genuine report's structure") reproduces on head. Fixture: an unclosed report one, a genuine well-formed report two, then a trailing unmatched </details>. The parent extracts the clean report two; head commingles report one + report two under report one's anchor and drops the trailing content. Because report one's <details> is still open at the sibling anchor, the depth gate skips the bound, and the (unchanged) balance loop then rebalances across report two via the trailing close. That is silent data loss / a wrong published report where the parent was correct.
Calibration (honest): Reviewer 1 rated the narrower variant — a self-quoting report one + a lone stray </details> with no genuine report two — as arguably working-as-intended, since there the parent only "preserved" the tail as a side effect of the round-3 over-truncation this commit deliberately removes. That framing is fair for that sub-case. It does not cover the genuine-second-report commingle above, where the parent produces a correct clean extraction and head regresses it. Given this lands exactly on the caveat I flagged, and given this PR's precedent of review-response commits introducing new defects (the b585fb14 fail-open), I'm surfacing it at ❌ rather than discarding — but it's a comment, not a block. Reachability is bounded ($Content is the model's own concatenated report attempts, and the contract keeps content inside the top-level <details>), yet a truncated first attempt + a successful retry concatenated together is precisely the scenario this whole fix chain exists to handle.
⚠️ The new caveat test is vacuous (3/3)
See inline on Review-Tests.Tests.ps1:430. The one test framed as covering the caveat passes identically on parent and head, so it exercises the pre-existing balance loop, not the depth-aware delta — it gives false confidence the caveat is closed. Swapping in the borrow fixture above (which fails on head) would make it a real gate.
💡 Minor (1/3, non-blocking)
The $AnchorIndices bound scan restarts at index 0 and re-skips every earlier anchor on each depth-0 line, so the "near O(1) per line" comment overstates it (worst case ~O(A²·L)). Correctness is unaffected and real anchor counts are tiny; either start the scan past the candidate's own offset or drop the O(1) claim.
Bottom line
The commit is a real improvement for the common self-quote case and regresses none of the round-2/3/4 protections. But the specific unbalanced case the round-4 caveat named is still open (commingle + trailing-content loss), and the test meant to prove it closed doesn't exercise it. Worth closing the borrow (reject the candidate once a later sibling anchor is crossed at depth > 0 and a subsequent <details> opens) and making the caveat test non-vacuous before merge.
3 independent reviewers with adversarial consensus (one contested finding taken through a dispute round). Third consecutive review-response commit scrutinized for self-introduced defects, per the pattern earlier in this PR. Claims verified empirically in pwsh, A/B vs parent b1ba3194. CI status is out of scope.
Round-5 review (PureWeen, 3rd scrutinized response commit) found the round-4 depth-aware bound still open to the borrow caveat it was meant to close: - ❌ Data loss / commingle: for an UNCLOSED report one + a genuine well-formed report two + a TRAILING unmatched </details>, report one's <details> is still open at report two's marker, so the depth gate skipped the bound; the balance loop then rebalanced across report two via the trailing close and published a mis-anchored blob of ONE+TWO, dropping the tail. The parent extracted the clean report two. Fix (PureWeen's direction): once a later same-tier anchor is crossed while inside our own <details> (depth > 0), a subsequent <details> open means a DISTINCT sibling report — reject the candidate (return $null) instead of letting the balance loop borrow its structure. A legitimate self-quote opens no <details>, so the round-4 self-quote fix is preserved; the block reset on a clean close keeps later independent content unaffected. -⚠️ Vacuous test: the old "unclosed earlier report" test passed identically on parent and head (its report two had its own <details>, so the pre-existing balance loop did the work). Replaced with PureWeen's diverging borrow fixture (report two + trailing unmatched close + FINAL CONTENT), asserting ONE and TWO never publish together. It fails on the r4 parent (26/1) and passes now — a real gate. - Perf (non-blocking): the anchor bound scan no longer restarts at index 0 per line (which re-skipped every earlier anchor, ~O(A^2*L)); a monotonic cursor advances once through the ascending indices. ~0.74s vs ~0.92s at a pathological N=1500. Suite 27/0; round-2/3/4 protections and all self-quote cases still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen — round-5 borrow caveat closed in ❌ commingle → once a later same-tier anchor is crossed at depth > 0 (bound skipped for a self-quote), a subsequent 💡 perf → the bound scan no longer restarts at index 0 per line; a monotonic cursor walks the ascending indices once (~0.74s vs ~0.92s at N=1500). Suite 27/0; round-2/3/4 protections and all self-quote cases still pass. This is the fourth response commit in the chain — I A/B'd every case against the parent to make sure it converges rather than trading one defect for another. Ready for another look. |
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Round 6 — re-review of 9f42284 (reject cross-report commingle)
Re-reviewed the single new commit — the round-5 response that implements the depth-aware borrow rejection I suggested. Three independent reviewers, adversarial consensus; every claim was verified empirically in pwsh against the real functions (AST-extracted) and A/B'd against the parent cc8bb167. CI status is out of scope.
The round-5 fix works — verified
The commingle case I flagged is genuinely closed: on the new test's own fixture, parent commingles Report ONE + Report TWO (and drops FINAL CONTENT); head returns the clean Report TWO. All three reviewers confirmed the new test is non-vacuous (diverges parent vs head), the $anchorCursor rewrite is behavior-equivalent to the removed foreach (no off-by-one at the line boundaries or with multiple anchors on a line), and the round-4 self-quote case (no nested <details>) still extracts correctly.
❌ But the fix over-corrects into the opposite failure — silent data loss (3/3, each independently reproduced)
See the inline comment on Review-Tests.ps1. The new return $null gate rejects a legitimate single report whenever it (a) self-quotes its own marker/heading inside its open <details> — the realistic behavior round-4 exists for — and (b) then opens a nested <details>, which the production report template always does. Head then either publishes a mis-anchored fragment that drops the verdict/recommendation, or (self-quote at depth ≥ 2) drops the whole report to a $null skeleton. Parent handles all of these correctly. This is the fourth consecutive round in which a fix to this function trades one extraction bug for another in the opposite direction.
I'll flag my own role again: this defect lives in the change I asked for in round 5. The narrow "reject on the next <details> open" mechanism can't tell a genuine sibling report from the report's own nested evidence — the two are structurally identical under the accepted grammar — so the borrow case needs to be resolved at the balance-loop stage rather than by eager rejection.
Testing
The commit adds a regression test for the commingle direction it fixes but none for the direction it regresses (self-quote + nested <details>). Pinning both shapes — depth 1 and depth ≥ 2, asserting evidence and verdict survive — is what stops this ping-pong.
Methodology: 3 independent reviewers with adversarial consensus; all findings reproduced against the live function, parent vs head. Not gated on CI.
…ew r6) Round-6 review found the round-5 eager `return $null` over-corrects into the opposite data loss: it rejects a LEGITIMATE single report whenever it self-quotes its marker inside its own <details> and then opens a nested <details> — which the production report template ALWAYS does. Head dropped Evidence + the Verdict/Recommendation (depth-1 quote) or returned the whole report as $null (depth >= 2 quote). As round 6 establishes, a genuine sibling report and a report's own nested evidence block are STRUCTURALLY IDENTICAL under the accepted grammar, so no eager per-<details>-open gate can tell them apart. Fix: revert the round-5 eager rejection and restore the round-4 depth-aware bound (bound a later same-tier anchor only at <details> depth 0; ignore it inside the report's own open block). This keeps the realistic self-quote-plus-nested template whole AND still handles the genuinely-unclosed earlier report — its details never rebalance to 0, so the balance loop rejects it and extraction falls through to the next report, no borrow. The round-5 trailing-close "commingle" fixture is structurally a single nested report, indistinguishable from the legitimate case, so it now extracts as one report rather than being rejected. The monotonic-cursor perf improvement from round 5 is retained. Tests: replaced the (now structurally-impossible) commingle test with three guards — a self-quoting report + nested <details> at depth 1 and depth 2 (asserting evidence AND verdict survive; both fail on the r5 parent), and a genuinely-unclosed first report falling through to a well-formed second. Suite 29/0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen — round-6 over-correction fixed in ❌ over-correction → the eager On the borrow → the genuinely-unclosed case is still protected (unbalanced → rejected → falls through, new test). The round-5 trailing-close fixture is structurally a single nested report, so I've accepted extracting it as one rather than reintroducing the template regression — this is the consistent choice your round-6 note points to. Tests → replaced the commingle test with three guards: self-quote+nested at depth 1 and depth 2 (evidence + verdict; both fail on the r5 parent) and the genuinely-unclosed fall-through. Suite 29/0; monotonic-cursor perf retained. This is the fifth response commit; I A/B'd your exact fixtures against the parent to make sure this converges on the realistic behavior rather than continuing the ping-pong. Ready for another look. |
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Round 7 — re-review of a46479ec (revert eager commingle-reject; restore depth-aware bound)
Re-reviewed the single new commit — the round-6 response that reverts the round-5 eager return $null gate and restores the round-4 depth-aware bound. Three independent reviewers, adversarial consensus; every claim was verified empirically in pwsh against the real functions (AST-extracted) and A/B'd against the parent 9f42284. This is the seventh review round on this function, and this commit directly undoes a change my own round-6 review asked for — so it got extra scrutiny. CI status is out of scope.
The round-6 regression is genuinely fixed — verified
The round-5 return $null gate mangled the common, production-shaped report. Confirmed against the parent by mutation testing: a well-formed report that self-quotes its marker inside its own <details> and then opens a nested evidence <details> (which the production template at lines 552–556 always does) lost its Evidence + Verdict at depth 1, and returned $null (the whole report dropped) at depth ≥ 2. Head keeps the whole report in both cases. The two new depth-1/depth-2 tests are non-vacuous mutation-killers (fail on parent, pass on head); full suite is 29/0 on head.
The revert re-admits the round-5 commingle — but it's the right tradeoff (3/3)
All three reviewers independently reproduced that the round-5 commingle fixture (unclosed Report ONE → full Report TWO → trailing unmatched </details>) now extracts ONE+TWO as a single over-captured report on head, where the parent returned a clean Report TWO. Two things make this acceptable rather than blocking:
- It's over-capture, not new data loss. The only delta vs the parent is including Evidence ONE (a superset).
FINAL CONTENTafter the outer</details>is dropped by both parent and head, so this commit introduces no new trailing-content loss. - It is structurally unavoidable. The
<details>/anchor depth traces of the legitimate round-6 report and the round-5 commingle fixture are byte-identical under the accepted grammar (OPEN → anchor@depth1 → OPEN → CLOSE → CLOSE). One reviewer proposed narrowing the gate using the## Tests Failure Analysisheading as a distinguishing signal; it was rejected on cross-check: the production template (lines 552–556) emits the marker immediately followed by that heading, so a report self-quoting the exact template it was told to emit would carry marker + heading + nested<details>in its own body — and the heading-gate would false-reject it, re-creating the round-6 data-loss bug. No purely-structural per-<details>-open gate can separate "report self-quotes its marker then opens nested evidence" from a genuine sibling report. Keeping the common case whole clearly outweighs rejecting the rare, malformed over-capture.
Non-blocking follow-ups (inline)
⚠️ Testing — the re-admitted commingle over-capture is now unpinned by any test; the replacementfalls through … truly unclosedtest is vacuous w.r.t. this delta (passes on both parent and head). Suggest a regression test that pins the intended single-report extraction of the rebalancing fixture.- 💡 Documentation — the depth-0 comment overstates the balance loop as a complete anti-borrow backstop; it only rejects the truly-unclosed subcase, not the accepted rebalancing-commingle case.
Verified clean
No dangling $siblingCrossedInsideDetails references; clean parse; earliest-wins plus the round-2/3/4 protections all still hold (a trailing structurally-valid duplicate does not displace the real report; a bare / fenced / 4-space-indented self-quote inside the report's own <details> is not truncated; CRLF, no-trailing-newline, and <details open> are all handled).
Methodology: 3 independent reviewers with adversarial consensus; findings verified empirically by A/B execution against the parent commit. "Reviewer 1/2/3" refers to the three independent passes. CI status intentionally out of scope.
…review r7) Round-7 review verified the round-6 revert fixes the regression and accepted the re-admitted commingle as the right structural-identity tradeoff (3/3). Two non-blocking follow-ups: - Testing: the rebalancing-commingle over-capture (an earlier report rebalanced to depth 0 by a trailing </details>, extracted as a single superset report) was left unpinned when the round-5 test was removed. Added a regression test that feeds the rebalancing fixture and asserts the intended single-report extraction (Evidence ONE + Evidence TWO present; trailing FINAL CONTENT excluded on both parent and head, so no new loss). It fails on the r5 gate, so it guards against a future re-add of the eager rejection silently altering the accepted behavior. - Documentation: reworded the depth-0 comment -- the balance loop is only a PARTIAL anti-borrow backstop (it rejects the truly-unclosed subcase; the rebalancing- commingle case is knowingly accepted as over-capture, the unavoidable cost of the structural-identity tradeoff). Suite 30/0; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen — thanks for the round-7 sign-off on the tradeoff. Both non-blocking follow-ups are in
Suite 30/0, no behavior change. That closes out the two follow-ups from a review chain that ran seven rounds — appreciate the rigor throughout; the structural-identity conclusion is now both documented and pinned. |
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Round 8 — re-review of 2f7de18d (pin the accepted commingle over-capture; reword the borrow comment)
Re-reviewed the single new commit — the round-7 response that implements both of my round-7 non-blocking follow-ups: (1) a regression test pinning the knowingly-accepted rebalancing-commingle over-capture, and (2) rewording the depth-0 comment to scope the balance loop's anti-borrow protection as partial. Three independent reviewers, adversarial consensus. Because this commit directly implements changes my own prior review asked for, the Multi-Round Self-Correction rule applied (lowered threshold for any finding against it) and it got extra scrutiny — there is real precedent on this PR (b585fb1434) for a review-response commit introducing a bug. Every claim was verified empirically in pwsh against the AST-extracted functions and A/B'd against both the parent a46479ec and the reverted round-5 eager-reject gate (9f42284). CI status is out of scope.
No issues found in the new commit — verified (3/3)
- The new test is a genuine mutation-killer, not a vacuous guard. On head the rebalancing fixture extracts the intended superset —
Evidence ONE+Evidence TWOpresent,FINAL CONTENTafter the outer</details>excluded (bounded, not merely absent from raw text). Run through the round-5 eager-reject gate (9f42284), the same fixture extracts only Report TWO, soShould -Match 'Evidence ONE'fails — the test genuinely catches a future re-add of the eager rejection, exactly as the commit claims. - It exercises a distinct shape, not a duplicate. The existing truly-unclosed fixture drops
Evidence ONE(borrow rejected); the new rebalancing fixture keeps it (over-capture accepted) — opposite outcomes on the same marker. - The reworded comment is factually accurate against the code (docs-match-reality): truly-unclosed → rejected by the balance loop and falls through; trailing-close-rebalanced → knowingly accepted as over-capture. The tag sequence
OPEN → anchor@depth1 → OPEN → CLOSE → CLOSEwas confirmed identical to the legitimate depth-1 self-quote fixture, upholding the "structurally indistinguishable" claim. Review-Tests.ps1is provably comment-only. Comment-stripped / semantic-token diff vs the parent is zero — theif ($openDetailsDepth -eq 0 …)guard and the balance loop are byte-identical. No logic snuck in under a "comment reword" (no repeat of theb585fb1434fail-open pattern).- Full suite 30/0 on head, matching the commit message.
No behavior change; the delta is a test + comment that correctly close out the two round-7 follow-ups.
Methodology: 3 independent reviewers with adversarial consensus; every claim verified empirically by A/B execution against the parent and the reverted r5 gate. "Reviewer 1/2/3" refers to the three independent passes. CI status intentionally out of scope.
Get-ValidatedAssetBranchState accepts a root tree without the .review-tests-assets marker. That is deliberate, but the reason was only recorded in a PR thread, so the same predicate has now drawn two contradictory review findings -- one arguing it is too loose (marker not enforced), one arguing it is too strict (over-blocking with no recovery). Record the rationale next to the predicate: -AssetBranch is a fixed constant no caller overrides, so branch identity comes from the ref name rather than a marker blob, and a missing marker cannot produce the nested .github/workflows/ path the workflow-scope 403 requires. Enforcing it would yield no true positives while turning a deleted marker into a lockout this script has no path to repair. Comment-only; the comment-stripped diff is empty and both Pester suites still pass 58/58. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 56ae58e8-a78b-4f24-9920-bc096dfb01fa
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 of Change
Fixes screenshot publication for the merged
/review testsvisual-comparison workflow.New visual assets are now published from an orphan, asset-only
review-tests-assets-v2branch. The publisher initializes that branch with a root commit containing only a marker blob. On every publication attempt it permits top-level blobs andpr-<number>directories, while rejecting trees at any other path, submodule gitlinks, malformed entries, and truncated root-tree responses.The local runner also recognizes report markers only when they occupy a standalone line, preventing a marker quoted in agent prose from leaking a junk preamble or duplicate marker into the posted comment.
The legacy
review-tests-assetsbranch remains untouched so all existingraw.githubusercontent.comlinks pinned to its historical commit SHAs remain reachable.Root Cause
Production validation on PRs #36628 and #36404 showed that the merged workflow activated correctly, gathered evidence, invoked the agent, and posted the ordinary analysis comments:
Both comments contained zero screenshot panels. Their trusted pre-activation jobs had
contents: write, prepared visual evidence, and successfully created Git blobs, trees, and commits, but failed when advancing the asset ref:review-tests-assetshad originally been initialized by pointing it atmain, so every generated asset commit inherited the entire repository tree, including.github/workflows. Updating a ref whose resulting tree contains workflow files requires GitHub's separate Workflows write permission, which cannot be granted toGITHUB_TOKEN.Fix
review-tests-assets-v2..review-tests-assets.pr-<positive-number>directory. Plain blobs cannot carry.github/workflows, so optional files such as a README remain safe.Validation
GITHUB_TOKENwithcontents: write. It created a zero-parent, marker-only root; advancedreview-tests-assets-v2to an asset commit; verified the root contains only the marker andpr-36785; and downloaded the immutable published PNG.maui-pr-uitestsbuild for PR [Windows] Replace Header with PlaceholderText forTitleproperty ofPicker#33007 through the unchanged publisher and panel merger. It gathered 98 real comparisons, published the bounded 24 comparisons as 72 baseline/actual/diff PNGs at asset commit5d114f9, rendered 15 screenshot panels within the 45-URL comment limit, and verified every embedded immutable PNG URL./review testsrun on PR [Windows] Replace Header with PlaceholderText forTitleproperty ofPicker#33007 gathered all three available MAUI builds, ran theclaude-opus-4.8analysis, published 24 comparisons to the upstream orphan asset branch, merged 13 bounded panels, and posted the complete report. ItsNot readyverdict matches the deterministic ceiling, and all 39 embedded PNG URLs were verified.f50f5a1, merged 12 bounded panels, and posted the complete report. ItsNot readyverdict matches the deterministic ceiling, and all 36 embedded immutable PNG URLs return HTTP 200.aad33ceon PR [Net11] [iOS/MacCatalyst] Migrate TabbedPage to handler architecture #36507 published 3 comparisons at asset commit3cb2045, merged 3 bounded panels, and updated the complete report. ItsNot readyverdict matches the deterministic ceiling, all 9 embedded immutable PNG URLs return HTTP 200, and replaying the exact quoted-marker output through parser commit30cfb3dproduces one local marker with no junk preamble.review-tests-assets-v2still contains only.review-tests-assetsand top-levelpr-<number>directories; its tip3cb2045fast-forwards fromf67ed0e.gh awv0.82.14 compilescopilot-review-testswith 0 errors and 0 warnings; the generated lock and actions lock remain unchanged.Issues Fixed
Follow-up to #36666.