From 6cda5b45b3547ec44adedcfcea8bd9387ec99ead Mon Sep 17 00:00:00 2001 From: Adam Frisby Date: Sun, 8 Nov 2026 07:19:39 +0000 Subject: [PATCH] Fix auto-merge race against GitHub async mergeability computation After pushing, poll GET /pulls/{n} until head.sha equals the pushed sha and mergeable is non-null (bounded 60s settle, hot-reloadable knobs), merging only when mergeable. A settled false with an unchanged base is a genuine conflict routed to ReworkingForConflict outside the race-recovery cap; an unknown mergeable retries the merge call without re-running the merger. Failure text now carries the actual mergeable/mergeable_state values and never blames another writer when the base did not move. CodeyBox-Prompt-Revision: 1 Co-Authored-By: CodeyBox Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/CodeyBox.Api/AgentConfigHotReload.cs | 2 + src/CodeyBox.Core/IUpstreamRemote.cs | 45 ++- .../PipelineRunner.UpstreamPush.cs | 381 +++++++++++++++--- .../PipelineTuningHotReloadPolicy.cs | 2 + .../PipelineTuningOptions.cs | 33 ++ .../GitHubUpstreamRemote.cs | 180 ++++++++- .../AutoMergeRaceSettleTests.cs | 200 +++++++++ .../ConfigReloadClassificationTests.cs | 6 + .../CodeyBox.Tests/GitHubMergeSettleTests.cs | 197 +++++++++ .../UpstreamAutoMergeRaceRecoveryTests.cs | 39 +- 10 files changed, 1004 insertions(+), 81 deletions(-) create mode 100644 tests/CodeyBox.Tests/AutoMergeRaceSettleTests.cs create mode 100644 tests/CodeyBox.Tests/GitHubMergeSettleTests.cs diff --git a/src/CodeyBox.Api/AgentConfigHotReload.cs b/src/CodeyBox.Api/AgentConfigHotReload.cs index db6912bd0..f98225aba 100644 --- a/src/CodeyBox.Api/AgentConfigHotReload.cs +++ b/src/CodeyBox.Api/AgentConfigHotReload.cs @@ -1648,6 +1648,8 @@ internal static string SerializePipelineTuning(PipelineTuningOptions opts) => opts.AgentSessionResumeMaxAttempts, opts.MaxRetainedAgentTurnSandboxes, opts.AutoMergeRaceRecoveryMaxAttempts, + AutoMergeSettleTimeoutSeconds = opts.AutoMergeSettleTimeout.TotalSeconds, + AutoMergeSettlePollIntervalSeconds = opts.AutoMergeSettlePollInterval.TotalSeconds, opts.MergeLandingMaxAttempts, opts.MergeGuardReworkMaxAttempts, opts.MergeResultBuildVerificationEnabled, diff --git a/src/CodeyBox.Core/IUpstreamRemote.cs b/src/CodeyBox.Core/IUpstreamRemote.cs index 0c7e97746..56ded1c35 100644 --- a/src/CodeyBox.Core/IUpstreamRemote.cs +++ b/src/CodeyBox.Core/IUpstreamRemote.cs @@ -303,11 +303,20 @@ Task PublishAuditCheckAsync( $"Upstream kind '{Name}' does not publish audit check runs.")); } +/// +/// Forge-reported state of one pull request. , +/// and are null when +/// the upstream kind does not report them (or the forge has not computed +/// mergeability yet) — callers must treat null as unknown, never as clean. +/// public sealed record UpstreamPullRequestState( int Number, string Url, PullRequestStatus Status, - string? MergeCommitSha); + string? MergeCommitSha, + string? HeadSha = null, + bool? Mergeable = null, + string? MergeableState = null); /// /// One commit in a branch's exclusive range (<base>..<head>), @@ -753,6 +762,24 @@ public sealed record UpstreamCompletionRequest /// Merge strategy: "merge", "squash", or "rebase". Matches Upstream.MergeMethod. public string MergeMethod { get; init; } = "merge"; + /// + /// Post-push mergeability settle bound for forges that compute + /// mergeability asynchronously after a push (GitHub: mergeable + /// stays null until the computation finishes, and the PR head lags the + /// just-pushed tip). When set, the implementation polls the PR until + /// its head equals the pushed tip and mergeability is known, then + /// merges only when mergeable. Null (legacy callers) skips the settle + /// and merges immediately. + /// + public TimeSpan? MergeSettleTimeout { get; init; } + + /// + /// Initial interval between mergeability settle polls; the delay backs + /// off exponentially within . Ignored + /// when is null. + /// + public TimeSpan? MergeSettlePollInterval { get; init; } + /// /// PR number already opened on the forge from a prior CompleteAsync attempt. /// When set, the implementation skips PR creation and proceeds directly to @@ -813,6 +840,22 @@ public sealed record UpstreamCompletionOutcome /// public bool AutoMergeRaced { get; init; } + /// + /// Forge-reported mergeability observed around the merge attempt + /// (GitHub mergeable). Null when the forge had not computed it + /// yet or the upstream kind does not report it. Lets the orchestrator + /// tell a genuine conflict (false with an unchanged base — re-merging + /// cannot help) from a still-computing PR (null — retry the merge call + /// without re-running the merger) without guessing from the base sha. + /// + public bool? Mergeable { get; init; } + + /// + /// Forge-reported mergeability detail (GitHub mergeable_state, + /// e.g. "clean", "dirty", "blocked", "unknown"). Diagnostic only. + /// + public string? MergeableState { get; init; } + /// /// Tip sha of the work branch the implementation actually pushed (plain /// or lease-guarded). The orchestrator persists it as the work item's diff --git a/src/CodeyBox.Orchestrator/PipelineRunner.UpstreamPush.cs b/src/CodeyBox.Orchestrator/PipelineRunner.UpstreamPush.cs index b2e4376b5..0239e9831 100644 --- a/src/CodeyBox.Orchestrator/PipelineRunner.UpstreamPush.cs +++ b/src/CodeyBox.Orchestrator/PipelineRunner.UpstreamPush.cs @@ -151,6 +151,8 @@ private async Task RunUpstreamPushPhaseAsync( TokenEnvVar = project.Upstream.TokenEnvVar, AutoMerge = project.Upstream.AutoMerge, MergeMethod = project.Upstream.MergeMethod, + MergeSettleTimeout = _pipelineTuning.Current.AutoMergeSettleTimeout, + MergeSettlePollInterval = _pipelineTuning.Current.AutoMergeSettlePollInterval, }; // Restart evidence: a previous attempt may have opened (or raced) @@ -273,6 +275,11 @@ _preMergeVerifier is not null && // from UpstreamPushMaxAttempts, which caps total upstream API // calls including transient infrastructure retries. var raceRecoveryCount = 0; + // Last classification of an AutoMergeRaced outcome (mergeability + // verdict + whether the base moved). The post-loop park uses it + // to describe an unchanged base with the actual mergeable values + // instead of blaming another writer. + AutoMergeRaceAssessment? lastRaceAssessment = null; // Each iteration may either retry transient failures OR recover from // an auto-merge race (405 on PUT /pulls/N/merge). The shared cap // prevents pathological loops since each race-recovery iteration @@ -340,8 +347,143 @@ _preMergeVerifier is not null && outcome.BranchPushed && !outcome.Skipped, request, ct); lastIterationRaced = outcome.AutoMergeRaced; - if (outcome.AutoMergeRaced) + if (outcome.AutoMergeRaced && reRunMergePhase is not null) { + // GitHub said the PR is unmergeable. Classify before + // spending an LLM merge-phase re-invocation, using the + // forge's own mergeability verdict plus a fresh base + // comparison — not the 405 alone: + // - base unchanged + mergeable=false: genuine + // conflict; re-merging cannot help → conflict + // handling, outside the race-recovery cap. + // - base unchanged + mergeable unknown/clean: the + // forge had not finished computing (or the PUT + // flaked) → retry the merge call with the same + // tree, no re-merge, outside the cap. + // - base moved: real race → full recovery (re-run + // the merge phase), counted against the cap. + var assessment = await AssessAutoMergeRaceAsync( + item, upstream, repoId, baseBranch, + outcome.PullRequestNumber, + outcome, + attempt, + ct); + lastRaceAssessment = assessment; + if (assessment.ParkReason is not null) + { + _log.LogWarning( + "Auto-merge race recovery declined to retry (attempt {Attempt}): {Reason}", + attempt, assessment.ParkReason); + // Stale-base remediation: before parking, try to route + // the item into conflict-rework (rebase onto the + // refreshed base + agentic resolution). CapExhausted / + // NotEnabled fall through to the existing + // MergeConflictResolutionFailed park below. + if (await TryRouteStaleBasePushConflictAsync(item, ct) == StaleBaseReworkOutcome.Routed) + { + raceRecoveryParked = true; + break; + } + var failed = await _store.GetAsync(item.Id, ct) ?? item; + var failedWithReason = failed.With(WorkItemState.MergeConflictResolutionFailed, assessment.ParkReason); + await _store.UpdateAsync(failedWithReason, ct); + var revision = await BuildTerminalRevisionAsync(failedWithReason, ct); + await _webhooks.PublishAsync(new WebhookEvent + { + Event = "work_item.merge_conflict_resolution_failed", + WorkItem = failedWithReason, + Project = project, + PromptRevision = revision?.PromptRevision, + RevisionAtCompletion = revision?.RevisionAtCompletion, + RevisionMatches = revision?.RevisionMatches, + }, ct); + raceRecoveryParked = true; + break; + } + + if (!assessment.BaseMoved && assessment.Mergeable == false) + { + // The forge settled the PR as conflicting against + // an unchanged base: re-running the merger against + // the same base cannot produce a mergeable tree. + // Route to conflict handling without consuming the + // race-recovery budget, and never describe this as + // another writer mutating the base — the base did + // not move. The actual mergeable verdict travels in + // the message instead. + var conflictReason = + $"GitHub reports PR #{outcome.PullRequestNumber} unmergeable " + + $"(mergeable=false, mergeable_state='{SanitizeMergeableState(assessment.MergeableState)}') " + + $"with base branch '{baseBranch}' unchanged at {assessment.PostFetchBaseSha ?? "(unknown)"}; " + + "re-running the merge phase cannot resolve a settled conflict. Rework the branch onto the current base or resolve manually."; + _log.LogWarning( + "Auto-merge settled unmergeable (attempt {Attempt}): PR #{Pr} mergeable=false, mergeable_state='{State}', base '{Branch}' unchanged at {Sha}; routing to conflict handling without consuming the race-recovery budget", + attempt, outcome.PullRequestNumber, SanitizeMergeableState(assessment.MergeableState), + baseBranch, assessment.PostFetchBaseSha ?? "(unknown)"); + // Hand off to the conflict-rework machine (bounded by + // its own attempt cap) before falling back to the park. + if (await TryRouteStaleBasePushConflictAsync(item, ct) == StaleBaseReworkOutcome.Routed) + { + raceRecoveryParked = true; + break; + } + var failed = await _store.GetAsync(item.Id, ct) ?? item; + var failedWithReason = failed.With(WorkItemState.MergeConflictResolutionFailed, conflictReason); + await _store.UpdateAsync(failedWithReason, ct); + var revision = await BuildTerminalRevisionAsync(failedWithReason, ct); + await _webhooks.PublishAsync(new WebhookEvent + { + Event = "work_item.merge_conflict_resolution_failed", + WorkItem = failedWithReason, + Project = project, + PromptRevision = revision?.PromptRevision, + RevisionAtCompletion = revision?.RevisionAtCompletion, + RevisionMatches = revision?.RevisionMatches, + }, ct); + raceRecoveryParked = true; + break; + } + + if (!assessment.BaseMoved) + { + // Base unchanged and the PR is not settled + // unmergeable: the 405 raced GitHub's asynchronous + // mergeability computation (or the PUT itself + // flaked). The tree is fine — retry the merge call + // with the same merge sha, without re-running the + // merge phase and without consuming the + // race-recovery budget. The loop's own + // UpstreamPushMaxAttempts still bounds retries. + _log.LogInformation( + "Auto-merge race check (attempt {Attempt}): upstream base '{Branch}' unchanged at {Sha} and PR #{Pr} mergeability is {Mergeable} ({State}); retrying the merge call with the same tree (race-recovery budget untouched)", + attempt, baseBranch, assessment.PostFetchBaseSha ?? "(unknown)", + outcome.PullRequestNumber, FormatMergeable(assessment.Mergeable), + SanitizeMergeableState(assessment.MergeableState)); + // Persist the PR identity before looping: if the process + // restarts between create/merge/persist, the next run + // reseeds ExistingPullRequestNumber from the store + // instead of creating a duplicate PR. + if (outcome.PullRequestNumber is not null || outcome.PullRequestUrl is not null) + { + var racedPersist = await _store.GetAsync(item.Id, ct) ?? item; + await _store.UpdateAsync(WorkItemPushHistory.RecordPush( + racedPersist with + { + MergedPrNumber = outcome.PullRequestNumber ?? racedPersist.MergedPrNumber, + MergedPrUrl = outcome.PullRequestUrl ?? racedPersist.MergedPrUrl, + }, + outcome.PushedWorkBranchSha, + PushHistoryBound), ct); + } + request = RefreshMergeSettle(request with + { + ExistingPullRequestNumber = outcome.PullRequestNumber, + }); + continue; + } + + // Genuine race: the base moved under us. Re-run the + // merge phase against the fresh tip. // Recorded for operator visibility: a raced landing // re-queues through race recovery below without // consuming the resolver-guard rework budget. @@ -350,20 +492,9 @@ await RecordMergeRetryAsync( $"upstream landing raced on base branch '{baseBranch}'; re-running merge against the fresh base", bumpLandingAttempt: true, ct); - } - if (outcome.AutoMergeRaced && reRunMergePhase is not null) - { - // GitHub said the PR is unmergeable. Two plausible causes: - // 1) Upstream main moved (a race we can fix by re-running - // the LLM merger against the fresh base). - // 2) Branch protection / unrelated unmergeability (re-running - // won't help; base sha will be unchanged). - // Distinguish via the base sha before/after refetch. - var raceRecovery = await TryRecoverFromAutoMergeRaceAsync( - item, project, upstream, repoId, baseBranch, workBranch, - outcome.PullRequestNumber, + var raceRecovery = await ReRunMergeForRaceAsync( + item, repoId, baseBranch, workBranch, reRunMergePhase, - attempt, ct); if (raceRecovery.ParkReason is not null) { @@ -402,9 +533,16 @@ await _webhooks.PublishAsync(new WebhookEvent var maxRaceRecovery = _pipelineTuning.Current.AutoMergeRaceRecoveryMaxAttempts; if (raceRecoveryCount >= maxRaceRecovery) { + // Reachable only when every counted recovery + // observed real base motion (an unchanged base + // routes to conflict handling or a same-tree retry + // without touching this counter), so the + // another-writer diagnosis is evidence-backed here. _log.LogWarning( - "Work item {Id} auto-merge race recovery cap ({Cap}) exhausted after {Count} recoveries; baseBranch likely being mutated by another writer", - item.Id, maxRaceRecovery, raceRecoveryCount); + "Work item {Id} auto-merge race recovery cap ({Cap}) exhausted after {Count} recoveries; baseBranch likely being mutated by another writer (last mergeable={Mergeable}, mergeable_state='{State}')", + item.Id, maxRaceRecovery, raceRecoveryCount, + FormatMergeable(lastRaceAssessment?.Mergeable), + SanitizeMergeableState(lastRaceAssessment?.MergeableState)); // Hand off to the conflict-rework machine (bounded by // its own attempt cap) before falling back to the park. if (await TryRouteStaleBasePushConflictAsync(item, ct) == StaleBaseReworkOutcome.Routed) @@ -433,12 +571,12 @@ await _webhooks.PublishAsync(new WebhookEvent mergeSha = raceRecovery.NewMergeSha; agentStdout = raceRecovery.NewAgentStdout; - request = request with + request = RefreshMergeSettle(request with { MergeSha = mergeSha, AgentStdout = agentStdout, ExistingPullRequestNumber = outcome.PullRequestNumber, - }; + }); // Persist the PR identity before looping: if the process // restarts between create/merge/persist, the next run // reseeds ExistingPullRequestNumber from the store @@ -558,8 +696,24 @@ racedPersist with if (completed is null && lastIterationRaced && reRunMergePhase is not null && !raceRecoveryParked) { var lastAttemptItem = await _store.GetAsync(item.Id, ct) ?? item; - const string raceExhaustionMessage = - "GitHub merge failed repeatedly after re-running LLM merger; baseBranch likely being mutated by another writer. Resolve manually."; + // An unchanged base must never be blamed on another writer: + // report the forge's own mergeability verdict instead. The + // another-writer diagnosis is reserved for the case where the + // base actually moved between attempts. + string raceExhaustionMessage; + if (lastRaceAssessment is { ParkReason: null, BaseMoved: false } settled) + { + raceExhaustionMessage = + $"GitHub merge did not complete after {_opts.UpstreamPushMaxAttempts} attempts with base branch '{baseBranch}' " + + $"unchanged at {settled.PostFetchBaseSha ?? "(unknown)"} " + + $"(last mergeable={FormatMergeable(settled.Mergeable)}, mergeable_state='{SanitizeMergeableState(settled.MergeableState)}'); " + + "the forge never accepted the merge. Resolve manually."; + } + else + { + raceExhaustionMessage = + "GitHub merge failed repeatedly after re-running LLM merger; baseBranch likely being mutated by another writer. Resolve manually."; + } _log.LogWarning( "Work item {Id} hit upstream-push attempt cap ({UpstreamCap}) with {RaceRecoveryCount} auto-merge recoveries (recovery cap {RaceRecoveryCap}) without resolving", item.Id, _opts.UpstreamPushMaxAttempts, raceRecoveryCount, _pipelineTuning.Current.AutoMergeRaceRecoveryMaxAttempts); @@ -690,15 +844,96 @@ private readonly record struct AutoMergeRaceRecovery( string NewMergeSha, string? NewAgentStdout); - private async Task TryRecoverFromAutoMergeRaceAsync( + /// + /// Classification of one AutoMergeRaced outcome, combining the + /// forge's own mergeability verdict with a fresh upstream-base + /// comparison. is non-null when the assessment + /// itself cannot proceed (no PR number, refetch failure, base not + /// advertised); otherwise tells a genuine race + /// (re-run the merge phase) from an unchanged base, where + /// decides conflict handling (false) from + /// a same-tree merge-call retry (unknown or clean). Neither non-race path + /// consumes the race-recovery budget. + /// + private readonly record struct AutoMergeRaceAssessment( + string? ParkReason, + string? PreMergeBaseSha, + string? PostFetchBaseSha, + bool BaseMoved, + bool? Mergeable, + string? MergeableState); + + /// + /// Reads the current mergeability verdict for + /// from the live PR first (authoritative at classification time) and + /// falls back to the values the remote observed around its merge attempt. + /// Unknown when neither reports — never assumed clean. A failing detail + /// read degrades to the outcome snapshot, never to an exception: the + /// classifier must still reach the base comparison. + /// + private async Task<(bool? Mergeable, string? MergeableState)> ReadPullRequestMergeabilityAsync( + IUpstreamRemote upstream, + int prNumber, + UpstreamCompletionOutcome outcome, + CancellationToken ct) + { + try + { + var live = await upstream.GetPullRequestAsync(prNumber, ct); + if (live?.Mergeable is not null || live?.MergeableState is not null) + return (live!.Mergeable, live.MergeableState); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + _log.LogDebug(ex, + "Auto-merge race classification: live PR #{Pr} read failed; falling back to the merge-attempt snapshot", + prNumber); + } + return (outcome.Mergeable, outcome.MergeableState); + } + + /// + /// Re-reads the hot-reloadable settle knobs onto a retried completion + /// request so a same-tree merge-call retry observes configuration edits + /// without rebuilding the request from scratch. + /// + private UpstreamCompletionRequest RefreshMergeSettle(UpstreamCompletionRequest request) + { + var tuning = _pipelineTuning.Current; + return request with + { + MergeSettleTimeout = tuning.AutoMergeSettleTimeout, + MergeSettlePollInterval = tuning.AutoMergeSettlePollInterval, + }; + } + + private static string SanitizeMergeableState(string? state) + { + if (string.IsNullOrWhiteSpace(state)) + return "(unknown)"; + var scrubbed = state.Replace("\r", "\\r", StringComparison.Ordinal).Replace("\n", "\\n", StringComparison.Ordinal); + return scrubbed.Length > 64 ? scrubbed[..64] + "…" : scrubbed; + } + + private static string FormatMergeable(bool? mergeable) => + mergeable switch + { + true => "true", + false => "false", + _ => "unknown", + }; + + private async Task AssessAutoMergeRaceAsync( WorkItem item, - Project project, IUpstreamRemote upstream, string repoId, string baseBranch, - string workBranch, int? existingPrNumber, - Func> reRunMergePhase, + UpstreamCompletionOutcome outcome, int attempt, CancellationToken ct) { @@ -706,12 +941,22 @@ private async Task TryRecoverFromAutoMergeRaceAsync( { // CompleteAsync set AutoMergeRaced=true but produced no PR number; // we have nothing to retry against. Treat as a hard failure. - return new AutoMergeRaceRecovery( + return new AutoMergeRaceAssessment( ParkReason: "auto-merge raced but no PR number returned; cannot retry merge", - NewMergeSha: string.Empty, - NewAgentStdout: null); + PreMergeBaseSha: null, + PostFetchBaseSha: null, + BaseMoved: false, + Mergeable: outcome.Mergeable, + MergeableState: outcome.MergeableState); } + // The forge's own verdict first: a settled `false` against an + // unchanged base is a genuine conflict (re-merging cannot help), and + // `null` means the computation had not finished when the merge call + // raced it (retry the call, not the merger). + var (mergeable, mergeableState) = await ReadPullRequestMergeabilityAsync( + upstream, existingPrNumber.Value, outcome, ct); + // Step 1: capture the upstream base sha at merge time for diagnostic // logging. We can't read the current local base ref because the local // merge phase already advanced it to mergeSha (the merge commit). The @@ -766,49 +1011,67 @@ private async Task TryRecoverFromAutoMergeRaceAsync( catch (Exception ex) when (ex is not OperationCanceledException) { _log.LogWarning(ex, "Auto-merge race recovery: refetch of upstream base failed"); - return new AutoMergeRaceRecovery( + return new AutoMergeRaceAssessment( ParkReason: $"could not refetch upstream base '{baseBranch}': {ex.Message}", - NewMergeSha: string.Empty, - NewAgentStdout: null); + PreMergeBaseSha: preMergeBaseSha, + PostFetchBaseSha: null, + BaseMoved: false, + Mergeable: mergeable, + MergeableState: mergeableState); } if (postFetchBaseSha is null) { - return new AutoMergeRaceRecovery( + return new AutoMergeRaceAssessment( ParkReason: $"upstream does not advertise base branch '{baseBranch}' for race recovery", - NewMergeSha: string.Empty, - NewAgentStdout: null); + PreMergeBaseSha: preMergeBaseSha, + PostFetchBaseSha: null, + BaseMoved: false, + Mergeable: mergeable, + MergeableState: mergeableState); } - // Step 3: the upstream said the PR is unmergeable. Whether or not - // we can detect base motion from the local pre/post sha comparison, - // always re-run the merge phase against the freshly-fetched base. - // The "base didn't move" check (which used to park here) is now a - // diagnostic only — premature escalation on a true race (where the - // local bare-repo base ref was too stale to show the movement) is - // the defect this change eliminates. The merge itself will reveal - // real semantic conflicts, which the in-VM resolver and bounded - // retry cap handle. - if (preMergeBaseSha is not null) + // Step 3: compare. Motion means a genuine race (the caller re-runs + // the merge phase against the fresh tip). An unchanged base pairs + // with the mergeability verdict above: settled `false` is a real + // conflict, anything else is the forge's async computation lagging + // the push — never evidence of another writer. + var baseMoved = preMergeBaseSha is null + || !string.Equals(preMergeBaseSha, postFetchBaseSha, StringComparison.Ordinal); + if (baseMoved) { - if (string.Equals(preMergeBaseSha, postFetchBaseSha, StringComparison.Ordinal)) - { - _log.LogInformation( - "Auto-merge race recovery (attempt {Attempt}): upstream base '{Branch}' sha ({Sha}) unchanged since merge; re-running merge phase anyway in case local base was stale", - attempt, baseBranch, preMergeBaseSha); - } - else - { - _log.LogInformation( - "Auto-merge race detected (attempt {Attempt}): upstream base '{Branch}' moved {Old} → {New}; re-running merge phase", - attempt, baseBranch, preMergeBaseSha, postFetchBaseSha); - } + _log.LogInformation( + "Auto-merge race detected (attempt {Attempt}): upstream base '{Branch}' moved {Old} → {New}; re-running merge phase", + attempt, baseBranch, preMergeBaseSha ?? "(unknown)", postFetchBaseSha); + } + else + { + _log.LogInformation( + "Auto-merge race check (attempt {Attempt}): upstream base '{Branch}' sha ({Sha}) unchanged since merge (mergeable={Mergeable}, mergeable_state='{State}')", + attempt, baseBranch, postFetchBaseSha, FormatMergeable(mergeable), SanitizeMergeableState(mergeableState)); } - // Step 4: re-run the merge phase against the freshly-fetched base. The + return new AutoMergeRaceAssessment( + ParkReason: null, + PreMergeBaseSha: preMergeBaseSha, + PostFetchBaseSha: postFetchBaseSha, + BaseMoved: baseMoved, + Mergeable: mergeable, + MergeableState: mergeableState); + } + + private async Task ReRunMergeForRaceAsync( + WorkItem item, + string repoId, + string baseBranch, + string workBranch, + Func> reRunMergePhase, + CancellationToken ct) + { + // Re-run the merge phase against the freshly-fetched base. The // merge phase reads local baseBranch via ResolveCommitAsync, which now - // reflects the upstream tip after step 2. The LLM merger produces a new - // merge commit M with parents (postFetchBaseSha, workBranch_tip). + // reflects the upstream tip after the assessment refetch. The LLM merger + // produces a new merge commit M with parents (postFetchBaseSha, workBranch_tip). string newMergeSha; string? newAgentStdout; try @@ -825,7 +1088,7 @@ private async Task TryRecoverFromAutoMergeRaceAsync( $"re-run of merge phase against refreshed base '{baseBranch}' (auto-merge race recovery) failed: {ex.Message}", ex); } - // Step 5: advance local workBranch to the new merge commit. The push + // Advance local workBranch to the new merge commit. The push // step inside CompleteAsync will then publish this tip to upstream as // a fast-forward (M has the prior workBranch tip as a parent, so the // upstream workBranch fast-forwards without a force). diff --git a/src/CodeyBox.Orchestrator/PipelineTuningHotReloadPolicy.cs b/src/CodeyBox.Orchestrator/PipelineTuningHotReloadPolicy.cs index 06308c324..a4c07745c 100644 --- a/src/CodeyBox.Orchestrator/PipelineTuningHotReloadPolicy.cs +++ b/src/CodeyBox.Orchestrator/PipelineTuningHotReloadPolicy.cs @@ -41,6 +41,8 @@ public static class PipelineTuningHotReloadPolicy nameof(PipelineTuningOptions.AgentSessionResumeMaxAttempts), nameof(PipelineTuningOptions.MaxRetainedAgentTurnSandboxes), nameof(PipelineTuningOptions.AutoMergeRaceRecoveryMaxAttempts), + nameof(PipelineTuningOptions.AutoMergeSettleTimeout), + nameof(PipelineTuningOptions.AutoMergeSettlePollInterval), nameof(PipelineTuningOptions.MergeLandingMaxAttempts), nameof(PipelineTuningOptions.MergeResultBuildVerificationEnabled), nameof(PipelineTuningOptions.MergeResultBuildVerificationTimeout), diff --git a/src/CodeyBox.Orchestrator/PipelineTuningOptions.cs b/src/CodeyBox.Orchestrator/PipelineTuningOptions.cs index af86fea4e..c8e54f57c 100644 --- a/src/CodeyBox.Orchestrator/PipelineTuningOptions.cs +++ b/src/CodeyBox.Orchestrator/PipelineTuningOptions.cs @@ -110,6 +110,27 @@ public sealed class PipelineTuningOptions /// public int AutoMergeRaceRecoveryMaxAttempts { get; set; } = 3; + /// + /// Upper bound for the post-push mergeability settle before each GitHub + /// auto-merge call: after pushing, the orchestrator asks the remote to + /// poll the PR until its head equals the pushed tip and mergeability is + /// computed, then merges only when mergeable. GitHub recomputes + /// mergeability asynchronously after a push, so merging without this + /// wait observes the previous head and returns a spurious 405. A settle + /// that never converges falls through to the merge call best-effort. + /// Default 60 seconds. Hot-reloaded with the rest of + /// PipelineTuning; read fresh on every upstream-push attempt. + /// + public TimeSpan AutoMergeSettleTimeout { get; set; } = TimeSpan.FromSeconds(60); + + /// + /// Initial interval between post-push mergeability settle polls; the + /// delay backs off exponentially within + /// . Default 2 seconds. + /// Hot-reloaded with the rest of PipelineTuning. + /// + public TimeSpan AutoMergeSettlePollInterval { get; set; } = TimeSpan.FromSeconds(2); + /// /// Maximum times a single merge phase re-queues its landing after /// detecting the base branch moved between composition and the @@ -655,6 +676,18 @@ public void Validate() nameof(MergeResultBuildVerificationInfraMaxAttempts), "MergeResultBuildVerificationInfraMaxAttempts must be >= 1"); } + if (AutoMergeSettleTimeout <= TimeSpan.Zero || AutoMergeSettleTimeout > TimeSpan.FromMinutes(10)) + { + throw new ArgumentOutOfRangeException( + nameof(AutoMergeSettleTimeout), + "AutoMergeSettleTimeout must be positive and at most 10 minutes"); + } + if (AutoMergeSettlePollInterval <= TimeSpan.Zero || AutoMergeSettlePollInterval > TimeSpan.FromMinutes(5)) + { + throw new ArgumentOutOfRangeException( + nameof(AutoMergeSettlePollInterval), + "AutoMergeSettlePollInterval must be positive and at most 5 minutes"); + } if (EarlyEndedTurnMaxNudges < 0) { throw new ArgumentOutOfRangeException( diff --git a/src/CodeyBox.Upstream.GitHub/GitHubUpstreamRemote.cs b/src/CodeyBox.Upstream.GitHub/GitHubUpstreamRemote.cs index 695fd2a9d..b572e98a7 100644 --- a/src/CodeyBox.Upstream.GitHub/GitHubUpstreamRemote.cs +++ b/src/CodeyBox.Upstream.GitHub/GitHubUpstreamRemote.cs @@ -43,6 +43,7 @@ public sealed class GitHubUpstreamRemote : IUpstreamRemote private readonly IPullRequestDescriptionGenerator? _descriptionGenerator; private readonly CommitAttribution _attribution; private readonly UpstreamBranchOwnershipSnapshot _ownership; + private readonly TimeProvider _clock; public GitHubUpstreamRemote( IGitHost gitHost, @@ -52,7 +53,8 @@ public GitHubUpstreamRemote( ITimingStore? timings = null, IPullRequestDescriptionGenerator? descriptionGenerator = null, CommitAttribution? attribution = null, - UpstreamBranchOwnershipSnapshot? ownership = null) + UpstreamBranchOwnershipSnapshot? ownership = null, + TimeProvider? clock = null) { _gitHost = gitHost; _httpClientFactory = httpClientFactory; @@ -66,6 +68,7 @@ public GitHubUpstreamRemote( _descriptionGenerator = descriptionGenerator; _attribution = attribution ?? CommitAttribution.Default; _ownership = ownership ?? new UpstreamBranchOwnershipSnapshot(new UpstreamBranchOwnershipOptions()); + _clock = clock ?? TimeProvider.System; if (!IsValidRemoteName(_opts.Owner)) throw new ArgumentException($"GitHub Owner contains invalid characters: '{_opts.Owner}'", nameof(opts)); if (!IsValidRemoteName(_opts.Repository)) @@ -328,16 +331,47 @@ static bool HasInvalidChars(string s) => }; } - // Step 4: auto-merge + // Step 4: post-push mergeability settle, then auto-merge. + // GitHub recomputes a PR's mergeability asynchronously after a push, + // so a PUT issued ~1 s after the push observes the PREVIOUS + // (conflicting) head and returns 405 "not mergeable" for a tree that + // is actually clean. Poll GET /pulls/{n} until the reported head + // equals the just-pushed tip and `mergeable` is non-null, then merge + // only when mergeable. A settled `false` skips the doomed PUT and + // surfaces as AutoMergeRaced with the observed values so the + // orchestrator routes it to conflict handling instead of burning a + // race-recovery re-merge. A settle that never converges (timeout, + // unparsable responses) falls through to the PUT — best effort, as + // before — and a 405 there carries whatever was last observed. + var settled = await WaitForMergeSettledAsync(prNumber, pushedSha, request, ct); string? mergedSha; string? mergeNotes; bool autoMergeRaced; + bool? mergeable = settled.Mergeable; + string? mergeableState = settled.MergeableState; await using (var mergeScope = await TimingScope.BeginAsync( _timings, request.WorkItemId, "upstream_push", "upstream.api_merge_pr", log: _log)) { - (mergedSha, mergeNotes, autoMergeRaced) = await MergePullRequestAsync( - prNumber, prTitle, prDescription, request, ct); + if (settled.Mergeable == false) + { + var settledState = SanitizeForLog(settled.MergeableState); + mergeNotes = $"GitHub PR #{prNumber} settled unmergeable before the merge call " + + $"(mergeable=false, mergeable_state='{settledState}'); skipping PUT /pulls/merge so the orchestrator routes it to conflict handling"; + _log.LogWarning( + "GitHub PR #{N} settled unmergeable (mergeable=false, mergeable_state='{State}'); skipping merge call", + prNumber, settledState); + AuditLog.UpstreamApiCallFailed("PUT /pulls/merge", 405, _opts.Owner, _opts.Repository); + mergedSha = null; + autoMergeRaced = true; + } + else + { + bool raced; + (mergedSha, mergeNotes, raced, mergeable, mergeableState) = await MergePullRequestAsync( + prNumber, prTitle, prDescription, request, settled, ct); + autoMergeRaced = raced; + } } if (mergedSha is not null) @@ -355,6 +389,8 @@ static bool HasInvalidChars(string s) => MergedSha = mergedSha, Notes = mergeNotes, AutoMergeRaced = autoMergeRaced, + Mergeable = mergeable, + MergeableState = mergeableState, }; } @@ -468,7 +504,10 @@ public async Task> ListOpenPullRequestsAsync( detail.Number, detail.HtmlUrl ?? $"https://github.com/{_opts.Owner}/{_opts.Repository}/pull/{detail.Number}", status, - detail.Merged ? detail.MergeCommitSha : null); + detail.Merged ? detail.MergeCommitSha : null, + detail.Head?.Sha, + detail.Mergeable, + detail.MergeableState); } /// @@ -1871,11 +1910,118 @@ private bool IsExactRevisionMatch(GitHubPrDetailMergeable detail, UpstreamComple } } - private async Task<(string? Sha, string? Notes, bool AutoMergeRaced)> MergePullRequestAsync( + /// + /// Snapshot of the post-push mergeability settle: the last + /// mergeable/mergeable_state values observed on the PR. + /// is true only when the forge reported the + /// just-pushed head with a computed (non-null) mergeability — anything + /// else means the merge call proceeds best-effort or is skipped on + /// settled false, never on assumed values. + /// + private sealed record MergeSettleResult(bool? Mergeable, string? MergeableState, bool Converged); + + /// Fallback first poll delay when the request carries no interval. + private static readonly TimeSpan DefaultSettlePollInterval = TimeSpan.FromSeconds(2); + + /// Ceiling for the exponential settle backoff between polls. + private static readonly TimeSpan MaxSettlePollDelay = TimeSpan.FromSeconds(10); + + /// + /// Waits for GitHub to finish computing the PR's mergeability for the + /// just-pushed revision: polls GET /pulls/{n} until the reported + /// head equals and mergeable is + /// non-null, bounded by the request's settle timeout with exponential + /// backoff. Runs only when the request carries a settle timeout (the + /// orchestrator sets it from hot-reloadable tuning on every attempt); + /// legacy callers with no timeout skip straight to the merge call. + /// Poll failures (non-success status, unparsable body) are retried + /// until the deadline — never thrown — so a flaky detail read degrades + /// to a best-effort merge, not a failed push. Cancellation always throws. + /// + private async Task WaitForMergeSettledAsync( + int prNumber, + string? pushedSha, + UpstreamCompletionRequest request, + CancellationToken ct) + { + if (request.MergeSettleTimeout is not { } timeout || timeout <= TimeSpan.Zero) + return new MergeSettleResult(null, null, false); + + var pollInterval = request.MergeSettlePollInterval is { } configured && configured > TimeSpan.Zero + ? configured + : DefaultSettlePollInterval; + var expectedHead = pushedSha?.Trim(); + var deadline = _clock.GetUtcNow() + timeout; + var delay = pollInterval; + bool? mergeable = null; + string? mergeableState = null; + + while (true) + { + ct.ThrowIfCancellationRequested(); + try + { + var detail = await FetchPullRequestDetailAsync(prNumber, ct); + if (detail is not null) + { + mergeable = detail.Mergeable; + mergeableState = detail.MergeableState; + var headMatches = string.IsNullOrWhiteSpace(expectedHead) + || string.Equals(detail.Head?.Sha?.Trim(), expectedHead, StringComparison.OrdinalIgnoreCase); + if (headMatches && detail.Mergeable is not null) + { + _log.LogInformation( + "GitHub PR #{N} mergeability settled for pushed head: mergeable={Mergeable}, mergeable_state='{State}'", + prNumber, detail.Mergeable, SanitizeForLog(detail.MergeableState)); + return new MergeSettleResult(detail.Mergeable, detail.MergeableState, true); + } + _log.LogDebug( + "GitHub PR #{N} mergeability not yet settled (head matches pushed tip: {HeadMatches}, mergeable computed: {Computed}); polling again", + prNumber, headMatches, detail.Mergeable is not null); + } + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + _log.LogDebug(ex, "GitHub PR #{N} settle poll failed; retrying until the deadline", prNumber); + } + + var now = _clock.GetUtcNow(); + if (now >= deadline) + break; + var remaining = deadline - now; + var sleep = delay > remaining ? remaining : delay; + if (sleep > TimeSpan.Zero) + { + try + { + await Task.Delay(sleep, _clock, ct); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + } + var doubledTicks = delay.Ticks * 2; + var cappedTicks = Math.Min(doubledTicks, MaxSettlePollDelay.Ticks); + delay = TimeSpan.FromTicks(Math.Max(cappedTicks, pollInterval.Ticks)); + } + + _log.LogWarning( + "GitHub PR #{N} mergeability did not settle within {Timeout}s (last mergeable={Mergeable}, mergeable_state='{State}'); proceeding to the merge call best-effort", + prNumber, timeout.TotalSeconds, mergeable, SanitizeForLog(mergeableState)); + return new MergeSettleResult(mergeable, mergeableState, false); + } + + private async Task<(string? Sha, string? Notes, bool AutoMergeRaced, bool? Mergeable, string? MergeableState)> MergePullRequestAsync( int prNumber, string prTitle, PrDescriptionResult? prDescription, UpstreamCompletionRequest completionRequest, + MergeSettleResult settled, CancellationToken ct) { var url = $"https://api.github.com/repos/{_opts.Owner}/{_opts.Repository}/pulls/{prNumber}/merge"; @@ -1922,12 +2068,20 @@ private bool IsExactRevisionMatch(GitHubPrDetailMergeable detail, UpstreamComple // against the new tip, and retries this PUT. Branch protection can // also surface as 405; in that case re-fetching shows base unchanged // and the orchestrator parks the item rather than spinning. - const string note = "GitHub PUT /pulls/N/merge returned 405 (PR not mergeable — likely a race against upstream base; orchestrator will re-fetch base and re-run merge phase)"; + // The settle snapshot travels with the outcome so the + // orchestrator can tell "still computing" (null — retry the + // call without re-merging) from a genuine conflict (false + // with an unchanged base — re-merging cannot help) instead of + // treating every 405 as base motion. + var observedState = SanitizeForLog(settled.MergeableState); + var note = settled.Mergeable is null + ? "GitHub PUT /pulls/N/merge returned 405 (PR not mergeable — mergeability still unknown after the post-push settle; orchestrator will re-fetch base and retry)" + : $"GitHub PUT /pulls/N/merge returned 405 (PR not mergeable — mergeable={settled.Mergeable}, mergeable_state='{observedState}'; orchestrator will re-fetch base and classify)"; _log.LogWarning( - "GitHub PUT /pulls/{N}/merge returned 405 (PR not mergeable); orchestrator will re-fetch base and re-run merge phase", - prNumber); + "GitHub PUT /pulls/{N}/merge returned 405 (PR not mergeable; mergeable={Mergeable}, mergeable_state='{State}')", + prNumber, settled.Mergeable, observedState); AuditLog.UpstreamApiCallFailed("PUT /pulls/merge", 405, _opts.Owner, _opts.Repository); - return (null, note, true); + return (null, note, true, settled.Mergeable, settled.MergeableState); } if (!response.IsSuccessStatusCode) @@ -1942,7 +2096,7 @@ private bool IsExactRevisionMatch(GitHubPrDetailMergeable detail, UpstreamComple throw new InvalidOperationException( $"GitHub PUT /pulls/{prNumber}/merge returned success but did not prove a merge " + $"(merged={result?.Merged}, sha present={!string.IsNullOrWhiteSpace(result?.Sha)}); refusing to report delivery."); - return (result.Sha, null, false); + return (result.Sha, null, false, settled.Mergeable, settled.MergeableState); } } @@ -1953,7 +2107,7 @@ private bool IsExactRevisionMatch(GitHubPrDetailMergeable detail, UpstreamComple /// alone is insufficient). Null when unprovable — the caller rethrows the /// original transport error into the bounded retry loop. /// - private async Task<(string? Sha, string? Notes, bool AutoMergeRaced)?> TryVerifyMergeAfterUncertaintyAsync( + private async Task<(string? Sha, string? Notes, bool AutoMergeRaced, bool? Mergeable, string? MergeableState)?> TryVerifyMergeAfterUncertaintyAsync( int prNumber, UpstreamCompletionRequest request, CancellationToken ct) { try @@ -1973,7 +2127,7 @@ private bool IsExactRevisionMatch(GitHubPrDetailMergeable detail, UpstreamComple } _log.LogInformation("GitHub PR #{N} merge verified after transport failure: {Sha}", prNumber, detail.MergeCommitSha); AuditLog.UpstreamPrMerged(prNumber, detail.MergeCommitSha); - return (detail.MergeCommitSha, "Merge response was lost in transport; delivery verified by re-reading the merged PR state.", false); + return (detail.MergeCommitSha, "Merge response was lost in transport; delivery verified by re-reading the merged PR state.", false, detail.Mergeable, detail.MergeableState); } catch (OperationCanceledException) when (ct.IsCancellationRequested) { diff --git a/tests/CodeyBox.Tests/AutoMergeRaceSettleTests.cs b/tests/CodeyBox.Tests/AutoMergeRaceSettleTests.cs new file mode 100644 index 000000000..e35c68a79 --- /dev/null +++ b/tests/CodeyBox.Tests/AutoMergeRaceSettleTests.cs @@ -0,0 +1,200 @@ +using CodeyBox.Core; +using CodeyBox.Orchestrator; + +namespace CodeyBox.Tests; + +/// +/// Classification of AutoMergeRaced outcomes by the forge's own +/// mergeability verdict plus a fresh base comparison (the foligen#12 +/// post-mortem: a push followed ~1 s later by the merge call races GitHub's +/// asynchronous mergeability computation, producing a spurious 405 against +/// the previous head while the base never moved). +/// +/// Drives the full RunAsync pipeline with the shared +/// scripted with outcome + live-PR +/// mergeability, through real wiring (real , +/// real local git host): +/// +/// base unchanged + mergeable unknown → same-tree merge-call retry +/// that does NOT re-run the merge phase and does NOT consume the +/// race-recovery budget; +/// base unchanged + mergeable=false → genuine conflict routed to the +/// existing conflict path, outside the cap, never blaming another +/// writer. +/// +/// +[Collection("Pipeline integration")] +public sealed class AutoMergeRaceSettleTests : IDisposable +{ + private readonly string _workspace = + Directory.CreateTempSubdirectory("codeybox-settle-").FullName; + + public void Dispose() => Directory.Delete(_workspace, recursive: true); + + private static WorkItem NewItem(string workBranch) => new() + { + Id = WorkItemId.New(), + ProjectId = new ProjectId("test-project"), + Title = "test", + Prompt = "do thing", + BaseBranch = "main", + WorkBranch = workBranch, + PushUpstream = true, + }; + + private static TestPipeline BuildSettlePipeline( + string workspace, + string seed, + RacingUpstreamRemote remote, + int raceRecoveryCap, + StalePullRequestSweeperOptions? staleBaseReworkOptions = null) + { + var tp = TestSupport.BuildPipeline( + workspace, seed, + upstream: new ProjectUpstream + { + Kind = "racing-upstream", + AutoMerge = true, + MergeMethod = "squash", + }, + upstreamFactory: new SingleRemoteFactory(remote), + pipelineOptions: new PipelineOptions + { + SandboxImageReference = "ignored", + AgentAllowedHosts = [], + UpstreamPushMaxAttempts = 5, + UpstreamPushBackoff = TimeSpan.Zero, + }, + pipelineTuning: new PipelineTuningSnapshot(new PipelineTuningOptions + { + AutoMergeRaceRecoveryMaxAttempts = raceRecoveryCap, + }), + staleBaseReworkOptions: staleBaseReworkOptions, + mergeStrategy: [MergeStrategy.RealMerge, MergeStrategy.RealMerge]); + remote.BareRepoRoot = tp.GitRoot; + tp.Agent.WorkPlan.Enqueue(new FileWrite("settle-fix.txt", "fixes settle\n")); + return tp; + } + + [Fact] + public async Task RaceWithUnchangedBaseAndUnknownMergeable_RetriesSameTreeWithoutConsumingRecovery() + { + var seed = await TestSupport.CreateSeedRepoAsync(_workspace); + + // The 405 raced the forge's mergeability computation: base never + // moves, mergeability stays unknown. The orchestrator must retry the + // merge call with the identical tree — no merge-phase re-run, no + // race-recovery budget spent. With the cap at 1, the old behaviour + // (every 405 burns a re-merge) would park after the first recovery; + // success here proves the retry did not consume it. + var remote = new RacingUpstreamRemote + { + SeedRepoPath = seed, + ResponsePlan = + { + new RacingResponse(AutoMergeRaced: true, AdvanceSeedBeforeReturning: false), + new RacingResponse(AutoMergeRaced: false, AdvanceSeedBeforeReturning: false), + }, + }; + using var tp = BuildSettlePipeline(_workspace, seed, remote, raceRecoveryCap: 1); + + var item = NewItem("codeybox/settle-unknown"); + await tp.Store.CreateAsync(item); + await tp.Pipeline.RunAsync(item, CancellationToken.None); + + var final = await tp.Store.GetAsync(item.Id); + Assert.Equal(WorkItemState.Done, final!.State); + Assert.Equal(2, remote.CompleteCalls); + // Same tree on the retry: the merge phase ran once, not twice. + Assert.Equal(remote.Requests[0].MergeSha, remote.Requests[1].MergeSha); + Assert.Equal(1, remote.Requests[1].ExistingPullRequestNumber); + Assert.Equal(2, final.UpstreamPushAttempts); + // The orchestrator read the live PR for classification (unknown + // here) and passed its hot-reloadable settle knobs to the remote. + Assert.Equal(1, remote.PullReads); + Assert.Equal(TimeSpan.FromSeconds(60), remote.Requests[0].MergeSettleTimeout); + Assert.Equal(TimeSpan.FromSeconds(2), remote.Requests[0].MergeSettlePollInterval); + } + + [Fact] + public async Task RaceWithUnchangedBaseAndSettledFalse_RoutesToConflictRework() + { + var seed = await TestSupport.CreateSeedRepoAsync(_workspace); + + // The forge settled the PR as conflicting against an unchanged base: + // re-running the merger cannot help. The item must enter the existing + // conflict path with no further merge attempt and no race-recovery + // spent — and the base must never be blamed on another writer. + var remote = new RacingUpstreamRemote + { + SeedRepoPath = seed, + ResponsePlan = + { + new RacingResponse( + AutoMergeRaced: true, + AdvanceSeedBeforeReturning: false, + Mergeable: false, + MergeableState: "dirty"), + }, + }; + remote.PullStates.Enqueue(new UpstreamPullRequestState( + 1, "https://example.invalid/pr/1", PullRequestStatus.Open, null, + HeadSha: null, Mergeable: false, MergeableState: "dirty")); + using var tp = BuildSettlePipeline( + _workspace, seed, remote, raceRecoveryCap: 3, + staleBaseReworkOptions: new StalePullRequestSweeperOptions + { + RouteToConflictRework = true, + MaxReworkAttempts = 2, + }); + + var item = NewItem("codeybox/settle-conflict"); + await tp.Store.CreateAsync(item); + await tp.Pipeline.RunAsync(item, CancellationToken.None); + + var final = await tp.Store.GetAsync(item.Id); + Assert.Equal(WorkItemState.ReworkingForConflict, final!.State); + Assert.Equal(1, final.ConflictReworkAttempts); + Assert.Equal(1, remote.CompleteCalls); + Assert.Equal(1, final.UpstreamPushAttempts); + var queue = Assert.IsType(tp.Queue); + Assert.Equal(item.Id, await queue.DequeueAsync(CancellationToken.None)); + } + + [Fact] + public async Task RaceWithUnchangedBaseAndSettledFalse_RouterDisabled_ParksWithMergeableVerdict() + { + var seed = await TestSupport.CreateSeedRepoAsync(_workspace); + + var remote = new RacingUpstreamRemote + { + SeedRepoPath = seed, + ResponsePlan = + { + new RacingResponse( + AutoMergeRaced: true, + AdvanceSeedBeforeReturning: false, + Mergeable: false, + MergeableState: "dirty"), + }, + }; + remote.PullStates.Enqueue(new UpstreamPullRequestState( + 1, "https://example.invalid/pr/1", PullRequestStatus.Open, null, + HeadSha: null, Mergeable: false, MergeableState: "dirty")); + using var tp = BuildSettlePipeline(_workspace, seed, remote, raceRecoveryCap: 3); + + var item = NewItem("codeybox/settle-conflict-park"); + await tp.Store.CreateAsync(item); + await tp.Pipeline.RunAsync(item, CancellationToken.None); + + var final = await tp.Store.GetAsync(item.Id); + Assert.Equal(WorkItemState.MergeConflictResolutionFailed, final!.State); + Assert.Equal(1, remote.CompleteCalls); + // The park message carries the forge's actual verdict — never the + // another-writer story, which would be false: the base did not move. + Assert.Contains("mergeable=false", final.LastError, StringComparison.OrdinalIgnoreCase); + Assert.Contains("dirty", final.LastError, StringComparison.Ordinal); + Assert.Contains("unchanged", final.LastError, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("another writer", final.LastError, StringComparison.OrdinalIgnoreCase); + } +} diff --git a/tests/CodeyBox.Tests/ConfigReloadClassificationTests.cs b/tests/CodeyBox.Tests/ConfigReloadClassificationTests.cs index 0cefafb4f..a5d6b481e 100644 --- a/tests/CodeyBox.Tests/ConfigReloadClassificationTests.cs +++ b/tests/CodeyBox.Tests/ConfigReloadClassificationTests.cs @@ -508,6 +508,12 @@ private static void MutatePipelineTuningField(PipelineTuningOptions opts, string case nameof(PipelineTuningOptions.AutoMergeRaceRecoveryMaxAttempts): opts.AutoMergeRaceRecoveryMaxAttempts += 100; break; + case nameof(PipelineTuningOptions.AutoMergeSettleTimeout): + opts.AutoMergeSettleTimeout += TimeSpan.FromHours(1); + break; + case nameof(PipelineTuningOptions.AutoMergeSettlePollInterval): + opts.AutoMergeSettlePollInterval += TimeSpan.FromHours(1); + break; case nameof(PipelineTuningOptions.EnableSandboxReuse): opts.EnableSandboxReuse = !opts.EnableSandboxReuse; break; diff --git a/tests/CodeyBox.Tests/GitHubMergeSettleTests.cs b/tests/CodeyBox.Tests/GitHubMergeSettleTests.cs new file mode 100644 index 000000000..9587f6952 --- /dev/null +++ b/tests/CodeyBox.Tests/GitHubMergeSettleTests.cs @@ -0,0 +1,197 @@ +using System.Net; +using System.Text; +using System.Text.Json; +using CodeyBox.Core; +using CodeyBox.Upstream.GitHub; +using Microsoft.Extensions.Logging.Abstractions; + +namespace CodeyBox.Tests; + +/// +/// Post-push mergeability settle in : +/// GitHub recomputes a PR's mergeability asynchronously after a push, so a +/// merge issued ~1 s after the push observes the previous head and returns a +/// spurious 405. After the push the remote polls GET /pulls/{n} until +/// the reported head equals the pushed tip and mergeable is non-null, +/// then merges only when mergeable. Uses the shared fake +/// (same assembly) so no real API is called. +/// +public sealed class GitHubMergeSettleTests +{ + private static readonly GitHubUpstreamOptions SettleOpts = new() + { + Owner = "myorg", + Repository = "myrepo", + Token = "test-token-not-a-real-pat", + MergeMethod = "merge", + AutoMerge = true, + }; + + private const string PushedSha = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + private const string StaleSha = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + + private static UpstreamCompletionRequest SettleRequest() => new() + { + RepositoryId = "repo-id", + WorkItemId = new WorkItemId(Guid.Parse("00000000-0000-0000-0000-000000000003")), + ProjectId = new ProjectId("test-project"), + WorkBranch = "codeybox/abc123", + BaseBranch = "main", + MergeSha = "deadbeef", + Title = "Add feature X", + Description = "Automated via CodeyBox", + MergeSettleTimeout = TimeSpan.FromSeconds(5), + MergeSettlePollInterval = TimeSpan.FromMilliseconds(10), + }; + + private static GitHubUpstreamRemote BuildRemote( + FakeGitHost gitHost, FakeHttpMessageHandler handler) => + new( + gitHost, + new FakeHttpClientFactory(handler, userAgent: "codeybox"), + NullLogger.Instance, + SettleOpts); + + private static FakeGitHost GitHostWithTip() => + new() { ResolveCommits = { ["codeybox/abc123"] = PushedSha } }; + + private static HttpResponseMessage JsonResponse(HttpStatusCode status, string json) => + new(status) { Content = new StringContent(json, Encoding.UTF8, "application/json") }; + + private static HttpResponseMessage PrCreatedResponse(int number, string htmlUrl) => + JsonResponse(HttpStatusCode.Created, + $$"""{"number":{{number}},"html_url":"{{htmlUrl}}"}"""); + + private static HttpResponseMessage PrDetailResponse(string headSha, bool? mergeable, string? state) => + JsonResponse(HttpStatusCode.OK, JsonSerializer.Serialize(new + { + number = 99, + html_url = "https://github.com/myorg/myrepo/pull/99", + state = "open", + merged = false, + mergeable, + mergeable_state = state, + head = new { @ref = "codeybox/abc123", sha = headSha }, + @base = new { @ref = "main", sha = "basesha" }, + })); + + private static HttpResponseMessage MergeOkResponse(string sha) => + JsonResponse(HttpStatusCode.OK, + $$"""{"sha":"{{sha}}","merged":true,"message":"Pull Request successfully merged"}"""); + + private static bool IsDetailGet(HttpRequestMessage r) => + r.Method == HttpMethod.Get && r.RequestUri!.PathAndQuery == "/repos/myorg/myrepo/pulls/99"; + + private static bool IsMergePut(HttpRequestMessage r) => + r.Method == HttpMethod.Put && r.RequestUri!.PathAndQuery == "/repos/myorg/myrepo/pulls/99/merge"; + + [Fact] + public async Task Settle_MergeableNullThenTrue_MergesOnceWithoutRace() + { + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrCreatedResponse(99, "https://github.com/myorg/myrepo/pull/99")); + handler.Enqueue(PrDetailResponse(PushedSha, null, "unknown")); + handler.Enqueue(PrDetailResponse(PushedSha, true, "clean")); + handler.Enqueue(MergeOkResponse("merge-sha-99")); + + var outcome = await BuildRemote(GitHostWithTip(), handler) + .CompleteAsync(SettleRequest(), CancellationToken.None); + + Assert.Equal("merge-sha-99", outcome.MergedSha); + Assert.False(outcome.AutoMergeRaced); + Assert.True(outcome.Mergeable); + Assert.Equal("clean", outcome.MergeableState); + // Two settle polls (unknown, then computed) before exactly one PUT. + Assert.Equal(2, handler.Requests.Count(IsDetailGet)); + Assert.Single(handler.Requests, IsMergePut); + } + + [Fact] + public async Task Settle_SettledFalse_SkipsPutAndFlagsRaceWithVerdict() + { + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrCreatedResponse(99, "https://github.com/myorg/myrepo/pull/99")); + handler.Enqueue(PrDetailResponse(PushedSha, false, "dirty")); + + var outcome = await BuildRemote(GitHostWithTip(), handler) + .CompleteAsync(SettleRequest(), CancellationToken.None); + + Assert.True(outcome.AutoMergeRaced); + Assert.Null(outcome.MergedSha); + Assert.False(outcome.Mergeable); + Assert.Equal("dirty", outcome.MergeableState); + Assert.Contains("mergeable", outcome.Notes, StringComparison.OrdinalIgnoreCase); + // The doomed PUT is skipped: only create + the settle read. + Assert.DoesNotContain(handler.Requests, IsMergePut); + } + + [Fact] + public async Task Settle_ComputedButStaleHead_WaitsForPushedHead() + { + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrCreatedResponse(99, "https://github.com/myorg/myrepo/pull/99")); + // First poll: mergeability computed, but for the PREVIOUS head — the + // exact stale read that used to produce the spurious 405. + handler.Enqueue(PrDetailResponse(StaleSha, true, "clean")); + handler.Enqueue(PrDetailResponse(PushedSha, true, "clean")); + handler.Enqueue(MergeOkResponse("merge-sha-99")); + + var outcome = await BuildRemote(GitHostWithTip(), handler) + .CompleteAsync(SettleRequest(), CancellationToken.None); + + Assert.Equal("merge-sha-99", outcome.MergedSha); + Assert.False(outcome.AutoMergeRaced); + Assert.Equal(2, handler.Requests.Count(IsDetailGet)); + Assert.Single(handler.Requests, IsMergePut); + } + + [Fact] + public async Task Merge405_AfterSettledTrue_CarriesObservedVerdict() + { + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrCreatedResponse(99, "https://github.com/myorg/myrepo/pull/99")); + handler.Enqueue(PrDetailResponse(PushedSha, true, "clean")); + handler.Enqueue(JsonResponse(HttpStatusCode.MethodNotAllowed, + """{"message":"Pull Request is not mergeable"}""")); + + var outcome = await BuildRemote(GitHostWithTip(), handler) + .CompleteAsync(SettleRequest(), CancellationToken.None); + + Assert.True(outcome.AutoMergeRaced); + Assert.Null(outcome.MergedSha); + Assert.True(outcome.Mergeable); + Assert.Equal("clean", outcome.MergeableState); + Assert.Contains("405", outcome.Notes); + } + + [Fact] + public async Task NoSettleTimeout_SkipsDetailPollEntirely() + { + // Legacy callers (no settle timeout on the request) keep the + // historical create-then-merge sequence with no detail read. + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrCreatedResponse(99, "https://github.com/myorg/myrepo/pull/99")); + handler.Enqueue(MergeOkResponse("merge-sha-99")); + + var request = SettleRequest() with { MergeSettleTimeout = null, MergeSettlePollInterval = null }; + var outcome = await BuildRemote(GitHostWithTip(), handler) + .CompleteAsync(request, CancellationToken.None); + + Assert.Equal("merge-sha-99", outcome.MergedSha); + Assert.DoesNotContain(handler.Requests, IsDetailGet); + } + + [Fact] + public async Task GetPullRequestAsync_ExposesHeadAndMergeability() + { + var handler = new FakeHttpMessageHandler(); + handler.Enqueue(PrDetailResponse(PushedSha, false, "dirty")); + + var state = await BuildRemote(GitHostWithTip(), handler).GetPullRequestAsync(99); + + Assert.NotNull(state); + Assert.Equal(PushedSha, state!.HeadSha); + Assert.False(state.Mergeable); + Assert.Equal("dirty", state.MergeableState); + } +} diff --git a/tests/CodeyBox.Tests/UpstreamAutoMergeRaceRecoveryTests.cs b/tests/CodeyBox.Tests/UpstreamAutoMergeRaceRecoveryTests.cs index 21ae181c1..b66d2862c 100644 --- a/tests/CodeyBox.Tests/UpstreamAutoMergeRaceRecoveryTests.cs +++ b/tests/CodeyBox.Tests/UpstreamAutoMergeRaceRecoveryTests.cs @@ -100,16 +100,15 @@ public async Task AutoMergeRaces_OrchestratorReFetchesBaseAndRetriesUntilDone() } [Fact] - public async Task AutoMergeRace_BaseNotAdvanced_OrchestratorStillReRunsMergeAndAutoMerges() + public async Task AutoMergeRace_BaseNotAdvanced_RetriesMergeCallWithoutReworkAndAutoMerges() { var seed = await TestSupport.CreateSeedRepoAsync(_workspace); // Programme the remote to race once, but DON'T mutate the seed. - // With the fixed staleness check (live upstream base ref), the - // orchestrator re-runs the merge phase against the refreshed base - // regardless of whether it can detect motion. Since the base didn't - // actually move, the re-merge produces an equivalent tree and the - // retry succeeds — no operator intervention needed. + // The base is unchanged and mergeability is unknown, so the + // orchestrator retries the merge call with the same tree instead of + // re-running the merge phase (no race-recovery budget consumed). + // The retry succeeds — no operator intervention needed. var remote = new RacingUpstreamRemote { SeedRepoPath = seed, @@ -676,7 +675,7 @@ public async Task AutoMergeRace_RerunMergePhaseFails_ParksAsLlmMergerGaveUp() /// /// Decorating IGitHost that throws on SetBranchToCommitAsync to exercise the -/// recovery park-branch in TryRecoverFromAutoMergeRaceAsync. +/// recovery park-branch in ReRunMergeForRaceAsync. /// internal sealed class SetBranchThrowingGitHost : IGitHost { @@ -770,7 +769,12 @@ internal sealed record RacingResponse( // When true, force MergedSha=null on the outcome regardless of the // override. Mirrors GitHub's AutoMerge=false response (PR opened, no // merge commit yet — a human will merge later). - bool ForceMergedShaNull = false); + bool ForceMergedShaNull = false, + // Forge-reported mergeability travelling on the outcome, mirroring the + // values GitHubUpstreamRemote reports after its post-push settle. Null + // (default) means the forge had not computed it yet. + bool? Mergeable = null, + string? MergeableState = null); internal sealed class ThrowingNthTimingPhaseSandboxProvider : ISandboxProvider { @@ -887,6 +891,15 @@ internal sealed class RacingUpstreamRemote : IUpstreamRemote public List Requests { get; } = new(); public int CompleteCalls { get; private set; } public int FetchCalls { get; private set; } + public int PullReads { get; private set; } + + /// + /// Scripted live-PR reads for the orchestrator's mergeability + /// classification (consumed in order by ). + /// Empty (default) means unknown — the classifier falls back to the + /// outcome snapshot, matching remotes that do not report mergeability. + /// + public Queue PullStates { get; } = new(); /// When set, throws on every call. public Exception? FetchThrows { get; set; } @@ -945,10 +958,20 @@ await File.WriteAllTextAsync( ? null : response.MergedShaOverride ?? request.MergeSha ?? "merged-sha", AutoMergeRaced = response.AutoMergeRaced, + Mergeable = response.Mergeable, + MergeableState = response.MergeableState, Notes = response.AutoMergeRaced ? "racing" : null, }; } + public Task GetPullRequestAsync(int number, CancellationToken ct = default) + { + PullReads++; + if (PullStates.Count > 0) + return Task.FromResult(PullStates.Dequeue()); + return Task.FromResult(null); + } + public Task TryMergeUpstreamBranchAsync(string targetBranch, string sourceBranch, CancellationToken ct = default) => Task.FromResult(true);