diff --git a/scripts/pm/check-governed-queue-guard.mjs b/scripts/pm/check-governed-queue-guard.mjs index 2c3d1e2a7fe..bf66b041629 100644 --- a/scripts/pm/check-governed-queue-guard.mjs +++ b/scripts/pm/check-governed-queue-guard.mjs @@ -266,6 +266,15 @@ * record IS one (#18060). Every governed path outside Tier S is Tier H, the * rules layer, and keeps the predicate above byte-for-byte. * + * ⚖️ AND, ON A PULL REQUEST BUILT IN A PROJECT THREAD, FROM ANOTHER ACCOUNT + * (#25415, maintainer 2026-10-23). The relay authors every PR and comment as + * `objectstack-fleet[bot]`, so the only place a project-thread PR names its + * account is its body's `claude-projects-attribution` line. When that line is + * there, the record must also carry `Reviewed-by-account:` naming a DIFFERENT + * login (`accountIndependence`); same, missing or unreadable refuses, and the + * refusal and the clear both print the two logins compared. A body without the + * line (a Routine seat, a human) owes nothing here — today's reading unchanged. + * * ⭐ THE TWO CARRIERS ARE ONE DELIVERY, and the location is the governed text's * to decide rather than this file's — quoted, untranslated, because it IS the * criterion: 「复核记录 = 一条评论落 PR 或卡」 @@ -560,6 +569,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ 'the per-row repository scope (ruling B2, 2026-10-13)': 14, '⭐ B′ with C: the record stands at the tier its FACE maps to, read off the committed paths': 8, 'the CODEOWNERS mirror: every Tier H register row routes to every GOVERNED_APPROVERS account, nothing else is routed, and the file says so': 12, + '⭐ #25415: an attributed pull request lands on a review of record from ANOTHER account': 10, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -569,7 +579,8 @@ const SELF_TEST_BATTERIES = Object.freeze({ // Raised 23 → 24 with the queued pull-back battery: one battery, one row. // Raised 25 → 26 with the B′-with-C face battery (ruling record 6159870311): one battery, one row. // Raised 26 → 27 with the CODEOWNERS mirror battery: one battery, one row. -const SELF_TEST_BATTERY_FLOOR = 27; +// Raised 27 → 28 with the #25415 account-independence battery: one battery, one row. +const SELF_TEST_BATTERY_FLOOR = 28; // The key an assertion is filed under when no battery is open. It is not a // declared battery, so it reds by the same set difference rather than silently @@ -815,11 +826,12 @@ export async function loadRecordRecognisers(load = (specifier) => import(specifi servedTierStands: tier?.servedTierStands, isModelIdentifierToken: tier?.isModelIdentifierToken, reviewFace: tier?.reviewFace, + attributionLogin: tier?.readAttributionLogin, threads: tier?.REVIEW_OF_RECORD_THREADS, location: tier?.REVIEW_OF_RECORD_LOCATION, }; const missing = [ - ...['locate', 'deliveredCard', 'servedTierStands', 'isModelIdentifierToken', 'reviewFace'].filter((key) => typeof found[key] !== 'function'), + ...['locate', 'deliveredCard', 'servedTierStands', 'isModelIdentifierToken', 'reviewFace', 'attributionLogin'].filter((key) => typeof found[key] !== 'function'), ...(Array.isArray(found.threads) && found.threads.length > 0 ? [] : ['threads']), ...(typeof found.location === 'string' && found.location.length > 0 ? [] : ['location']), ]; @@ -884,7 +896,7 @@ export async function loadRecordRecognisers(load = (specifier) => import(specifi * and the rule cannot drift. ⛔ A pair that carries no path list cannot say * which face it is on: UNREADABLE, never the cheaper tier. */ -export function recordVerdict({ pair, recognisers, cardNote = null }) { +export function recordVerdict({ pair, recognisers, cardNote = null, attribution = { state: 'absent' } }) { if (recognisers?.available !== true) { return { state: 'unreadable', reason: recognisers?.reason ?? 'the contract-review recognisers were not loaded', cardNote }; } @@ -944,7 +956,41 @@ export function recordVerdict({ pair, recognisers, cardNote = null }) { }; if (located.state === 'unsigned') return { state: 'unsigned', ...found }; if (!recognisers.servedTierStands(located.served, face.tierName)) return { state: 'below-tier', ...found }; - return { state: 'stands', ...found }; + if (attribution?.state === 'unreadable' && attribution.reason) { + return { state: 'unreadable', cardNote, reason: attribution.reason }; + } + const independence = accountIndependence(attribution, located.account); + if (independence.state !== 'holds' && independence.state !== 'not-owed') return { state: 'not-independent', ...found, independence }; + return { state: 'stands', ...found, independence }; +} + +/** + * Was the review of record written from a DIFFERENT ACCOUNT than the one the + * pull request was built under? (#25415, maintainer 2026-10-23.) + * + * A pull request landed from a Claude project thread carries its authoring + * account in the body's attribution line and nowhere else — the relay authors + * every PR and comment as `objectstack-fleet[bot]`, so neither `Reviewed-by:` + * (a session) nor the comment author can tell two accounts apart. So when the + * body names an account, the record must name its reviewer's account on a + * `Reviewed-by-account:` line, and the two must differ; a same-account + * subagent's review is a review, not an independent one. + * + * ⭐ A pull request with NO attribution line (a Routine seat, a human) owes + * nothing here: `not-owed`, today's behaviour byte for byte. ⛔ Everything else + * that is not a readable, different login is a refusal — an unreadable + * attribution, a missing or unreadable account line, or the same login (GitHub + * logins compare case-insensitively). + */ +export function accountIndependence(attribution, account) { + if (!attribution || attribution.state === 'absent') return { state: 'not-owed' }; + if (attribution.state !== 'read') return { state: 'attribution-unreadable', line: attribution.line ?? null }; + const author = attribution.login; + if (account?.state === 'unreadable') return { state: 'account-unreadable', author, line: account.line }; + if (account?.state !== 'read') return { state: 'account-missing', author }; + const reviewer = account.login; + if (author.toLowerCase() === reviewer.toLowerCase()) return { state: 'same-account', author, reviewer }; + return { state: 'holds', author, reviewer }; } /** @@ -1263,6 +1309,41 @@ export function guardVerdict({ event, repo, governed = [], unattributed = [], ap return { ...base, conclusion: 'cleared', exitCode: EXIT_CLEAR, refusalKind: null }; } +/** + * The refusal for a record that stands on tier but not on account (#25415): + * it names both logins it compared (or the one it could read) and the rule. + */ +function independenceRefusalLines(record) { + const ind = record.independence ?? {}; + const at = `comment ${record.id ?? '(no readable id)'}, ${record.at ?? 'undated'}`; + const rule = [ + ' Rule (#25415, maintainer 2026-10-23): a pull request landed from a project thread lands on a review of record', + ' from ANOTHER account — add `Reviewed-by-account: LOGIN` to the record, LOGIN being the reviewing session\'s', + ' account and different from the PR body\'s `claude-projects-attribution` github_login.', + ]; + if (ind.state === 'same-account') { + return [ + ` ⛔ the review of record on this head (${at}) was written from the SAME account that built the pull request:`, + ` attribution \`${ind.author}\` = Reviewed-by-account \`${ind.reviewer}\`. A same-account review is a review, not an independent one.`, + ...rule, + ]; + } + if (ind.state === 'account-missing' || ind.state === 'account-unreadable') { + return [ + ` ⛔ the review of record on this head (${at}) names no reviewer ACCOUNT the guard can compare against the attribution \`${ind.author}\`:`, + ind.state === 'account-missing' + ? ' it carries NO `Reviewed-by-account:` line.' + : ` its \`Reviewed-by-account:\` line carries no single GitHub login (${ind.line}).`, + ...rule, + ]; + } + return [ + ` ⛔ the pull request body carries a \`claude-projects-attribution\` line whose github_login cannot be read (${ind.line ?? 'no line quoted'}),`, + ' so the account the record must differ from is unknown.', + ...rule, + ]; +} + /** * The words a reader gets. Requirement (e) of the card lives here: every * rendering names the exact paths that matched and states what would satisfy @@ -1395,6 +1476,12 @@ export function renderGuardVerdict(verdict) { (entry.record.served?.stamps ? ` on a stamp control of ${entry.record.served.stamps.atTier}/${entry.record.served.stamps.total}.` : ' (no stamp control declared, which the rule permits).'), + ...(entry.record.independence?.state === 'holds' + ? [ + ` ⭐ accounts compared (#25415): built under \`${entry.record.independence.author}\` (the PR body's attribution line), ` + + `reviewed under \`${entry.record.independence.reviewer}\` (\`Reviewed-by-account:\`) — different accounts, so it is independent.`, + ] + : []), ...(entry.record.carriedFrom ? [ ` ⭐ that record names head \`${String(entry.record.carriedFrom).slice(0, 12)}\`, carried forward over ` + @@ -1434,6 +1521,8 @@ export function renderGuardVerdict(verdict) { ] : entry.record.state === 'unreadable' ? [` ⛔ the review of record could NOT be read — ${entry.record.reason}`] + : entry.record.state === 'not-independent' + ? independenceRefusalLines(entry.record) : entry.record.state === 'unsigned' ? [ ` ⛔ a \`## Contract review\` comment names this head (comment ${entry.record.id ?? '(no readable id)'}, ` + @@ -1765,7 +1854,12 @@ export async function runGuard({ event, repo, rows, fetchReviews, fetchPull, fet records.set(entry.pr, { state: 'unreadable', reason: unreadable, cardNote }); continue; } - records.set(entry.pr, recordVerdict({ pair, recognisers, cardNote })); + // ⭐ The authoring account, off the body this guard already read (#25415). + // A row that could not be read cannot say whether one is owed: unreadable. + const attribution = pull + ? recognisers.attributionLogin(pull.body) + : { state: 'unreadable', reason: `PR #${entry.pr}'s body could not be read, so whether its record owes a second account is unknown` }; + records.set(entry.pr, recordVerdict({ pair, recognisers, cardNote, attribution })); } } return guardVerdict({ event, repo, governed, unattributed, approvals, records, apiCalls, headNotes, lifted }); @@ -4234,6 +4328,85 @@ export async function selfTest() { // live leg is pinned at the contract-review tier; `recordVerdict` is pure, so // the declaration face — which no governed entry can be on — is driven on it // directly, with the real recognisers. + battery('⭐ #25415: an attributed pull request lands on a review of record from ANOTHER account'); + { + const ATTRIBUTED = (login) => `\n_Requested by **Elon**_\n\nFixes #${REF_CARD}`; + const withAccount = (login) => { + const c = recordComment(); + return { ...c, body: `${c.body.replace('Reviewed-by: `session_01SEAT`', `Reviewed-by: \`session_01SEAT\`\nReviewed-by-account: \`${login}\``)}` }; + }; + const plain = await tierRun({ comments: [recordComment()] }); + assert( + 'an-UNATTRIBUTED-PR-keeps-todays-behaviour-the-record-stands-and-no-account-is-owed', + plain.exitCode === EXIT_CLEAR && plain.entries[0].record.state === 'stands' && plain.entries[0].record.independence.state === 'not-owed' && + !/accounts compared/.test(renderGuardVerdict(plain)), + JSON.stringify(plain.entries[0].record.independence), + ); + const differ = await tierRun({ prBody: ATTRIBUTED('lane-account-a'), comments: [withAccount('lane-account-b')] }); + const differText = renderGuardVerdict(differ); + assert( + 'an-attributed-PR-whose-record-names-a-DIFFERENT-account-PASSES-and-the-clear-prints-both-logins', + differ.exitCode === EXIT_CLEAR && differ.entries[0].record.state === 'stands' && differ.entries[0].record.independence.state === 'holds' && + /accounts compared/.test(differText) && differText.includes('`lane-account-a`') && differText.includes('`lane-account-b`'), + JSON.stringify(differ.entries[0].record.independence), + ); + const same = await tierRun({ prBody: ATTRIBUTED('lane-account-a'), comments: [withAccount('@Lane-Account-A')] }); + const sameText = renderGuardVerdict(same); + assert( + '⛔ the-SAME-account-is-REFUSED-logins-compare-case-insensitively-and-the-refusal-names-both-and-the-rule', + same.exitCode === EXIT_REFUSED_UNAPPROVED && same.entries[0].record.state === 'not-independent' && + same.entries[0].record.independence.state === 'same-account' && + sameText.includes('`lane-account-a`') && sameText.includes('`Lane-Account-A`') && /Reviewed-by-account: LOGIN/.test(sameText), + JSON.stringify(same.entries[0].record.independence), + ); + const missing = await tierRun({ prBody: ATTRIBUTED('lane-account-a'), comments: [recordComment()] }); + assert( + '⛔ an-attributed-PR-whose-record-carries-NO-Reviewed-by-account-line-is-REFUSED', + missing.exitCode === EXIT_REFUSED_UNAPPROVED && missing.entries[0].record.independence.state === 'account-missing' && + /carries NO `Reviewed-by-account:` line/.test(renderGuardVerdict(missing)), + JSON.stringify(missing.entries[0].record.independence), + ); + const prose = await tierRun({ prBody: ATTRIBUTED('lane-account-a'), comments: [withAccount('the other account')] }); + assert( + '⛔ a-Reviewed-by-account-line-that-is-not-ONE-login-is-REFUSED', + prose.exitCode === EXIT_REFUSED_UNAPPROVED && prose.entries[0].record.independence.state === 'account-unreadable', + JSON.stringify(prose.entries[0].record.independence), + ); + const botAccount = await tierRun({ prBody: ATTRIBUTED('lane-account-a'), comments: [withAccount('objectstack-fleet[bot]')] }); + assert( + '⛔ the-relay-BOT-is-no-reviewer-account-REFUSED', + botAccount.exitCode === EXIT_REFUSED_UNAPPROVED && botAccount.entries[0].record.independence.state === 'account-unreadable', + ); + const badAttr = await tierRun({ prBody: `\n\nFixes #${REF_CARD}`, comments: [withAccount('lane-account-b')] }); + assert( + '⛔ an-attribution-line-naming-NO-readable-login-is-REFUSED-never-read-as-unattributed', + badAttr.exitCode === EXIT_REFUSED_UNAPPROVED && badAttr.entries[0].record.independence.state === 'attribution-unreadable', + JSON.stringify(badAttr.entries[0].record.independence), + ); + const lower = await tierRun({ prBody: `Fixes #${REF_CARD}\n\n${ATTRIBUTED('lane-account-a')}`, comments: [withAccount('lane-account-a')] }); + assert( + '⛔ the-attribution-line-is-read-on-ANY-line-an-edit-above-it-does-not-switch-the-check-off', + lower.exitCode === EXIT_REFUSED_UNAPPROVED && lower.entries[0].record.independence.state === 'same-account', + ); + const approvedSame = await tierRun({ + prBody: ATTRIBUTED('lane-account-a'), + reviews: [{ state: 'APPROVED', user: { login: GOVERNED_APPROVERS[0] }, commit_id: REF_HEAD }], + comments: [withAccount('lane-account-a')], + }); + assert( + 'MONOTONE: an-authorized-approval-still-clears-an-attributed-PR-the-account-leg-is-never-consulted', + approvedSame.exitCode === EXIT_CLEAR && approvedSame.entries[0].approval.state === 'approved' && tierThreadsRead.length === 0, + JSON.stringify({ code: approvedSame.exitCode, threads: tierThreadsRead }), + ); + assert( + 'the-pure-reading-attribution-absent-is-not-owed-and-nothing-else-is', + accountIndependence({ state: 'absent' }, { state: 'missing' }).state === 'not-owed' && + accountIndependence(undefined, { state: 'missing' }).state === 'not-owed' && + accountIndependence({ state: 'read', login: 'a' }, { state: 'read', login: 'b' }).state === 'holds' && + accountIndependence({ state: 'read', login: 'a' }, { state: 'read', login: 'A' }).state === 'same-account', + ); + } + battery('⭐ B′ with C: the record stands at the tier its FACE maps to, read off the committed paths'); const { TIER_DEFAULT_NAME: DEFAULT_TOKEN, TIER_DEFAULT: DEFAULT_VALUE } = await import('./dispatch-gates.mjs'); const tierDefaultOnS = await tierRun({ comments: [recordComment({ tierLine: `Served-tier: 75/75 \`${DEFAULT_TOKEN}\`` })] }); diff --git a/scripts/pm/record-recognisers.mjs b/scripts/pm/record-recognisers.mjs index e0e82d93bad..d3b878e0e07 100644 --- a/scripts/pm/record-recognisers.mjs +++ b/scripts/pm/record-recognisers.mjs @@ -25,6 +25,8 @@ * reviewFace / REVIEW_FACES which face made the review owed, and the ONE tier NAME it stands at * reviewScopeHits the at-tier review's scope floor — `reviewFace`'s `hits`, the record's `Scope:` line * readLocalRuns the `Local-runs:` line, and `localRunsStand` on it — judged when present, absent not re-judged + * readReviewedByAccount the `Reviewed-by-account:` line — the reviewer's account (#25415) + * readAttributionLogin the PR body's project-thread attribution login (#25415) * isModelIdentifierToken the token a refusal must never quote back * REVIEW_OF_RECORD_THREADS the ONE thread set, and REVIEW_OF_RECORD_LOCATION its words * unexplainedPathsBetween the pure-regeneration test a carried record re-runs, prose arm included @@ -533,6 +535,69 @@ function stripValueDecoration(raw) { */ export const REVIEWED_BY_LINE = keyLineRegex('Reviewed-by'); +/** + * The `Reviewed-by-account:` key line (#25415) — the ACCOUNT the reviewing + * session ran under, which `Reviewed-by:` cannot carry: every relay comment is + * authored `objectstack-fleet[bot]`, so the session id names a session and no + * comment field names the account behind it. Read in `keyLineRegex`'s one + * convention; `Reviewed-by:` does not match it (the key is literal up to its colon). + * + * ⛔ The value is ONE GitHub login, a leading `@` tolerated. A bot login, prose, + * or two logins read `unreadable`: the writer account is never a reviewer, and a + * line the guard cannot compare is not an independent review. + */ +export const REVIEWED_BY_ACCOUNT_LINE = keyLineRegex('Reviewed-by-account'); + +const GITHUB_LOGIN = /^@?([A-Za-z0-9](?:[A-Za-z0-9-]{0,38}))$/; + +/** + * What one comment declares about the reviewer's account. + * + * @returns {{ state: 'missing' } | { state: 'unreadable', line: string } | { state: 'read', login: string }} + */ +export function readReviewedByAccount(text) { + for (const line of String(text ?? '').split(/\r?\n/)) { + const m = REVIEWED_BY_ACCOUNT_LINE.exec(line); + if (!m) continue; + const rest = stripValueDecoration(m[1]).replace(/(?:[ \t]|\*\*|`)+$/, ''); + const login = GITHUB_LOGIN.exec(rest); + return login ? { state: 'read', login: login[1] } : { state: 'unreadable', line: quoteLine(line) }; + } + return { state: 'missing' }; +} + +/** + * The project-thread attribution line a pull request body carries (#25220, + * #25415): ``, the + * only place the AUTHORING account is written, since the relay authors every PR + * and comment as `objectstack-fleet[bot]`. + * + * ⛔ Read on ANY line of the body, not only the first: the marker is invisible + * once rendered, and a body that carries it anywhere is an attributed pull + * request. Reading only line 1 would let an edit above it turn the check off — + * the fail-open direction. A marker whose JSON names no login reads `unreadable`, + * which the guard refuses on. + * + * @returns {{ state: 'absent' } | { state: 'unreadable', line: string } | { state: 'read', login: string }} + */ +const ATTRIBUTION_LINE = //; + +export function readAttributionLogin(body) { + for (const line of String(body ?? '').split(/\r?\n/)) { + if (!/claude-projects-attribution/.test(line)) continue; + const m = ATTRIBUTION_LINE.exec(line); + let login = null; + try { + login = m ? JSON.parse(m[1])?.github_login : null; + } catch { + login = null; + } + const token = GITHUB_LOGIN.exec(String(login ?? '')); + return token ? { state: 'read', login: token[1] } : { state: 'unreadable', line: quoteLine(line) }; + } + return { state: 'absent' }; +} + /** * The `Served-tier:` key line -- the THIRD provenance fact a verdict declares * about itself, read with `Reviewed-by:`'s own discipline (#17915). @@ -1324,6 +1389,9 @@ export function locateReviewOfRecord(pair, carriedHead = null) { // The provenance fact C7 judges, read off the SAME comment this row chose, // so the two can never disagree about which verdict governs this head. served: readServedTier(row?.body), + // The reviewer's ACCOUNT, off the same comment (#25415). Read, never judged + // here: whether it must differ from the PR's attribution is the guard's leg. + account: readReviewedByAccount(row?.body), }; const signed = String(row?.body ?? '').split(/\r?\n/).some((line) => REVIEWED_BY_LINE.test(line)); return signed ? { state: 'found', ...found } : { state: 'unsigned', ...found }; @@ -1364,6 +1432,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ '⭐ B′ with C: a PROSE-ONLY head move keeps the record, decided on the COMMITTED trees like a regeneration': 6, '⭐ letter C: the at-tier review of a mixed diff is SCOPED — `hits` plus what they claim, on the `Scope:` line': 18, '⭐ letter B: a declared NARROWING is a face of its own at CONTRACT_REVIEW_TIER — keyed on the `Clause-②:` line, exact': 17, + '#25415: the reviewer ACCOUNT and the PR attribution login — read, never judged here': 7, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -1371,7 +1440,8 @@ const SELF_TEST_BATTERIES = Object.freeze({ // Raised 8 → 10 with the two B′-with-C batteries (ruling record 6159870311): two batteries, two rows. // Raised 10 → 11 with the letter-C scope battery (ruling record 6184805932): one battery, one row. // Raised 11 → 12 with the letter-B narrowing-face battery (ruling record 6203713500): one battery, one row. -const SELF_TEST_BATTERY_FLOOR = 12; +// Raised 12 → 13 with the #25415 account battery: one battery, one row. +const SELF_TEST_BATTERY_FLOOR = 13; // The key an assertion is filed under when no battery is open. It is not a // declared battery, so it reds by the same set difference rather than silently @@ -1741,6 +1811,15 @@ export async function selfTest() { // READS the record now EMITS it, and these cases are what make "copy it" a // safe instruction: the printed bytes are driven back through the very // readers that judge a real record. + battery('#25415: the reviewer ACCOUNT and the PR attribution login — read, never judged here'); + t('`Reviewed-by-account:` reads ONE login, decoration and a leading @ tolerated', readReviewedByAccount('- **Reviewed-by-account:** `@lane-a`').login === 'lane-a' && readReviewedByAccount('Reviewed-by-account: lane-b').login === 'lane-b'); + t('…absent is `missing`; and `Reviewed-by:` is NOT that key', readReviewedByAccount('Reviewed-by: `session_x`').state === 'missing' && readReviewedByAccount('').state === 'missing'); + t('⛔ prose, two logins or a bot login read `unreadable`', ['Reviewed-by-account: the other account', 'Reviewed-by-account: a, b', 'Reviewed-by-account: `objectstack-fleet[bot]`'].every((x) => readReviewedByAccount(x).state === 'unreadable')); + t('`Reviewed-by:` still matches only its own key, so an account line does not sign a record', !REVIEWED_BY_LINE.test('Reviewed-by-account: lane-a') && REVIEWED_BY_ACCOUNT_LINE.test('Reviewed-by-account: lane-a')); + t('the attribution login reads off the body, on any line', readAttributionLogin('\nbody').login === 'os-x' && readAttributionLogin('Fixes #1\n\n').login === 'os-y'); + t('…no marker is `absent`; a marker with no readable login is `unreadable`, never absent', readAttributionLogin('Fixes #1').state === 'absent' && readAttributionLogin('').state === 'unreadable' && readAttributionLogin('').state === 'unreadable'); + t('the locator carries the account read off the SAME comment it chose', (() => { const r = locateReviewOfRecord(pairOf({ prComments: [RECORD(HEAD_9AF9, ['Reviewed-by: `session_x`', 'Reviewed-by-account: lane-a'])] })); return r.state === 'found' && r.account.login === 'lane-a'; })()); + battery('#18042: the copyable record TEMPLATE — the one machine-read artefact with nothing to copy'); const TPL_TEXT = contractReviewTemplateLines().join('\n'); const TPL_RECORD = contractReviewRecordLines().join('\n');