Repository navigation
Retry Warp display regression once before failing CI - #1761
lawrencecchen wants to merge 24 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplit CI regression jobs into two parallel "attempt" runs (attempt-1 and attempt-2) for both lag and UI display resolution tests, expose Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions (workflow)"
participant Runner as "macOS Runner"
participant Cache as "Cache / Artifacts"
participant Aggregator as "Aggregation Job"
GH->>Runner: start tests-build-and-lag-attempt-1
Runner->>Cache: restore GhosttyKit / DerivedData / SwiftPM caches
Runner->>Runner: set DEVELOPER_DIR, install Zig, resolve packages, build
Runner->>Runner: create virtual display, run lag regression (emit test_started/passed)
Runner-->>GH: return attempt-1 outputs (test_started/passed)
alt attempt-1 cancelled or failed-to-start
GH->>Runner: start tests-build-and-lag-attempt-2
Runner->>Cache: restore caches or download artifacts
Runner->>Runner: repeat setup/build/regression
Runner-->>GH: return attempt-2 outputs (test_started/passed)
end
GH->>Aggregator: collect attempt outputs
Aggregator->>GH: mark overall job success if attempt-1 OR attempt-2 passed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ci.yml:
- Around line 605-611: The wrapper job ui-display-resolution-regression is
pinned but currently uses a fragile Warp runner (runs-on:
warp-macos-15-arm64-6x); change its runs-on to a stable hosted runner (e.g.,
ubuntu-latest) so the final aggregation job doesn't fail due to Warp
availability while keeping the existing if:, needs:, and timeout-minutes:
settings unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
402-568: Consider extracting shared steps into a reusable workflow.The attempt-1 and attempt-2 jobs for both
tests-build-and-lagandui-display-resolution-regressionduplicate ~150 and ~110 lines of identical setup and test steps respectively. This creates maintenance burden—changes must be applied twice and can drift out of sync.GitHub Actions reusable workflows can encapsulate the shared steps, allowing each attempt job to call the workflow with its own
continue-on-errorand output handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 402 - 568, The tests-build-and-lag jobs duplicate a large setup/test sequence across tests-build-and-lag-attempt-1 and tests-build-and-lag-attempt-2; extract the shared steps (e.g., Select Xcode, Cache GhosttyKit.xcframework, Install zig, Cache DerivedData, Resolve Swift packages, Build app, Create virtual display, Run workspace churn typing-lag regression) into a reusable workflow that exposes inputs for attempt-specific knobs (e.g., continue-on-error behavior, run conditionals, outputs like test_started and passed, runs-on), implement workflow_call in that new workflow, and replace the duplicated job bodies for tests-build-and-lag-attempt-1 and tests-build-and-lag-attempt-2 with calls to the reusable workflow while preserving the original job-level if/needs/timeout and output wiring.
🤖 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/ci.yml:
- Around line 678-681: Rename the GitHub Actions step whose id is
mark-test-started (the step that runs echo "test_started=true" >>
"$GITHUB_OUTPUT") so its name reads "Mark display regression test started"
instead of "Run display resolution churn UI regression"; this aligns the step's
descriptive text with its purpose (setting the test_started output) and avoids
duplicating the subsequent "Run display resolution churn UI regression" step
name used elsewhere (attempt-2).
- Around line 570-576: Change the aggregator job runner from the WarpBuild macOS
image to an Ubuntu runner because it only executes a simple shell check; update
the job definition for tests-build-and-lag to use runs-on: ubuntu-latest (or
ubuntu-22.04) instead of warp-macos-15-arm64-6x, leaving the job condition,
needs, and timeout-minutes intact so only the runner label is changed while
behavior and dependencies remain the same.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 402-568: The tests-build-and-lag jobs duplicate a large setup/test
sequence across tests-build-and-lag-attempt-1 and tests-build-and-lag-attempt-2;
extract the shared steps (e.g., Select Xcode, Cache GhosttyKit.xcframework,
Install zig, Cache DerivedData, Resolve Swift packages, Build app, Create
virtual display, Run workspace churn typing-lag regression) into a reusable
workflow that exposes inputs for attempt-specific knobs (e.g., continue-on-error
behavior, run conditionals, outputs like test_started and passed, runs-on),
implement workflow_call in that new workflow, and replace the duplicated job
bodies for tests-build-and-lag-attempt-1 and tests-build-and-lag-attempt-2 with
calls to the reusable workflow while preserving the original job-level
if/needs/timeout and output wiring.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ci.yml:
- Around line 351-356: The marker step ("Mark lag regression test started", id:
mark-test-started) is emitting test_started too early; move the echo
"test_started=true" >> "$GITHUB_OUTPUT" into the actual regression steps (e.g.,
the "Run workspace churn typing-lag regression" step and the corresponding
display-regression run steps referenced) so the flag is written only when the
regression command (python3/xcodebuild) is actually launched; update each
affected block (also the other occurrences mentioned) by removing the standalone
marker step and adding the echo at the start of the corresponding regression
step's run script.
In `@tests/test_ci_self_hosted_guard.sh`:
- Around line 30-82: Add the same awk-based assertions for the two new paid
attempt jobs so the test also enforces the fork guard and WarpBuild runner on
tests-build-and-lag-attempt-2 and ui-display-resolution-regression-attempt-2:
duplicate the pattern used for tests-build-and-lag-attempt-1 and
ui-display-resolution-regression-attempt-1, but target the job names
tests-build-and-lag-attempt-2 and ui-display-resolution-regression-attempt-2,
checking for runs-on: warp-macos-15-arm64-6x and the guard condition
github.event.pull_request.head.repo.full_name == github.repository and emitting
a FAIL message if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0d343e1-eb9a-49ae-98f2-1b6b114ce2fd
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/test_ci_self_hosted_guard.sh
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_ci_self_hosted_guard.sh">
<violation number="1" location="tests/test_ci_self_hosted_guard.sh:30">
P1: Missing validation check for `attempt-2` jobs. Since `attempt-2` also uses the paid Warp runner, it must be verified by this guard script to prevent accidental removal of the fork check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/actions/run-display-resolution-regression/action.yml (2)
98-100: Heredoc indentation produces leading whitespace in JSON.The heredoc content has leading spaces before the JSON object, resulting in whitespace-prefixed content in the manifest file. While
JSONDecodertypically handles leading whitespace, this is technically malformed.♻️ Use a heredoc without indentation or echo
- cat >"$MANIFEST_PATH" <<EOF - {"helperBinaryPath":"$HELPER_PATH"} - EOF + echo "{\"helperBinaryPath\":\"$HELPER_PATH\"}" >"$MANIFEST_PATH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/run-display-resolution-regression/action.yml around lines 98 - 100, The heredoc writing to MANIFEST_PATH currently includes indentation that inserts leading spaces into the JSON; change the write so the JSON object has no leading whitespace by using an unindented heredoc (ensure the line with {"helperBinaryPath":"$HELPER_PATH"} starts at column 0) or replace the heredoc with a non-indented write (e.g., use echo/printf to write the JSON) so the content written to MANIFEST_PATH is a clean, non-indented JSON string referencing HELPER_PATH.
33-35: Consider caching GhosttyKit.xcframework for consistency with lag regression.The lag regression action (
.github/actions/run-lag-regression/action.yml) cachesGhosttyKit.xcframeworkbefore downloading (lines 32-42), but this action downloads unconditionally. This may cause unnecessary downloads on cache-warm runs.♻️ Suggested caching pattern (matching lag regression)
+ - name: Cache GhosttyKit.xcframework + id: cache-ghosttykit-display + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4 + with: + path: GhosttyKit.xcframework + key: ghosttykit-${{ hashFiles('.gitmodules', 'ghostty') }} + - name: Download pre-built GhosttyKit.xcframework + if: steps.cache-ghosttykit-display.outputs.cache-hit != 'true' shell: bash run: ./scripts/download-prebuilt-ghosttykit.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/run-display-resolution-regression/action.yml around lines 33 - 35, Add a cache step for GhosttyKit.xcframework to .github/actions/run-display-resolution-regression/action.yml matching the pattern used in .github/actions/run-lag-regression/action.yml (the "Cache GhosttyKit.xcframework" step) so the framework is restored from cache when available; then wrap the existing run step that invokes ./scripts/download-prebuilt-ghosttykit.sh with a conditional that only runs when the cache miss occurs (e.g., if: steps.cache.outputs.cache-hit != 'true'), and ensure the cache step uses the same cache key and path settings as the lag-regression action to keep behavior consistent..github/actions/run-lag-regression/action.yml (1)
108-117: Virtual display process is not cleaned up on exit.The virtual display helper is started in the background (line 114) and its PID is stored in
GITHUB_ENV, but there's no trap or cleanup step to terminate it. While GitHub Actions will kill orphan processes when the job ends, explicit cleanup would be more robust and prevent resource leaks if the action is reused in composite workflows.♻️ Add cleanup trap for virtual display
- name: Create virtual display shell: bash run: | set -euo pipefail clang -framework Foundation -framework CoreGraphics \ -o /tmp/create-virtual-display scripts/create-virtual-display.m /tmp/create-virtual-display & VDISPLAY_PID=$! echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV" + # Note: cleanup handled by GitHub Actions job termination sleep 3Alternatively, add explicit cleanup in a post step or trap within the test step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/run-lag-regression/action.yml around lines 108 - 117, Add explicit cleanup for the background virtual display helper: when launching /tmp/create-virtual-display (from scripts/create-virtual-display.m) store its PID in VDISPLAY_PID as you do, then register a trap to kill that PID on EXIT (or add a dedicated post step) so the process is terminated if the job or step exits; ensure the trap references VDISPLAY_PID and still exports VDISPLAY_PID to GITHUB_ENV so downstream steps can see it, and handle the case where the PID may not exist before attempting to kill it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/actions/run-display-resolution-regression/action.yml:
- Around line 98-100: The heredoc writing to MANIFEST_PATH currently includes
indentation that inserts leading spaces into the JSON; change the write so the
JSON object has no leading whitespace by using an unindented heredoc (ensure the
line with {"helperBinaryPath":"$HELPER_PATH"} starts at column 0) or replace the
heredoc with a non-indented write (e.g., use echo/printf to write the JSON) so
the content written to MANIFEST_PATH is a clean, non-indented JSON string
referencing HELPER_PATH.
- Around line 33-35: Add a cache step for GhosttyKit.xcframework to
.github/actions/run-display-resolution-regression/action.yml matching the
pattern used in .github/actions/run-lag-regression/action.yml (the "Cache
GhosttyKit.xcframework" step) so the framework is restored from cache when
available; then wrap the existing run step that invokes
./scripts/download-prebuilt-ghosttykit.sh with a conditional that only runs when
the cache miss occurs (e.g., if: steps.cache.outputs.cache-hit != 'true'), and
ensure the cache step uses the same cache key and path settings as the
lag-regression action to keep behavior consistent.
In @.github/actions/run-lag-regression/action.yml:
- Around line 108-117: Add explicit cleanup for the background virtual display
helper: when launching /tmp/create-virtual-display (from
scripts/create-virtual-display.m) store its PID in VDISPLAY_PID as you do, then
register a trap to kill that PID on EXIT (or add a dedicated post step) so the
process is terminated if the job or step exits; ensure the trap references
VDISPLAY_PID and still exports VDISPLAY_PID to GITHUB_ENV so downstream steps
can see it, and handle the case where the PID may not exist before attempting to
kill it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25f8bde6-3636-40dc-9e4d-829c0ac26f9a
📒 Files selected for processing (4)
.github/actions/run-display-resolution-regression/action.yml.github/actions/run-lag-regression/action.yml.github/workflows/ci.ymltests/test_ci_self_hosted_guard.sh
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/actions/run-lag-regression/action.yml">
<violation number="1" location=".github/actions/run-lag-regression/action.yml:37">
P1: The GhosttyKit cache key does not include `ghostty` contents, so cache invalidation can miss Ghostty changes.</violation>
<violation number="2" location=".github/actions/run-lag-regression/action.yml:56">
P2: Reinstalling Zig can create `/usr/local/lib/zig/lib` and leave stale libraries, causing version-mismatched Zig installations.</violation>
</file>
<file name=".github/actions/run-display-resolution-regression/action.yml">
<violation number="1" location=".github/actions/run-display-resolution-regression/action.yml:38">
P2: The GhosttyKit cache key does not include Ghostty submodule contents, so stale framework artifacts can be reused after submodule revisions change.</violation>
<violation number="2" location=".github/actions/run-display-resolution-regression/action.yml:53">
P1: Verify the Zig tarball checksum before extraction/install to prevent supply-chain tampering in CI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/install-zig.sh">
<violation number="1" location="scripts/install-zig.sh:19">
P2: The early-exit guard only checks `zig version`, so it can skip reinstall after an interrupted prior install that removed `/usr/local/lib/zig`. Validate the installed lib directory before exiting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/build-ghosttykit.yml">
<violation number="1" location=".github/workflows/build-ghosttykit.yml:13">
P1: Using job-level `continue-on-error` with `needs.<job>.result` makes the retry/pass-fail logic unreliable and can produce false-green CI results.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:180">
P1: The shard pass/fail check uses `needs.*.result` from `continue-on-error` jobs, which can report success for failed attempts and mask real test failures.</violation>
<violation number="2" location=".github/workflows/ci.yml:674">
P1: The retry condition relies on `needs.<job>.result` from a `continue-on-error` job, which can suppress the fallback attempt after infra failures. Gate retries on outputs (like `test_started`) instead of `result`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_ci_unit_shard_timeout.sh">
<violation number="1" location="tests/test_ci_unit_shard_timeout.sh:15">
P2: The timeout assertion uses substring matching, so `timeout-minutes: 100` would incorrectly satisfy the expected `10` and let this regression test pass falsely.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:186">
P1: `tests-shard-1` can pass even when its post-unit regression steps fail, because success is now gated only by `unit-test-shard`'s `passed` output.</violation>
</file>
<file name="tests/test_ci_retry_output_gating.sh">
<violation number="1" location="tests/test_ci_retry_output_gating.sh:63">
P2: This regression test is implementation-shape checking (grep on workflow YAML) instead of behavioral verification, which violates the repo’s test-quality policy.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/verify_retry_result.sh">
<violation number="1" location="scripts/verify_retry_result.sh:17">
P1: Duplicate retry verification logic across 9 workflow job wrappers instead of using the centralized script</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ATTEMPT_2_COMPLETED="$6" | ||
| ATTEMPT_2_PASSED="$7" | ||
|
|
||
| if [ "$ATTEMPT_1_PASSED" = "true" ] || [ "$ATTEMPT_2_PASSED" = "true" ]; then |
There was a problem hiding this comment.
P1: Duplicate retry verification logic across 9 workflow job wrappers instead of using the centralized script
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify_retry_result.sh, line 17:
<comment>Duplicate retry verification logic across 9 workflow job wrappers instead of using the centralized script</comment>
<file context>
@@ -0,0 +1,25 @@
+ATTEMPT_2_COMPLETED="$6"
+ATTEMPT_2_PASSED="$7"
+
+if [ "$ATTEMPT_1_PASSED" = "true" ] || [ "$ATTEMPT_2_PASSED" = "true" ]; then
+ echo "$LABEL passed."
+ exit 0
</file context>
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/actions/run-macos-compat-check/action.yml">
<violation number="1" location=".github/actions/run-macos-compat-check/action.yml:160">
P2: The summary parser only matches plural `failures`, so runs with `1 failure (0 unexpected)` are misclassified and can fail CI unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| set -e | ||
|
|
||
| if [ "$EXIT_CODE" -ne 0 ]; then | ||
| SUMMARY=$(echo "$OUTPUT" | grep "Executed.*tests.*with.*failures" | tail -1) |
There was a problem hiding this comment.
P2: The summary parser only matches plural failures, so runs with 1 failure (0 unexpected) are misclassified and can fail CI unexpectedly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/actions/run-macos-compat-check/action.yml, line 160:
<comment>The summary parser only matches plural `failures`, so runs with `1 failure (0 unexpected)` are misclassified and can fail CI unexpectedly.</comment>
<file context>
@@ -0,0 +1,212 @@
+ set -e
+
+ if [ "$EXIT_CODE" -ne 0 ]; then
+ SUMMARY=$(echo "$OUTPUT" | grep "Executed.*tests.*with.*failures" | tail -1)
+ if echo "$SUMMARY" | grep -q "(0 unexpected)"; then
+ echo "Compatibility test slice only hit expected failures, treating as pass"
</file context>
…-followup-ci # Conflicts: # .github/workflows/ci-macos-compat.yml # .github/workflows/ci.yml # tests/test_ci_self_hosted_guard.sh
Summary
ui-display-resolution-regressionso branch protection stays stableTesting
./tests/test_ci_self_hosted_guard.shContext
Summary by cubic
Retries the Warp display/lag regressions, all 6
cmux-unitshards,build-ghosttykit, and macOS compatibility checks once with deterministic cancellation-only gating and hosted aggregation. Caps shard hangs to 10 minutes per attempt, restores and stabilizes macOS 15/26 compat, and hardens Zig/GhosttyKitreuse across CI.Bug Fixes
build-ghosttykit, and macOS compat: attempt 2 runs only if attempt 1 never reached real work (test_started/build_started) or nevercompleted. Hosted aggregators onubuntu-latestrequire explicitcompleted/passed.scripts/install-zig.sh(lib_dircheck); cached/prebuiltGhosttyKit.xcframeworkkeyed to theghosttysubmodule; SPM resolve retry. FixedGhosttyKitchecksum guard across composite actions.Refactors
./.github/actions/run-display-resolution-regression,./.github/actions/run-lag-regression,./.github/actions/run-unit-test-shard,./.github/actions/run-macos-compat-checkthat emittest_started/completed/passed.build-ghosttykitand macOS compat into attempt‑1/attempt‑2 and verify viascripts/verify_retry_result.sh. Unified Zig install andGhosttyKitcache keys.Written for commit b5f7545. Summary will update on new commits.
Summary by CodeRabbit