fix(server): PR watches wake when checks finish after an early failure#28020

External image Major · Functional Correctness · Quick win

Keep previously owed checks based on their own pending status.

When a required check appears after a failure, gate contains only required checks. The current running filter therefore removes previously owed non-required checks that are still pending, clears owedChecks, and can emit checks-finished too early.

Use the gate only for a newly reported failure. Retain existing owed names by checking all current checks:

✅ Confirmed as addressed by @Rhinomcd

[P3] Finding: Owed checks were pruned against the gate's running checks, so an owed advisory check still running was dropped when the gate changed. Cause: running came from gate; once a check turns required, gate holds only the required checks and excludes the owed advisory ones. Impact: sonar fails while e2e runs (none required), so e2e is owed; sonar then turns required, running is empty, and checks-finished fires while e2e is still running. Model: coderabbit · confirmed by ran test: the new assertion (no wake, owedChecks stays ["e2e"]) fails on the previous head and passes now; orchestration suite 1983/1983 Action: pushed e29e94b. A new gate failure still owes only the gate's running checks, and existing owed checks are pruned against every running check, as suggested.

@Rhinomcd The supplied code addresses this finding. It retains existing owedChecks against all pending checks in detail.checks. Only a new gate failure resets owedChecks to pending gate checks.

Your reported regression test covers the gate-change case. I have not independently run the tests.

📝 Learnings were not changed: only organization members or users with write access to this repository can add or remove learnings.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Review thread resolved.

_{You are interacting with an AI system.}

[P3] Finding: A new advisory failure re-armed the owed set with every pending gate check, including gate checks that weren't owed. Cause: newlyFailed includes advisory checks, so any new failure replaced owedChecks instead of only a new gate failure. Impact: Required A fails while B is owed, required C starts, then advisory X fails: owed becomes [B, C], and the wake for B waits on C. Model: coderabbit · confirmed by ran test: the new assertion (owedChecks stays ["unit"]) fails on the previous code and passes with the fix; orchestration suite 1983/1983 Action: pushed 006682a

@Rhinomcd The supplied code now uses gateNewlyFailed to replace owedChecks only after a new gate failure. Advisory failures preserve and prune owedBefore. This addresses the finding.

Thanks for adding the regression assertion and reporting the suite results.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Preserve owed checks during a pending rerun.

When passedNow is false, the evaluator clears owedChecks. If an owed check remains pending or succeeds before the rerun fails again, the later failure wake no longer knows that the completion wake is owed. Retain each owed check until the gate resolves that obligation, but remove it when that check itself fails.

[P3] Finding: A pending rerun of the failed gate check clears owedChecks. Cause: with no failing gate check, the evaluator takes the existing owed path, which is empty unless the gate passed. Impact: none found. Clearing the set loses no wake, because the rerun always produces one: if the gate passes, checks-passed; if it fails again, checks-failed, which re-arms with whatever is still running. If the owed checks finished during the rerun, that failure wake is already the last news. Keeping owed checks through a rerun would only add a second wake about checks the agent can already see as finished. Model: coderabbit · disproved by tracing evaluatePullRequestWatch (lines 61-105): every rerun outcome emits a wake Action: comment-only (no lost wake; would add a redundant wake)

@Rhinomcd You are correct for the pending-rerun sequence. I withdraw this finding.

In the supplied evaluatePullRequestWatch code:

• When the failed gate check becomes pending, failedChecks no longer contains its name, and passed becomes false.
• If the gate later passes, the evaluator emits checks-passed.
• If that check fails again, the evaluator emits checks-failed and records the gate checks still pending in owedChecks.

If the previously owed checks have already finished, the rerun outcome supplies the wake. My finding did not account for that wake. No code change is needed for this finding.

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Guard completion on newly failed owed checks.

newlyFailed.length === 0 suppresses checks-finished for an unrelated advisory failure, even when the final owed check finishes. A guard using only !gateNewlyFailed would emit both wakes when the owed check itself fails after becoming advisory. Check for overlap with owedBefore instead.

✅ Confirmed as addressed by @Rhinomcd

[P3] Finding: An unrelated new failure stopped the checks-finished wake when the last owed check finished in the same read. Cause: the guard was newlyFailed.length === 0 instead of "no owed check newly failed". Impact: e2e (owed) succeeds in the same read where advisory bot fails. The agent gets checks-failed for bot but isn't told the owed checks are done. Model: coderabbit · confirmed by ran test: with your guard the read gives [checks-failed bot, checks-finished [sonar, bot]]; with the old guard only checks-failed Action: pushed 202d97b

@Rhinomcd The supplied code addresses this finding. owedFailed checks for newly failed checks in owedBefore. An unrelated failure no longer suppresses checks-finished when the last owed check finishes. A newly failed owed check still suppresses the duplicate completion wake.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Preserve existing owed checks when a new gate failure appears.

If an owed check becomes advisory while still pending, a different newly failed required check replaces owedChecks with only pending required checks. The advisory check is then forgotten, so its completion cannot emit the required checks-finished wake.

The different failure does not satisfy the original obligation. The evaluator treats it as a separate checks-failed wake and preserves unrelated owed checks.

✅ Confirmed as addressed by @Rhinomcd

[P3] Finding: A new gate failure replaced the owed set, dropping an owed check that was still running outside the gate. Cause: the gateNewlyFailed branch assigned only the gate's running checks to owedChecks. Impact: sonar fails while e2e runs, so e2e is owed; sonar turns required; required lint then fails while e2e runs. e2e is dropped, and its finishing sends no wake. Model: coderabbit · confirmed by ran test: owedChecks stays ["e2e"] after the lint failure; it fails on the previous head with expected [] to deeply equal [ 'e2e' ] Action: pushed 202d97b. Still-owed checks are kept, and the gate's running checks are added (your union). This also covers the duplicate on lines 96-97.

@Rhinomcd The supplied code addresses this finding. The union retains still-running checks from owedBefore, adds pending gate checks, and removes duplicates. A new gate failure no longer drops the owed e2e check.

Your reported regression test covers this sequence. I did not independently run it.

I couldn't check this conversation for new learnings, so no new learning was saved.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Maintainability & Code Quality · Quick win

Describe both owedChecks states.

The evaluator also persists checks that were pending when a gate check failed. Update the field comment so it documents both checks pending after required checks pass and checks pending after a gate failure.

[P3] Finding: The owedChecks field comment describes the early-success case and omits the gate-failure case. Cause: Maria explicitly reverted that field-comment edit in the takeover update to keep packages/contracts out of this PR. Impact: The field comment is incomplete; the evaluator's gate-failure branch explains the added condition and its tests cover both cases. The persisted schema is unchanged. Model: gpt-6.1-sol · confirmed by reading the current field comment and evaluator branch, and comparing the exact source at f2544f8 and 9a48b34. Action: comment-only (design decision: preserve the takeover's contract scope; the new condition is documented beside its implementation).

Review summary at 9a48b34: no fixes pushed and no open P1/P2 findings. Checked clean: the watch implementation and tests are byte-identical to the council-reviewed version, that version is an ancestor of this head, and the contract delta only restores the existing comment. Current required checks and the CodeRabbit review pass. This resolves the optional documentation request without changing the branch.

@Rhinomcd Understood. The comment beside the gate-failure branch explains the added owedChecks behavior. I withdraw the request to change the contract field comment. No change to packages/contracts is needed for this finding.

Review thread resolved.

_{You are interacting with an AI system.}