diff --git a/ai/scripts/lifecycle/validateMergeReady.mjs b/ai/scripts/lifecycle/validateMergeReady.mjs index a247d01361..21ded31090 100644 --- a/ai/scripts/lifecycle/validateMergeReady.mjs +++ b/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 diff --git a/ai/services/github-workflow/PullRequestService.mjs b/ai/services/github-workflow/PullRequestService.mjs index a2b7a6d578..89341fb158 100644 --- a/ai/services/github-workflow/PullRequestService.mjs +++ b/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,6 +787,35 @@ 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, @@ -781,6 +823,7 @@ async function buildMergeReadinessProjection({ checksGreen, mergeStateStatus: snapshot.mergeStateStatus, reviewRequests, + crossFamilyVerdict, approvedAtOid, headRefOid : snapshot.headRefOid }); diff --git a/ai/services/github-workflow/queries/pullRequestQueries.mjs b/ai/services/github-workflow/queries/pullRequestQueries.mjs index 7778221b58..06035b352c 100644 --- a/ai/services/github-workflow/queries/pullRequestQueries.mjs +++ b/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 } diff --git a/ai/services/graph/agentFamilyResolution.mjs b/ai/services/graph/agentFamilyResolution.mjs index 757cf1bed5..aa58260f0d 100644 --- a/ai/services/graph/agentFamilyResolution.mjs +++ b/ai/services/graph/agentFamilyResolution.mjs @@ -14,6 +14,23 @@ import logger from '../../mcp/server/memory-core/logger.mjs'; * `GoldenPathSynthesizer` keeps thin static delegating shims so its public API stays stable. */ +/** + * The roster's family value for a seat whose underlying model is not publicly known — an unreleased + * preview behind a codename, where the seat itself cannot state its engine. + * + * **It COUNTS as differing for the cross-family mandate. Operator ruling, 2026-08-24.** That is a + * decision, not a consequence of string comparison, and it is recorded here because the next reader + * would otherwise re-derive it from `'unknown' !== 'claude'` and be right by accident. + * + * The trade, stated so nobody has to rediscover it: an `unknown` family cannot be SHOWN uncorrelated + * with the author's, so admitting it assumes part of what the mandate checks. It was weighed against + * the alternative — a guest seat whose approvals can never unblock anything is a seat with no + * merge-path value — and for a Claude-family author it is the difference between two eligible + * cross-family seats and three. The permissive reading won on that basis. + * @type {String} + */ +export const UNKNOWN_FAMILY = 'unknown'; + /** * Social Name → `@`-stripped GitHub login, derived from the canonical identity roster. The PR-body * self-id leads with the Social Name (`Authored by (…)`); this resolves it to the login @@ -158,6 +175,44 @@ export function parseSelfIdLogin(body) { return socialMatch ? (SOCIAL_NAME_TO_LOGIN[socialMatch[1].trim()] ?? null) : null } +/** + * @summary Resolves an author family from ALREADY-PARSED logins, so a caller can carry the canonical + * self-id without carrying the body it came from. + * + * {@link resolveAuthorFamily} takes a PR and parses `body` itself, which is right for a caller that + * already holds the body. The merge-readiness projection does not: its snapshot is compared by + * `stableStringify` across two reads to detect source drift, so putting the raw `body` on it would + * make **any** prose edit mid-read invalidate the observation. Parsing the self-id once and carrying + * only the derived login keeps the drift signal pointed at what actually matters — a change to the + * declared author invalidates it, a typo fix in the description does not. + * + * Precedence is identical to the body-parsing form and that is the whole point: the declared self-id + * WINS over the opener login, because the GitHub opener can mis-resolve (an MCP `@me` drift stamps a + * different agent's login on the PR) while the body declares its own canonical author. A gate that + * read the opener would let that drift decide whether a merge is eligible. + * + * @param {Object} logins + * @param {String|null} [logins.selfIdLogin] `@`-stripped login parsed from the body self-id. + * @param {String|null} [logins.openerLogin] `@`-stripped GitHub opener login (advisory). + * @param {Object} [agentFamilies=getCoreSwarmAgentFamilies()] Login-to-family map. + * @returns {(String|undefined)} The model family, or undefined when neither login resolves. + */ +export function resolveAuthorFamilyFromLogins({selfIdLogin = null, openerLogin = null} = {}, agentFamilies = getCoreSwarmAgentFamilies()) { + const + selfIdFamily = selfIdLogin ? agentFamilies[selfIdLogin] : undefined, + openerFamily = openerLogin ? agentFamilies[openerLogin] : undefined; + + if (selfIdFamily) { + if (openerFamily && openerFamily !== selfIdFamily) { + logger.warn(`[agentFamilyResolution] author identity drift — body self-id @${selfIdLogin} (${selfIdFamily}) != opener @${openerLogin} (${openerFamily}); using the canonical self-id.`); + } + + return selfIdFamily + } + + return openerFamily +} + /** * @summary Resolves a PR author's model family from the canonical body self-id (Social-Name-led, or * legacy `@identity`), falling back to the drift-prone GitHub login as an advisory source. @@ -255,16 +310,83 @@ 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 + // A caller that already resolved the canonical author passes it directly; everyone else + // keeps the body-parsing path. Same precedence either way — the self-id wins over the opener. + authorFamily = (pr?.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)), + + // `UNKNOWN_FAMILY` participates as an ordinary family value — see its declaration for the + // operator ruling and the trade behind it. Deliberately NOT filtered out: an earlier cut of + // this function treated it as unresolved, which was the honest-but-costly reading and is not + // the one the swarm chose. + approvingFamilies = [...new Set(resolved.filter(item => item.classified).map(item => item.family))], + unclassifiedApprovers = resolved.filter(item => !item.classified).map(item => item.login).filter(Boolean); + + const crossFamily = authorFamily === null + ? null + : approvingFamilies.some(family => family !== authorFamily); + + return { + authorFamily, + approvingFamilies, + unclassifiedApprovers, + approvalsTruncated: Boolean(pr?.approvalsTruncated), + // A POSITIVE witness is decisive; a NEGATIVE over a truncated window is not a negative. + // + // The approvals connection is bounded (`reviews(last: 100)`), so a qualifying older approval + // can sit outside the retained suffix. Finding a cross-family approver inside the window + // settles the question whatever lies beyond it — one witness is enough. Finding none does + // NOT, because the witness may simply be off the end: that is missing evidence, not evidence + // of absence, and reporting it as `false` would state a fact the data cannot support. + // + // So truncation degrades a negative to `null`, which the consumer already fails closed on + // with its own message. An unknown AUTHOR short-circuits before any of it: comparing against + // `null` would make every classified approver look cross-family. + crossFamily: authorFamily === null || (crossFamily === false && Boolean(pr?.approvalsTruncated)) + ? null + : crossFamily + } } diff --git a/test/playwright/unit/ai/scripts/lifecycle/validateMergeReady.spec.mjs b/test/playwright/unit/ai/scripts/lifecycle/validateMergeReady.spec.mjs index 3c6d89decd..fde363509b 100644 --- a/test/playwright/unit/ai/scripts/lifecycle/validateMergeReady.spec.mjs +++ b/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); + }) +}); diff --git a/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs b/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs index d8883dc9df..ac4b8cdc6b 100644 --- a/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs +++ b/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs @@ -387,7 +387,14 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read reviewers = [], // `null` models a connection GitHub did not return at all — the silence case — which is // distinct from an empty node list (fetched, no approvals). - reviews = [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}}], + // Rostered logins, and DIFFERENT families on purpose: the default fixture models a healthy + // PR, and after the §6.1 rule a same-family default would block every arm here for a reason + // none of them is about. `neo-opus-vega` is claude, `neo-gpt-emmy` is gpt. + authorLogin = 'neo-opus-vega', + // Default body carries a self-id matching the opener, so arms about other things are not + // silently exercising author drift. + body = 'Authored by Vega (Claude Opus 5, Claude Code).', + reviews = [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-gpt-emmy'}}], reviewsHasPreviousPage = false, state = 'OPEN' } = {}) => ({ @@ -398,6 +405,8 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read headRefOid, mergeStateStatus, reviewDecision, + author : authorLogin === null ? null : {login: authorLogin}, + body, reviewRequests: { pageInfo: {hasNextPage: reviewHasNextPage, endCursor: null}, nodes : reviewers.map(login => ({ @@ -513,7 +522,7 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read const moved = () => pullRequest({ checkCommit: NEXT_HEAD, headRefOid : NEXT_HEAD, - reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}}] + reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-gpt-emmy'}}] }); const result = await project(dependencies({snapshots: [moved(), moved()]})); @@ -576,11 +585,69 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read const deps = dependencies({snapshots: [pullRequest({reviews: null}), pullRequest({reviews: null})]}); const result = await project(deps); - // The inverse of this module's fail-closed rule, and deliberately so: the anchor certifies - // nothing, so a caller that never asked for it is not making a weaker claim. What must NOT - // happen is an advisory asserting freshness it never observed. - expect(result.verdict).toBe('merge-ready-observed'); + // The arm's subject, unchanged: the anchor certifies nothing, so a caller that never asked + // for it is not making a weaker claim, and what must NOT happen is an advisory asserting + // freshness it never observed. expect(result.predicate.advisories).toEqual([]); + + // The verdict, however, DID move with the §6.1 rule, and the two facts sit either side of + // this module's fail-closed line. The same unfetched connection that leaves the anchor + // silent also means nobody can see WHO approved — and the cross-family mandate is a + // predicate field, not a reporting channel, so it must block rather than certify. Asserting + // the reason as well as the outcome, so a future change cannot flip this back by accident. + expect(result.verdict).not.toBe('merge-ready-observed'); + expect(result.predicate.blockers.some(entry => entry.includes('cross-family review mandate'))).toBe(true); + }); + + test('the CANONICAL body author wins over the opener login — drift cannot certify a merge', async () => { + // The failure this arm exists for: an MCP `@me` drift stamps a different agent's login on + // the PR. Body declares Grace (claude); the opener resolves to Emmy (gpt); the only approver + // is Vega (claude). Reading the OPENER gives gpt-vs-claude and certifies. Reading the body + // gives claude-vs-claude and blocks, which is the truth about who wrote it. + const deps = dependencies({snapshots: [ + pullRequest({ + authorLogin: 'neo-gpt-emmy', + body : 'Authored by Grace (Claude Opus 5, Claude Code).', + reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-opus-vega'}}] + }), + pullRequest({ + authorLogin: 'neo-gpt-emmy', + body : 'Authored by Grace (Claude Opus 5, Claude Code).', + reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-opus-vega'}}] + }) + ]}); + const result = await project(deps); + + expect(result.predicate.strictMergeReady).toBe(false); + expect(result.predicate.blockers.some(entry => entry.includes('cross-family review mandate unsatisfied'))).toBe(true); + }); + + test('a truncated approvals window turns a NEGATIVE into unresolved, never a factual unsatisfied', async () => { + // `reviews(last: 100)` is a suffix. With `hasPreviousPage: true`, a qualifying older approval + // may sit outside it — so "no cross-family approver here" is missing evidence, not evidence + // of absence. It must block, but with the could-not-evaluate reason, because the factual + // message would assert something the data cannot support. + const truncated = () => pullRequest({ + reviewsHasPreviousPage: true, + reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-opus-ada'}}] + }); + const result = await project(dependencies({snapshots: [truncated(), truncated()]})); + + expect(result.predicate.strictMergeReady).toBe(false); + expect(result.predicate.blockers.some(entry => entry.includes('could not be evaluated'))).toBe(true); + expect(result.predicate.blockers.some(entry => entry.includes('mandate unsatisfied'))).toBe(false); + }); + + test('a POSITIVE cross-family witness inside a truncated window is still decisive', async () => { + // The control that stops truncation-awareness from collapsing into "bounded means unknown": + // one qualifying witness settles the question however many approvals lie beyond the suffix. + const witnessed = () => pullRequest({ + reviewsHasPreviousPage: true, + reviews : [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-gpt-emmy'}}] + }); + const result = await project(dependencies({snapshots: [witnessed(), witnessed()]})); + + expect(result.predicate.blockers.filter(entry => entry.includes('cross-family'))).toEqual([]); }); test('#16902: query carries exact workflow-run coordinates instead of inferring attempts by job name', () => { diff --git a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs index 13a564f198..698976e77c 100644 --- a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs +++ b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs @@ -20,6 +20,7 @@ import fs from 'fs'; import path from 'path'; import os from 'os'; import child_process from 'child_process'; +import {resolveCrossFamilyVerdict} from '../../../../../../ai/services/graph/agentFamilyResolution.mjs'; import {TestLifecycleHelper} from '../../services/memory-core/util.mjs'; test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { @@ -267,7 +268,9 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { test('hasCrossFamilyReview accepts injected identity-family maps', () => { const pr = { author : {login: 'author-agent'}, - reviews: [{author: {login: 'reviewer-agent'}}] + // `state` is explicit because coverage now means an APPROVED review, not any review. + // The fixture predates that distinction; the arm's subject is the injected map. + reviews: [{author: {login: 'reviewer-agent'}, state: 'APPROVED'}] }; expect(Synthesizer.hasCrossFamilyReview(pr, { @@ -281,6 +284,75 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { })).toBe(true); }); + test('cross-family coverage requires an APPROVED review — attention is not coverage', () => { + const families = {'author-agent': 'gpt', 'reviewer-agent': 'claude'}; + + // Same cross-family reviewer, three states. Only one of them is coverage under the mandate, + // and CHANGES_REQUESTED is the case where counting any state would be actively backwards. + for (const state of ['COMMENTED', 'CHANGES_REQUESTED']) { + const pr = {author: {login: 'author-agent'}, reviews: [{author: {login: 'reviewer-agent'}, state}]}; + + expect(Synthesizer.hasCrossFamilyReview(pr, families), state).toBe(false); + } + + const approved = {author: {login: 'author-agent'}, reviews: [{author: {login: 'reviewer-agent'}, state: 'APPROVED'}]}; + + expect(Synthesizer.hasCrossFamilyReview(approved, families)).toBe(true); + }); + + test("a family recorded as 'unknown' COUNTS as differing — operator ruling, not a string accident", () => { + // The roster records `unknown` for a seat whose engine nobody can state. Operator ruling + // 2026-08-24: it counts as differing, so a guest seat's approvals can unblock a merge. + // Asserted as a DECISION rather than left to fall out of `'unknown' !== 'claude'`, because + // an arm that merely observes the string comparison would pass under either policy and + // could not tell a future reader which one was chosen. + const approverUnknown = { + author : {login: 'author-agent'}, + reviews: [{author: {login: 'preview-agent'}, state: 'APPROVED'}] + }; + + expect(resolveCrossFamilyVerdict(approverUnknown, { + 'author-agent' : 'claude', + 'preview-agent': 'unknown' + }).crossFamily).toBe(true); + + // Symmetric: an `unknown` AUTHOR is differed from by a known family. + const authorUnknown = { + author : {login: 'preview-agent'}, + reviews: [{author: {login: 'author-agent'}, state: 'APPROVED'}] + }; + + expect(resolveCrossFamilyVerdict(authorUnknown, { + 'preview-agent': 'unknown', + 'author-agent' : 'claude' + }).crossFamily).toBe(true); + + // …but SAME-family is still same-family, so the permissive reading has not swallowed the + // rule: two seats both carrying `unknown` do not differ from each other. + expect(resolveCrossFamilyVerdict(approverUnknown, { + 'author-agent' : 'unknown', + 'preview-agent': 'unknown' + }).crossFamily).toBe(false); + }); + + test('an unrostered author reports null, and the report keeps its external-contributor charity', () => { + const pr = { + author : {login: 'external-dev'}, + reviews: [{author: {login: 'reviewer-agent'}, state: 'APPROVED'}] + }; + + // Three states, not two. The mandate exists to stop one model family self-approving, which + // an external human's PR does not risk — so the REPORT stays permissive here, unchanged. + // The verdict keeps `null` distinct so a merge gate can apply its own policy instead of + // inheriting that charity by accident. + expect(resolveCrossFamilyVerdict(pr, {'reviewer-agent': 'claude'}).crossFamily).toBe(null); + expect(Synthesizer.hasCrossFamilyReview(pr, {'reviewer-agent': 'claude'})).toBe(true); + + // …but charity is not blanket: with no classified approver at all there is nothing to be + // charitable about, so it stays false rather than passing on the author's absence alone. + expect(Synthesizer.hasCrossFamilyReview(pr, {})).toBe(false); + }); + test('getRecentSummaryDocuments returns the N most-recent summaries by timestamp, newest-first (#13800)', async () => { const docMap = {s1: 'doc-old', s2: 'doc-newest', s3: 'doc-mid'}; const collection = { @@ -367,7 +439,12 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { expect(handoffContent).toContain('## Active PR Cycle State'); expect(handoffContent).toContain('(Source: GitHub Live; Status at generation: current; Fresh until:'); expect(handoffContent).toContain('### Recent Open PRs (`1` of `1` items)'); - expect(handoffContent).toContain('cross-family reviewed: yes'); + // The fixture's ONLY review is a CHANGES_REQUESTED from a cross-family reviewer, so the + // honest answer is `no` — a blocked PR is the opposite of covered. This assertion read + // `yes` while the predicate counted reviews of any state: the Golden Path was telling + // readers that a PR its cross-family reviewer BLOCKED was covered by them. + // Pinning the distinction rather than the value, so the arm fails in either direction. + expect(handoffContent).toContain('cross-family reviewed: no'); expect(handoffContent).toContain('- **PR #11178**: feat(ai): Automate PR Cycle State Extraction'); expect(handoffContent).not.toContain('](https://github.com/'); expect(handoffContent).not.toContain('### @neo-gemini-pro');