Sign up
View stargazers13Sign up

fix(server): wake PR watches when no checks report#23863

merged6
main
fix/issue-23701
opened1d agobyJJulius Marminge
7 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

A watched GitHub pull request whose workflow skips its changes can wait forever without waking the agent. A docs-only pull request with pull_request.paths-ignore: ["docs/**"] reproduces this: no checks appear, and the watch reports no update.

Change

Report once when a GitHub head has remained without checks beyond a grace period. Keep the existing read cadence, preserve suppression after checks have been observed, and retain notification state across restarts and rewatching. Older watch records are handled conservatively. Other forge providers retain their existing behavior.

Scope and approval

Fixes #23701

Triaged bug via the automated issue flow; GitCafe issue #23701.

Verification

  • The regression fails on main with expected +0 to equal 1 and passes with this fix. It verifies one notice, no CI-success claim, and suppression after pending checks were observed. Added regressions cover legacy saved watches and providers that do not report checks.
  • Focused server suites, scoped lint and server/contracts typechecks passed during implementation. After merging current main, all three watch suites passed: 105 tests in runtimeLayer.test.ts, pullRequestWatch.test.ts, and pullRequestWatch.forgejoComments.test.ts.
  • Runtime evidence report and sanitized transcript: the real dev server read a docs-only GitHub head with zero checks and emitted exactly one updated notice after 18 minutes. The notice made no passing-check claim. Its persisted clock cleared; rewatching and restarting retained that state without another notice.
  • The eligibility floor is 15 minutes; unchanged GitHub fingerprints normally defer the next detail read to about 30 minutes. A dev reload preceded the captured notice, so this run does not establish uninterrupted quiet-read timing.
  • Server-only change with no changed render path: visual fixture matrix is empty. No screenshots are applicable. No platform-specific code changed.

Implemented with Claude Opus 5.5 in the Claude Agent harness; triage, review, and proof with GPT-6.1-Sol in the Codex harness.

Review and audit log

Read-only audit of 1fbb142 against refreshed merge-base 2c8bdf6. Two fresh parallel reviewers inspected the complete diff and relevant callers/contracts. No code changes, installation, server/browser launch, or suite reruns in this round.

  • Reviewer A — Codex / gpt-6.1-sol — VERDICT: MERGE. No blocking defect. Read full diff and traced persistence, compatibility, check evaluation, provider gating, and delivery. Reports passing in-memory diagnostics for threshold boundaries, late success/failure, serialized state, schema compatibility and cadence; these are reviewer diagnostics, not a committed regression or the supplied runtime transcript. Suggested coverage: late checks and no-checks-specific restart/rewatch.
  • Reviewer B — Claude Agent / claude-opus-5-5 — VERDICT: MERGE. No blocking defect. Independently traced full seven-file diff; no tests run. Optional nullable contract field preserves older records/servers; rewatch copies both timestamp and cleared null state. Legacy seen heads intentionally stay silent until the next push. Other providers do not enter the new GitHub-only branch.
  • Timing/product decision: 15 minutes is an eligibility floor from the first empty head read, not a delivery guarantee. Unchanged GitHub fingerprints normally defer detail rereads to about 30 minutes. Both reviewers consider this conservative informational behavior mergeable; the notice reports actual elapsed minutes. The runtime test exercises the null-fingerprint/10-minute reread path, rather than uninterrupted GitHub fingerprint timing.
  • Late checks: Code inspection indicates a second real success/failure update: no-checks clears only the clock, leaves passed false, and keeps watching. The existing check-evaluation path remains active. The notice says “No checks have reported” and uses updated, never claims CI passed.
  • All-green case: This PR does not fix a separate report of checks existing and all turning green without waking. The passing-check decision and wake-dispatch path are unchanged; normal pending-to-green still produces checks-passed. That report requires a separate reproduction.
  • Test quality: New runtime coverage uses TestClock.adjust; pure evaluator tests receive explicit time. No wall-clock waits. Existing GitCafe head status reports 35 successful checks; the author's 105-test result was not independently rerun here.

Proof audit — runtime transcript and evidence report fetched with curl. Line references below refer to the original pretty-printed JSON transcript.

Required rowEvidenceStatus
Zero-checks head notifies onceTranscript lines 60–128: persisted clock, cleared clock, one updated notice without pass wording; lines 2–3 record one unique notice. runtimeLayer.test.ts, test “wakes once when no checks report on the head, without saying they passed”.Proven by transcript and test
Checks observed then none → no noticeSame named runtime test, lines 2834–2842: another head observed pending, then empty across an hour of TestClock sweeps; total notice count stays one.Proven by test
Late checks after the noticeNo supplied transcript or committed test takes the already-notified same head through late pending checks to success/failure and asserts the second wake. Reviewer A reports passing in-memory diagnostics and both reviewers traced the correct path, but no reproducible proof artifact was supplied for that sequence.Unproven by transcript or named committed test
Restart keeps stateTranscript lines 133–168 retain null on ended watch and rewatch; lines 183–205 record restart, healthy sweep, restored null clock and notice count one. Proves startup/sweep survival; does not separately instrument a successful post-restart host reread.Proven by transcript within stated limit
Legacy watch recordpullRequestWatch.test.ts, test “does not tell a watch saved before the no-checks clock that its seen head has none”: seen head remains silent, pushed head notifies once.Proven by test
Non-GitHub forge unchangedprovider exclusion test, test “never reports missing checks on a host whose checks it cannot read”: Azure stays silent over an hour; existing Forgejo boundary test remains, and runtime logic is explicitly GitHub-only.Proven by test

