From afc87015e8f1b86cb11f1146b3af6da50b1c56b5 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 24 Aug 2026 01:36:13 +0200 Subject: [PATCH 1/4] fix(github-workflow): merge-readiness enforces the cross-family mandate (#17661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateMergeReady` claimed to validate "the full review/merge contract" in five rules. The cross-family mandate was not among them, and rule 2 — `reviewDecision === 'APPROVED'` — cannot stand in for it: GitHub models no notion of model family, so no field it exposes can express the gate. Observed live before this change: a pull request whose only approval came from the author's own family returned `strictMergeReady: true` with zero blockers and zero advisories. Not a warning, not an unknown. Silent green. Every discipline this fleet holds about merge signals is about obtaining the freshest value of a field. All of it was applied correctly to that PR. A field that is fresh and irrelevant is invisible to all of it. The mechanism turned out to exist already. `agentFamilyResolution.mjs` exports author resolution, reviewer resolution and a coverage predicate, all unit-tested — wired only to a Golden Path report line, never to a gate. So this adds no resolver; it gives that module a verdict-returning form and consumes it. Two defects surfaced while wiring it: The coverage predicate counted reviews of ANY state, so a cross-family COMMENT read as coverage. The mandate requires an APPROVED review. An existing fixture pinned the consequence: a PR whose only cross-family review was a CHANGES_REQUESTED rendered as "cross-family reviewed: yes" — the report told readers a blocked PR was covered by the reviewer blocking it. Corrected, with the assertion now pinning the distinction. An unresolvable author family is a third state, not a boolean. The report treats it as satisfied, correctly — an unrostered author is an external contributor, and the mandate exists to stop one model family self-approving. The verdict keeps it as `null` so a gate decides for itself instead of inheriting a report's charity, and blocks with its own reason rather than claiming a mandate breach. The gate fails closed on an unresolved verdict, matching every other predicate field. Merge authority is untouched: the operator may still merge past this. The point is that the override is informed rather than depending on a reviewer happening to notice. Co-Authored-By: Grace --- ai/scripts/lifecycle/validateMergeReady.mjs | 27 +++++- .../github-workflow/PullRequestService.mjs | 25 +++++- .../queries/pullRequestQueries.mjs | 6 ++ ai/services/graph/agentFamilyResolution.mjs | 70 ++++++++++++--- .../lifecycle/validateMergeReady.spec.mjs | 89 ++++++++++++++++++- .../PullRequestService.spec.mjs | 24 +++-- .../graph/GoldenPathSynthesizer.spec.mjs | 46 +++++++++- 7 files changed, 264 insertions(+), 23 deletions(-) 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..7e2fb3bd73 100644 --- a/ai/services/github-workflow/PullRequestService.mjs +++ b/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,6 +779,23 @@ 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, @@ -781,6 +803,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..51209ae06a 100644 --- a/ai/services/github-workflow/queries/pullRequestQueries.mjs +++ b/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 } diff --git a/ai/services/graph/agentFamilyResolution.mjs b/ai/services/graph/agentFamilyResolution.mjs index 757cf1bed5..e3b8d548db 100644 --- a/ai/services/graph/agentFamilyResolution.mjs +++ b/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) + } } 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..b1fcbd80d5 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,11 @@ 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', + reviews = [{state: 'APPROVED', submittedAt: '2026-07-29T07:00:00.000Z', commit: {oid: HEAD}, author: {login: 'neo-gpt-emmy'}}], reviewsHasPreviousPage = false, state = 'OPEN' } = {}) => ({ @@ -398,6 +402,7 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read headRefOid, mergeStateStatus, reviewDecision, + author : authorLogin === null ? null : {login: authorLogin}, reviewRequests: { pageInfo: {hasNextPage: reviewHasNextPage, endCursor: null}, nodes : reviewers.map(login => ({ @@ -513,7 +518,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 +581,18 @@ 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('#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..847cf6333c 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,40 @@ 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('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 +404,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'); From bbd766e98c699c55c6b27244b09315f4cad7d684 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 24 Aug 2026 02:29:05 +0200 Subject: [PATCH 2/4] fix(github-workflow): a family recorded as unknown cannot satisfy the mandate (#17661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The roster records `unknown` for a seat whose underlying model nobody can state — an unreleased preview behind a codename, where the seat itself does not know what it is running on. That value is truthy, so a difference test reads it as a family that differs from the author's and certifies the cross-family mandate on it. That inverts the question. §6.1 asks whether the approval came from a DIFFERENT family. `'unknown' !== 'claude'` is true as a string comparison and unknowable as a fact, so certifying on it promises a guarantee nobody can make — and it does so in exactly the case where the least is known. `unknown` is now treated as unresolved on both sides: an approver carrying it contributes no approving family, and an author carrying it yields the null verdict the gate already fails closed on. A genuinely different, known family still satisfies the mandate, which is the arm that stops this from collapsing into nothing ever counting. The value is named as a constant with the reasoning attached, because the next reader will see a string equal to a family name and reasonably assume it is one. Found because the operator explained what the codename means; the roster was accurate and my consumer was wrong about what its accuracy meant. Co-Authored-By: Grace --- ai/services/graph/agentFamilyResolution.mjs | 27 +++++++++++++-- .../graph/GoldenPathSynthesizer.spec.mjs | 34 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/ai/services/graph/agentFamilyResolution.mjs b/ai/services/graph/agentFamilyResolution.mjs index e3b8d548db..a20d6dd0fd 100644 --- a/ai/services/graph/agentFamilyResolution.mjs +++ b/ai/services/graph/agentFamilyResolution.mjs @@ -14,6 +14,15 @@ import logger from '../../mcp/server/memory-core/logger.mjs'; * `GoldenPathSynthesizer` keeps thin static delegating shims so its public API stays stable. */ +/** + * The roster's placeholder for a seat whose underlying model is not publicly known — an unreleased + * preview behind a codename. It is a recorded VALUE, never a family: two seats both carrying it are + * not thereby the same family, and one carrying it is not thereby different from any other. Any + * consumer asking whether two families DIFFER must treat it as unresolved. + * @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 @@ -296,13 +305,25 @@ export function hasCrossFamilyReview(pr, agentFamilies = getCoreSwarmAgentFamili */ export function resolveCrossFamilyVerdict(pr, agentFamilies = getCoreSwarmAgentFamilies()) { const - authorFamily = resolveAuthorFamily(pr, agentFamilies) ?? null, + rawAuthorFamily = resolveAuthorFamily(pr, agentFamilies) ?? null, + authorFamily = rawAuthorFamily === UNKNOWN_FAMILY ? null : rawAuthorFamily, 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); + // `'unknown'` is a recorded family VALUE, not a family. The roster uses it for a seat whose + // underlying model nobody can state — an unreleased preview behind a codename, where even + // the seat itself does not know what it is running on. It is truthy, so a naive difference + // test reads it as "a family that differs from claude" and certifies the mandate on it. + // + // That inverts the gate's whole question. §6.1 asks whether the approval came from a + // DIFFERENT family; `'unknown' !== 'claude'` is true as a string comparison and unknowable + // as a fact. Treating it as unresolved is the only reading that cannot certify a guarantee + // nobody can make — and it fails toward the blocker, which is where an unknown belongs. + knowable = family => Boolean(family) && family !== UNKNOWN_FAMILY, + + approvingFamilies = [...new Set(resolved.filter(item => item.classified && knowable(item.family)).map(item => item.family))], + unclassifiedApprovers = resolved.filter(item => !item.classified || !knowable(item.family)).map(item => item.login).filter(Boolean); return { authorFamily, diff --git a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs index 847cf6333c..54b2e1c047 100644 --- a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs +++ b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs @@ -300,6 +300,40 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { expect(Synthesizer.hasCrossFamilyReview(approved, families)).toBe(true); }); + test("a family recorded as 'unknown' is UNRESOLVED, never a family that differs", () => { + // The roster records `unknown` for a seat whose underlying model nobody can state — an + // unreleased preview behind a codename, where the seat itself does not know what it runs on. + // It is truthy, so a naive `!==` reads it as "a family that differs from claude" and would + // certify the cross-family mandate on a difference nobody can know. Both directions matter. + const approverUnknown = { + author : {login: 'author-agent'}, + reviews: [{author: {login: 'preview-agent'}, state: 'APPROVED'}] + }; + + expect(resolveCrossFamilyVerdict(approverUnknown, { + 'author-agent' : 'claude', + 'preview-agent': 'unknown' + }).crossFamily).toBe(false); + + // …and an `unknown` AUTHOR cannot be differed from either: unresolved, not covered. + const authorUnknown = { + author : {login: 'preview-agent'}, + reviews: [{author: {login: 'author-agent'}, state: 'APPROVED'}] + }; + + expect(resolveCrossFamilyVerdict(authorUnknown, { + 'preview-agent': 'unknown', + 'author-agent' : 'claude' + }).crossFamily).toBe(null); + + // A genuinely different, KNOWN family still satisfies it — the arm that stops this from + // collapsing into "nothing ever counts". + expect(resolveCrossFamilyVerdict(approverUnknown, { + 'author-agent' : 'claude', + 'preview-agent': 'gpt' + }).crossFamily).toBe(true); + }); + test('an unrostered author reports null, and the report keeps its external-contributor charity', () => { const pr = { author : {login: 'external-dev'}, From 2a289d2899e43e1568cd3e08b75378b0dd592601 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 24 Aug 2026 02:51:09 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(github-workflow):=20unknown=20counts=20?= =?UTF-8?q?as=20differing=20=E2=80=94=20operator=20ruling=20(#17661)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the previous commit's reading after an operator ruling, and records the ruling where the code is rather than leaving it to be re-derived. I had made `unknown` unresolved, on the argument that a family nobody can state cannot be shown to differ, so certifying on it promises a guarantee nobody can make. That reasoning is sound and it is not the trade the swarm chose: a guest seat whose approvals can never unblock anything is a seat with no merge-path value, and for a Claude-family author the difference is two eligible cross-family seats versus three. So `unknown` participates as an ordinary family value. The cost is stated at the constant rather than buried: admitting it assumes part of what the mandate checks, because an unknown family cannot be shown uncorrelated with the author's. The arm asserts the DECISION, not the string comparison. An arm that merely observed `'unknown' !== 'claude'` would pass under either policy and could not tell a future reader which one was chosen — so it also pins that two seats both carrying `unknown` do NOT differ from each other, which is the half a permissive reading could have swallowed. Seat liveness is deliberately not consulted: the gate asks what an approval WAS, not who is available now, and a benched peer's past approval was still genuinely cross-family. That also keeps merge eligibility from being handed out by a hand-maintained roster whose participation rows are known stale. Co-Authored-By: Grace --- ai/services/graph/agentFamilyResolution.mjs | 38 +++++++++---------- .../graph/GoldenPathSynthesizer.spec.mjs | 27 ++++++------- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/ai/services/graph/agentFamilyResolution.mjs b/ai/services/graph/agentFamilyResolution.mjs index a20d6dd0fd..f260741687 100644 --- a/ai/services/graph/agentFamilyResolution.mjs +++ b/ai/services/graph/agentFamilyResolution.mjs @@ -15,10 +15,18 @@ import logger from '../../mcp/server/memory-core/logger.mjs'; */ /** - * The roster's placeholder for a seat whose underlying model is not publicly known — an unreleased - * preview behind a codename. It is a recorded VALUE, never a family: two seats both carrying it are - * not thereby the same family, and one carrying it is not thereby different from any other. Any - * consumer asking whether two families DIFFER must treat it as unresolved. + * 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'; @@ -305,25 +313,17 @@ export function hasCrossFamilyReview(pr, agentFamilies = getCoreSwarmAgentFamili */ export function resolveCrossFamilyVerdict(pr, agentFamilies = getCoreSwarmAgentFamilies()) { const - rawAuthorFamily = resolveAuthorFamily(pr, agentFamilies) ?? null, - authorFamily = rawAuthorFamily === UNKNOWN_FAMILY ? null : rawAuthorFamily, + 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'` is a recorded family VALUE, not a family. The roster uses it for a seat whose - // underlying model nobody can state — an unreleased preview behind a codename, where even - // the seat itself does not know what it is running on. It is truthy, so a naive difference - // test reads it as "a family that differs from claude" and certifies the mandate on it. - // - // That inverts the gate's whole question. §6.1 asks whether the approval came from a - // DIFFERENT family; `'unknown' !== 'claude'` is true as a string comparison and unknowable - // as a fact. Treating it as unresolved is the only reading that cannot certify a guarantee - // nobody can make — and it fails toward the blocker, which is where an unknown belongs. - knowable = family => Boolean(family) && family !== UNKNOWN_FAMILY, - - approvingFamilies = [...new Set(resolved.filter(item => item.classified && knowable(item.family)).map(item => item.family))], - unclassifiedApprovers = resolved.filter(item => !item.classified || !knowable(item.family)).map(item => item.login).filter(Boolean); + // `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); return { authorFamily, diff --git a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs index 54b2e1c047..698976e77c 100644 --- a/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs +++ b/test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs @@ -300,11 +300,12 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { expect(Synthesizer.hasCrossFamilyReview(approved, families)).toBe(true); }); - test("a family recorded as 'unknown' is UNRESOLVED, never a family that differs", () => { - // The roster records `unknown` for a seat whose underlying model nobody can state — an - // unreleased preview behind a codename, where the seat itself does not know what it runs on. - // It is truthy, so a naive `!==` reads it as "a family that differs from claude" and would - // certify the cross-family mandate on a difference nobody can know. Both directions matter. + 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'}] @@ -313,9 +314,9 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { expect(resolveCrossFamilyVerdict(approverUnknown, { 'author-agent' : 'claude', 'preview-agent': 'unknown' - }).crossFamily).toBe(false); + }).crossFamily).toBe(true); - // …and an `unknown` AUTHOR cannot be differed from either: unresolved, not covered. + // Symmetric: an `unknown` AUTHOR is differed from by a known family. const authorUnknown = { author : {login: 'preview-agent'}, reviews: [{author: {login: 'author-agent'}, state: 'APPROVED'}] @@ -324,14 +325,14 @@ test.describe('Neo.ai.daemons.services.GoldenPathSynthesizer', () => { expect(resolveCrossFamilyVerdict(authorUnknown, { 'preview-agent': 'unknown', 'author-agent' : 'claude' - }).crossFamily).toBe(null); + }).crossFamily).toBe(true); - // A genuinely different, KNOWN family still satisfies it — the arm that stops this from - // collapsing into "nothing ever counts". + // …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' : 'claude', - 'preview-agent': 'gpt' - }).crossFamily).toBe(true); + 'author-agent' : 'unknown', + 'preview-agent': 'unknown' + }).crossFamily).toBe(false); }); test('an unrostered author reports null, and the report keeps its external-contributor charity', () => { From 8b812bd83fc155af966051785ff0c3c28faf4924 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 24 Aug 2026 03:04:42 +0200 Subject: [PATCH 4/4] fix(github-workflow): the canonical author decides, and a bounded window cannot say no (#17661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both real, and the second one turned out to be about a contract this change had quietly invalidated elsewhere. The gate resolved the author family from the GitHub opener login. The opener can mis-resolve — an MCP `@me` drift stamps a different agent's login on the PR — so a body declaring a Claude author, opened under a GPT login, certified on a same-family approval. Opener drift deciding merge eligibility. §5's Social Name is the canonical author for exactly this reason, and the gate now reads it first. I had declared that a residual on the grounds that carrying the mutable body into a drift-compared snapshot would make any prose edit invalidate the observation. That concern was right and the conclusion was not: parse the self-id once and carry only the derived login. A change to the declared author invalidates the read; a typo fix in the description does not. The approvals connection is a bounded suffix, so a qualifying older approval can sit outside it. A positive witness inside the window is decisive — one is enough, whatever lies beyond. A negative is not: that is missing evidence, not evidence of absence, and reporting it as a factual "mandate unsatisfied" asserts something the data cannot support. Truncation now degrades a negative to unresolved, which the consumer already fails closed on with its own message. That makes the query's own JSDoc stale, and the correction is the more useful half: it said `hasPreviousPage` gates nothing because truncation could not change any decision this query feeds. True until this change made the mandate one of them. The same flag is inert for the approval anchor and load-bearing for the mandate, because "the latest approval" and "any approval" are different questions over one bounded list. Both fixes mutation-proved: reading the opener reddens the drift arm alone, ignoring truncation reddens the truncated-window arm alone. Co-Authored-By: Grace --- .../github-workflow/PullRequestService.mjs | 26 +++++++- .../queries/pullRequestQueries.mjs | 21 ++++-- ai/services/graph/agentFamilyResolution.mjs | 65 +++++++++++++++++-- .../PullRequestService.spec.mjs | 55 ++++++++++++++++ 4 files changed, 152 insertions(+), 15 deletions(-) diff --git a/ai/services/github-workflow/PullRequestService.mjs b/ai/services/github-workflow/PullRequestService.mjs index 7e2fb3bd73..89341fb158 100644 --- a/ai/services/github-workflow/PullRequestService.mjs +++ b/ai/services/github-workflow/PullRequestService.mjs @@ -11,6 +11,8 @@ import RepositoryService from './RepositoryService.mjs'; import {validateMergeReady} from '../../scripts/lifecycle/validateMergeReady.mjs'; import { groupReviewsByFamily, + parseSelfIdLogin, + resolveAuthorFamilyFromLogins, resolveCrossFamilyVerdict, resolveReviewerFamily } from '../graph/agentFamilyResolution.mjs'; @@ -448,6 +450,12 @@ function normalizeMergeReadinessSnapshot(pullRequest) { 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), @@ -786,13 +794,25 @@ async function buildMergeReadinessProjection({ // 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({ - author : {login: snapshot.authorLogin}, - reviews: snapshot.approvals.nodes.map(node => ({state: 'APPROVED', author: {login: node.login}})) + 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.authorLogin + authorLogin: snapshot.authorSelfIdLogin || snapshot.authorLogin } : undefined; diff --git a/ai/services/github-workflow/queries/pullRequestQueries.mjs b/ai/services/github-workflow/queries/pullRequestQueries.mjs index 51209ae06a..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,7 @@ export const GET_MERGE_READINESS = ` headRefOid mergeStateStatus reviewDecision + body author { login } diff --git a/ai/services/graph/agentFamilyResolution.mjs b/ai/services/graph/agentFamilyResolution.mjs index f260741687..aa58260f0d 100644 --- a/ai/services/graph/agentFamilyResolution.mjs +++ b/ai/services/graph/agentFamilyResolution.mjs @@ -175,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. @@ -313,7 +351,9 @@ export function hasCrossFamilyReview(pr, agentFamilies = getCoreSwarmAgentFamili */ export function resolveCrossFamilyVerdict(pr, agentFamilies = getCoreSwarmAgentFamilies()) { const - authorFamily = resolveAuthorFamily(pr, agentFamilies) ?? null, + // 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)), @@ -325,15 +365,28 @@ export function resolveCrossFamilyVerdict(pr, agentFamilies = getCoreSwarmAgentF 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, - // 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 + 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 - : approvingFamilies.some(family => family !== authorFamily) + : crossFamily } } 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 b1fcbd80d5..ac4b8cdc6b 100644 --- a/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs +++ b/test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs @@ -391,6 +391,9 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read // 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' @@ -403,6 +406,7 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read mergeStateStatus, reviewDecision, author : authorLogin === null ? null : {login: authorLogin}, + body, reviewRequests: { pageInfo: {hasNextPage: reviewHasNextPage, endCursor: null}, nodes : reviewers.map(login => ({ @@ -595,6 +599,57 @@ test.describe('Neo.ai.services.github-workflow.PullRequestService — merge-read 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', () => { for (const field of ['workflowRun', 'runNumber', 'runAttempt', 'resourcePath', 'databaseId']) { expect(GET_MERGE_READINESS).toContain(field);