Skip to content

Fix /review trigger when comment has leading whitespace#35438

Merged
JanKrivanek merged 1 commit into
mainfrom
fix/review-trigger-leading-whitespace
May 20, 2026
Merged

Fix /review trigger when comment has leading whitespace#35438
JanKrivanek merged 1 commit into
mainfrom
fix/review-trigger-leading-whitespace

Conversation

@JanKrivanek

Copy link
Copy Markdown
Member

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Problem

The /review slash command in .github/workflows/review-trigger.yml is silently skipped when the comment body has any leading whitespace before /review.

Concrete example:

Root cause

The job-level guard was:

if: >-
  github.event_name == 'workflow_dispatch' ||
  (github.event.issue.pull_request &&
   (github.event.comment.body == '/review' ||
    startsWith(github.event.comment.body, '/review ')))

startsWith(' /review ...', '/review ') returns false, so the job is skipped. GitHub expression syntax has no trim or regex, so we can't fix this purely at the expression level. The Parse parameters step had the same blind spot — sed -n 's|^/review[[:space:]]*||p' produces empty ARGS if the body doesn't start with /review.

Fix

  1. New tiny match pre-filter job that uses a bash regex (^[[:space:]]*/review([[:space:]]|$)) to decide whether the comment is a /review command. It allows arbitrary leading whitespace (spaces, tabs, newlines) but still requires /review to be a standalone token (won't match /reviewfoo or comments that merely mention /review mid-sentence).
  2. trigger-review now needs: match and gates on its output, keeping the rest of the job structure intact.
  3. Trim leading whitespace before sed in Parse parameters, so flag/positional parsing works on prefixed comments like /review -b feature/foo.

Verification

Local check of the regex against representative inputs:

Body Should match Matches
/review yes yes
/review android yes yes
/review -b feature/regression-check (the failing case) yes yes
\t/review yes yes
/review -p ios yes yes
/reviewfoo no no
please /review this no no
not a command no no

The full end-to-end behavior will be exercised by the next /review invocation on a PR that targets this branch.

The job-level if used startsWith(github.event.comment.body, '/review '), which fails when the comment body starts with whitespace (e.g. ' /review -b feature/foo'). GitHub expression syntax has no trim/regex, so we can't reliably handle this at the expression level.

Add a tiny pre-filter match job that uses a bash regex (^[[:space:]]*/review([[:space:]]|\$)) to decide whether to run. The main 	rigger-review job now
eeds: match and gates on its output. Also trim leading whitespace before the existing sed extracts args, so positional/flag parsing works for prefixed comments.
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35438

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35438"

@JanKrivanek

Copy link
Copy Markdown
Member Author

/azp run maui-pr-uitests, maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@JanKrivanek JanKrivanek marked this pull request as ready for review May 14, 2026 16:49
@JanKrivanek JanKrivanek merged commit 568bbfb into main May 20, 2026
6 of 7 checks passed
@JanKrivanek JanKrivanek deleted the fix/review-trigger-leading-whitespace branch May 20, 2026 11:19
@github-actions github-actions Bot added this to the .NET 10.0 SR8 milestone May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants