Repository navigation
Gate visual screenshot harness failures - #4915
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.
|
📝 WalkthroughWalkthroughCI workflow adds Python syntax validation for harness files, and both visual screenshot test frameworks remove non-blocking failure classification to treat all failures uniformly. The tests_v2 suite enhances terminal detection resilience and enables closing panes by captured surface ID instead of fixed indices. ChangesTest Framework and CI Guard Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (15 passed)
✨ 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 |
Greptile SummaryThis PR hardens the visual screenshot test harness by removing the non-blocking failure allowlist, fixing the root cause of D12's
Confidence Score: 4/5The changes are safe to merge — the test logic fixes a real flakiness root cause and the harness is strictly more correct than before. The D12 surface-ID fix and the non-blocking allowlist removal are both well-reasoned improvements. The only concern is the CI xargs invocation that can silently pass without running py_compile if no Python files are found at the hardcoded paths — a real but low-likelihood gap that does not affect current behavior. .github/workflows/ci.yml — the Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as test_d12_nested_close_top
participant Client as cmux client
participant CAV as _close_and_verify
participant VAR as verify_all_responsive
Test->>Client: list_surfaces()
Client-->>Test: [(0, "uuid-A", …)]
Note over Test: top_surface_id = "uuid-A"
Test->>Client: new_split("down")
Test->>Client: focus_surface(1)
Test->>Client: new_split("right")
Test->>CAV: "close_idx="uuid-A", expected=2"
CAV->>Client: close_surface("uuid-A")
CAV->>Client: wait_surface_count(2)
loop up to 3 attempts
CAV->>VAR: verify_all_responsive(after_label)
VAR->>Client: surface_health()
VAR-->>CAV: blank_err or None
alt blank_err
CAV->>Client: refresh_surfaces()
Note over CAV: sleep(0.8 + 0.4*attempt)
else no error
CAV-->>Test: change (passed)
end
end
CAV-->>Test: change (passed or failed)
Reviews (1): Last reviewed commit: "Gate visual screenshot harness failures" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
|
|
||
| - name: Validate WarpBuild runner guards | ||
| - name: Validate macOS runner guards |
There was a problem hiding this comment.
On macOS, BSD xargs without -r still invokes the utility when stdin is empty. python3 -m py_compile called with no filename arguments reads from its own stdin; in CI that stdin is /dev/null, so it exits 0 without compiling anything. If the Python test files are ever moved out of tests/, tests_v2/, or scripts/ the step will appear to pass while checking nothing. Adding xargs -r (GNU) or restructuring the command to fail-fast on empty input would prevent this silent pass-through.
This branch was successfully deployed
1 participant
Summary
Validation
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Cursor Bugbot is generating a summary for commit 88085ec. Configure here.
Summary by cubic
Make visual screenshot harness failures block CI by removing the non-blocking allowlist, and stabilize the D12 “close top” scenario to reduce flakiness. Also add Python byte-compilation in CI to catch syntax errors early.
Bug Fixes
Migration
Written for commit 88085ec. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Chores
Tests