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,9 @@ import RepositoryService    from './RepositoryService.mjs';
 import {validateMergeReady} from '../../scripts/lifecycle/validateMergeReady.mjs';
 import {
     groupReviewsByFamily,
+    parseSelfIdLogin,
+    resolveAuthorFamilyFromLogins,
+    resolveCrossFamilyVerdict,
     resolveReviewerFamily
 }                           from '../graph/agentFamilyResolution.mjs';
 import {
@@ -421,7 +424,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 +449,13 @@ function normalizeMergeReadinessSnapshot(pullRequest) {
         headRefOid      : pullRequest.headRefOid,
         mergeStateStatus: pullRequest.mergeStateStatus,
         reviewDecision  : pullRequest.reviewDecision,
+        authorLogin     : pullRequest.author?.login ?? null,
+        // The DERIVED self-id, never the body itself. This snapshot is compared by
+        // `stableStringify` across two reads to detect source drift, so carrying the raw `body`
+        // would make any prose edit mid-read invalidate the observation. Parsing once and keeping
+        // only the declared author login points the drift signal at what matters: a change to the
+        // declared author invalidates the read, a typo fix in the description does not.
+        authorSelfIdLogin: parseSelfIdLogin(pullRequest.body ?? '') ?? null,
         reviewRequests  : {
             available  : Boolean(reviewConnection && Array.isArray(reviewConnection.nodes)),
             hasNextPage: Boolean(reviewConnection?.pageInfo?.hasNextPage),
@@ -774,13 +787,43 @@ 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.
+    //
+    // The author family resolves from the CANONICAL self-id first, opener login only as fallback.
+    // The GitHub opener can mis-resolve — an MCP `@me` drift stamps a different agent's login on the
+    // PR — and a gate reading the opener would let that drift decide merge eligibility: a body
+    // declaring a Claude author, opened under a GPT login, would certify on a same-family approval.
+    //
+    // `approvalsTruncated` rides along because the connection is bounded: a positive witness inside
+    // the window is decisive, a negative over a truncated one is missing evidence.
+    const crossFamilyVerdict = snapshot.approvals.available
+        ? {
+            ...resolveCrossFamilyVerdict({
+                authorFamily      : resolveAuthorFamilyFromLogins({
+                    selfIdLogin: snapshot.authorSelfIdLogin,
+                    openerLogin: snapshot.authorLogin
+                }) ?? null,
+                approvalsTruncated: snapshot.approvals.hasPreviousPage,
+                reviews           : snapshot.approvals.nodes.map(node => ({state: 'APPROVED', author: {login: node.login}}))
+            }),
+            authorLogin: snapshot.authorSelfIdLogin || 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

@@ -49,12 +49,20 @@ export const GET_CONVERSATION = `
  * it feeds the approval ANCHOR — which commit earned `reviewDecision: APPROVED` — and only the most
  * recent approval can answer that. Fetching the oldest 100 would truncate away exactly the reviews
  * the question is about. `hasPreviousPage` is fetched to bound the connection and **deliberately
- * neither gates nor surfaces**: an approval found inside the most-recent window IS the latest one
- * however many older reviews exist, and an empty window already yields silence rather than a claim —
- * so truncation cannot change any decision this query feeds. It is carried on the normalized
- * snapshot so the bound is visible to a maintainer reading the shape, and for no other reason; do
- * not add a consumer that treats it as evidence, and do not describe it as "reported" to a caller
- * who has no way to see it.
+ * neither gates nor surfaces the APPROVAL ANCHOR**: an approval found inside the most-recent window
+ * IS the latest one however many older reviews exist, so truncation cannot change the anchor.
+ *
+ * It DOES gate the cross-family mandate, and that is a later addition rather than an exception. The
+ * mandate asks whether ANY approval came from a differing family, which is a question about the
+ * whole population rather than the most recent member: a qualifying older approval can sit outside
+ * the retained suffix. So a positive witness inside the window is decisive, while a negative over a
+ * truncated connection is missing evidence rather than evidence of absence, and degrades to an
+ * unresolved verdict the consumer fails closed on.
+ *
+ * The distinction is worth holding: the same flag is inert for one consumer and load-bearing for
+ * another, because "the latest approval" and "any approval" are different questions over the same
+ * bounded list. This paragraph used to say truncation could change no decision this query feeds —
+ * true until the mandate became one of them.
  *
  * Variables required:
  * - $owner: String! - Repository owner
@@ -72,6 +80,10 @@ export const GET_MERGE_READINESS = `
         headRefOid
         mergeStateStatus
         reviewDecision
+        body
+        author {
+          login
+        }
         reviewRequests(first: 100) {
           pageInfo {
             hasNextPage
@@ -99,6 +111,9 @@ export const GET_MERGE_READINESS = `
           nodes {
             state
             submittedAt
+            author {
+              login
+            }
             commit {
               oid
             }