fix(github-workflow): merge-readiness enforces the cross-family mandate (#17661)

ai/scripts/lifecycle/validateMergeReady.mjs

@@ -33,6 +33,9 @@ export const MERGEABLE_STATES = ['CLEAN', 'UNSTABLE'];
  *     (mergeability not yet computed) and DIRTY/BEHIND/BLOCKED all fail closed.
  *  5. `reviewRequests` is fetched AND every explicitly-requested reviewer is disposed — i.e. `reviewRequests`
  *     minus `disposedReviewers` is empty (the reviewer-contract gate).
+ *  6. `crossFamilyVerdict` is resolved AND reports `crossFamily === true` — the §6.1 cross-family mandate.
+ *     GitHub models no model family, so rule 2 cannot stand in for this one: an APPROVED badge earned
+ *     entirely within the author's own family satisfies `reviewDecision` and violates the mandate.
  *
  * Fail-CLOSED contract: `state`, `mergedAt`, `checksGreen`, `mergeStateStatus`, and `reviewRequests`
  * that were NOT fetched (`undefined`) each block readiness — an un-queried field cannot certify a
@@ -47,6 +50,7 @@ export const MERGEABLE_STATES = ['CLEAN', 'UNSTABLE'];
  * @param {String} [pr.mergeStateStatus] GitHub mergeStateStatus (`CLEAN` | `UNSTABLE` | `DIRTY` | `BEHIND` | `BLOCKED` | ...); `undefined` (not fetched) fails closed.
  * @param {String[]} [pr.reviewRequests] Logins of still-requested reviewers (the explicit author/operator contract); `undefined` (not fetched) fails closed, `[]` asserts fetched-and-empty.
  * @param {String[]} [pr.disposedReviewers] Requested reviewers already disposed (formal review / visible step-out / unrequest).
+ * @param {Object} [pr.crossFamilyVerdict] Verdict from `resolveCrossFamilyVerdict` — `{crossFamily, authorFamily, approvingFamilies, authorLogin}`. `undefined` (not resolved) fails closed; `crossFamily: null` (author family unresolved) blocks with its own reason rather than collapsing into pass or fail.
  * @param {String} [pr.approvedAtOid] Commit oid the approving review was submitted against. Optional: absent means the anchor is not reported, never that it is fresh.
  * @param {String} [pr.headRefOid] Current head oid, paired with `approvedAtOid` to surface a stale approval anchor.
  * @returns {{strictMergeReady: Boolean, blockers: String[], advisories: String[]}}
@@ -61,7 +65,8 @@ export function validateMergeReady(pr = {}) {
         reviewRequests,
         disposedReviewers = [],
         approvedAtOid,
-        headRefOid
+        headRefOid,
+        crossFamilyVerdict
     } = pr;
 
     const blockers   = [],
@@ -83,6 +88,26 @@ export function validateMergeReady(pr = {}) {
         blockers.push(`reviewDecision is '${reviewDecision ?? 'none'}', not APPROVED.`);
     }
 
+    // `reviewDecision` answers "did someone with review rights approve". It cannot answer "is the
+    // approval one our rules accept", because GitHub models no notion of model family — so no
+    // GitHub-derived field will ever express the cross-family mandate, and a validator that mirrors
+    // those fields certifies a PR the mandate forbids. Observed live: a same-family-only approval
+    // returned strictMergeReady with ZERO blockers and zero advisories, so the reader got no signal
+    // at all — not a warning, not an unknown. The operator's override stays theirs; the point of
+    // this rule is that the override is INFORMED rather than depending on a reviewer noticing.
+    //
+    // Fail CLOSED like every other predicate field: an unresolved verdict blocks. `null` is the
+    // author-not-rostered case, which is external-contributor territory rather than a mandate
+    // breach — reported as its own blocker so the reader can see WHY it could not be certified,
+    // instead of being silently folded into either boolean.
+    if (crossFamilyVerdict === undefined) {
+        blockers.push('crossFamilyVerdict was not resolved — cannot certify the cross-family review mandate; failing closed.');
+    } else if (crossFamilyVerdict?.crossFamily === null) {
+        blockers.push(`cross-family mandate could not be evaluated: the author family did not resolve${crossFamilyVerdict.authorLogin ? ` for '${crossFamilyVerdict.authorLogin}'` : ''}. An unrostered author is usually an external contributor, for whom the mandate does not apply — confirm that before merging.`);
+    } else if (crossFamilyVerdict?.crossFamily === false) {
+        blockers.push(`cross-family review mandate unsatisfied: author family '${crossFamilyVerdict.authorFamily}', approving families [${(crossFamilyVerdict.approvingFamilies || []).join(', ') || 'none'}]. pull-request-workflow.md §6.1 requires at least one APPROVED review from a different model family; GitHub's reviewDecision cannot express this, so an APPROVED badge is not evidence of it.`);
+    }
+
     // The anchor, and it is deliberately an ADVISORY rather than a blocker.
     //
     // `reviewDecision: APPROVED` says a verdict exists; it never says which commit earned it. A

ai/services/github-workflow/PullRequestService.mjs

@@ -11,6 +11,7 @@ import RepositoryService    from './RepositoryService.mjs';
 import {validateMergeReady} from '../../scripts/lifecycle/validateMergeReady.mjs';
 import {
     groupReviewsByFamily,
+    resolveCrossFamilyVerdict,
     resolveReviewerFamily
 }                           from '../graph/agentFamilyResolution.mjs';
 import {
@@ -421,7 +422,10 @@ function normalizeMergeReadinessSnapshot(pullRequest) {
     const reviewsConnection = pullRequest.reviews;
     const approvals         = (reviewsConnection?.nodes || [])
         .filter(node => node?.state === 'APPROVED' && node?.commit?.oid && node?.submittedAt)
-        .map(node => ({oid: node.commit.oid, submittedAt: node.submittedAt}))
+        // `login` rides along because the cross-family mandate is a question about WHO approved,
+        // and this map is the only place the reviewer identity survives into the snapshot. It is
+        // drift-safe: a login is a fixed property of a node whose arrival already moves this array.
+        .map(node => ({oid: node.commit.oid, submittedAt: node.submittedAt, login: node.author?.login ?? null}))
         // Sorted rather than trusting connection order: the caller reads `.at(-1)` as "latest", and
         // an ordering assumption that holds today would fail silently — as a WRONG anchor, not a
         // missing one. `oid` breaks ties so two approvals sharing a timestamp stay deterministic
@@ -443,6 +447,7 @@ function normalizeMergeReadinessSnapshot(pullRequest) {
         headRefOid      : pullRequest.headRefOid,
         mergeStateStatus: pullRequest.mergeStateStatus,
         reviewDecision  : pullRequest.reviewDecision,
+        authorLogin     : pullRequest.author?.login ?? null,
         reviewRequests  : {
             available  : Boolean(reviewConnection && Array.isArray(reviewConnection.nodes)),
             hasNextPage: Boolean(reviewConnection?.pageInfo?.hasNextPage),
@@ -774,13 +779,31 @@ async function buildMergeReadinessProjection({
     const approvedAtOid = snapshot.approvals.available
         ? snapshot.approvals.nodes.at(-1)?.oid
         : undefined;
+    // The §6.1 mandate, resolved here rather than inside the predicate so `validateMergeReady`
+    // stays a pure function over primitives. `approvals.nodes` is already filtered to APPROVED
+    // upstream, so re-stamping the state is a shape adapter, not a second filter.
+    //
+    // Unavailable approvals yield `undefined`, NOT an empty verdict: "the reviews were not fetched"
+    // and "nobody from another family approved" are different facts, and only the first should read
+    // as an unresolved gate. Collapsing them would report a mandate breach for a connection error.
+    const crossFamilyVerdict = snapshot.approvals.available
+        ? {
+            ...resolveCrossFamilyVerdict({
+                author : {login: snapshot.authorLogin},
+                reviews: snapshot.approvals.nodes.map(node => ({state: 'APPROVED', author: {login: node.login}}))
+            }),
+            authorLogin: snapshot.authorLogin
+        }
+        : undefined;
+
     const predicate = validateMergeReady({
         state           : snapshot.state,
         mergedAt        : snapshot.mergedAt,
         reviewDecision  : snapshot.reviewDecision,
         checksGreen,
         mergeStateStatus: snapshot.mergeStateStatus,
         reviewRequests,
+        crossFamilyVerdict,
         approvedAtOid,
         headRefOid      : snapshot.headRefOid
     });

ai/services/github-workflow/queries/pullRequestQueries.mjs

@@ -72,6 +72,9 @@ export const GET_MERGE_READINESS = `
         headRefOid
         mergeStateStatus
         reviewDecision
+        author {
+          login
+        }
         reviewRequests(first: 100) {
           pageInfo {
             hasNextPage
@@ -99,6 +102,9 @@ export const GET_MERGE_READINESS = `
           nodes {
             state
             submittedAt
+            author {
+              login
+            }
             commit {
               oid
             }

ai/services/graph/agentFamilyResolution.mjs

@@ -255,16 +255,64 @@ export function groupReviewsByFamily(reviews = [], agentFamilies = getCoreSwarmA
  * @returns {Boolean}
  */
 export function hasCrossFamilyReview(pr, agentFamilies = getCoreSwarmAgentFamilies()) {
-    const authorFamily = resolveAuthorFamily(pr, agentFamilies);
-    const reviews      = Array.isArray(pr.reviews) ? pr.reviews : [];
-
-    return reviews.some(review => {
-        const reviewerLogin  = review.author?.login || review.author?.name || review.author?.login;
-        const reviewerFamily = agentFamilies[reviewerLogin];
-
-        if (!reviewerFamily) return false;
-        if (!authorFamily) return true;
+    const verdict = resolveCrossFamilyVerdict(pr, agentFamilies);
+
+    // `null` (author family unresolvable) maps to TRUE here, preserving this function's original
+    // reading: an unrostered author is an EXTERNAL contributor, and the mandate exists to stop one
+    // model family self-approving — a risk an external human's PR does not carry. That is why any
+    // classified approver satisfies it. The verdict form below keeps `null` distinct so a gate can
+    // decide for itself rather than inheriting a report's charity.
+    return verdict.crossFamily === null
+        ? verdict.approvingFamilies.length > 0
+        : verdict.crossFamily
+}
 
-        return reviewerFamily !== authorFamily
-    })
+/**
+ * @summary The cross-family mandate as a VERDICT rather than a boolean — the shape a merge gate needs.
+ *
+ * `hasCrossFamilyReview` above answers a report line, where an optimistic guess costs a wrong word.
+ * A gate cannot spend the same optimism, and the two differences are load-bearing:
+ *
+ * **1. Only an APPROVED review counts.** The mandate is "at least one cross-family *Approved*
+ * review", so a cross-family `COMMENT` or `CHANGES_REQUESTED` is not coverage — it is the opposite
+ * of coverage in the `CHANGES_REQUESTED` case. Reviews arrive in every state on the same
+ * connection, so the state filter is the difference between counting approvals and counting
+ * attention.
+ *
+ * **2. An unresolvable author family reports `null`, not a verdict.** The boolean form treats that
+ * case as satisfied, and deliberately so — an unrostered author is an external contributor, and the
+ * mandate exists to stop one model family self-approving, which is not a risk an external human's
+ * PR carries. That charity is right for a report and is not a decision this function should make
+ * for a caller. `null` is deliberately neither `true` nor `false`: "the author is not one of ours,
+ * so the mandate may not even apply" is a third state, and a consumer that collapses it into either
+ * boolean has silently chosen a policy. The gate decides; the resolver reports.
+ *
+ * The approving families are returned alongside the verdict because a blocker that cannot name who
+ * approved and what family they belong to sends the reader back to the API to find out.
+ *
+ * @param {Object} pr GitHub PR payload (`author`, `body`, `reviews`).
+ * @param {Object} [agentFamilies=getCoreSwarmAgentFamilies()] Login-to-family map.
+ * @returns {{crossFamily: (Boolean|null), authorFamily: (String|null), approvingFamilies: String[], unclassifiedApprovers: String[]}}
+ */
+export function resolveCrossFamilyVerdict(pr, agentFamilies = getCoreSwarmAgentFamilies()) {
+    const
+        authorFamily = resolveAuthorFamily(pr, agentFamilies) ?? null,
+        reviews      = Array.isArray(pr?.reviews) ? pr.reviews : [],
+        approvals    = reviews.filter(review => review?.state === 'APPROVED'),
+        resolved     = approvals.map(review => resolveReviewerFamily(review, agentFamilies)),
+
+        approvingFamilies     = [...new Set(resolved.filter(item => item.classified).map(item => item.family))],
+        unclassifiedApprovers = resolved.filter(item => !item.classified).map(item => item.login).filter(Boolean);
+
+    return {
+        authorFamily,
+        approvingFamilies,
+        unclassifiedApprovers,
+        // Order matters: an unknown author short-circuits BEFORE the comparison, because comparing
+        // against `null` would silently make every classified approver look cross-family — the same
+        // fail-open the boolean form takes deliberately and a gate must not.
+        crossFamily: authorFamily === null
+            ? null
+            : approvingFamilies.some(family => family !== authorFamily)
+    }
 }

test/playwright/unit/ai/scripts/lifecycle/validateMergeReady.spec.mjs

@@ -11,7 +11,15 @@ import * as core            from '../../../../../../src/core/_export.mjs';
 import {validateMergeReady} from '../../../../../../ai/scripts/lifecycle/validateMergeReady.mjs';
 
 test.describe('validateMergeReady — strict merge-readiness contract', () => {
-    const openSource = overrides => ({state: 'OPEN', mergedAt: null, ...overrides});
+    // A healthy default for the §6.1 mandate, so arms about OTHER rules stay about those rules.
+    // The gate itself is fail-closed on an unresolved verdict, which its own arms assert explicitly
+    // rather than relying on this builder's silence.
+    const openSource = overrides => ({
+        state             : 'OPEN',
+        mergedAt          : null,
+        crossFamilyVerdict: {crossFamily: true, authorFamily: 'claude', approvingFamilies: ['gpt'], authorLogin: 'neo-opus-grace'},
+        ...overrides
+    });
 
     test('a fully-disposed approved PR is strict-merge-ready', () => {
         const result = validateMergeReady(openSource({
@@ -136,7 +144,15 @@ test.describe('validateMergeReady — strict merge-readiness contract', () => {
 });
 
 test.describe('validateMergeReady — the approval anchor', () => {
-    const openSource = overrides => ({state: 'OPEN', mergedAt: null, ...overrides});
+    // A healthy default for the §6.1 mandate, so arms about OTHER rules stay about those rules.
+    // The gate itself is fail-closed on an unresolved verdict, which its own arms assert explicitly
+    // rather than relying on this builder's silence.
+    const openSource = overrides => ({
+        state             : 'OPEN',
+        mergedAt          : null,
+        crossFamilyVerdict: {crossFamily: true, authorFamily: 'claude', approvingFamilies: ['gpt'], authorLogin: 'neo-opus-grace'},
+        ...overrides
+    });
 
     test('a stale anchor is REPORTED, never a blocker', () => {
         const result = validateMergeReady(openSource({
@@ -207,3 +223,72 @@ test.describe('validateMergeReady — the approval anchor', () => {
         expect(result.advisories).toHaveLength(1)
     })
 });
+
+/**
+ * @summary The §6.1 cross-family mandate, which no GitHub field can express.
+ *
+ * The anchor is a live incident rather than a constructed case: a real pull request carried exactly
+ * the values below and returned `strictMergeReady: true` with ZERO blockers and zero advisories,
+ * for a PR whose only approval came from the author's own model family. These arms are red without
+ * the sixth rule.
+ */
+test.describe('validateMergeReady — the cross-family mandate', () => {
+    // Deliberately NOT the shared `openSource` builder: these arms are about the verdict field, so
+    // inheriting a healthy default would be the one thing that makes them vacuous.
+    const green = overrides => ({
+        state           : 'OPEN',
+        mergedAt        : null,
+        reviewDecision  : 'APPROVED',
+        checksGreen     : true,
+        mergeStateStatus: 'CLEAN',
+        reviewRequests  : [],
+        ...overrides
+    });
+
+    test('a same-family-only approval is NOT strict-merge-ready', () => {
+        const result = validateMergeReady(green({
+            crossFamilyVerdict: {crossFamily: false, authorFamily: 'claude', approvingFamilies: ['claude'], authorLogin: 'neo-opus-vega'}
+        }));
+
+        expect(result.strictMergeReady).toBe(false);
+        // The blocker must name both families — a reader who cannot see WHO approved has to go back
+        // to the API to find out why their green PR is blocked.
+        expect(result.blockers.some(entry => entry.includes("author family 'claude'") && entry.includes('[claude]'))).toBe(true);
+    });
+
+    test('a genuinely cross-family approval passes, on the same otherwise-identical surface', () => {
+        const result = validateMergeReady(green({
+            crossFamilyVerdict: {crossFamily: true, authorFamily: 'claude', approvingFamilies: ['gpt'], authorLogin: 'neo-opus-vega'}
+        }));
+
+        expect(result.strictMergeReady).toBe(true);
+        expect(result.blockers).toEqual([]);
+    });
+
+    test('a THIRD family satisfies it — the mandate is cross-family, not one specific other family', () => {
+        const result = validateMergeReady(green({
+            crossFamilyVerdict: {crossFamily: true, authorFamily: 'gpt', approvingFamilies: ['gemini'], authorLogin: 'neo-gpt-emmy'}
+        }));
+
+        expect(result.strictMergeReady).toBe(true);
+    });
+
+    test('an UNRESOLVED verdict fails closed, like every other predicate field', () => {
+        const result = validateMergeReady(green());
+
+        expect(result.strictMergeReady).toBe(false);
+        expect(result.blockers.some(entry => entry.includes('was not resolved'))).toBe(true);
+    });
+
+    test('an unrostered author blocks with its OWN reason, not a mandate-breach claim', () => {
+        const result = validateMergeReady(green({
+            crossFamilyVerdict: {crossFamily: null, authorFamily: null, approvingFamilies: ['claude'], authorLogin: 'external-dev'}
+        }));
+
+        expect(result.strictMergeReady).toBe(false);
+        // Three states, three messages. Reporting "mandate unsatisfied" for an external contributor
+        // would send the reader hunting for a reviewer who was never required.
+        expect(result.blockers.some(entry => entry.includes('could not be evaluated') && entry.includes('external-dev'))).toBe(true);
+        expect(result.blockers.some(entry => entry.includes('mandate unsatisfied'))).toBe(false);
+    })
+});