diff --git a/.github/scripts/upstream-sync.mjs b/.github/scripts/upstream-sync.mjs index 0a9d3844c..d0ef8b50a 100644 --- a/.github/scripts/upstream-sync.mjs +++ b/.github/scripts/upstream-sync.mjs @@ -75,8 +75,10 @@ async function load({ fetchImpl = globalThis.fetch, env = process.env } = {}) { // A comparison needs both commits to exist; a fork descendant of upstream always does. let aheadBy = 'unknown', behindBy = 'unknown', status = 'unknown', commits = null - const cmp = await get(`/repos/${upstream}/compare/${upstreamSha}...${forkSha}`) - if (cmp.status === 200 && cmp.json) { + const compareUrl = `/repos/${upstream}/compare/${upstreamSha}...${forkSha}` + const cmp = await get(compareUrl) + const compareOk = cmp.status === 200 && cmp.json !== null + if (compareOk) { aheadBy = cmp.json.ahead_by ?? 'unknown' behindBy = cmp.json.behind_by ?? 'unknown' status = cmp.json.status ?? 'unknown' @@ -93,7 +95,7 @@ async function load({ fetchImpl = globalThis.fetch, env = process.env } = {}) { } : { readFailed: true, httpStatus: r.status } - return { code: null, upstreamSha, forkSha, aheadBy, behindBy, status, commits, ruleset, rulesetId } + return { code: null, upstreamSha, forkSha, aheadBy, behindBy, status, commits, compareOk, compareStatus: cmp.status, compareUrl, ruleset, rulesetId } } export async function check({ fetchImpl = globalThis.fetch, env = process.env, log = console.log, argv = [] } = {}) { @@ -111,6 +113,12 @@ export async function check({ fetchImpl = globalThis.fetch, env = process.env, l log(`ahead_by: ${s.aheadBy} behind_by: ${s.behindBy} status: ${s.status}`) const problems = [] + // An unreadable comparison is not an absent problem: when the two mains are already equal it is the + // exactly wrong moment to report a clean finish without having asked how far apart they are (#826). + if (!s.compareOk) { + log(`comparison ${short(s.upstreamSha)}...${short(s.forkSha)}: unreadable (HTTP ${s.compareStatus})`) + problems.push(`the comparison ${short(s.upstreamSha)}...${short(s.forkSha)} could not be read: HTTP ${s.compareStatus} from ${s.compareUrl}`) + } if (s.ruleset.readFailed) { log(`ruleset ${s.rulesetId}: unreadable (HTTP ${s.ruleset.httpStatus})`) problems.push(`ruleset ${s.rulesetId} cannot be read: HTTP ${s.ruleset.httpStatus}`) diff --git a/.github/scripts/upstream-sync.test.mjs b/.github/scripts/upstream-sync.test.mjs index 2c27c00ef..8f384c926 100644 --- a/.github/scripts/upstream-sync.test.mjs +++ b/.github/scripts/upstream-sync.test.mjs @@ -16,7 +16,7 @@ const PUSHED = [ // A transport that answers the four reads this script makes and refuses anything else — including a // non-GET on a URL it knows, so a future write reddens every case instead of one. -function transport({ upstreamSha = UP, forkSha = FORK, aheadBy = 0, behindBy = 0, status = 'identical', ruleset = RULESET, rulesetStatus = 200, commits: list = [] } = {}) { +function transport({ upstreamSha = UP, forkSha = FORK, aheadBy = 0, behindBy = 0, status = 'identical', ruleset = RULESET, rulesetStatus = 200, compareStatus = 200, commits: list = [] } = {}) { const seen = [] const json = (body) => ({ status: 200, text: async () => JSON.stringify(body) }) const fetchImpl = async (url, init = {}) => { @@ -25,7 +25,10 @@ function transport({ upstreamSha = UP, forkSha = FORK, aheadBy = 0, behindBy = 0 if (method !== 'GET') throw new Error(`refused a ${method}: this check never writes (${url})`) if (url.endsWith(`/repos/${DEFAULTS.upstream}/git/ref/heads/main`)) return json({ object: { sha: upstreamSha } }) if (url.endsWith(`/repos/${DEFAULTS.fork}/git/ref/heads/main`)) return json({ object: { sha: forkSha } }) - if (url.includes(`/repos/${DEFAULTS.upstream}/compare/`)) return json({ ahead_by: aheadBy, behind_by: behindBy, status, commits: list }) + if (url.includes(`/repos/${DEFAULTS.upstream}/compare/`)) { + if (compareStatus !== 200) return { status: compareStatus, text: async () => JSON.stringify({ message: 'Bad Gateway' }) } + return json({ ahead_by: aheadBy, behind_by: behindBy, status, commits: list }) + } if (url.endsWith(`/repos/${DEFAULTS.upstream}/rulesets/${DEFAULTS.rulesetId}`)) { if (rulesetStatus !== 200) return { status: rulesetStatus, text: async () => JSON.stringify({ message: 'Not Found' }) } return json({ id: Number(DEFAULTS.rulesetId), ...ruleset }) @@ -56,6 +59,35 @@ test('equal mains and an active ruleset with no bypass actor: exit 0', async () assert.match(out, /CHECK ok/) }) +test('a comparison that cannot be read fails the check, and the numbers still print', async () => { + // #826 acceptance 1: an unreadable read is not an absent problem. The post-check runs right after + // the two mains became the same commit, so a 5xx there must not read as a clean finish. + const { code, out } = await run({ forkSha: UP, compareStatus: 502 }) + assert.equal(code, 1) + assert.match(out, new RegExp(`upstream main : ${UP}`)) + assert.match(out, new RegExp(`fork main : ${UP}`)) + assert.match(out, /ahead_by: unknown {2}behind_by: unknown {2}status: unknown/) + assert.match(out, /CHECK FAILED — the comparison .*could not be read: HTTP 502/) + assert.equal((out.match(/CHECK FAILED —/g) || []).length, 1) +}) + +test('a readable identical comparison keeps the equal-mains path green', async () => { + // #826 acceptance 2: the negative control against "fail whenever status is unknown". + const { code, out } = await run({ forkSha: UP, aheadBy: 0, behindBy: 0, status: 'identical' }) + assert.equal(code, 0) + assert.match(out, /ahead_by: 0 {2}behind_by: 0 {2}status: identical/) + assert.match(out, /CHECK ok/) +}) + +test('--commits exits 2 on a comparison it could not read, and --check fails the same fake', async () => { + // #826 acceptance 3: the two commands keep their 0/1/2 contract on one and the same failure. + const c = await runCommits({ forkSha: UP, compareStatus: 502 }) + assert.equal(c.code, 2) + assert.match(c.out, /the comparison .*could not be read/) + const k = await run({ forkSha: UP, compareStatus: 502 }) + assert.equal(k.code, 1) +}) + test('the numbers are printed, not only the verdict', async () => { // #815 acceptance 8: ahead_by/behind_by have to be readable from the output. A run that reports // only "not equal" cannot say how far the fast-forward would move upstream. diff --git a/changelog.d/826-upstream-sync-compare.fixed.md b/changelog.d/826-upstream-sync-compare.fixed.md new file mode 100644 index 000000000..37fc9ba87 --- /dev/null +++ b/changelog.d/826-upstream-sync-compare.fixed.md @@ -0,0 +1 @@ +`upstream-sync.mjs --check` now fails, naming the read and the HTTP status, when it cannot ask how far apart the two `main` refs are, instead of reporting `CHECK ok` on equal SHAs ([#826](https://github.com/ThinkFlowLab/FlyBot/issues/826)).