chore: Implement automated screenshot diff reporting#182
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces automated screenshot diff reporting for visual regression testing. It adds three new GitHub Actions workflows that capture screenshot comparisons, generate diff artifacts, and post results as PR comments, along with a Gradle property enabling the comparison mode. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/screenshot-comparison-comment.yml (1)
63-67: Fragile shell patterns in the report generator.
for file in $(ls *.png | grep _compare.png)parseslsand word-splits on whitespace — any PNG name containing spaces will break the loop, and if no_compare.pngfiles exist,ls *.pngemits an error and the step fails (remember this step only runs onenv.exists == 'true', i.e., when some PNGs were pushed, which may not include any_compare.png).DISPLAY_NAME=$(echo $file | sed -r 's/(.{20})/\1<br>/g')is unquoted.Prefer a glob with
nullglob:shopt -s nullglob for file in *_compare.png; do URL="https://github.com/${REPO}/blob/${BRANCH}/${file}?raw=true" DISPLAY_NAME=$(printf '%s' "$file" | sed -r 's/(.{20})/\1<br>/g') echo "| $DISPLAY_NAME |  |" >> "$GITHUB_OUTPUT" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/screenshot-comparison-comment.yml around lines 63 - 67, Replace the fragile ls+grep pattern and unquoted expansions with a nullglob-aware glob loop: enable shopt -s nullglob, iterate with for file in *_compare.png; do, build URL using the existing REPO/BRANCH variables, generate DISPLAY_NAME with printf '%s' "$file" piped to sed, and always quote expansions when writing to the output (e.g. echo "| $DISPLAY_NAME |  |" >> "$GITHUB_OUTPUT") so filenames with spaces and the case of zero matches are handled safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/screenshot-comparison-comment.yml:
- Around line 25-26: The step id get-pull-request-number reads
attacker-controllable diff-reports/pr/NR and uses it as pull_request_number for
peter-evans/find-comment and create-or-update-comment; sanitize and validate
that the artifact contains only a single numeric PR id (strip non-digits,
truncate to a reasonable length, ensure non-empty and single-line) and fail the
job if validation fails so an invalid or multi-line payload cannot be passed to
the actions as issue-number.
- Around line 88-100: The cleanup step fails to see remote companion_* branches
because actions/checkout@v4 uses fetch-depth: 1; update the workflow so the
checkout uses fetch-depth: 0 or explicitly fetch the companion refs before
running the loop (e.g., run a git fetch --prune origin
'+refs/heads/companion_*:refs/remotes/origin/companion_*'), or replace the loop
with GitHub REST/gh API calls to list and delete branches; also enable safer
shell settings (set -euo pipefail) at the top of the step and ensure the
arithmetic and variables are quoted (e.g., quote $((now_timestamp -
last_commit_date_timestamp)) and guard empty last_commit_date_timestamp) so a
missing ref won't break the loop.
- Around line 28-48: The current Push to Companion Branch step flattens PNGs
(find ... -exec cp {} . \;) causing filename collisions and only writes
exists=true inside the git status check so re-runs with no new commit won't
refresh/clear stale comments; update the step that sets BRANCH_NAME and runs git
checkout --orphan to copy files using preserved paths (use cp --parents or
module-prefixed renaming when running find diff-reports ...) so files aren’t
overwritten, and change the existence logic so exists=true is set whenever the
companion branch contains any diffs (e.g., after copying, check for any *.png in
the working tree or check if the branch already existed and contains files) and
write echo "exists=true" >> "$GITHUB_ENV" in both the "new commit created" and
"no new commit but branch contains files" paths so downstream steps always know
when a companion branch with diffs exists.
- Around line 50-68: The generate_report step currently injects untrusted
github.event.workflow_run.head_branch directly into the shell via
BRANCH="companion_${{ github.event.workflow_run.head_branch }}", which enables
command injection; move the branch construction into the step's env mapping
(e.g., set an env variable like BRANCH: "companion_${{
github.event.workflow_run.head_branch }}") and remove direct interpolation from
the run block, then reference $BRANCH inside the run script; update the step
with id generate_report to use env: BRANCH and use $BRANCH in the loop so the
branch value is passed safely to the shell.
In @.github/workflows/screenshot-comparison.yml:
- Around line 43-52: The workflow should detect when the download artifact step
fails and skip or change behavior of the compare/upload steps: add an id to the
"Download Base Screenshot" step (e.g., id: download_base) and change the
following "Run Comparison" (and any upload/report steps) to run only when
steps.download_base.outcome == 'success' (or post a clear "no baseline
available" message when outcome != 'success'); reference the "Download Base
Screenshot" step id and the Gradle task compareRoborazziDebug to gate or alter
downstream steps accordingly so the compare is not executed against an empty
baseline.
---
Nitpick comments:
In @.github/workflows/screenshot-comparison-comment.yml:
- Around line 63-67: Replace the fragile ls+grep pattern and unquoted expansions
with a nullglob-aware glob loop: enable shopt -s nullglob, iterate with for file
in *_compare.png; do, build URL using the existing REPO/BRANCH variables,
generate DISPLAY_NAME with printf '%s' "$file" piped to sed, and always quote
expansions when writing to the output (e.g. echo "| $DISPLAY_NAME |  |"
>> "$GITHUB_OUTPUT") so filenames with spaces and the case of zero matches are
handled safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99681bcc-2cfc-4a5d-849d-791ac2a17b7e
📒 Files selected for processing (4)
.github/workflows/screenshot-comparison-comment.yml.github/workflows/screenshot-comparison.yml.github/workflows/unit-test.ymlgradle.properties
| - id: get-pull-request-number | ||
| run: echo "pull_request_number=$(cat diff-reports/pr/NR)" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Validate pull_request_number read from the untrusted artifact.
diff-reports/pr/NR originates from the PR-triggered screenshot-comparison workflow's artifact, which is attacker-controllable content in the workflow_run threat model. Its value is then passed to peter-evans/find-comment and peter-evans/create-or-update-comment as issue-number (Lines 75, 84). A non-numeric or multi-line payload could cause the bot to comment on an arbitrary issue/PR in the repo, or fail in non-obvious ways.
Sanitize before use, e.g.:
NR=$(tr -cd '0-9' < diff-reports/pr/NR | head -c 10)
[ -n "$NR" ] || { echo "Invalid PR number"; exit 1; }
echo "pull_request_number=$NR" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/screenshot-comparison-comment.yml around lines 25 - 26,
The step id get-pull-request-number reads attacker-controllable
diff-reports/pr/NR and uses it as pull_request_number for
peter-evans/find-comment and create-or-update-comment; sanitize and validate
that the artifact contains only a single numeric PR id (strip non-digits,
truncate to a reasonable length, ensure non-empty and single-line) and fail the
job if validation fails so an invalid or multi-line payload cannot be passed to
the actions as issue-number.
| - name: Checkout Default Branch | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.repository.default_branch }} | ||
|
|
||
| - name: Push to Companion Branch | ||
| env: | ||
| BRANCH_NAME: companion_${{ github.event.workflow_run.head_branch }} | ||
| run: | | ||
| git checkout --orphan "$BRANCH_NAME" | ||
| git rm -rf . | ||
| find diff-reports -type f -name "*.png" -exec cp {} . \; | ||
|
|
||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git config --global user.name "ScreenshotBot" | ||
| git config --global user.email "bot@github.com" | ||
| git add . | ||
| git commit -m "Add screenshot diffs" | ||
| git push origin "$BRANCH_NAME" -f | ||
| echo "exists=true" >> "$GITHUB_ENV" | ||
| fi |
There was a problem hiding this comment.
Companion-branch push step has two correctness gaps.
- Filename collisions on flatten.
find diff-reports -type f -name "*.png" -exec cp {} . \;flattens all PNGs from every module into the branch root. Different feature modules frequently share snapshot names (e.g.,HomeScreen_compare.png), so latercpcalls silently overwrite earlier ones and the PR comment will show incomplete diffs. Prefercp --parents(preserving module paths) or a module-prefixed rename. exists=trueonly set on subsequent runs. The flag is only written insideif [ -n "$(git status --porcelain)" ]. On the first push to a brand-new companion branch,git status --porcelainwill correctly be non-empty, so that case is fine — but if someone re-runs with identical diffs the subsequent steps are skipped and the existing stale comment is never refreshed/cleaned up. Consider also handling the "no changes, branch already exists" path (e.g., still regenerate the comment from the existing branch contents, or explicitly clear the bot comment when there are no diffs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/screenshot-comparison-comment.yml around lines 28 - 48,
The current Push to Companion Branch step flattens PNGs (find ... -exec cp {} .
\;) causing filename collisions and only writes exists=true inside the git
status check so re-runs with no new commit won't refresh/clear stale comments;
update the step that sets BRANCH_NAME and runs git checkout --orphan to copy
files using preserved paths (use cp --parents or module-prefixed renaming when
running find diff-reports ...) so files aren’t overwritten, and change the
existence logic so exists=true is set whenever the companion branch contains any
diffs (e.g., after copying, check for any *.png in the working tree or check if
the branch already existed and contains files) and write echo "exists=true" >>
"$GITHUB_ENV" in both the "new commit created" and "no new commit but branch
contains files" paths so downstream steps always know when a companion branch
with diffs exists.
| - name: Generate Report Body | ||
| if: env.exists == 'true' | ||
| id: generate_report | ||
| run: | | ||
| BRANCH="companion_${{ github.event.workflow_run.head_branch }}" | ||
| REPO="${{ github.repository }}" | ||
|
|
||
| echo "REPORT_BODY<<EOF" >> $GITHUB_OUTPUT | ||
| echo "### Roborazzi Snapshot Diff Report" >> $GITHUB_OUTPUT | ||
| echo "" >> $GITHUB_OUTPUT | ||
| echo "| File Name | Comparison |" >> $GITHUB_OUTPUT | ||
| echo "|---|---|" >> $GITHUB_OUTPUT | ||
|
|
||
| for file in $(ls *.png | grep _compare.png); do | ||
| URL="https://github.com/${REPO}/blob/${BRANCH}/${file}?raw=true" | ||
| DISPLAY_NAME=$(echo $file | sed -r 's/(.{20})/\1<br>/g') | ||
| echo "| $DISPLAY_NAME |  |" >> $GITHUB_OUTPUT | ||
| done | ||
| echo "EOF" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions workflow_run script injection head_branch security
💡 Result:
GitHub Actions workflows triggered by the workflow_run event carry significant security risks, particularly for script injection vulnerabilities involving potentially untrusted data like head_branch. Official GitHub documentation warns that running untrusted code on workflow_run can lead to cache poisoning, unintended write access, and secret exposure. The workflow_run event provides access to the triggering workflow's details via github.event.workflow_run, including fields like head_branch, head_sha, event (e.g., 'pull_request'), ref, and head_repository. If the triggering workflow was executed from a pull request (especially from a fork via pull_request trigger), head_branch reflects the PR's source branch, which attackers can control to inject malicious payloads (e.g., branch names like zzz";echo${IFS}"hello";# or fooidbar). Script injection occurs when expressions like ${{ github.event.workflow_run.head_branch }} are interpolated directly into run: steps without sanitization. GitHub evaluates ${{ }} expressions before shell execution, substituting untrusted values into scripts, enabling command injection. This is exacerbated in privileged workflow_run workflows (with secrets/write permissions) triggered by unprivileged ones compromised via PRs. Examples from GitHub Security Lab and advisories (e.g., GHSL-2024-326) show branch name injection in workflows, analogous to head_branch. Tools like OSS-F Scorecard flag workflow_run usage due to chaining risks allowing PRs to trigger privileged execution. Mitigations from GitHub docs: - Treat github.event.* (especially ending in ref, head_ref, head_branch) as untrusted. - Use env: for intermediate variables: env: BRANCH: ${{ github.event.workflow_run.head_branch }} then run: echo "$BRANCH". - Prefer JavaScript actions for processing. - Add guards: if: github.event_name != 'pull_request' && github.repository == github.event.workflow_run.head_repository.full_name. - Pin branches: to trusted ones, limit permissions: { contents: read }. - Use branch filters and verify event origin. No specific CVE for workflow_run.head_branch, but pattern matches documented script injection risks in privileged triggers.
Citations:
- 1: https://docs.github.com/en/actions/concepts/security/script-injections
- 2: https://docs.github.com/actions/reference/workflows-and-actions/events-that-trigger-workflows
- 3: https://docs.github.com/en/actions/reference/events-that-trigger-workflows
- 4: Feature: Check for usage of on: workflow_run trigger ossf/scorecard#3861
- 5: https://securitylab.github.com/advisories/GHSL-2024-325_GHSL-2024-326_Actual/
- 6: https://docs.github.com/en/actions/reference/security/secure-use
- 7: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests
- 8: https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/
- 9: https://docs.github.com/en/actions/reference/contexts-reference
🏁 Script executed:
# Locate the workflow file
git ls-files | grep -E '\.github/workflows.*screenshot.*\.yml'Repository: komodgn/meta-android
Length of output: 159
🏁 Script executed:
cat -n .github/workflows/screenshot-comparison-comment.yml | head -80Repository: komodgn/meta-android
Length of output: 3374
Script injection vulnerability: workflow_run.head_branch interpolated directly into shell at line 54.
BRANCH="companion_${{ github.event.workflow_run.head_branch }}" expands untrusted branch names directly into the bash script. Since this workflow runs with contents: write and pull-requests: write, a branch name crafted with shell metacharacters (valid git refs allow many characters like ;, $(), etc.) executes arbitrary commands with elevated permissions. The same step block already demonstrates the correct mitigation at lines 34–37, using an env: variable to safely pass the value. Apply the same pattern here:
Fix
- name: Generate Report Body
if: env.exists == 'true'
id: generate_report
+ env:
+ HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }}
+ REPO: ${{ github.repository }}
run: |
- BRANCH="companion_${{ github.event.workflow_run.head_branch }}"
- REPO="${{ github.repository }}"
+ BRANCH="companion_${HEAD_BRANCH}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/screenshot-comparison-comment.yml around lines 50 - 68,
The generate_report step currently injects untrusted
github.event.workflow_run.head_branch directly into the shell via
BRANCH="companion_${{ github.event.workflow_run.head_branch }}", which enables
command injection; move the branch construction into the step's env mapping
(e.g., set an env variable like BRANCH: "companion_${{
github.event.workflow_run.head_branch }}") and remove direct interpolation from
the run block, then reference $BRANCH inside the run script; update the step
with id generate_report to use env: BRANCH and use $BRANCH in the loop so the
branch value is passed safely to the shell.
| - name: Cleanup outdated companion branches | ||
| shell: bash | ||
| run: | | ||
| git branch -r --format="%(refname:lstrip=3)" | grep companion_ | while read -r branch; do | ||
| last_commit_date_timestamp=$(git log -1 --format=%ct "origin/$branch") | ||
| now_timestamp=$(date +%s) | ||
|
|
||
| # 30 days | ||
| if [ $((now_timestamp - last_commit_date_timestamp)) -gt 2592000 ]; then | ||
| echo "Deleting outdated branch: $branch" | ||
| git push origin --delete "$branch" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Cleanup step won't see remote branches due to shallow checkout.
actions/checkout@v4 defaults to fetch-depth: 1 and fetches only the checked-out ref, so git branch -r in this step typically lists just origin/<default_branch> — no companion_* refs are present, and git log -1 origin/$branch would fail for any it did find. The 30-day cleanup is effectively a no-op.
Either set fetch-depth: 0 on the checkout at Line 29 (and ensure remote branches are fetched, e.g., add git fetch --prune origin '+refs/heads/companion_*:refs/remotes/origin/companion_*' before this loop), or implement the cleanup via the GitHub REST API (gh api repos/{owner}/{repo}/branches + gh api -X DELETE .../git/refs/heads/...) which doesn't depend on local refs.
Also consider adding set -euo pipefail and quoting $((now_timestamp - last_commit_date_timestamp)) guards so a single bad ref doesn't abort the loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/screenshot-comparison-comment.yml around lines 88 - 100,
The cleanup step fails to see remote companion_* branches because
actions/checkout@v4 uses fetch-depth: 1; update the workflow so the checkout
uses fetch-depth: 0 or explicitly fetch the companion refs before running the
loop (e.g., run a git fetch --prune origin
'+refs/heads/companion_*:refs/remotes/origin/companion_*'), or replace the loop
with GitHub REST/gh API calls to list and delete branches; also enable safer
shell settings (set -euo pipefail) at the top of the step and ensure the
arithmetic and variables are quoted (e.g., quote $((now_timestamp -
last_commit_date_timestamp)) and guard empty last_commit_date_timestamp) so a
missing ref won't break the loop.
| - name: Download Base Screenshot | ||
| uses: dawidd6/action-download-artifact@v3 | ||
| continue-on-error: true | ||
| with: | ||
| name: test-reports | ||
| workflow: unit-test.yml | ||
| commit: ${{ steps.get_base_branch_head.outputs.sha }} | ||
|
|
||
| - name: Run Comparison | ||
| run: ./gradlew compareRoborazziDebug --stacktrace |
There was a problem hiding this comment.
Handle missing base-branch artifact explicitly.
Download Base Screenshot is marked continue-on-error: true, so when the base branch has no test-reports artifact yet (e.g., first run after this PR merges, retention expired, or base unit-test was skipped via skip-ci), the next step still runs ./gradlew compareRoborazziDebug against an empty baseline. Depending on Roborazzi's behavior this will either fail the job or silently report every screenshot as changed, which is then posted to the PR as a misleading "diff report."
Consider detecting the download outcome (id: + steps.<id>.outcome == 'success') and either skipping the compare/upload steps or posting a clear "no baseline available" message instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/screenshot-comparison.yml around lines 43 - 52, The
workflow should detect when the download artifact step fails and skip or change
behavior of the compare/upload steps: add an id to the "Download Base
Screenshot" step (e.g., id: download_base) and change the following "Run
Comparison" (and any upload/report steps) to run only when
steps.download_base.outcome == 'success' (or post a clear "no baseline
available" message when outcome != 'success'); reference the "Download Base
Screenshot" step id and the Gradle task compareRoborazziDebug to gate or alter
downstream steps accordingly so the compare is not executed against an empty
baseline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Android CI Summary
Step Results:
- Debug Build: ⏭️ Skipped (0s)
- Code Style Check: ⏭️ Skipped (0s)
- Compose Stability: ⏭️ Skipped (0s)
Total Time: 0s
See the Actions Log for details.
1 participant
Related issue
Work Description ✏️
Note
The workflow_run based reporting bot is not yet registered in the develop branch, preventing it from being triggered.
Screenshot 📸
Summary by CodeRabbit
Release Notes