From ad28b23f703f192948f6eacf36ee04188f4f1779 Mon Sep 17 00:00:00 2001 From: Andrew Mikofalvy <5668128+amikofalvy@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:51:16 -0700 Subject: [PATCH] fix(ok): resolve mirror-PR fix references by their origin trailer (PRD-7718) (#3085) * fix(ok): resolve mirror-PR fix references by their origin trailer (PRD-7718) Linear's GitHub linkback attaches the Copybara mirror PR to a ticket whenever the PR title carries the ticket identifier. Write-back treated that reference's merge SHA as a private SHA and trailer-searched it, which can never match (the merge commit IS the mirrored copy), and the all-or-nothing gate then made the whole ticket version-underivable. Self-repo PR references now recover the origin SHA from the merge commit's own GitOrigin-RevId trailer and resolve through the normal mirrored-copy walk, so point-release cherry-picks are found; a trailerless or ambiguous merge commit falls back to direct tag containment of its own SHA. Observed live on the v0.45.1 write-back run: PRD-7715 and PRD-7696 skipped as version-underivable until the echo attachments were deleted by hand. * Keep private-repo PRs on the trailer-search path when selfRepo is the origin GITHUB_REPOSITORY equals the private repo in agents-private CI, which routed private PR refs through the mirror-side branch and broke derivation there. * Log the trailerless/ambiguous fallback and pin it plus isSelfRepoPr with tests GitOrigin-RevId: 21c2fd4388e02c20790eb9933f3e2d087d772a68 --- .github/scripts/write-back.mjs | 67 ++++++++++++++- .github/scripts/write-back.test.mjs | 126 +++++++++++++++++++++++++++- 2 files changed, 189 insertions(+), 4 deletions(-) diff --git a/.github/scripts/write-back.mjs b/.github/scripts/write-back.mjs index 83305f36..6f2acb61 100644 --- a/.github/scripts/write-back.mjs +++ b/.github/scripts/write-back.mjs @@ -40,9 +40,12 @@ import { execFileSync, spawnSync } from 'node:child_process'; import { pathToFileURL } from 'node:url'; import { + firstContainingStableTag, parseFixRef, + parseGitOriginRevIds, resolvePrivateSha, resolveShippedVersion, + sortStableTagsAscending, } from './resolve-shipped-version.mjs'; import { composeReply, evaluateFanIn, partitionAttachments } from './write-back-gate.mjs'; @@ -157,12 +160,28 @@ export function isFixRepoInRemit({ kind, owner, repo }, { defaultRepo = DEFAULT_ return reachable.includes(target); } +/** + * Is this parsed fix reference a pull request on the MIRROR side — the repo + * this workflow runs on, when that repo is not also the private origin? The + * second half matters: in a test or rehearsal run executing inside the private + * monorepo itself, `GITHUB_REPOSITORY` equals the default private repo, and + * its pull requests are private references that must keep the trailer-search + * path. + */ +export function isSelfRepoPr({ kind, owner, repo }, selfRepo, defaultRepo = DEFAULT_PRIVATE_REPO) { + if (kind !== 'pr') return false; + const self = String(selfRepo ?? '').trim().toLowerCase(); + if (!self || self === String(defaultRepo ?? '').trim().toLowerCase()) return false; + return self === `${owner}/${repo}`.toLowerCase(); +} + export function deriveVersionForFixRefs({ fixReferences = [], stableTags, findMirroredCommits, contains, resolvePrMergeSha, + readCommitMessage = () => null, defaultRepo = DEFAULT_PRIVATE_REPO, selfRepo = process.env.GITHUB_REPOSITORY, log = () => {}, @@ -180,12 +199,49 @@ export function deriveVersionForFixRefs({ ); return null; } - const privateSha = resolvePrivateSha(parsed, { resolvePrMergeSha }); - if (!privateSha) { + const refSha = resolvePrivateSha(parsed, { resolvePrMergeSha }); + if (!refSha) { log(`::notice::write-back: ${ref.url} was closed without merging, so it carries no fix commit.`); return null; } - const result = resolveShippedVersion({ privateSha, stableTags, findMirroredCommits, contains }); + let result; + if (isSelfRepoPr(parsed, selfRepo, defaultRepo)) { + // A pull request in this repo (the Copybara mirror lands every export + // through one, and Linear's linkback attaches it to the ticket) merges a + // commit that is already on the mirror side, so its SHA must not be + // trailer-searched as if it were a private one: nothing carries + // `GitOrigin-RevId: `, and treating it that way made the + // whole ticket underivable. Recover the origin SHA from the merge + // commit's own trailer and resolve THAT, so cherry-picked copies on + // point-release tags are found too. A merge commit with no trailer — or + // an ambiguous one that embedded someone else's footer — falls back to + // containment of the merge SHA itself. + // A batched sync PR (several rebased commits landing through one PR, + // which only happens when the serialized mirror is catching up) makes + // this trailer belong to the LAST sibling in the batch rather than the + // reporter's fix. Highest-wins across refs keeps that error one-sided: + // the reporter can be told a slightly later version, never an earlier + // one that lacks their fix. + const revIds = parseGitOriginRevIds(readCommitMessage(refSha) ?? ''); + if (revIds.length === 1) { + result = resolveShippedVersion({ privateSha: revIds[0], stableTags, findMirroredCommits, contains }); + } else { + log( + `::notice::write-back: ${ref.url} merges a commit with ${revIds.length === 0 ? 'no' : 'more than one'} ` + + 'origin trailer; falling back to direct tag containment of the merge commit itself.', + ); + const tag = firstContainingStableTag({ + sortedStableTags: sortStableTagsAscending(stableTags), + sha: refSha, + contains, + }); + result = tag + ? { shipped: true, version: tag.replace(/^v/, ''), tag } + : { shipped: false, reason: 'not-in-any-stable' }; + } + } else { + result = resolveShippedVersion({ privateSha: refSha, stableTags, findMirroredCommits, contains }); + } if (!result.shipped) { log(`::notice::write-back: ${ref.url} has not reached a stable release yet (${result.reason}).`); return null; @@ -742,6 +798,10 @@ function realStableTags() { .filter((t) => STABLE_TAG_RE.test(t)); } +function realReadCommitMessage(sha) { + return runGit(['show', '-s', '--format=%B', sha]); +} + const REC_SEP = '\x1e'; const UNIT_SEP = '\x1f'; @@ -982,6 +1042,7 @@ async function main() { findMirroredCommits: realFindMirroredCommits, contains: realContains, resolvePrMergeSha: realResolvePrMergeSha, + readCommitMessage: realReadCommitMessage, log, }); diff --git a/.github/scripts/write-back.test.mjs b/.github/scripts/write-back.test.mjs index 2d7e1769..830ebb8e 100644 --- a/.github/scripts/write-back.test.mjs +++ b/.github/scripts/write-back.test.mjs @@ -8,6 +8,7 @@ import { findChangesetPath, isFixRepoInRemit, isRetryableNetworkError, + isSelfRepoPr, isRetryableStatus, LINEAR_RETRY_ATTEMPTS, LINEAR_RETRY_CAP_MS, @@ -159,6 +160,125 @@ describe('version derivation', () => { expect(derive({ fixReferences: [] })).toBeNull(); }); + test('a mirror pull request in this repo resolves through its merge commit trailer', () => { + // The Copybara mirror lands every export through a short-lived PR in this + // repo, and Linear's linkback attaches it to the ticket. Its merge commit + // IS the mirrored copy, so the resolvable identity is the origin SHA in + // its own trailer — which also finds the cherry-picked copy a point + // release carries when the main-line copy is in no stable yet. + const mirrorMain = 'd'.repeat(40); + const cherryPick = 'e'.repeat(40); + expect( + deriveVersionForFixRefs({ + fixReferences: [{ channel: 'pull-request', url: 'https://github.com/inkeep/open-knowledge/pull/928' }], + stableTags: STABLE_TAGS, + selfRepo: 'inkeep/open-knowledge', + resolvePrMergeSha: () => mirrorMain, + readCommitMessage: (sha) => (sha === mirrorMain ? `subject\n\nGitOrigin-RevId: ${PRIVATE_SHA}\n` : null), + findMirroredCommits: (sha) => + sha === PRIVATE_SHA + ? [ + { sha: mirrorMain, message: `GitOrigin-RevId: ${PRIVATE_SHA}` }, + { sha: cherryPick, message: `GitOrigin-RevId: ${PRIVATE_SHA}` }, + ] + : [], + contains: containsFrom({ [cherryPick]: ['v0.35.6', 'v0.36.0'] }), + }), + ).toBe('0.35.6'); + }); + + test('a private fix reference still derives with the mirror PR echo attached beside it', () => { + // Regression: the echo used to be trailer-searched as a private SHA, find + // nothing, and null the whole ticket — every ticket whose fix PR title + // carried an identifier became permanently underivable. + const mirrorEcho = 'f'.repeat(40); + expect( + deriveVersionForFixRefs({ + fixReferences: [ + { channel: 'pull-request', url: GH_PULL }, + { channel: 'pull-request', url: 'https://github.com/inkeep/open-knowledge/pull/928' }, + ], + stableTags: STABLE_TAGS, + selfRepo: 'inkeep/open-knowledge', + resolvePrMergeSha: ({ repo }) => (repo === 'agents-private' ? PRIVATE_SHA : mirrorEcho), + readCommitMessage: (sha) => (sha === mirrorEcho ? `subject\n\nGitOrigin-RevId: ${PRIVATE_SHA}\n` : null), + findMirroredCommits: (sha) => + sha === PRIVATE_SHA ? [{ sha: MIRRORED_SHA, message: `GitOrigin-RevId: ${PRIVATE_SHA}` }] : [], + contains: containsFrom({ [MIRRORED_SHA]: ['v0.36.0'] }), + }), + ).toBe('0.36.0'); + }); + + test('a mirror pull request with no trailer falls back to containment of its merge commit', () => { + const directSha = 'a1'.repeat(20); + expect( + deriveVersionForFixRefs({ + fixReferences: [{ channel: 'pull-request', url: 'https://github.com/inkeep/open-knowledge/pull/500' }], + stableTags: STABLE_TAGS, + selfRepo: 'inkeep/open-knowledge', + resolvePrMergeSha: () => directSha, + readCommitMessage: () => 'subject with no trailer\n', + findMirroredCommits: () => { + throw new Error('a trailerless self-repo merge must not be trailer-searched'); + }, + contains: containsFrom({ [directSha]: ['v0.36.0'] }), + }), + ).toBe('0.36.0'); + }); + + test('a mirror merge commit with more than one origin trailer is ambiguous and falls back to containment', () => { + // A message can embed someone else's footer; picking either trailer would + // be a guess, so the merge commit's own containment is the answer. + const otherSha = '9'.repeat(40); + const mergeSha = 'c3'.repeat(20); + const logs = []; + expect( + deriveVersionForFixRefs({ + fixReferences: [{ channel: 'pull-request', url: 'https://github.com/inkeep/open-knowledge/pull/502' }], + stableTags: STABLE_TAGS, + selfRepo: 'inkeep/open-knowledge', + resolvePrMergeSha: () => mergeSha, + readCommitMessage: () => `GitOrigin-RevId: ${PRIVATE_SHA}\nGitOrigin-RevId: ${otherSha}\n`, + findMirroredCommits: () => { + throw new Error('an ambiguous trailer must not be trailer-searched'); + }, + contains: containsFrom({ [mergeSha]: ['v0.35.6'] }), + log: (m) => logs.push(m), + }), + ).toBe('0.35.6'); + expect(logs.join(' ')).toContain('more than one origin trailer'); + }); + + test('isSelfRepoPr is repo-identity plus a not-the-origin guard', () => { + const pr = (owner, repo) => ({ kind: 'pr', owner, repo, number: 1 }); + expect(isSelfRepoPr(pr('inkeep', 'open-knowledge'), 'inkeep/open-knowledge')).toBe(true); + expect(isSelfRepoPr(pr('inkeep', 'Open-Knowledge'), 'inkeep/open-knowledge')).toBe(true); + expect(isSelfRepoPr(pr('inkeep', 'agents-private'), 'inkeep/open-knowledge')).toBe(false); + expect(isSelfRepoPr({ kind: 'sha', sha: 'a'.repeat(40) }, 'inkeep/open-knowledge')).toBe(false); + expect(isSelfRepoPr(pr('inkeep', 'open-knowledge'), undefined)).toBe(false); + // A run inside the private monorepo: its own PRs are private references, + // not mirror-side ones, even though repo identity matches selfRepo. + expect(isSelfRepoPr(pr('inkeep', 'agents-private'), 'inkeep/agents-private')).toBe(false); + expect(isSelfRepoPr(pr('inkeep', 'some-fork'), 'inkeep/some-fork', 'inkeep/some-fork')).toBe(false); + }); + + test('a mirror pull request contained in no stable yet yields no version', () => { + const logs = []; + expect( + deriveVersionForFixRefs({ + fixReferences: [{ channel: 'pull-request', url: 'https://github.com/inkeep/open-knowledge/pull/501' }], + stableTags: STABLE_TAGS, + selfRepo: 'inkeep/open-knowledge', + resolvePrMergeSha: () => 'b2'.repeat(20), + readCommitMessage: () => 'subject with no trailer\n', + findMirroredCommits: () => [], + contains: () => false, + log: (m) => logs.push(m), + }), + ).toBeNull(); + expect(logs.join(' ')).toContain('not-in-any-stable'); + }); + test('a fix reference naming a pull request that was closed unmerged is an answer, not a fault', () => { // Real shape: PRD-7539 still carries agents-private#2844, which was closed // in favour of #2864. A stale attachment is for a human to fix in Linear, @@ -209,7 +329,9 @@ describe('version derivation', () => { test('the two repos it CAN read are still attempted, so the 404 bug cannot come back', () => { // A blanket 404 suppression would have masked the missing-permission bug on - // agents-private. These two must always reach the API. + // agents-private. These two must always reach the API. The self-repo PR + // resolves through its merge commit's own trailer rather than a trailer + // search for its SHA, but it is attempted all the same. for (const url of [ 'https://github.com/inkeep/agents-private/pull/2864', 'https://github.com/inkeep/open-knowledge/pull/12', @@ -223,6 +345,8 @@ describe('version derivation', () => { attempted = true; return PRIVATE_SHA; }, + readCommitMessage: (sha) => + sha === PRIVATE_SHA ? `subject\n\nGitOrigin-RevId: ${PRIVATE_SHA}\n` : null, }), ).toBe('0.36.0'); expect(attempted, `${url} must be attempted`).toBe(true);