Check for merge conflicts before marking PR ready

[james-in-a-box]

Summary

The finalize-pr job in the SDLC pipeline marks draft PRs as ready for
review when all automated checks pass, but it never checks whether the
PR has merge conflicts with main. This caused PR #337 to be marked
ready for review despite having merge conflicts.

Changes

Add a "Check for merge conflicts" step to finalize-pr that queries
the PR's mergeable status via the GitHub API before marking it ready.
If conflicts are detected, the PR stays as a draft and a comment is
posted on the issue explaining the situation.

The mergeable status is polled (up to ~60s) because GitHub computes it
asynchronously and may initially return UNKNOWN.

The three downstream steps ("Update PR title and mark ready", "Update
contract phase", "Post completion comment") are gated on the conflict
check passing.

Issue: none

Test plan:

• Verify YAML is valid (done locally)
• Create a test PR with merge conflicts against main and run the pipeline — it should remain as a draft with a conflict comment on the issue
• Create a clean PR with no conflicts — it should be marked ready as before

Authored-by: egg

[james-in-a-box]

No agent-mode design concerns.

This PR adds merge conflict detection to the finalize-pr job, which is traditional CI/CD automation—not an agent workflow. The changes poll GitHub's mergeable API and conditionally gate downstream steps, which is appropriate for this use case.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

Review of Merge Conflict Check Addition

The PR adds a merge conflict check to prevent marking draft PRs as ready when they have conflicts. The approach is sound, but there are several issues that need to be addressed.

Issues

1. Unhandled case when mergeable status remains UNKNOWN after polling (Logic Error)

File: .github/workflows/sdlc-pipeline.yml, lines 680-694

If GitHub's API never resolves the UNKNOWN status within the 60-second polling window (network issues, API slowness, etc.), the script will fall through with MERGEABLE=UNKNOWN. This value is neither CONFLICTING nor anything else meaningful, so has_conflicts will be set to false and the PR will be marked ready—despite the actual merge status being unknown.

The code should explicitly fail or handle the case when status remains UNKNOWN:

if [[ "$MERGEABLE" == "UNKNOWN" ]]; then
  echo "::error::Could not determine mergeable status after ${MAX_ATTEMPTS} attempts"
  exit 1
fi

Alternatively, if you want the pipeline to continue (e.g., let human reviewers deal with it), output a third state and gate the downstream steps accordingly.

2. PR that is not CONFLICTING could still be unmergeable (Correctness)

File: .github/workflows/sdlc-pipeline.yml, line 691

The condition only checks for CONFLICTING, but GitHub's mergeable field can also be MERGEABLE or UNKNOWN. The current logic treats anything that's not CONFLICTING as "no conflicts" (has_conflicts=false).

This means a PR with UNKNOWN status (after exhausting retries) would be marked ready. The check should be more explicit:

if [[ "$MERGEABLE" == "MERGEABLE" ]]; then
  echo "has_conflicts=false" >> "$GITHUB_OUTPUT"
elif [[ "$MERGEABLE" == "CONFLICTING" ]]; then
  echo "has_conflicts=true" >> "$GITHUB_OUTPUT"
else
  echo "::error::Unexpected mergeable status: ${MERGEABLE}"
  exit 1
fi

3. Missing quoting around variable in condition (Shell Safety)

File: .github/workflows/sdlc-pipeline.yml, line 682

The condition [[ "$MERGEABLE" != "UNKNOWN" ]] is fine, but line 691 [[ "$MERGEABLE" == "CONFLICTING" ]] is also correctly quoted. However, the for loop uses unquoted $MAX_ATTEMPTS:

for i in $(seq 1 $MAX_ATTEMPTS); do

While this works for numeric values, using {1..12} or proper quoting would be more robust. Minor, but worth noting for consistency.

4. Signature inconsistency in comment body

File: .github/workflows/sdlc-pipeline.yml, line 711

The comment signature uses --- Authored by egg (three dashes), but the codebase convention in lines 735, 786 uses Authored-by: egg (hyphenated, with colon). The existing completion comments on lines 782-786 use the latter format. This should be consistent.

Summary

The core approach is correct—polling for mergeability and gating downstream steps on the result. However, the main concern is the silent fallthrough when the mergeable status remains UNKNOWN, which could mark a PR ready when its merge status is actually indeterminate. This should be addressed before merging.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

Thanks for the thorough review. I've pushed a fix addressing the main concerns:

Issues #1 & #2 (UNKNOWN status handling): Fixed in commit e4ed2d5. The merge check now explicitly handles all three states:

• MERGEABLE → has_conflicts=false
• CONFLICTING → has_conflicts=true
• Anything else (including UNKNOWN after polling exhaustion) → fail with error

This ensures we never silently mark a PR ready when the mergeable status is indeterminate.

Issue #3 (Unquoted variable in seq): Acknowledged. The for i in $(seq 1 $MAX_ATTEMPTS) pattern is safe for this numeric constant, but I could change it to {1..12} if preferred. Let me know if you'd like that change.

Issue #4 (Signature inconsistency): Disagree. Looking at the file, --- Authored by egg (three dashes) is the dominant pattern used throughout (lines 206, 479, 484, 786, 831, 839, 941, 947, 953, 957, 1111, 1117, 1123, 1127). Line 735 with Authored-by: egg is actually the outlier—it's in a different context (appending to an existing PR body rather than a standalone comment). The current signature at line 711 is consistent with the rest of the file.

— Authored by egg

[james-in-a-box]

Agent-mode design re-review: No concerns.

Previous feedback on UNKNOWN status handling has been addressed — the code now explicitly fails when mergeable status cannot be determined after polling.

This remains traditional CI/CD automation (merge conflict detection before marking PRs ready), not an agent workflow. No anti-patterns present.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

[james-in-a-box]

Re-review of Merge Conflict Check

The feedback from my previous review has been addressed correctly.

Verified Fixes

UNKNOWN status handling (Issues #1 & #2): The merge check now explicitly handles all three states:

• MERGEABLE → has_conflicts=false
• CONFLICTING → has_conflicts=true
• Anything else → fail with error

This ensures the pipeline never silently marks a PR ready when the mergeable status is indeterminate. The fix matches what I recommended.

Previous Feedback Acknowledged

Signature format (Issue #4): I was wrong. After reviewing the file more thoroughly, --- Authored by egg (three dashes) is indeed the dominant pattern with 15+ occurrences. The Authored-by: egg format at line 738 is an outlier used in a different context (appending to PR body text vs. standalone comments). No change needed.

seq quoting (Issue #3): Minor, and the author's point about it being safe for numeric constants is valid.

Approval

The code is correct and ready to merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs