fix(scm-github): avoid conflict misclassification for draft/blocking states

[ChiragArora31]

Summary

• compute noConflicts from explicit mergeable === CONFLICTING only
• prevent blocked/draft/unknown GitHub merge states from being interpreted as merge conflicts
• add regression assertions for BLOCKED and UNKNOWN mergeability outcomes

Root cause

getMergeability() set noConflicts to true only for MERGEABLE. For draft or branch-protection-blocked PRs, GitHub can return non-MERGEABLE values without actual conflicts, which caused conflict-based reactions to trigger via !noConflicts.

What changed

• updated conflict derivation to treat only CONFLICTING as a conflict signal
• updated tests to assert non-conflicting behavior for BLOCKED/UNKNOWN states

Why this approach

It matches GitHub semantics more closely and keeps conflict reactions tied to explicit conflict data, while preserving existing blockers for review/branch-protection/draft conditions.

Testing

• pnpm --filter @aoagents/ao-plugin-scm-github test -- test/index.test.ts
• pnpm --filter @aoagents/ao-core test -- src/__tests__/lifecycle-manager.test.ts --testNamePattern "merge conflict"

Closes #1314.

[harsh-batheja]

Review

Overview

Two-line behavior fix in getMergeability() (packages/plugins/scm-github/src/index.ts:1000): noConflicts flips from "true only when mergeable === MERGEABLE" to "true unless mergeable === CONFLICTING". This stops the lifecycle conflict reaction (lifecycle-manager.ts:1117) from firing on draft / BLOCKED / UNKNOWN PRs that have no real conflict. Closes #1314.

Correctness — looks right

• Aligns with the batch path. graphql-batch.ts:791 already computes hasConflicts = mergeable === "CONFLICTING". Before this PR, the individual-call fallback in lifecycle-manager.ts:1113-1120 used stricter semantics than the batch, so behavior flipped depending on whether batch enrichment was cached. The two paths now agree — a real latent correctness win beyond the visible bug.
• Downstream consumers are safe. SessionCard.tsx:759, SessionDetail.tsx:689, web/src/lib/types.ts:188,249 only read noConflicts to gate conflict UI / attention-level. Broadening it to true for BLOCKED/UNKNOWN won't falsely mark a PR as mergeable, because mergeable = blockers.length === 0 still includes the "Merge status unknown" / "Merge is blocked" / "PR is still a draft" blockers.
• Empty-string branch. (data.mergeable ?? "").toUpperCase() → "" now yields noConflicts = true, consistent with UNKNOWN handling.

Test coverage — one gap

• Good: renamed UNKNOWN test and the new noConflicts === true assertion on the REVIEW_REQUIRED + BLOCKED case.
• Missing the literal Draft PRs can be misreported as merge conflicts #1314 repro: a draft PR (isDraft: true) with mergeStateStatus: "BLOCKED" and mergeable: "MERGEABLE" (or UNKNOWN) asserting noConflicts === true. The existing reports draft status as blocker test (line 1226) doesn't assert on noConflicts. One line there (or a dedicated test) would cement the regression guard.
• The PR description advertises a BLOCKED regression test, but what's added is really the REVIEW_REQUIRED case that happens to have a BLOCKED merge state.

Style / minor

• A one-line comment on the noConflicts line explaining why the negation form was chosen (matches graphql-batch.ts semantics; avoids false-positive conflict reactions on draft/BLOCKED) would help the next reader. Optional.
• The semantic of noConflicts drifted from "known clean" to "not explicitly conflicting." Not worth renaming — just noting.

Risks

• Low. The only way this regresses is if a caller treats noConflicts === true as "safe to merge" without also checking mergeable/blockers. None of the current callers do.
• The behavioral flip on UNKNOWN is intentional and correct: old code could spam rebase-and-resolve prompts while GitHub was still computing; new code waits for an explicit CONFLICTING signal. mergeable: false still holds via the "Merge status unknown" blocker.

Verdict

Approve with a small request: add one draft-PR regression test (isDraft: true, mergeable: "MERGEABLE", mergeStateStatus: "BLOCKED" → noConflicts: true) matching the exact #1314 scenario. Everything else is clean.

[ChiragArora31]

@harsh-batheja, I have fixed the issues; please review

[greptile-apps]

Greptile Summary

This PR fixes a bug where GitHub's BLOCKED, UNKNOWN, or empty mergeable states were incorrectly interpreted as merge conflicts by lifecycle conflict reactions, because noConflicts was derived positively (=== "MERGEABLE") rather than negatively (!== "CONFLICTING").

• index.ts: One-line change from mergeable === "MERGEABLE" to mergeable !== "CONFLICTING", so only explicit conflict data sets noConflicts = false; all other states (UNKNOWN, empty, BLOCKED) no longer masquerade as conflicts.
• index.test.ts: Flips the UNKNOWN assertion from false to true, renames the corresponding test, adds noConflicts coverage to the review-required case, and adds a dedicated regression test for draft + UNKNOWN + BLOCKED that demonstrates the pre-fix failure scenario.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted one-line fix with correct semantics and full test coverage for the regression scenario.

The fix correctly narrows the conflict signal to CONFLICTING only, preserving all other blockers through the existing blockers array. The regression test (draft + UNKNOWN + BLOCKED) now exercises exactly the scenario described in the bug report and would have failed against the old code. No other logic paths are touched.

No files require special attention.

Important Files Changed

Filename	Overview
packages/plugins/scm-github/src/index.ts	Single-line logic fix: noConflicts now derives from mergeable !== "CONFLICTING" instead of mergeable === "MERGEABLE", correctly isolating conflict detection from draft/BLOCKED/UNKNOWN GitHub states.
packages/plugins/scm-github/test/index.test.ts	Test updates flip the UNKNOWN assertion from false→true, rename the test to reflect new semantics, add a noConflicts assertion to the review-required case, and add a proper regression test for draft+UNKNOWN+BLOCKED that would have failed under old logic.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getMergeability called] --> B[Fetch PR data from GitHub]
    B --> C{mergeable value}
    C -->|CONFLICTING| D["blockers: 'Merge conflicts'\nnoConflicts = false"]
    C -->|UNKNOWN or empty| E["blockers: 'Merge status unknown'\nnoConflicts = true ✅"]
    C -->|MERGEABLE| F[noConflicts = true]
    D --> G{mergeStateStatus}
    E --> G
    F --> G
    G -->|BLOCKED| H["blockers: 'Merge is blocked by branch protection'"]
    G -->|BEHIND| I["blockers: 'Branch is behind base branch'"]
    G -->|UNSTABLE| J["blockers: 'Required checks are failing'"]
    G -->|other| K[no additional blocker]
    H --> L{isDraft?}
    I --> L
    J --> L
    K --> L
    L -->|true| M["blockers: 'PR is still a draft'"]
    L -->|false| N[return result]
    M --> N
    N --> O{blockers.length === 0?}
    O -->|yes| P[mergeable: true]
    O -->|no| Q[mergeable: false]

    style E fill:#d4edda,stroke:#28a745
    style D fill:#f8d7da,stroke:#dc3545

Loading

_{Reviews (2): Last reviewed commit: "test(scm-github): cover unknown draft me..." | Re-trigger Greptile}