Item 41: GUI-app-aware caveat hint for the silent-timeout case - #447
Conversation
~exe_smokerun.ps1's activity-aware kill only force-stops a verification run that produced zero stdout/stderr bytes by the ~30s deadline -- the same shape a correctly-behaving GUI app (tkinter/PyQt, a mainloop with no console output) produces. HP_EXE_EXIT=="-1" already implies this (the kill is gated on $sawOutput being false), so no new runtime signal was needed: record HP_EXE_TIMEDOUT_SILENT at the same point HP_EXE_VERIFY_FAILED is set, reset once per fresh build attempt alongside HP_DLL_HINT_STATE's own reset, and print a distinguishing note from the postflight caveat panel via a new :pfb_gui_hint subroutine, mirroring :pfb_dll_hint's existing dispatch shape. New tests/selfapps_gui_timeout_hint.ps1 (uv lane, non-gating) reproduces the signal directly: a real PyInstaller-built EXE that sleeps with zero output, verified with HP_SMOKERUN_KILL_MS shortened via its pre-existing test-only override. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review Generated by Claude Code |
|
Warning Review limit reached
Next review available in: 58 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe bootstrapper now tracks silent executable verification timeouts per build attempt and displays GUI-specific caveat guidance. A Windows PowerShell self-test validates the behavior, and CI runs it as a non-gating ChangesGUI timeout caveat handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SelfTest
participant Bootstrapper
participant SilentExecutable
participant NDJSON
SelfTest->>Bootstrapper: run with HP_SMOKERUN_KILL_MS
Bootstrapper->>SilentExecutable: build and verify executable
SilentExecutable-->>Bootstrapper: remain silent until timeout
Bootstrapper-->>SelfTest: emit timeout and GUI caveat output
SelfTest->>NDJSON: record self.exe.timeout_gui_hint result
Possibly related PRs
Suggested reviewers: ✨ 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 review Generated by Claude Code |
|
|
The workflow's own fallback logic called github.rest.pulls.merge() directly whenever GraphQL's viewerCanEnableAutoMerge read false, gated only on pr.mergeable === 'MERGEABLE' -- which means "no git conflicts with the base branch," not "required checks passed." An admin-capable AUTOMERGE_TOKEN makes viewerCanEnableAutoMerge read false immediately on a fresh PR (nothing to queue -- the actor can already bypass required checks), so this fallback merged PR #447 within ~20 seconds of it opening, before any CI check had even started. Confirmed directly from the actual workflow run's job log. Removed both direct-merge fallback branches (in the "Enable Auto-merge via PAT" and "Enable Auto-merge via GITHUB_TOKEN" steps). The workflow now only ever queues a merge via enablePullRequestAutoMerge, which GitHub itself will not complete until required checks and reviews genuinely pass -- it never calls pulls.merge() directly under any code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV
…448) * pr-automerge.yml: remove the direct-merge fallback, never bypass CI The workflow's own fallback logic called github.rest.pulls.merge() directly whenever GraphQL's viewerCanEnableAutoMerge read false, gated only on pr.mergeable === 'MERGEABLE' -- which means "no git conflicts with the base branch," not "required checks passed." An admin-capable AUTOMERGE_TOKEN makes viewerCanEnableAutoMerge read false immediately on a fresh PR (nothing to queue -- the actor can already bypass required checks), so this fallback merged PR #447 within ~20 seconds of it opening, before any CI check had even started. Confirmed directly from the actual workflow run's job log. Removed both direct-merge fallback branches (in the "Enable Auto-merge via PAT" and "Enable Auto-merge via GITHUB_TOKEN" steps). The workflow now only ever queues a merge via enablePullRequestAutoMerge, which GitHub itself will not complete until required checks and reviews genuinely pass -- it never calls pulls.merge() directly under any code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV * pr-automerge.yml: attempt enable regardless of viewerCanEnableAutoMerge CodeRabbit review finding on PR #448: skipping outright when viewerCanEnableAutoMerge reads false meant a genuinely still-pending PR could fail to ever get queued if that client-side hint happened to read false for an unrelated reason (e.g. before AUTOMERGE_TOKEN is rescoped off admin/bypass). The GraphQL mutation itself is the real source of truth -- always attempt it now; a failure is just logged, never treated as license to merge directly (no pulls.merge() call was reintroduced). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV * Try AUTOMERGE_TOKEN_NONADMIN_NO_BYPASS before the admin-capable legacy token Adds a new, preferred first step in the auto-merge job that authenticates with AUTOMERGE_TOKEN_NONADMIN_NO_BYPASS (a fine-grained PAT scoped to Pull requests: Read and write only). The existing admin-capable AUTOMERGE_TOKEN step stays wired as a fallback -- its own per-PR "already armed (REST)" check makes it a safe no-op for anything the new token already enabled -- until the new token is confirmed working across several real PRs, at which point AUTOMERGE_TOKEN's own step should be removed. The new step never blocks the fallback: every per-PR REST/GraphQL call is wrapped in try/catch and any failure -- including an outright auth failure on the very first call -- is logged via core.warning (not core.info) so it stands out in the Actions UI, and continue-on-error guards against the action itself failing to initialize (e.g. a malformed token) before the script body even runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
~exe_smokerun.ps1's activity-aware kill only force-stops a verification run that producedZERO stdout/stderr bytes by the ~30s deadline -- exactly the intended behavior for a genuinely
hung process, but also exactly the shape a correctly-behaving GUI app (tkinter/PyQt, a
mainloop()with no console output) produces. It gets killed,HP_EXE_EXIT=-1,[STATUS] Run Status: TIMED OUT, and the postflight panel reads "SETUP COMPLETE -- WITH ACAVEAT" with nothing anywhere explaining that this specific shape is expected and does not mean
something is broken.
~exe_smokerun.ps1's own$killedonly ever becomes truewhen
$sawOutputwas still false at the kill decision, soHP_EXE_EXIT=="-1"at the pointrun_setup.batalready checks it already implies zero output was observed. A newHP_EXE_TIMEDOUT_SILENTflag is set at that exact point (alongside the existingHP_EXE_VERIFY_FAILED=1), reset once per fresh build attempt alongsideHP_DLL_HINT_STATE'sown reset (same cascaded-away-provider-must-not-leak-a-stale-hint reasoning).
:print_postflight_briefing's:pfb_caveatbranch now calls a new:pfb_gui_hintsubroutinewhen the flag is defined, mirroring
:pfb_dll_hint's existing dispatch shape -- prints a notedistinguishing "this can happen for a GUI app that opens its own window and prints nothing to
the console" from a genuine crash/hang, right in the caveat panel text itself, not just the
pre-launch warning
:warn_user_code_launchalready gives.Test plan
tests/selfapps_gui_timeout_hint.ps1(uvlane, non-gating) does not launch a real GUI(no display on a headless Windows CI runner) -- it reproduces the exact signal instead: a
real PyInstaller-built EXE (
import time; time.sleep(600), zero output) verified withHP_SMOKERUN_KILL_MSshortened to 12000ms via its pre-existing test-only override (alreadyused the same way by
tests/test_exe_smokerun.pyat the Python-unit-test level -- norun_setup.batcode change needed to support it).[STATUS] Run Status: TIMED OUT, the caveat panel header, the new GUI-hint text,and that
dist\genuinely exists (proving the EXE was really built, not the unrelated:print_no_exe_briefingno-EXE path).docs/agent-ndjson.mdanddocs/agent-closed-backlog.mdupdated with the new row and afull Item 41 closure entry; removed from CLAUDE.md's Active Backlog.
python tools/check_delimiters.py run_setup.bat-- clean.python -m yamllint .github/workflows/andactionlint-- clean.tools/run_sanity_sweep.sh): compileall, pyflakes, delimitercheck, CRLF check, markdownlint, yamllint, actionlint, ASCII sweep, PowerShell AST parse
sweep, and the full pytest suite (530 passed, 3 skipped) -- all green.
python tools/check_ndjson_registry.py-- PASS, no doc/code registry mismatches.real/conda-fullgating lanes, plus the newuv-lane step) to confirm onreal Windows runners.
Co-Authored-By: Claude Sonnet 5
https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV
Generated by Claude Code