Skip to content

fix(create-pr): skip auto-complete API call on draft PRs#200

Merged
jamesadevine merged 1 commit intomainfrom
fix/draft-autocomplete-conflict
Apr 14, 2026
Merged

fix(create-pr): skip auto-complete API call on draft PRs#200
jamesadevine merged 1 commit intomainfrom
fix/draft-autocomplete-conflict

Conversation

@jamesadevine
Copy link
Copy Markdown
Collaborator

Closes #194.

Problem

When draft: true (now the default since #155) and auto-complete: true are both configured, the executor makes a pointless autoCompleteSetBy API call that ADO silently ignores on draft PRs. The operator gets a log warning but may not realize auto-complete isn't actually set.

Fix

Option 2 from the issue: skip the auto-complete API call when draft: true. The warning is preserved so operators are aware of the conflict.

// Before: always set autoCompleteSetBy when auto_complete is true
if config.auto_complete {

// After: skip on draft PRs (ADO silently ignores it anyway)

Testing

  • Added unit tests for default config values and config deserialization
  • All 812 tests pass

When both draft: true and auto-complete: true are configured, skip the
autoCompleteSetBy API call since ADO silently ignores auto-complete on
draft PRs. The existing warning is preserved so operators are aware of
the conflict.

This prevents a pointless API call and avoids silent behavior loss when
draft: true became the default in PR #155.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🔍 Rust PR Review

Summary: Looks good — clean, minimal fix for a real behavioral issue.

Findings

✅ What Looks Good

  • Logic is correct: The completionOptions PATCH (delete source branch, squash merge) is still sent for draft PRs — only the autoCompleteSetBy field is omitted. This is the right split: completion options take effect when the draft is later marked ready, while auto-complete being set on a draft is meaningless.
  • Warning is preserved: The pre-existing warn! at line 528 ("auto-complete cannot be set on a draft PR...") continues to inform operators of the conflict at execution time, even though the API call is now skipped.
  • No double-PATCH: The code correctly keeps a single PATCH call for all completionOptions updates and only conditionally adds autoCompleteSetBy to the body — avoids an extra HTTP round-trip.

⚠️ Suggestions

  • src/safeoutputs/create_pr.rs:2258: The two new tests cover config deserialization, but neither exercises the scenario being fixed (draft: true + auto_complete: true). Adding a third test documenting this combination would make the intent explicit and serve as a regression guard:

    #[test]
    fn test_config_deserialize_draft_true_autocomplete_true() {
        let yaml = r#"
            target-branch: main
            draft: true
            auto-complete: true
        "#;
        let config: CreatePrConfig = serde_yaml::from_str(yaml).unwrap();
        assert!(config.draft);
        assert!(config.auto_complete);
        // When both are true, executor should skip autoCompleteSetBy (line 1396 guard)
    }

    This is optional — the actual guard behaviour isn't directly unit-testable without mocking HTTP — but it documents the "conflict" case at the config layer.

  • src/safeoutputs/create_pr.rs:2258: test_default_config_draft_true_autocomplete_false is nearly identical to the existing test at line 1982 (assert!(config.draft)). Not a problem, but worth knowing there's a small overlap.

Generated by Rust PR Reviewer for issue #200 · ● 478K ·

@jamesadevine jamesadevine merged commit d58dbeb into main Apr 14, 2026
10 of 13 checks passed
@jamesadevine jamesadevine deleted the fix/draft-autocomplete-conflict branch April 14, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

create-pr: auto-complete + draft warning should fail or skip auto-complete

1 participant