Skip to content

feat: Support GitHub pull request merge queues in workflows - #266

Open
finxo wants to merge 12 commits into
masterfrom
feat/merge_queue_support
Open

feat: Support GitHub pull request merge queues in workflows#266
finxo wants to merge 12 commits into
masterfrom
feat/merge_queue_support

Conversation

@finxo

@finxo finxo commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Pull Request

📝 Summary

Adds merge-queue awareness to GitHub pull request workflows so queue-required branches enqueue PRs instead of reporting an immediate merge failure. Workflows can now detect, enqueue, and verify queued or completed merge outcomes while retaining existing merge behavior when no queue is configured.

🔧 Changes Made

  • Added GraphQL-based merge-queue state retrieval, models, mapping, and a public GitHub client API.
  • Updated PR merging to omit merge-strategy flags for queue-required branches and return explicit queued status and queue position.
  • Added check_merge_queue and verify_merge_outcome workflow steps, with metadata for downstream branching.
  • Updated GitHub plugin documentation and generated step references to reflect the expanded public workflow and client contracts.

🧪 Testing

  • Unit tests added/updated (poetry run pytest)
  • All tests passing (make test)
  • Manual testing with titan-dev

Added coverage for queue detection, enqueue behavior, queue-position readback, GraphQL unavailability, detection fallback, client delegation, and regular/queued verification paths.

📊 Logs

  • No new log events
  • merge_queue_detection_failed (WARNING) — emitted when GraphQL detection fails before falling back to the existing regular-merge behavior; includes the PR number and lookup error.

✅ Checklist

  • Self-review done
  • Follows the project's logging rules (no secrets, no content in logs)
  • New and existing tests pass
  • Documentation updated if needed
  • Plugin documentation updated when plugin functions or parameters changed (Plugins > Git Plugin, GitHub Plugin, Jira Plugin)

@finxo finxo added the feature New feature or functionality label Sep 3, 2026
@finxo finxo self-assigned this Sep 3, 2026
ctx.textual.end_step("error")
return Error("No PR number in context")