SHA provenance: Transcript lines 170–171 and 183–184 identify implementation 171fec9, rather than current head. Compared all added/removed diff lines from evidence base ef92441 to that implementation against the current PR's merge-base-to-head diff: the only difference is export const PULL_REQUEST_WATCH_NO_CHECKS_MS becoming private const; no runtime semantic drift in the patch. Transcript's 6074dd4… is the watched scratch PR head, not the implementation SHA. The reload immediately before the captured notice prevents any claim of uninterrupted quiet-fingerprint delivery timing; the red result is supplied evidence, not independently rerun.

Audit VERDICT: INSUFFICIENT — late checks after the notice. This is a proof gap, not a confirmed implementation defect.

Models/harnesses: reviewer A gpt-6.1-sol / Codex; reviewer B claude-opus-5-5 / Claude Agent; proof audit and reconciliation gpt-6.1-sol / Codex. No reviewer fallback required.

Round 2

Added “wakes for checks that complete after the no-checks notice on the same head” in runtimeLayer.test.ts. It passes: the same head emits one no-checks notice after TestClock advances, then the normal checks-passed update when late checks complete, with no duplicate no-checks notice. This closes the audit’s missing late-checks row. All three watch suites pass (106 tests); scoped lint passes.

Test addition: GPT-6.1-Sol / Codex; read-only review: Claude Opus 5.5 / Claude Agent.

Checks

All checks passed

Added 4 commits

  • fix(server): only GitHub watches report a head with no checks171fec9
  • fix(server): a watched pull request with no checks wakes the thread once3ab3b83
  • Merge origin/main into fix/issue-237016503de5
  • test(server): reproduces issue 23701, a watch on a head with no checks never wakesada4c39
MmacroscopeappBot1d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This focused server fix adds a persisted, hard-coded 15-minute policy for notifying on GitHub pull requests with no reported checks. Because that timing changes default product behavior for existing watches, the change warrants human review.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot1d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 1fbb1426134e533d482bd930e865ff3058528f66 and 048e855e9e76bd8b4562cf2be92842187a022995.

📒 Files selected for processing: 1
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

GitHub pull-request watches now track how long the current head has no checks and emit a no-checks change after 15 minutes. The timer is stored in watch state and carried into a new watch when defined. Other providers do not apply the timeout.

Changes: Pull-request no-checks notification

Layer / File(s)Summary
Track and report missing checks
packages/contracts/src/threadPullRequest.ts, apps/server/src/orchestration-v2/pullRequestWatch.ts, apps/server/src/orchestration-v2/pullRequestWatch.test.ts, apps/server/src/orchestration-v2/pullRequestWatch.forgejoComments.test.ts		The watch contract adds an optional nullable checksAwaitedSince timestamp. For GitHub, the evaluator tracks empty checks and emits a no-checks change after 15 minutes. Tests cover elapsed time and provider behavior.
Carry timer through watch lifecycle
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/PullRequestWatchReactor.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts		The orchestrator carries the timer into a new watch when defined. The reactor compares timer state and passes the current time to the evaluator. Runtime tests cover no-check notifications and later check reads.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant PullRequestWatchReactor
3 participant evaluatePullRequestWatch
4 participant WatchState
5 PullRequestWatchReactor->>evaluatePullRequestWatch: Pass provider, pull-request details, remarks, and current time
6 evaluatePullRequestWatch->>WatchState: Read and update checksAwaitedSince
7 evaluatePullRequestWatch-->>PullRequestWatchReactor: Return changes and next watch state

Possibly related issues

  • Issue 23701: The PR adds the delayed informational no-check notice and preserves its state, addressing the reported missing wake when a GitHub head has no checks.
  • Issue 18728: The PR does not address the Azure DevOps case; non-GitHub providers do not apply the no-check timeout.

Suggested reviewers: maria-rcks

Merge Risk: ⚪ Minimal · up to 048e8

The same-head late-check sequence is covered, and no issue requiring a fix before merge is established.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title is concise, uses the repository's conventional commit format, and clearly describes waking server pull-request watches when checks do not report.
Description checkPassedThe description includes all required sections. It explains the problem, implementation, scope and approval, focused verification, limitations, runtime evidence, and agent details. It also clearly ide…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections. It explains the problem, implementation, scope and approval, focused verification, limitations, runtime evidence, and agent details. It also clearly identifies the remaining proof gap for late checks without presenting it as a confirmed defect.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Ggithub-actionsBot1d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB−7 B (−0.1%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB−7 B (−0.6%)2.0 KiB✅
CodexLive turn WebSocket decoded20.9 KiB20.9 KiB0 B (0.0%)29.3 KiB✅
CodexLive turn messages220 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB+24 B (+0.5%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB+24 B (+2.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.2 KiB21.2 KiB+41 B (+0.2%)29.3 KiB✅
ClaudeLive turn messages12+1 (+100.0%)8✅

Baseline: 2c8bdf6 · PR result: 048e855 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Added 1 commit

  • fix(server): keep the no-checks wait constant module-private1fbb142

Added 1 commit

  • test(server): cover checks arriving after no-checks notice048e855