Repository navigation
Merge plan PRs when GitHub auto-merge is off or there is no CI - #3280
Merged
Merged
Conversation
…e is no CI
Epic PR integry/agent-tank-website#43 reached its Ultrafix goal but never
merged. GitHub refused to arm auto-merge ("Auto merge is not allowed for this
repository"), the failure was only logged, and the direct-merge fallback never
ran: it is driven by check events, the PR targets an epic branch the repo's CI
does not run on, and a commit with no CI signal was never treated as mergeable.
- enableAutoMerge reports `unavailable` when GitHub cannot arm auto-merge at all
(repository setting off, or the PR is already in clean status).
- After Ultrafix reaches its goal, an unavailable arm falls back to the direct
merge path with all its gates, ignoring only the finishing job's own work.
- When the repository setting is off, ProPR comments on the PR suggesting
"Allow auto-merge" (Ultrafix finish and first PR arming).
- The direct merge accepts CI that has finished passing, or no CI at all for a
head that is older than a 2-minute grace, so unregistered checks are not
mistaken for missing CI. Closed PRs are skipped.
- The daemon sweeps open plan PRs every minute through the same gates, since a
repository without CI, or CI that finished before Ultrafix did, produces no
further event that would merge the PR.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
CI failed: Full Test Suite Shard 1/4 Please investigate and fix this CI failure.
Failure evidence
|
Contributor
|
CI failed: Run Full Test Suite Please investigate and fix this CI failure.
Failure evidence
|
Full Test Suite ResultsValidation failed during: Test shard (shard 1).
View shard 1/4 outputDetails truncated; see the uploaded artifacts for complete failure rows and logs. |
…e mock Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Owner
Author
|
/review |
Contributor
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR addresses stalled auto-merges by adding an Ultrafix fallback and periodic reevaluation. It needs changes before merge: the new no-CI path can mistake incomplete API results for absent CI, and its grace period does not reliably cover newly pushed heads. ✅ Shared merge gates — Both new entry points reuse label, Ultrafix, mergeability, active-work, and repository-policy checks. ✅ Sweep failure isolation — A failed candidate does not prevent subsequent PRs from being evaluated, and overlapping ticks are suppressed. ✅ Current checks pass — The supplied authoritative status reports 20 passed checks and no failed or pending checks; the historical shard failure is not current. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Preserve CI fetch failures before declaring no CI
F2: 🔴 Measure CI grace from head observation
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise sweep candidate selectionThe sweep test injects candidates, leaving the actual database query untested. A focused test covering duplicate PR links, terminal statuses, and the 14-day boundary would improve confidence in candidate selection. This is optional coverage; no query defect is established by the supplied code. ScoreThe shared-gate approach and passing current checks are strong, but two defects in the new no-CI decision can permit premature merges. Both can be corrected locally. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
Owner
Author
|
/ultrafix |
Contributor
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
Contributor
|
✅ Applied the requested follow-up changes in commit e4b7a8c
Summary of ChangesI've fixed F1 and F2. Both are in F1: a failed CI request no longer counts as "no CI"
F2: the 2-minute wait starts when ProPR first sees the head
Tests (
Side effects to know about:
There's nothing visual in this change, so I didn't capture any previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ackages/core/src/webhook/checkRunHelpers.ts`), plus its one caller in `checkRunHandler.ts`.
I've fixed F1 and F2. Both are in `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts`), plus its one caller in `checkRunHandler.ts`.
**F1: a failed CI request no longer counts as "no CI"**
- The shared CI fetcher now also reports whether both requests (check runs and legacy statuses) succeeded.
- If no CI shows up and either request failed, the answer is now `unavailable`, which blocks the merge. It no longer returns `no_ci`, and the 2-minute wait doesn't start.
- The other callers (`getCheckRunsStatus`, `areAllChecksPassing`) ignore the new flag, so their behaviour is unchanged.
**F2: the 2-minute wait starts when ProPR first sees the head**
- The commit-date lookup is gone. A new helper, `observePRHead`, saves the head SHA and the time ProPR first saw it with no CI. It is stored in Redis per PR (`auto-merge:head-observed:<owner>:<repo>:<pr>`, expiring after 30 days).
- A later sweep run, a webhook, or another process reuses the same saved time, so it carries over between sweep runs.
- A different head replaces the saved entry and restarts the wait. Returning to an earlier head also starts over rather than reusing the old time.
- If two processes write at once, the saved time can only move later, so the wait never gets shorter. A Redis failure blocks the merge as `unavailable`.
- `getCiMergeReadiness` now needs the PR number: the call is now `getCiMergeReadiness(owner, repo, ref, { prNumber })`. The Ultrafix fallback and the sweep both go through this one call site.
**Tests** (`test/checkRunHandler.test.ts`)
- One test each for the check-runs request failing and the status request failing while the other comes back empty. Both get `unavailable` and don't start the wait. With the F1 fix removed, both fail.
- An old commit seen as the head for the first time still waits, and no commit lookup is made.
- The saved time carries over between runs: still waiting at 2 minutes minus 1 ms, mergeable at 2 minutes.
- A different head restarts the wait, and so does returning to an earlier one.
- A broken Redis blocks the merge.
`checkRunHandler`, the auto-merge fallback, the sweep and the related Ultrafix suites all pass (221 tests). Core lint with zero warnings, core `tsc`, and the root `tsc` all pass.
**Side effects to know about:**
- If the GitHub App can't read legacy commit statuses on a repo (the "Resource not accessible by integration" response), a repo with no CI now never merges through this path; it stays `unavailable`. That follows the rule that unreadable CI isn't evidence of no CI, but it is stricter than before.
- The first sighting is only recorded once the earlier gates (labels, Ultrafix state, head match) pass. So the wait can start later than the actual push. That only makes it more cautious.
There's nothing visual in this change, so I didn't capture any previews.
PR: #3280
Comment by: @integry (ID: 6287455285)
Model: Claude Opus 5.5
Contributor
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
Contributor
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR addresses stalled auto-merges with an Ultrafix fallback and periodic reconciliation. It needs localized corrections before merge. ✅ Shared merge gates — Both new entry points reuse the existing label, Ultrafix, mergeability, active-work, and repository-policy checks. ✅ Conservative empty-CI handling — Empty results require both endpoints to succeed, and observation-store failures block merging. ✅ Current checks pass — The supplied authoritative status reports 20 passed, no failures, and no pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F3: 🔴 Preserve the resolved merge method
F4: 🔴 Reset observations across heads with CI
SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Cover partial failures with signalsThe new readiness helper checks ScoreThe implementation is focused and current checks pass, but two reachable gaps remain in the new fallback and grace-period behavior. This review used static tracing only, as requested. 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
Contributor
|
✅ Applied the requested follow-up changes in commit c1da9a4
Summary of ChangesI fixed F3 and F4. The fixes are in the shared direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all get them. F3: merge method. Direct merges now use the same repository-supported method that arming uses.
F4: stale no-CI observation. Seeing CI on any head now ends the earlier no-CI grace period.
New tests
I reverted each fix temporarily and the matching new test failed, so they catch the original bugs. Verification
I found nothing else to report outside F3 and F4, and the change has no visible UI, so there are no previews. 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
… path, so the check-event merge, the Ultrafix fallback and the daemon sweep all get them. I fixed F3 and F4. The fixes are in the shared direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all get them. **F3: merge method.** Direct merges now use the same repository-supported method that arming uses. - `packages/core/src/services/autoMergeGate.ts` has a new export, `resolveDirectMergeMethod`. It returns the policy's method if one is set. Otherwise it uses the same lookup as `gateAutoMergeArming`: squash, unless the repository disallows squash, in which case merge commit, then rebase. - `processPRAutoMerge` in `packages/core/src/webhook/checkRunHandler.ts` calls it before `performMergeAndPostActions`, so a policy without a method no longer falls back to `squash`. - `processPRAutoMerge` is the only place that calls `performMergeAndPostActions`, so no other merge path has the same bug. **F4: stale no-CI observation.** Seeing CI on any head now ends the earlier no-CI grace period. - In `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts`), when the evaluated head has check runs or statuses, the PR's stored head observation is deleted. - So A without CI → B with CI → back to A records A again and waits a fresh 2 minutes. - Two concurrent sweeps can only make the observation later, never earlier, so the grace period can't be cut short. - One behaviour to know: if Redis is unavailable, that delete fails and the PR is reported `unavailable`, so passing checks don't merge either. That matches how an unreadable observation store already blocks merging. **New tests** - In `test/checkRunHandler.test.ts`: - The A (no CI) → B (pending CI) → A (no CI) case. - A check-event merge where the policy gives no method: the resolver's `merge` is what reaches GitHub's merge request. - In `packages/core/test/autoMergeGate.test.ts`: `resolveDirectMergeMethod` returns merge, rebase or squash depending on repository settings, and a configured method takes precedence. I reverted each fix temporarily and the matching new test failed, so they catch the original bugs. **Verification** - `checkRunHandler`: 120 pass. `autoMergeGate`: 30 pass. - `ultrafixAutoMergeFallback`, `autoMergeSweep`, `ultrafixCiFailureRecovery`, `ultrafixContinuationEntryPoint`, `ultrafixRecoveryIntake`, `taskIntentWithdrawal` and `e2e/webhook` all pass. - `tsc` on `packages/core` is clean. ESLint shows no errors or warnings on the changed source files; the test files are outside its config and were skipped, not linted. I found nothing else to report outside F3 and F4, and the change has no visible UI, so there are no previews. PR: #3280 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
Contributor
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
Contributor
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR addresses stalled auto-merges with a direct-merge fallback and periodic reevaluation. It needs a localized correctness fix before merge. ✅ Shared merge gates — The fallback and sweep reuse ✅ Observation-based grace period — No-CI readiness uses ProPR’s observation time rather than commit timestamps, with tests for head changes and Redis failures. The authoritative current checks report 20 passed, none failed or pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F5: 🔴 Block merges on incomplete CI reads
SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Test sweep candidate selectionThe sweep test injects candidates, so it does not exercise the database query’s status exclusions, deduplication, or fourteen-day boundary. A focused query test would improve confidence in those selection rules. This is optional coverage; no defect in that query is demonstrated. ScoreThe implementation is focused and current checks pass, but incomplete CI reads can incorrectly permit the new merge paths. The required correction is localized. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
Contributor
|
✅ Applied the requested follow-up changes in commit e01f770
Summary of ChangesI fixed F5: a merge is now blocked whenever either CI endpoint fails to read, even if the other one shows passing checks. What was wrong: in The fix: Tests: I added two regressions to the
All 122 tests in Separate finding, not changed: two older functions, This change has nothing visible, so I made no preview. 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…int fails to read, even if the other one shows passing checks.
I fixed F5: a merge is now blocked whenever either CI endpoint fails to read, even if the other one shows passing checks.
**What was wrong:** in `getCiMergeReadiness` (`packages/core/src/webhook/checkRunHelpers.ts:527`), the check for an incomplete read only ran after the "signals present" branch had already returned. So passing check runs with a failed status request (or the reverse) came back as `checks_passing`.
**The fix:** `if (!complete) return { ready: false, reason: 'unavailable' }` now runs straight after the fetch, before both the passing-CI and no-CI checks. I removed the old copy further down. This function is the only CI gate on the direct-merge path, so the check-event merge, the Ultrafix fallback and the daemon sweep all pick up the fix.
**Tests:** I added two regressions to the `getCiMergeReadiness` block in `test/checkRunHandler.test.ts`:
- passing check runs while the status request fails → `unavailable`
- passing statuses while the check-runs request fails → `unavailable`
All 122 tests in `test/checkRunHandler.test.ts` pass. With the old source put back, the two new tests fail and nothing else does. The core package's ESLint (zero warnings allowed) and `tsc --noEmit` are clean. To run the suite I had to install dependencies with `npm ci` and build `packages/shared`; neither changes any repo files.
**Separate finding, not changed:** two older functions, `areAllChecksPassing` and `getCheckRunsStatus` in the same file, also ignore `complete`. A failed endpoint there can still produce a passing result for the Ultrafix loop and the polling readiness checks. They behaved the same way before this PR and are outside its merge path, so I left them alone. They would need their own finding if you want them fixed.
This change has nothing visible, so I made no preview.
PR: #3280
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Opus 5.5
Contributor
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
Contributor
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR addresses stalled auto-merges by adding a gated direct-merge fallback and periodic reevaluation. The supplied code shows no verified merge blocker. It is merge-ready within scope, conditional on the four pending test shards completing successfully. ✅ Shared merge gates — Both the Ultrafix fallback and daemon sweep use ✅ Conservative CI readiness — Either CI endpoint failing blocks merging. Heads without CI must complete an observation-based grace period, with tests covering head changes and unavailable storage. ✅ Consistent merge method — Direct merges now resolve the repository-supported default using the same logic as auto-merge arming. This assessment is a static review of the supplied diff and context; no commands or tests were run. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Exercise sweep selection and scheduling
S5: 🟢 Test active-work exclusion end to endThe fallback test verifies that the finishing job ID reaches ScoreThe implementation addresses the stated failure paths with conservative gates and targeted regression coverage. The supplied current-head status has no failures, with four test shards still pending. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Epic PR integry/agent-tank-website#43 reached its Ultrafix goal (8/10, no findings) with auto-merge selected in the plan, but never merged and the epic stalled:
Auto merge is not allowed for this repository. ProPR only logged a warning.main, so this epic sub-PR had no checks and no event ever fired.The same arming failure hit integry/propr-fleet#153.
Fix
enableAutoMergereportsunavailablewhen GitHub cannot arm auto-merge at all: the repo setting is off, or the PR is already incleanstatus.reevaluatePRAutoMerge) with all its gates (labels, Ultrafix state, CI, mergeability, repository policy, active work). Only the finishing job's own task/job is ignored in the active-work check.getCiMergeReadiness): merges when blocking checks have finished passing, or when there is no CI at all for a head older than 2 minutes. The grace period keeps checks that haven't registered yet from being mistaken for missing CI. Pending or failing checks and API errors still block. Closed PRs are skipped.Tests
test/ultrafixAutoMergeFallback.test.ts,test/autoMergeSweep.test.ts,getCiMergeReadinesscases intest/checkRunHandler.test.ts, and theunavailablecase intest/autoMergeOperations.test.ts.test/intentWithdrawalWebhook.test.tsfails 4 tests onmainas well; that is unrelated.🤖 Generated with Claude Code