if not ctx.get("merge_queued"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using not ctx.get('merge_queued') conflates three cases: key absent (None), regular merge (False), and value 0. The absent-key case should be a hard Error so misconfigured workflows fail loudly. Consider:

merge_queued = ctx.get('merge_queued')
if merge_queued is None:
    ctx.textual.error_text("merge_queued not set — did merge_pull_request_step run?")
    ctx.textual.end_step("error")
    return Error("merge_queued not set in context")
if not merge_queued:
    # regular merge path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not pr_node_id:
return failed(msg.GitHub.PR_NOT_FOUND.format(pr_number=pr_number))

response = self.graphql.run_mutation(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the enqueuePullRequest mutation fails (e.g. PR not mergeable, check failures, already queued), GitHub returns a top-level errors array instead of—or alongside—a null data payload. The current code chains .get("data", {}).get("enqueuePullRequest", {}) unconditionally, so on an API-level error entry is {}, yet the result is still built with queued=True. The PR is reported as queued even though it was rejected.

You need to check for errors before reading the entry:

if errors := response.get("errors"):
    return failed(errors[0].get("message", "GraphQL mutation failed"))

entry = (
    response.get("data", {})
    .get("enqueuePullRequest", {})
    .get("mergeQueueEntry")
) or {}

Similarly, consider checking that entry is non-empty before marking as queued—if mergeQueueEntry is null in the response the entry dict will also be {} and queue_position will silently be None.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GraphQLNetwork._execute_graphql already checks for a top-level errors array and raises GitHubAPIError, which merge_pr catches and turns into failed(...) — so an API-level rejection never reaches the entry parsing (covered by test_merge_pr_reports_failed_enqueue_mutation). The empty-entry case is deliberate: with no errors the enqueue succeeded, only the position is unknown, so we report queued=True with queue_position=None (test_merge_pr_queues_without_position_when_entry_missing).

@@ -583,6 +606,12 @@ def merge_pr(
ui_result = from_network_pr_merge_result(network_result)
return ClientSuccess(data=ui_result, message="Invalid merge method")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning ClientSuccess for an invalid merge method mixes a programming error with a legitimate merge failure. Now that UIPRMergeResult has three states (merged, queued, and neither), callers that want to distinguish "was it queued?" from "did it simply fail?" have no way to know that the failure was caused by a bad argument rather than a GitHub rejection. Consider returning ClientError(error_message=..., error_code="INVALID_MERGE_METHOD") so the invalid-argument case is structurally distinct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is unchanged from master — the PR doesn't touch it, and the new queued flag doesn't create the ambiguity: "was it queued?" is answered by queued=True, so invalid-method is exactly as distinguishable from a GitHub rejection as it was before. Happy to move it to ClientError(error_code="INVALID_MERGE_METHOD") as a separate cleanup, but it's out of scope here and the only caller maps both branches to the same Error.

return failed("GraphQL network is not available to reach the merge queue")

try:
owner, repo = self.gh.get_repo_string().split('/')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unpacked split will raise ValueError with a confusing message if get_repo_string() ever returns a value that does not have exactly one / (bare name, enterprise org/team/repo, or empty string). The except GitHubAPIError below does not catch ValueError, so the exception escapes _enqueue_pr and merge_pr entirely. Use split('/', 1) and validate the length:

parts = self.gh.get_repo_string().split('/', 1)
if len(parts) != 2:
    return failed(f"Cannot parse repository string: {self.gh.get_repo_string()!r}")
owner, repo = parts

The same fix is needed in get_merge_queue_state (line ~773).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

match self.get_merge_queue_state(pr_number):
case ClientSuccess(data=queue_state):
return queue_state.is_merge_queue_enabled
case ClientError(error_message=err):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "fall back to direct merge on any detection error" policy is documented but has a silent state consistency risk: if the detection fails because GraphQL is temporarily unavailable or the token lacks repo scope, the code merges directly into a queue-protected branch. GitHub will likely reject it, but the user sees a merge failure without knowing why the queue check was skipped.

Consider returning a tri-state (True / False / None = unknown) and propagating the unknown case as a warning in merge_pr before falling back, so operators can distinguish "queue not configured" from "detection failed".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert "--squash" in mock_gh_network.run_command.call_args[0][0]


def test_merge_pr_falls_back_to_regular_merge_when_detection_fails(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback test verifies merged=True and the merge command, but never asserts result.data.queued is False. If a regression sets queued=True on the fallback path the test would still pass. Please add:

assert result.data.queued is False

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ui_result = from_network_pr_merge_result(network_result)
return ClientSuccess(data=ui_result, message="Invalid merge method")

if merge_queue_enabled is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no test for merge_queue_enabled=False as an explicit override. The parameter is documented as "known merge queue state, to avoid a second lookup", implying callers may pass False when they already know no queue exists. Without a test, a regression that treats explicit False differently from auto-detected False would go unnoticed. Consider adding:

def test_merge_pr_skips_detection_when_merge_queue_explicitly_disabled(queue_pr_service, mock_gh_network):
    mock_gh_network.run_command.return_value = "✓ Merged pull request #123 (abc123d)"
    result = queue_pr_service.merge_pr(123, merge_method="squash", merge_queue_enabled=False)
    assert isinstance(result, ClientSuccess)
    assert result.data.merged is True
    assert result.data.queued is False
    mock_graphql_network.run_query.assert_not_called()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread docs/plugins/github/workflow-steps.md Outdated

| Result | Saved for later steps | Description |
|--------|-----------------------|-------------|
| `Success` | `verified_pr_info`, `merge_queue_state` | If the PR is merged, or still queued when it was enqueued. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Returns table lists both verified_pr_info and merge_queue_state as saved for every Success, but the step only ever populates one or the other:

  • Regular merge path (merge_queued falsy): saves verified_pr_info only.
  • Queued merge path (merge_queued truthy): saves merge_queue_state only — whether the PR is still in the queue or was already merged by it.

A workflow author reading the current table will assume verified_pr_info is always safe to read after this step, leading to a KeyError/None when the PR went through the merge queue. Please split the Returns row into two entries (one per path) or add a note clarifying that these keys are mutually exclusive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert result.metadata == {"merge_queue_state": queue_state}


def test_verify_merge_outcome_step_errors_when_pr_left_the_queue():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The queued-path ClientError branch of verify_merge_outcome_step is untested. If get_merge_queue_state returns a ClientError when merge_queued=True, the step should return Error("Failed to verify the merge queue state: ...") — but there is no test asserting this. A regression that routes it to the 'left the merge queue' branch instead would go undetected and produce a misleading error. Please add a test like:

def test_verify_merge_outcome_step_errors_on_queue_state_lookup_failure():
    github = Mock()
    github.get_merge_queue_state.return_value = ClientError(error_message="graphql down")
    ctx = make_context(github, pr_number=123, merge_queued=True)

    result = verify_merge_outcome_step(ctx)

    assert isinstance(result, Error)
    assert "Failed to verify the merge queue state" in result.message
    ctx.textual.end_step.assert_called_once_with("error")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result = check_merge_queue_step(ctx)

assert isinstance(result, Success)
assert result.metadata == {"merge_queue_enabled": False}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check_merge_queue_step ClientError fallback intentionally omits "merge_queue_state" from metadata (only {"merge_queue_enabled": False}). There is no test that chains this into merge_pull_request_step to verify the merge step handles the absent key gracefully. Since merge_pull_request_step only reads ctx.get("merge_queue_enabled") today this is safe, but the asymmetry is undocumented. Consider adding a test that runs the full check→merge sequence through the error-fallback path to make this contract explicit and catch regressions if the merge step is ever extended.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@finxo
finxo requested a review from r-pedraza September 7, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants