Skip to content

Try to capture a screenshot on error - #284

Open
amezin wants to merge 1 commit into
masterfrom
failure-screenshot
Open

Try to capture a screenshot on error#284
amezin wants to merge 1 commit into
masterfrom
failure-screenshot

Conversation

@amezin

@amezin amezin commented Aug 6, 2026

Copy link
Copy Markdown
Member

No description provided.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 089321a1-d93c-49a6-bab8-432f476f23da

📥 Commits

Reviewing files that changed from the base of the PR and between cbc0f76 and 5dad922.

📒 Files selected for processing (1)
  • .github/workflows/box.yml
📜 Recent review details
⏰ Context from checks skipped due to timeout. (12)
  • GitHub Check: ci / box (alpine324) / build alpine324
  • GitHub Check: ci / box (archlinux) / build archlinux
  • GitHub Check: ci / box (ubuntu2404) / build ubuntu2404
  • GitHub Check: ci / box (fedora43) / build fedora43
  • GitHub Check: ci / box (nixos) / build nixos
  • GitHub Check: ci / box (opensuseleap16) / build opensuseleap16
  • GitHub Check: ci / box (opensusetumbleweed) / build opensusetumbleweed
  • GitHub Check: ci / box (silverblue44) / build silverblue44
  • GitHub Check: ci / box (fedora44) / build fedora44
  • GitHub Check: ci / box (debian13) / build debian13
  • GitHub Check: ci / box (ubuntu2604) / build ubuntu2604
  • GitHub Check: ci / box (silverblue43) / build silverblue43
🧰 Additional context used
🪛 zizmor (1.29.0)
.github/workflows/box.yml

[warning] 80-80: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🔇 Additional comments (3)
.github/workflows/box.yml (3)

93-95: Keep the existing timeout fix for qmp-shell.

Line 95 still invokes qmp-shell without a timeout. An unavailable or unresponsive monitor can consume the remaining job timeout before logs are uploaded. (docs.github.com)


49-49: LGTM!

Also applies to: 65-65


76-82: 🩺 Stability & Availability

Confirm QEMU survives the build timeout before relying on the screenshot handler.

The build step has its own 45-minute timeout-minutes, and the Fedora QEMU template keeps ssh_timeout = "1h". If provisioning hangs and GitHub Actions kills the step before Packer exits, the cleanup in the always() handler may fail to reach output-*/*.monitor, so no logs/screen.png is produced.


📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Improved build validation and failure handling.
    • Added automated diagnostic capture when builds fail, making issues easier to investigate.
    • Updated the build environment to support more reliable virtual machine image creation.
    • Extended build time allowances to reduce failures during lengthy image creation tasks.

Walkthrough

The workflow extends build timeouts, installs QEMU QMP support, configures Packer failure handling, and captures a QEMU screen dump when the build fails.

Changes

QEMU build diagnostics

Layer / File(s) Summary
Packer build configuration
.github/workflows/box.yml
The job timeout increases to 60 minutes. The workflow installs python3-qemu-qmp. The Packer step has a 45-minute timeout, an ID, and abort-on-error handling.
QEMU failure capture
.github/workflows/box.yml
When the build fails, an always-evaluated handler runs qmp-shell and saves a screen dump to logs/screen.png.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided, so the description does not communicate the purpose of the changes. Add a brief description that explains the failure screenshot capture and related workflow changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: capturing a screenshot when the build fails.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch failure-screenshot

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/box.yml:
- Around line 92-95: Bound the qmp-shell invocation in the failed-build handler
with an explicit step-level timeout so unavailable or unresponsive monitor files
cannot delay log upload until the job timeout; keep the existing
always/build-outcome condition and screendump command behavior unchanged.
🪄 Autofix

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4b35646c-598b-47ee-881a-058fd7749b8d

📥 Commits

Reviewing files that changed from the base of the PR and between cbc0f76 and 1e916c9.

📒 Files selected for processing (1)
  • .github/workflows/box.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: ci / setup
🧰 Additional context used
🪛 zizmor (1.29.0)
.github/workflows/box.yml

[warning] 79-79: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🔇 Additional comments (1)
.github/workflows/box.yml (1)

65-65: LGTM!

Also applies to: 76-81

Comment thread .github/workflows/box.yml
Comment on lines +92 to +95
- if: always() && steps.build.outcome != 'success' && steps.build.outcome != 'skipped'
run: |
echo 'screendump filename=logs/screen.png format=png' | qmp-shell -v output-*/*.monitor

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== workflow file excerpt =="
if [ -f .github/workflows/box.yml ]; then
  nl -ba .github/workflows/box.yml | sed -n '1,140p'
else
  echo "missing .github/workflows/box.yml"
fi

echo
echo "== timeout usage in workflow =="
rg -n "timeout|qmp-shell|shell-timeout|screendump|steps\.build\.outcome" .github/workflows/box.yml || true

echo
echo "== QMP references =="
rg -n "qmp-shell|output-\*|monitor|screendump" .github/workflows/box.yml . 2>/dev/null | head -200 || true

echo
echo "== GitHub Actions timeout usage nearby =="
rg -n "timeout-minutes:|shell:|run: |timeout " .github/workflows/box.yml

Repository: ddterm/gnome-shell-box

Length of output: 228


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== workflow file excerpt =="
if [ -f .github/workflows/box.yml ]; then
  cat -n .github/workflows/box.yml | sed -n '1,140p'
else
  echo "missing .github/workflows/box.yml"
fi

echo
echo "== timeout usage in workflow =="
rg -n "timeout|qmp-shell|shell-timeout|screendump|steps\.build\.outcome" .github/workflows/box.yml || true

echo
echo "== QMP references =="
rg -n "qmp-shell|output-\*|monitor|screendump" .github/workflows/box.yml . 2>/dev/null | head -200 || true

echo
echo "== GitHub Actions timeout usage nearby =="
rg -n "timeout-minutes:|shell:|run: |timeout " .github/workflows/box.yml

Repository: ddterm/gnome-shell-box

Length of output: 6311


Bound the QMP failure handler.

This failed-build step runs under the 45-minute job timeout and has no step-level timeout around qmp-shell. If output-*/*.monitor is unavailable or unresponsive, the log-upload path can be delayed until the job timeout.

Add a bounded timeout for the qmp-shell call.

Proposed fix
-          echo 'screendump filename=logs/screen.png format=png' | qmp-shell -v output-*/*.monitor
+          echo 'screendump filename=logs/screen.png format=png' |
+            timeout --signal=TERM --kill-after=5s 30s qmp-shell -v output-*/*.monitor
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- if: always() && steps.build.outcome != 'success' && steps.build.outcome != 'skipped'
run: |
echo 'screendump filename=logs/screen.png format=png' | qmp-shell -v output-*/*.monitor
- if: always() && steps.build.outcome != 'success' && steps.build.outcome != 'skipped'
run: |
echo 'screendump filename=logs/screen.png format=png' |
timeout --signal=TERM --kill-after=5s 30s qmp-shell -v output-*/*.monitor
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/box.yml around lines 92 - 95, Bound the qmp-shell
invocation in the failed-build handler with an explicit step-level timeout so
unavailable or unresponsive monitor files cannot delay log upload until the job
timeout; keep the existing always/build-outcome condition and screendump command
behavior unchanged.

@amezin
amezin force-pushed the failure-screenshot branch from 1e916c9 to 5dad922 Compare August 6, 2026 02:20
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

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.

1 participant