fix(automation): fail the upstream check when the compare read fails (#826)

.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}`)

.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.

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)).