Add PR platform labeling#35327
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35327Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35327" |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
MauiBot
added
s/agent-changes-requested
🤖 AI Summary
📊 Review Session —
|
| Aspect | Value |
|---|---|
| Surface area | CI / repository automation only |
| Production code touched | No |
| Public API touched | No |
| Test infra touched | No |
| Platforms affected (runtime) | None — affects label routing for all platform PRs |
| Risk | Low (failure mode = missing/extra label, not broken build) |
Code Review Summary
Verdict: NEEDS_CHANGES (confidence: high) — 0 errors, 2 warnings, 3 suggestions
Key findings (full details in code-review.md):
⚠️ .ios.csdual-platform gap (line 32, 42) — Per MAUI conventions,.ios.csfiles compile
for both iOS and MacCatalyst TFMs. The currentHAS_IOSbundles/iOS/(iOS-only) with
.ios.cs(iOS + MacCatalyst). PRs touching only.ios.cswill getplatform/iosbut never
platform/macos. Fix is to splitHAS_IOS_DIRandHAS_IOS_EXT, then applyplatform/macos
whenHAS_IOS_EXT > 0ORHAS_CATALYST > 0.⚠️ Labels never removed onsynchronize(line 61) — Workflow only--add-labels. After a
rebase that drops platform code, stale labels persist. Should remove labels no longer
warranted on each synchronize.- 💡
${{ github.repository }}interpolated directly into therun:script (line 25, 60) — should
pass viaenv:per GitHub script-injection guidance. - 💡 Missing
reopenedevent type (line 9) — closed-then-reopened PRs won't be evaluated. - 💡 No
HAS_TIZENdetection despitePlatforms/Tizen/,.tizen.csfiles, and aplatform/linux
label existing in the repo.
Failure-mode probes
- PR touches only
.ios.csfiles → getsplatform/iosonly → ❌ should also haveplatform/macos. - PR is rebased to remove all iOS code →
platform/iospersists → ❌ stale label. - PR is closed and reopened → no labels evaluated →
⚠️ coverage gap. - PR touches only
.tizen.csfiles → no labels →⚠️ coverage gap. - Fork PR with
${{ github.repository }}→ safe inpull_request(repo is base) but
theoretical injection if future trigger changes.
Blast radius
Workflow runs on every PR. A miss = wrong/missing label routing. No build/runtime impact.
Affects triage UX only.
Platform Selection
Requested platform: android (per task prompt). Not applicable to this PR — no platform
runtime code is changed; the workflow itself runs on ubuntu-latest. Try-fix variants below
focus on YAML correctness rather than device-test execution.
🔬 Code Review — Deep Analysis
Code Review — PR #35327
Independent Assessment
What this changes: Adds a new GitHub Actions workflow (.github/workflows/platform-label.yml)
that fires on pull_request: [opened, synchronize]. It queries the GitHub API for changed files
(with --paginate), runs four case-insensitive grep -c checks against path/extension patterns,
and applies the matching platform/android, platform/ios, platform/macos, or
platform/windows labels via gh pr edit --add-label.
Inferred motivation: Reviewers and triage currently have to manually identify which platforms
a PR touches. This automates the tagging.
Is the approach sound? Yes. Querying the files API with --paginate (instead of git diff
after a checkout) is a clean, efficient pattern for this use case. grep -ciE with || true to
yield a numeric result and then testing > 0 is idiomatically correct for shell. The
permissions: block is correctly scoped (read + PR write).
Reconciliation with PR Narrative
- Title: "Add PR platform labeling"
- Body: empty
- The code matches the title exactly. No discrepancies.
Findings
⚠️ Warning — .ios.cs files compile for BOTH iOS and MacCatalyst, but only get platform/ios
File: .github/workflows/platform-label.yml, lines 32 & 42
Per the repository's documented conventions
(.github/copilot-instructions.md: ".ios.cs — iOS and MacCatalyst TFMs (both)"), the HAS_IOS
variable today bundles two semantically different inputs into one boolean:
/iOS/directory matches → iOS-only (e.g.,Platforms/iOS/AppDelegate.cs).ios.csextension matches → iOS and MacCatalyst (e.g.,EditorHandler.ios.cs)
A PR touching only .ios.cs handler files (the most common shape of platform-specific changes
in this repo) currently gets platform/ios but silently misses platform/macos.
Fix:
HAS_IOS_DIR=$(echo "${FILES}" | grep -ciE '/iOS/' || true)
HAS_IOS_EXT=$(echo "${FILES}" | grep -ciE '\.ios\.cs' || true)
# … then:
if [ "${HAS_IOS_DIR}" -gt 0 ] || [ "${HAS_IOS_EXT}" -gt 0 ]; then
LABELS="${LABELS} platform/ios"
fi
if [ "${HAS_IOS_EXT}" -gt 0 ] || [ "${HAS_CATALYST}" -gt 0 ]; then
LABELS="${LABELS} platform/macos"
fi⚠️ Warning — Labels are only added, never removed on synchronize
File: .github/workflows/platform-label.yml, line 61
The workflow exclusively uses --add-label. When a PR is rebased / force-pushed and platform code
is removed, stale labels persist. For triage accuracy, synchronize events should also remove
labels no longer warranted.
💡 Suggestion — ${{ github.repository }} interpolated directly into the run: script
File: lines 25 and 60
GitHub's security hardening guide recommends passing context expressions via env:
rather than interpolating them inline. While github.repository for a pull_request event is
always the base repo and pull_request.number is always a positive integer, moving these to env
vars is the hardened-by-default pattern. Risk in this exact repo is near-zero, but it's a
one-line fix.
💡 Suggestion — Missing reopened trigger event
File: line 9
PRs that are closed and reopened (draft cycling, after merge-conflict resolution) won't have
their labels evaluated. Add reopened:
types: [opened, synchronize, reopened]💡 Suggestion — No detection for Tizen/Linux platform files
File: lines 30–36
Repo has Platforms/Tizen/ directories, .tizen.cs files, and a platform/linux label. PRs
touching Tizen code are never labelled. Add:
HAS_TIZEN=$(echo "${FILES}" | grep -ciE '(/Tizen/|\.tizen\.cs)' || true)…and apply platform/linux accordingly.
Devil's Advocate
- Is the
.ios.csfinding really a bug? Yes — the convention is documented, the file extension
multi-targets, and the rest of the platform labels are accurate. The miss is silent. - Is label staleness actually painful? Yes — common workflow is to push a draft covering many
platforms then trim. Stale labels mislead reviewers. - Is the injection risk real? Theoretical for now, but trivial to fix with env vars.
- Did I miss anything?
--paginatecorrectly handles >100-file PRs.xargsfor trimming and
tr ' ' ','for CSV are correct. No checkout step needed (API-only). Permissions are minimal.
Verdict: NEEDS_CHANGES
Confidence: high
The workflow is well-structured and the approach is sound, but there is a concrete correctness
gap: .ios.cs files compile for both iOS and MacCatalyst, but the current detection only applies
platform/ios. The label-staleness issue on synchronize is also worth addressing before merge.
The remaining items are minor suggestions.
🔧 Fix — Analysis & Comparison
Try-Fix Phase Summary — PR #35327
Models / dimensions
| Attempt | Model | Dimension explored |
|---|---|---|
| try-fix-1 | claude-opus-4.6 | maintainability / readability — has_match() helper + bash arrays |
| try-fix-2 | claude-sonnet-4.6 | architecture — switch to actions/labeler@v5 + .github/labeler.yml |
| try-fix-3 | gpt-5.3-codex | minimal surgical fix — only the .ios.cs dual-platform split |
| try-fix-4 | gemini-3-pro-preview | idempotence / state convergence — desired-vs-current label sync |
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| pr | PR #35327 | grep-based detection in shell, --add-label only |
1 file | Original — has .ios.cs correctness gap |
|
| pr-plus-reviewer | reviewer feedback applied to PR | Split iOS dir/ext, env vars, label removal on synchronize, reopened, Tizen | ✅ PASSED (lint) | 1 file | Most comprehensive single-file fix |
| try-fix-1 | claude-opus-4.6 | has_match() helper + bash arrays + set -euo pipefail |
✅ PASSED (lint) | 1 file | Cleanest shell; drops debug echo + label removal |
| try-fix-2 | claude-sonnet-4.6 | actions/labeler@v5 + .github/labeler.yml |
✅ PASSED (lint) | 2 files | Adds dependency; native sync-labels; uses pull_request_target |
| try-fix-3 | gpt-5.3-codex | Minimal surgical: only .ios.cs split |
✅ PASSED (lint) | 1 file | Smallest diff; correctness fix only |
| try-fix-4 | gemini-3-pro-preview | Idempotent desired-vs-current label sync; adds /MaciOS/ |
✅ PASSED (lint) | 1 file | Extra API call; broadest coverage |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | NO NEW IDEAS | "try-fix-4's idempotent approach already covers state convergence; nothing to add" |
| claude-sonnet-4.6 | 2 | NO NEW IDEAS | "actions/labeler is the canonical answer; staying with it" |
| gpt-5.3-codex | 2 | NO NEW IDEAS | "Minimal fix is intentionally minimal — broader scope belongs in pr-plus-reviewer" |
| gemini-3-pro-preview | 2 | NO NEW IDEAS | "Idempotence covers all staleness scenarios; further hardening is yak-shaving" |
Exhausted: Yes
Selected Fix: pr-plus-reviewer — best balance of correctness, coverage, and minimal author
disruption. See report/content.md for full comparison.
Notes on test execution
The PR changes only a GitHub Actions workflow YAML file. There is no unit-test, device-test, or
UI-test surface for it on either macOS-android or any other test runner. All candidates were
validated via python3 yaml.safe_load(...) for YAML well-formedness and bash -n against the
extracted run: block for shell syntax. The Gate phase ran by Review-PR.ps1 reported
Per task rules, candidates that failed regression tests must rank below those that passed.
None of the candidates failed: all five pass static validation, and there are no regression
tests to fail. Ranking is therefore decided on quality dimensions (correctness coverage,
minimal blast radius, author-style alignment).
📋 Report — Final Recommendation
Phase 3 — Comparative Report — PR #35327
Final Recommendation: REQUEST CHANGES — apply pr-plus-reviewer
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | 1 file changed, no linked issue, CI workflow only |
| Code Review | NEEDS_CHANGES (high) | 0 errors, 2 warnings, 3 suggestions |
| Gate | No tests detected — workflow-only PR | |
| Try-Fix | ✅ COMPLETE | 4 attempts, 1 expert-eval candidate, all ✅ on lint |
| Report | ✅ COMPLETE |
Candidate comparison
| Candidate | Fixes .ios.cs bug |
Removes stale labels | Tizen/Linux | reopened trigger |
env-var hardening | Diff size | Test result |
|---|---|---|---|---|---|---|---|
| pr (as submitted) | ❌ | ❌ | ❌ | ❌ | ❌ | n/a (baseline) | |
| pr-plus-reviewer | ✅ | ✅ (synchronize) | ✅ | ✅ | ✅ | +80 lines | ✅ lint |
| try-fix-1 | ✅ | ❌ | ✅ | ✅ | ✅ | +56 lines | ✅ lint |
| try-fix-2 | ✅ | ✅ (every run) | ✅ | ✅ | n/a (uses action) | +52 lines, 2 files | ✅ lint |
| try-fix-3 | ✅ | ❌ | ❌ | ❌ | ❌ | +6 lines | ✅ lint |
| try-fix-4 | ✅ | ✅ (every run) | ✅ | ✅ | ✅ | +62 lines | ✅ lint |
Per task rules, candidates that fail regression tests must rank below those that pass. None
fail (no regression tests exist for a workflow YAML). Ranking is on quality criteria below.
Ranking (best → worst)
- pr-plus-reviewer — addresses every reviewer finding while preserving the author's idiom
and staying single-file. Best balance of correctness coverage and review burden. - try-fix-4 — idempotent / state-convergent, also covers
MaciOS/shared folder. Larger
diff and an extra API call per run, but technically the most robust against rebases. - try-fix-2 — clean architectural switch to
actions/labeler@v5. Best long-term ergonomics
but introduces a third-party dep and apull_request_targettrigger change — bigger
conceptual shift than the author signed up for. - try-fix-1 — readable rewrite. Fixes correctness but skips label-removal on synchronize.
- try-fix-3 — minimal correctness fix only. Lowest review cost but leaves stale-label,
Tizen, and reopened gaps documented in the review unaddressed. - pr — has the documented
.ios.cscorrectness gap.
Selected winner: pr-plus-reviewer
Rationale:
- Resolves the major correctness finding (
.ios.csdual-platform). - Resolves the moderate hardening finding (env-var interpolation).
- Resolves all three minor findings (stale labels on synchronize, reopened, Tizen/Linux).
- Single file, same shell-script approach the author chose; reviewer can grok the delta in one
read. - Inline comments document the
.ios.csdual-TFM convention so future maintainers don't
re-introduce the bug.
Recommended next step
Author applies the pr-plus-reviewer diff (or the pr-plus-reviewer.yml candidate verbatim).
After CI passes, this PR is mergeable.
Notes
gh pr review --approve/--request-changeswere NOT invoked (skill rule).- No code-change comments were posted to the PR (skill rule). All output is in
CustomAgentLogsTmp/PRState/35327/PRAgent/. - The candidate diffs themselves are stored in
~/.copilot/session-state/.../files/candidates/diffs/for reference;
winner.jsoncarries the selected diff inline.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
| if [ "${HAS_ANDROID}" -gt 0 ]; then | ||
| LABELS="${LABELS} platform/android" | ||
| fi | ||
| if [ "${HAS_IOS}" -gt 0 ]; then |
There was a problem hiding this comment.
[major] Correctness — .ios.cs implies both platform/ios AND platform/macos
Per MAUI conventions, .ios.cs files compile for both iOS and MacCatalyst TFMs. The current HAS_IOS variable bundles /iOS/ directory matches (iOS-only) with .ios.cs extension matches (both platforms). This means a PR that only touches .ios.cs handler files will get platform/ios but never platform/macos.
The fix requires splitting the detection:
HAS_IOS_DIR=$(echo "${FILES}" | grep -ciE '/iOS/' || true)
HAS_IOS_EXT=$(echo "${FILES}" | grep -ciE '\.ios\.cs' || true)Then:
if [ "${HAS_IOS_DIR}" -gt 0 ] || [ "${HAS_IOS_EXT}" -gt 0 ]; then
LABELS="${LABELS} platform/ios"
fi
if [ "${HAS_IOS_EXT}" -gt 0 ] || [ "${HAS_CATALYST}" -gt 0 ]; then
LABELS="${LABELS} platform/macos"
fiThis way, .ios.cs extension changes add both labels, while Platforms/iOS/ directory-only changes add only platform/ios.
| run: | | ||
| PR_NUMBER="${{ github.event.pull_request.number }}" | ||
|
|
||
| FILES=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null || true) |
There was a problem hiding this comment.
[moderate] Safety — Context expressions interpolated directly into shell
${{ github.repository }} is interpolated inline into the shell script. GitHub's own security guidance (https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) recommends passing context values as env vars to prevent script injection:
env:
GH_TOKEN: ${{ github.token }}
GH_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}Then reference $GH_REPO and $PR_NUMBER in the shell. While dotnet/maui is safe, github.repository for forks could theoretically contain shell metacharacters.
| # Convert space-separated to comma-separated for gh cli | ||
| LABEL_CSV=$(echo "${LABELS}" | tr ' ' ',') | ||
| echo "Applying labels: ${LABELS}" | ||
| gh pr edit "${PR_NUMBER}" --repo "${{ github.repository }}" --add-label "${LABEL_CSV}" |
There was a problem hiding this comment.
[minor] Correctness — Labels are only added, never removed on synchronize
The workflow uses --add-label exclusively. When a PR is updated (e.g., git push --force rebasing away all iOS changes), stale labels like platform/ios persist permanently.
For synchronize events, the workflow should also remove platform labels that are no longer applicable. One approach:
# Remove platform labels not in current set
for label in platform/android platform/ios platform/macos platform/windows platform/linux; do
if ! echo "${LABELS}" | grep -q "${label}"; then
gh pr edit "${PR_NUMBER}" --repo "${GH_REPO}" --remove-label "${label}" 2>/dev/null || true
fi
done|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] |
There was a problem hiding this comment.
[minor] Coverage — Missing reopened trigger event
A PR closed and then reopened (common after merge conflicts are resolved or after a draft→ready transition that was previously closed) will not have its platform labels evaluated. Add reopened to ensure labels are applied:
types: [opened, synchronize, reopened]| exit 0 | ||
| fi | ||
|
|
||
| HAS_ANDROID=$(echo "${FILES}" | grep -ciE '(/Android/|\.android\.cs)' || true) |
There was a problem hiding this comment.
[minor] Coverage — No detection for Tizen/Linux platform files
The repo has a platform/linux label and substantial Tizen platform code (Platforms/Tizen/ directories, .tizen.cs extension files in Essentials and Controls). The workflow has no detection for these, so PRs touching Tizen/Linux code will never receive platform/linux.
Consider adding:
HAS_TIZEN=$(echo "${FILES}" | grep -ciE '(/Tizen/|\.tizen\.cs)' || true)And:
if [ "${HAS_TIZEN}" -gt 0 ]; then
LABELS="${LABELS} platform/linux"
fi
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
| if [ "${HAS_ANDROID}" -gt 0 ]; then | ||
| LABELS="${LABELS} platform/android" | ||
| fi | ||
| if [ "${HAS_IOS}" -gt 0 ]; then |
There was a problem hiding this comment.
[major] Correctness — .ios.cs implies both platform/ios AND platform/macos
Per MAUI conventions, .ios.cs files compile for both iOS and MacCatalyst TFMs. The current HAS_IOS variable bundles /iOS/ directory matches (iOS-only) with .ios.cs extension matches (both platforms). A PR that only touches .ios.cs handler files will get platform/ios but never platform/macos.
Fix by splitting detection:
HAS_IOS_DIR=$(echo "${FILES}" | grep -ciE '/iOS/' || true)
HAS_IOS_EXT=$(echo "${FILES}" | grep -ciE '\.ios\.cs' || true)Then apply platform/ios when HAS_IOS_DIR > 0 OR HAS_IOS_EXT > 0, and platform/macos when HAS_IOS_EXT > 0 OR HAS_CATALYST > 0.
| run: | | ||
| PR_NUMBER="${{ github.event.pull_request.number }}" | ||
|
|
||
| FILES=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null || true) |
There was a problem hiding this comment.
[moderate] Safety — Context expressions interpolated directly into shell
${{ github.repository }} is interpolated inline into the run: script. GitHub's script-injection guidance recommends passing context values as env: vars:
env:
GH_TOKEN: ${{ github.token }}
GH_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}Reference $GH_REPO and $PR_NUMBER in shell. Risk is theoretical for dotnet/maui but the env-var pattern is defense-in-depth.
| # Convert space-separated to comma-separated for gh cli | ||
| LABEL_CSV=$(echo "${LABELS}" | tr ' ' ',') | ||
| echo "Applying labels: ${LABELS}" | ||
| gh pr edit "${PR_NUMBER}" --repo "${{ github.repository }}" --add-label "${LABEL_CSV}" |
There was a problem hiding this comment.
[minor] Correctness — Labels are only added, never removed on synchronize
Workflow uses --add-label exclusively. When a PR is rebased to drop platform-specific changes, stale labels persist. For synchronize events, also remove platform labels no longer applicable. Example:
for label in platform/android platform/ios platform/macos platform/windows platform/linux; do
if ! echo "${LABELS}" | grep -q "${label}"; then
gh pr edit "${PR_NUMBER}" --repo "${GH_REPO}" --remove-label "${label}" 2>/dev/null || true
fi
done|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] |
There was a problem hiding this comment.
[minor] Coverage — Missing reopened trigger event
A PR closed and reopened (draft→ready cycling, or revived after merge-conflict resolution) will not have its platform labels evaluated. Add reopened:
types: [opened, synchronize, reopened]| exit 0 | ||
| fi | ||
|
|
||
| HAS_ANDROID=$(echo "${FILES}" | grep -ciE '(/Android/|\.android\.cs)' || true) |
There was a problem hiding this comment.
[minor] Coverage — No detection for Tizen/Linux platform files
The repo has a platform/linux label and substantial Tizen platform code (Platforms/Tizen/ dirs, .tizen.cs extension files in Essentials and Controls). PRs touching this code currently never receive platform/linux.
Add:
HAS_TIZEN=$(echo "${FILES}" | grep -ciE '(/Tizen/|\.tizen\.cs)' || true)And apply platform/linux accordingly.
|
Superseded by #35382 |
2 participants
No description provided.