Fix CI scanner manifest handoff and fingerprint casing - #36958
Conversation
Read scanner manifests from the fixed same-run agent artifact and canonicalize accepted ASCII fingerprint casing at the trusted publisher boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36958Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36958" |
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes two CI-scan production dry-run failures by (1) making fingerprint casing canonicalization an explicit trusted-boundary responsibility and (2) changing the scanner→publisher manifest handoff from nested JSON-in-string safe-output inputs to a fixed same-run artifact file path.
Changes:
- Switch
submit_ci_scanto an argument-free authorization tool, and require the manifest to be written to/tmp/gh-aw/agent/manifest_final.jsonand read from a fixed safe-job artifact location (${{ runner.temp }}/gh-aw/safe-jobs/agent/manifest_final.json). - Canonicalize accepted ASCII fingerprint casing at the trusted PowerShell boundary (lowercase), while still failing closed for unsafe characters and rejecting post-canonicalization collisions.
- Add/adjust Pester tests + mutation coverage to lock in the fixed-path handoff and the casing canonicalization behavior.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci-status-net11.md | Updates the net11 scanner workflow contract to fixed-path manifest handoff + argument-free submit_ci_scan. |
| .github/workflows/ci-status-net11.lock.yml | Regenerates the compiled workflow to reflect the updated safe-output tool schema (no manifest input) and env wiring. |
| .github/workflows/ci-status-main.md | Updates the main scanner workflow contract to fixed-path manifest handoff + argument-free submit_ci_scan. |
| .github/workflows/ci-status-main.lock.yml | Regenerates the compiled workflow to reflect the updated safe-output tool schema (no manifest input) and env wiring. |
| .github/scripts/Validate-CiScanPublisher.Tests.ps1 | Adds assertions ensuring the publisher reads only the fixed artifact manifest path and validates casing behavior. |
| .github/scripts/Validate-CiScanManifest.Tests.ps1 | Adds tests for casing canonicalization, collision rejection, fixed-file multiline body parsing, and authorization-only agent output. |
| .github/scripts/Validate-CiScanManifest.ps1 | Implements authorization-only agent output validation, fixed-file manifest reader with symlink/size guards, and trusted casing canonicalization. |
| .github/scripts/CiScanMutation.Tests.ps1 | Adds mutation coverage to prevent regressions back to nested-string transport or agent-chosen manifest paths. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 2
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review: no issues found.
Three independent reviewers with adversarial consensus examined the trusted fixed-file handoff, fingerprint canonicalization, generated twin locks, and regression/mutation coverage. The changed paths include exact production reproductions, multiline transport coverage, negative controls, canonical-collision checks, and load-bearing mutation tests. No prior code-review findings were present.
kubaflo
left a comment
There was a problem hiding this comment.
Adversarial multi-model review — PR #36958 (ci-scan manifest handoff + fingerprint casing)
Ensemble: Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro · GPT-5.6 Sol — each reviewed independently @ a0e4a819, then cross-pollinated and orchestrator-verified against real code + live repo state.
Verdict: 💬 COMMENT — code is clean (no blocking findings); withholding APPROVE only because the required maui-pr check is path-filtered/skipping (undetermined CI).
What this changes (independent read)
Hardens the submit_ci_scan safe-job boundary along the same fail-closed lineage as #36760/#36850/#36913/#36848/#36849:
submit_ci_scanis now argument-free (authorization-only). The agent writes its manifest to a fixed path (CI_SCAN_MANIFEST_PATH = ${runner.temp}/gh-aw/safe-jobs/agent/manifest_final.json); the trusted publisher reads only from there.Assert-ScannerSubmissionFromAgentOutputnow requires the safe-output item to be exactly{"type":"submit_ci_scan"}(Count -ne 1 -or [0] -cne 'type'), rejectingmanifest/manifest_pathsmuggling; the shell twin adds anunexpected_input_countjq gate.Get-ScannerManifestFromFilereads the fixed path with-PathType Leaf, rejects symlinks (ReparsePointattr), and caps at 500000 bytes.- Fingerprint case-canonicalization:
Assert-ValidFingerprintwidens the charset[a-z0-9…]→[A-Za-z0-9…]and applies.ToLowerInvariant(), returning the canonical fingerprint used for dedup, marker injection, and existing-issue verification. - Both workflow twins (
ci-status-main+ci-status-net11,.md+.lock.yml) regenerated in lockstep.
Adversarial probing — 7/7 attack probes independently disproven
Symlink/hardlink/TOCTOU · argument-free bypass · exactly-one-property (case-variant Type fails the case-sensitive -cne; duplicate-key last-wins can't hide a property; jq gate fails closed) · charset widening (ASCII-only, still forbids leading punctuation) · lock/source parity · byte/empty/BOM/UTF-16 semantics. Each was attempted and could not be broken. Pester .github/scripts suite: 239/239 pass (verified locally by two models independently).
The one contested point — and why it does not block
Two models (GPT-5.5, GPT-5.6 Sol) raised the same includes()), so canonicalization could file a duplicate issue for the same failure." GPT-5.5 was explicit this is a migration-correctness question, not a security bypass.
The other two models (Opus, Gemini) disproved it, and the orchestrator independently confirmed the disproof four ways:
- Mechanism (Opus, via
git show HEAD~1+git log -S; orchestrator via diff): the pre-PR validator regex was^[a-z0-9][a-z0-9 ._:/+()\-|]*$with case-sensitive-cnotmatch— i.e. lowercase-only. The old publisher could therefore never have filed an uppercase marker. The uppercase-allowing charset appears only in this commit. So every marker already in the wild is lowercase, and the new lowercased marker matches it. - Empirical (orchestrator): a live
gh apiaudit of allci-scanandci-scan-net11issues (state=all), run twice, found zero uppercase fingerprint markers.
Because the precondition (a pre-existing uppercase marker) cannot exist, the duplicate-issue scenario is unreachable. Per the code-review rubric, a failure mode disproven by exact state-transition tracing and live audit is legitimately downgraded. It becomes a 💡 defensive-hardening note, not a blocker.
Findings
- 💡 Legacy-marker canonicalization (from GPT-5.5/Sol, downgraded): the case-normalization is safe given that no uppercase marker can exist today. If you want belt-and-suspenders for a hypothetical hand-edited issue body, add a publisher-side case-insensitive marker comparison or a one-line test asserting an (impossible-today) uppercase marker is adopted rather than duplicated. Optional.
- 💡 Document the argument-free item-shape dependency (Opus): the exactly-one-property gate + jq
unexpected_input_counttwin couple fail-closed publication to gh-aw's current safe-output item shape. A future compiler adding any item-level key would trip both gates and halt publication (fails closed — availability, not security; lock is regenerated+retested on compiler bumps). A one-line comment atValidate-CiScanManifest.ps1:196would aid the next maintainer. - 💡 Note the symlink check is defense-in-depth (Opus): the
ReparsePointguard covers the leaf only; hardlinks / symlinked intermediate dirs are handled by external invariants (artifact upload flattens links to content; all landed content is fully revalidated byTest-CiScanManifest). A comment at:209stating that would prevent a future reader from over-trusting the leaf check.
CI
license/cla = pass. maui-pr = skipping (path-filtered — this PR touches only .github/**), so the required build is undetermined, not green. Non-required Pester (.github/scripts) passed (239/239). Per review policy, undetermined required CI caps this at COMMENT — I'm not auto-approving on a skipped required gate even though the code review is clean.
Bottom line
Strong, fail-closed security-hardening PR; the manifest-handoff redesign is sound and every adversarial probe was defended. The lone cross-model dissent (uppercase-marker duplicate) is factually unreachable against current repo state and is noted only as optional hardening. Code LGTM; disposition COMMENT solely because maui-pr is path-skipped. If a maintainer confirms the path-skip is expected for a .github/**-only change, this is safe to merge.
— Automated multi-model adversarial review (Opus 4.8 + GPT-5.5 + Gemini 3.1 Pro + GPT-5.6 Sol). Round 1.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
.github/scripts/Validate-CiScanPublisher.Tests.ps1:887
- New-PlannedIssue now supports varying FailureCategory/Platform (used in the fingerprint), but the generated Title is still hard-coded to "fails on Windows". This makes test fixtures misleading when simulating non-Windows failures (e.g., the new iOS-simulator test) and can hide title-related regressions.
$fingerprint = "$($script:TwinScannerId)|$($script:TwinBranch)|$Pipeline|$Identity|$FailureCategory|$Platform"
if (-not $EvidenceLine) {
$EvidenceLine = "Assertion failed for $Identity"
}
$proof = New-EvidenceProof -Line $EvidenceLine
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
PureWeen
left a comment
There was a problem hiding this comment.
Follow-up adversarial review: no issues found at e90c3d1055.
Three independent reviewers with adversarial consensus re-reviewed the full PR after the review-driven fixes. The fixed manifest is now validated explicitly and silently (jq -e … >/dev/null), preserving exit-status and stderr diagnostics without echoing up to 500 KB into the agent context. Twin source invariants and generated lock hashes cover the change; both prior inline threads are resolved.
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
.github/scripts/Validate-CiScanManifest.ps1:214
- Get-ScannerManifestFromFile rejects symlinks, but it doesn't ensure the path is a regular file (e.g., a FIFO/device node). Reading such a file with Get-Content can hang the trusted publisher job. Add an explicit regular-file check before the size/content reads to match the stated "regular non-symlink" requirement for this untrusted artifact input.
$manifestFile = Get-Item -LiteralPath $Path
if (($manifestFile.Attributes -band [System.IO.FileAttributes]::ReparsePoint) -ne 0) {
throw 'Fixed scanner manifest must not be a symbolic link.'
}
if ($manifestFile.Length -eq 0 -or $manifestFile.Length -gt 500000) {
throw 'Fixed scanner manifest is empty or exceeds the 500000 byte limit.'
}
.github/scripts/Validate-CiScanManifest.ps1:189
- Assert-ScannerSubmissionFromAgentOutput reads the full agent_output.json into memory before enforcing the 100000 limit, so a large (or special) file could cause excessive memory use or a hang before the guard triggers. Since this is a trusted boundary over untrusted artifacts, validate the file is a regular file and enforce the size limit via Get-Item.Length before calling Get-Content/ConvertFrom-Json.
This issue also appears on line 208 of the same file.
if (-not (Test-Path -LiteralPath $Path -PathType Leaf)) {
throw "Agent output '$Path' does not exist."
}
$rawOutput = Get-Content -Raw -LiteralPath $Path
if ([string]::IsNullOrWhiteSpace($rawOutput) -or $rawOutput.Length -gt 100000) {
throw 'Agent output is empty or exceeds the 100000 character limit.'
}
$payload = $rawOutput | ConvertFrom-Json
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
kubaflo
left a comment
There was a problem hiding this comment.
Adversarial re-review — PR #36958 @ e90c3d1055
Ensemble: Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro reviewed independently, then cross-pollinated every concern. The MAUI expert reviewer independently retained two findings, and GPT-5.6 Terra performed final arbitration.
Outcome: Two concrete, non-duplicate findings survived: the fixed-file manifest is no longer guaranteed to pass through gh-aw threat detection/sanitization, and the exact-once gates ignore the collector's non-fatal errors channel. Artifact layout, uppercase-marker migration, symlink/FIFO/TOCTOU, and test-naming concerns were disproved or reduced to optional hardening.
Prior findings: The earlier explicit jq path issue is fixed and its threads are resolved. No existing review covers the two findings below.
CI: Pester passed at the exact head (1,503/1,503; focused scanner suites 239/239) and license/cla passes. Required maui-pr is skipping, not passing, and Build Analysis remains pending, so required CI coverage is undetermined.
Title/description: The title is accurate and the description is otherwise strong, but it omits the detection/sanitizer regression and overstates the exact-once guarantee.
Comments-only verdict: NEEDS_CHANGES — low overall confidence because required CI is undetermined; high confidence in the two reproduced findings.
Address two adversarial-review findings on the manifest handoff. Finding 1 — manifest bypassed sanitization and threat detection: moving the agent-authored manifest to the `manifest_final.json` file artifact removed it from gh-aw's `sanitizeContent` pass and from the auto-generated threat-detection job (which stages only the prompt, agent_output.json, and patches). Add a `safe-outputs.threat-detection` block to both twins that stages the manifest into the detection directory, names it in the detection prompt, and fails closed if a submission was authorized without it. Add a deterministic trusted-boundary rejection (`Test-HiddenOrControlContent`) that fails closed on control/DEL/C1 characters, bidirectional/invisible format characters, and HTML comment sequences in the raw agent body — rejecting rather than stripping so hash-verified evidence lines are never corrupted. Finding 2 — exact-once gates ignored `.errors`: gh-aw diverts rejected, duplicate, or argument-carrying submissions into agent_output.json's sibling `.errors` array, so a duplicate `submit_ci_scan` vanished from `.items` while the run still reported success. Reject a non-empty `.errors` in both the post-steps jq gate and `Assert-ScannerSubmissionFromAgentOutput`, and add `max: 1` to the custom submit-ci-scan safe-job for MCP-time defense in depth. Both changes are mirrored across the main and net11 twins; lock files were regenerated with `gh aw compile --strict`. Adds producer-shaped `.errors` regression tests, hidden/control-content body-rejection tests, a `no-hidden-content-rejection` mutation with load-bearing coverage, and workflow-source invariants for the new controls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
.github/scripts/Validate-CiScanManifest.ps1:335
Test-HiddenOrControlContentis intended to fail closed on “zero-width” / invisible characters, but it currently does not reject U+200B (ZERO WIDTH SPACE). Since U+200B is invisible to reviewers, leaving it allowed lets an attacker smuggle hidden text into an issue body even with the new hidden-content gate. Explicitly include U+200B in the rejected set (this is distinct from the publisher later inserting U+200B duringConvertTo-SafeIssueBody).
if ($code -eq 0x00AD -or
($code -ge 0x200C -and $code -le 0x200F) -or
($code -ge 0x202A -and $code -le 0x202E) -or
($code -ge 0x2060 -and $code -le 0x206F) -or
$code -eq 0xFEFF) {
return "a bidirectional or invisible format character (U+$($code.ToString('X4')))"
}
- Files reviewed: 8/8 changed files
- Comments generated: 2
The round-3 commit accidentally deleted the `It 'rejects a null body'`
opener, which left an unmatched brace and made Pester fail *discovery* of
Validate-CiScanManifest.Tests.ps1. A discovery-failed container reports
tests=0 / result=Failed and contributes 0 to the failed count, so the whole
manifest test file was silently skipped while the suite still looked green.
Restore the opener; the file now discovers and runs 179 tests.
Harden the hidden/control-content gate in Test-HiddenOrControlContent to
reject the remaining Default_Ignorable_Code_Point ranges that can render
invisibly to a reviewer:
* U+FFF0-FFF8 (reserved default-ignorable Specials).
* The whole U+E0000-E0FFF tag/variation-supplement plane (replacing the two
narrower assigned-only ranges) — the entire plane is default-ignorable, so
widening closes the unassigned-but-invisible slots at zero legitimate loss.
U+FFFC (object replacement) and U+FFFD (replacement char) stay accepted since
they are visibly rendered and appear in real CI evidence.
Adopt the threat-detection staging hardening for the manifest artifact in
both twins (symlink/non-regular-file rejection, 0<size<=500000 byte cap,
cp --no-dereference, and a TOCTOU staged-type/size recheck), with matching
CiScanMutation invariant + mutation tests.
Add discriminating regression coverage: the new default-ignorable rejections
(U+FFF0, U+E0080, U+E0FFF), the previously untested explicit Hangul fillers
(U+115F, U+1160, U+FFA0), and a U+FFFD acceptance guardrail. Recompiled both
lock files via `gh aw compile --strict` (0/0, repeat-compile stable).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
.github/scripts/Validate-CiScanManifest.ps1:412
Test-HiddenOrControlContentcallsCharUnicodeInfo.GetUnicodeCategory($code)where$codeis a Unicode scalar value (can be > 0xFFFF).CharUnicodeInfo.GetUnicodeCategoryis char/string-based; passing anintrisks truncating supplementary-plane code points (or selecting the wrong overload), weakening the intended Cf-category backstop for astral format controls (e.g., U+1D173, U+1BCA0). Classify using thecharoverload for BMP and thestring,intoverload (viaConvertFromUtf32) for supplementary-plane scalars.
if ([System.Globalization.CharUnicodeInfo]::GetUnicodeCategory($code) -eq [System.Globalization.UnicodeCategory]::Format) {
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
Permit VS15/VS16 only after an exact allowlist of common CI status and callout emoji bases. This preserves ordinary agent-authored markers such as warning signs without reopening a generic variation-selector steganography channel after arbitrary symbols. Add symmetric positive and negative coverage for both presentation selectors, including warning-sign acceptance and rejection after text or a dollar symbol. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
kubaflo
left a comment
There was a problem hiding this comment.
Adversarial re-review — NEEDS_CHANGES
Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro reviewed this exact head independently, then cross-pollinated every concern. The MAUI expert reviewer independently reproduced the retained findings, and GPT-5.6 Terra performed final arbitration.
Two material, non-duplicate defects survived: the VS15/VS16 allowlist still rejects a task string emitted by the real scanner, and the parameterized hidden-content test makes Pester's NUnit report export fail after all assertions pass. Generalized Unicode-category bypasses, scalar-state handling, U+FFFD, threat-staging symlink/size/TOCTOU, collector .errors, numeric entities, and HTML-comment concerns were disproved or already fixed.
Prior findings: the earlier manifest threat-detection and collector .errors findings are fixed. No existing review or inline thread covers the two findings below.
CI: license/cla passes. Required maui-pr is skipping and Build Analysis is pending. Pester (.github/scripts) is PR-caused red at run 30660029169: 1,557 assertions pass, then NUnit XML export fails on 0x1B.
Narrative reconciliation: the description's claims that legitimate emoji presentation is handled, the full Pester suite passes, and no blocking/warning findings remain are not true at this head.
Comments-only verdict: NEEDS_CHANGES. Confidence: high in both reproduced findings; low merge-safety confidence because required CI is undetermined and Pester is red.
Allow the production hammer-and-wrench presentation sequence through the trusted manifest gate while retaining arbitrary-selector rejection. Parameterize hidden-content test names so NUnit XML report export remains valid for raw control-character cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
.github/scripts/Validate-CiScanManifest.ps1:273
Test-MarkerLikeContentfalls back to the raw, non-normalized value when NFKC normalization throws. That fallback is not fail-closed formatch_patternvalidation (which does not pass throughTest-HiddenOrControlContent), and it can let compatibility/homoglyph marker spellings slip past detection when normalization is skipped. Since a normalization exception indicates malformed/unsafe Unicode in untrusted input, treat it as an automatic rejection instead of continuing with the raw value.
# NFKC normalization throws on invalid Unicode -- an unpaired surrogate or a
# noncharacter (U+xFFFE/U+xFFFF, U+FDD0-FDEF). Such input can never fold into a
# valid marker, and every code point that makes Normalize throw is itself
# rejected by the downstream Test-HiddenOrControlContent gate (surrogates and
# noncharacters alike), so falling back to the raw value here is a sound
# backstop rather than a fail-open: a marker smuggled alongside a throw-inducing
# code point is still rejected before publication.
try {
$normalized = $Value.Normalize([System.Text.NormalizationForm]::FormKC)
}
catch {
$normalized = $Value
}
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
kubaflo
left a comment
There was a problem hiding this comment.
@PureWeen — AI-generated adversarial re-review: independent Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro passes; cross-pollination; MAUI expert verification; final GPT-5.6 Terra arbitration.
NEEDS_DISCUSSION
Both previously reported blockers are fixed: the exact production 🛠️ Build Microsoft.Maui.sln sequence now passes deterministic validation, and the <Case> test naming prevents raw control values from corrupting NUnit XML export. The exact-head PowerShell Script Tests run passed, including valid report export.
One non-duplicate policy contradiction remains inline: the upstream fail-closed AI threat detector still receives an instruction to flag variation selectors without the deterministic validator’s legitimate-presentation exception. If followed literally, it vetoes publication before this new allowlist runs. The description’s claim that emoji over-blocking is fully closed and no actionable findings remain should be qualified until both gates use the same policy.
CI: Pester (.github/scripts) and license/cla pass on this head. Required maui-pr is path-skipped and Build Analysis is pending, so overall merge-safety confidence remains low.
Confidence: high in the policy mismatch; low for merge safety because required CI is undetermined.
Mirror the trusted VS15/VS16 base allowlist in both threat-detection prompts so legitimate CI presentation sequences are not vetoed before deterministic validation. Add source and compiled-lock invariants plus mutations for removed, widened, weakened, and stale selector policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
kubaflo
left a comment
There was a problem hiding this comment.
@PureWeen — AI-generated adversarial re-review: independent Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro passes; cross-pollination; MAUI expert verification; final GPT-5.6 Terra arbitration.
CLEAN
All previously reported blockers are resolved on this head. The fixed manifest is now threat-scanned before publication, collector errors and argument-bearing submissions fail closed, the trusted Unicode gate accepts the production 🛠️ Build Microsoft.Maui.sln sequence while rejecting arbitrary selectors, NUnit-safe parameterized test names preserve report export, and both source prompts plus decoded compiled prompts enforce the validator's exact selector policy.
Exact-head validation passed 221 focused Pester tests; the MAUI expert pass independently ran 302/302. gh-aw v0.83.4 strict compilation reproduced both committed locks byte-for-byte with zero warnings. The supplementary-plane CharUnicodeInfo concern is not applicable because the runtime binds the Int32 scalar overload; the existing astral Cf regressions pass.
No actionable code-review findings remain. Title and description accurately match the implementation.
CI: PowerShell Script Tests and license/cla pass. Required maui-pr is path-skipped/neutral and Build Analysis is still pending, so this is a clean code-review verdict rather than a final merge-readiness determination.
Confidence: high for code correctness and security; CI confidence remains limited until required checks settle.
kubaflo
left a comment
There was a problem hiding this comment.
@PureWeen — exact-head adversarial re-review: independent Claude Opus 5, GPT-5.6 Sol, and Gemini 3.1 Pro passes; two cross-pollination passes; MAUI expert verification; final GPT-5.6 Terra arbitration.
NEEDS_DISCUSSION
No new actionable code findings survived. The prior sanitizer/threat-detection, collector-error, selector-policy, and report-export concerns remain resolved at this head. The contested residual sanitizer-parity concern was not retained: the fixed-file manifest is staged into the detector, publication fails closed on detector failure, and the bounded issue body remains treated as untrusted by downstream automation. The exact-once producer comments are slightly broader than the pinned gh-aw implementation, but the MCP and direct-JSONL paths still fail closed, so this is not a correctness defect.
The title and description accurately match the implementation. Focused Pester validation passed 302/302 and the exact-head PowerShell Script Tests check passes. Required maui-pr is path-skipped and Build Analysis remains pending, so CI coverage is still undetermined and the confidence remains low despite the clean code verdict.
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause The first post-merge dry runs after #36958 exposed two independent agent-manifest composition failures at `16671cadb63739c47da264953ee3e1267806cbb8`: - [Net11 run 31038805792](https://github.com/dotnet/maui/actions/runs/31038805792) produced five otherwise valid issue titles with U+2014 EM DASH even though the prompt required printable ASCII. The trusted validator correctly rejected the first title. - [Main run 31038805939](https://github.com/dotnet/maui/actions/runs/31038805939) selected the evidence-backed pattern `XHarness exit code: 1 (TESTS_FAILED)` for the CarouselView leak signature but omitted that duplicated value from its issue body. The trusted validator correctly rejected the mismatch. Replaying the net11 artifact also exposed two bodies that paraphrased rather than copied an entire trusted evidence line, plus an order-dependent `cap-reached` classification. Prompt compliance alone is not a reliable publication boundary for duplicated or normalized payload fields. ### Description of Change - Canonicalize only U+2013 EN DASH and U+2014 EM DASH to ASCII `-` in issue titles before the existing printable-ASCII gate. Curly quotes, non-breaking spaces, emoji, controls, and all other non-ASCII remain rejected. - Validate every agent-selected `match_pattern` against frozen same-run evidence before use. If an otherwise valid body omitted that exact pattern, append a bounded, safely rendered `Trusted Match Pattern` excerpt in trusted PowerShell. Hidden/control/marker content still fails closed. - Preserve the independent full-evidence-line hash check over the exact published body. The canonical pattern excerpt does not replace that requirement unless the pattern itself is the complete trusted line. - Define the five-issue cap across the complete manifest rather than traversal order. `cap-reached` may appear before or after the fifth filed entry, while substantive skip reasons remain valid regardless of position. - Align both gh-aw twins and their threat-detection prompts with trusted canonicalization, evidence-bound augmentation, timestamp ownership, and complete evidence-line requirements. - Regenerate both lock files with gh-aw v0.83.4 strict compilation. ### What NOT to Do - Do not broadly normalize Unicode titles; only the two unambiguous typographic dash separators are accepted. - Do not inject an agent-selected pattern before frozen evidence proves it in every claimed source log. - Do not weaken complete evidence-line identity to a substring check. - Do not make cap exhaustion depend on manifest traversal order. ### Validation - Focused scanner Pester suite: **360 passed / 0 failed**, four healthy containers, NUnit XML total parity. - Full `.github/scripts` Pester suite: **1,572 passed / 0 failed**, 22 healthy containers, NUnit XML total parity. - Both scanner twins strict-compiled twice with gh-aw v0.83.4, with byte-stable generated locks. - Exact unmodified main artifact replay: complete three-pipeline plan, **4 filed issues**, CarouselView payload repaired from evidence, no writes. - Net11 artifact replay with only the two required full-line body corrections and the substantive early skip reason: complete three-pipeline plan, **5 filed issues**, raw U+2014 titles canonicalized, order-independent cap accepted, no writes. - Three independent adversarial reviewers re-reviewed the final trusted injection path. No unresolved code findings remain; the final stale-lock observation was resolved by regenerating both locks after reviewers stopped restoring their backups. ### Safety and Rollout Both production runs failed before staged publication, and every local replay stopped at trusted plan generation. **No real `ci-scan` or `ci-scan-net11` issue was created, edited, labeled, closed, or reopened.** No new production dry runs will be dispatched until this PR merges. ### Issues Fixed N/A Co-authored-by: PureWeen <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41235a9a-79b4-433f-9e0d-7278c916a7c9
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 two production dry-run failures discovered after #36913:
runoniOS_MauiReleaseTrimFullbecause trusted validation depended on the agent lowercasing every fingerprint component.submit_ci_scanstring argument when multiline issue bodies contained literal newlines.The trusted PowerShell boundary now canonicalizes accepted ASCII fingerprint casing before structural validation, duplicate detection, marker injection, and plan publication. Unsafe characters still fail closed, and fingerprints that collide after canonicalization are rejected.
The scanner twins now use a same-run artifact handoff instead of nested string transport. The agent writes one manifest to
/tmp/gh-aw/agent/manifest_final.json, then calls the argument-freesubmit_ci_scantool exactly once to authorize publication. gh-aw v0.83.4 uploads that fixed directory in the current run's fixedagentartifact; the custom safe job downloads that artifact and reads only${{ runner.temp }}/gh-aw/safe-jobs/agent/manifest_final.json. The file is treated as untrusted, must be a regular non-symlink file, and is capped at 500,000 bytes before all existing inventory, evidence, cap, staged-mode, all-or-nothing preflight, and post-write checks run.This artifact is only an intra-run handoff. It is not cross-run scanner state or persistence. The safe-output request cannot select another file path or carry manifest data. This PR intentionally does not add stale issue lifecycle or reset behavior.
Review hardening
Adversarial review found and closed additional gaps while preserving twin symmetry:
0 < size ≤ 500000before copy, usescp --no-dereference, and revalidates the staged file type and byte size..errors. Both the post-steps jq gate andAssert-ScannerSubmissionFromAgentOutputnow fail closed on non-empty collector errors, andmax: 1rejects duplicate custom safe-output calls.🛠️ Build Microsoft.Maui.slnheading while selectors after ordinary text or arbitrary symbols still fail closed, so the fix does not reopen a generic variation-selector steganography channel.CUSTOM_PROMPTvalues so an omitted compile cannot silently restore the veto.Itopener is restored, soValidate-CiScanManifest.Tests.ps1is discovered and executed again.<Case>parameter instead of allowing raw XML-invalid values such as ESC U+001B into the NUnit test-name attribute.Validation
.github/scriptsPester 5.9.0 suite with NUnit XML export and report-total parity guard: 1,566 passedgh aw compile ci-status-main --strictgh aw compile ci-status-net11 --strictmanifest_final.jsonwith all seven multiline bodies through the fixed-file reader🛠️ Build Microsoft.Maui.slntext through trusted manifest validation while preserving arbitrary-symbol selector rejectionNo real
ci-scanorci-scan-net11issue was created, updated, closed, reopened, or otherwise mutated during development or validation.Issues Fixed
Follow-up to #36913; no separate tracking issue.