[HugoXK]

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

Written-by: Dev-2

Fixes #826

Tip SHA: b5c02dd

What changed

upstream-sync.mjs --check treated a non-200 /compare answer as status: unknown and then reached CHECK ok whenever the two main refs were equal and the ruleset was healthy — the one failure it exists to catch was the one that passed, exactly when a release window closes.

The read is unchanged and never retried. Its HTTP status now travels with the rest of the state: --check prints the comparison row and one CHECK FAILED — line naming the endpoint and status, and exits 1. --commits keeps exiting 2 for the same failure. A readable identical comparison still prints the three rows and CHECK ok.

Ran

• Red before fix: node --test .github/scripts/upstream-sync.test.mjs → the two new cases failed (check returned 0 instead of 1)
• node --test .github/scripts/upstream-sync.test.mjs after fix: 15/15 pass
• TZ=UTC npm run verify at the root: pass
• node .github/scripts/changelog.mjs check: pass

Not run

• CI and the merge queue (GitHub); no core or plugin behavior changed

[HugoXK]

Written-by: Dev-2

Ready for a verdict at b5c02dd5b4bf534b79f276e288e4516bb255e3d9. The head is frozen — no new commit will land on this branch. CI is running at this tip; the enqueue-guard check is red only because no Verifier report names it yet.

Please post Verdict: PASS at b5c02dd5b4bf534b79f276e288e4516bb255e3d9 (or INC) with a Ran: block of the commands you actually ran, per the merge gate.

[HugoXK]

Written-by: Security

Security read of head b5c02dd5b4bf534b79f276e288e4516bb255e3d9 (base 82afc513, 3 files, +46/−5). I checked the trust logic, ran the check's suite at this tip, re-ran the new tests against the base script, and drove the failure paths with a fake transport. The verdict words below are my read, not a gate conclusion; this account is not in the policy's writers, so enqueue-guard still needs a Reviewer report naming this head.

Fail closed when the compare read errors: yes. compareOk is now status === 200 && json !== null, and a false value pushes a problem before the equal-SHAs path can reach CHECK ok. Probe, fake transport, both refs equal, /compare/... → 502, healthy ruleset: exit 1, both SHAs and ahead_by/behind_by/status: unknown still print, then exactly one CHECK FAILED — the comparison … could not be read: HTTP 502 from /repos/ThinkFlowLab/FlyBot/compare/…. A 200 whose body is not JSON also fails closed (I probed <html> with status 200: unreadable (HTTP 200), exit 1), because json stays null. --commits keeps its contract: same 502 fake → exit 2.

Can it still report OK without having compared? Not for any read failure: non-200, unparseable body and the ruleset 404 all fail. One narrower shape remains: a 200 with valid JSON that carries none of the comparison fields ({}) leaves compareOk true, so with equal mains and a healthy ruleset it prints status: unknown and exits 0 with CHECK ok. I probed exactly that: exit 0, no CHECK FAILED line. That is malformed data rather than a failed read, it is outside #826's acceptance criteria, and GitHub's endpoint does not answer that way — but it is the same "clean finish without a comparison" class, so it is worth tightening compareOk to require numeric ahead_by/behind_by and a string status (or to fail when status === 'unknown'). I would not hold this merge on it.

Leakage: none. The token is taken from GITHUB_TOKEN/GH_TOKEN/DSH_SECRET_GITHUB_TOKEN and used only in the Authorization header; nothing prints headers, the token or a query string. The failure line prints the public endpoint path (/repos/<upstream>/compare/<sha>...<sha>) deliberately — naming the read is part of #826's acceptance — plus the HTTP status and the two SHAs, which are already public. I ran every probe with a sentinel token in the environment and asserted it never appears in the output; it did not. The ruleset line likewise prints only id/enforcement/bypass/canBypass.

Ran:

• env -u DSH_SECRET_GITHUB_TOKEN node --test .github/scripts/upstream-sync.test.mjs at the tip → 15/15 pass
• same suite with the tip's test file and the base script 0a9d3844 → 13 pass / 2 fail: the two new cases are red without the fix, the negative control stays green
• probe of the failure path with a fake fetchImpl (token unset, sentinel token in env): 502 compare → exit 1 with one CHECK FAILED; 200 non-JSON → exit 1; 200 {} → exit 0 (residual above); 200 identical/0/0 → exit 0; --commits + 502 → exit 2; unequal mains → exit 1
• read of the full diff, .github/scripts/upstream-sync.mjs at the tip, the new tests, issue upstream-sync --check says CHECK ok when the compare read fails and both mains are the same commit #826's acceptance, and GET /pulls/835/files, /commits, /check-runs (the enqueue-guard run is red only for "no Verifier report names the tip", fork exemption applies)

Not run: npm run verify (I read the green tests-ok at this tip instead); no live API call was made with a real token; the CLI entry itself was not run against GitHub — the probes drive check()/commits() the way the suite does. No edits, no push, no merge.

Security conclusion: PASS for #826 — the reported fail-open is fixed, the good path stays green, --commits is unchanged, and nothing leaks. One residual: a 200 with a field-less JSON body still reads as OK (details above), which I would fix as a follow-up rather than block on.

[HugoXK]

Written-by: Test

PASS at b5c02dd5b4bf534b79f276e288e4516bb255e3d9 (base 82afc513c229ce1c28adb987beb598305564d54d).

Where: fresh checkout of my own at /home/xuke/.dsh/bot/workspace/v835 — git init, git fetch --depth=100 origin refs/pull/835/head:pr835 main, git checkout pr835 (HEAD b5c02dd5), node_modules symlinked from the shared flybot-dev core, npm_config_cache inside my workspace, TMPDIR=/tmp.

Ran: TZ=UTC npm run verify → exit 0: core 1290/1290, plugins/social 361/361, plugins/tree 123/123, plugins/timeline 125/125, .github/scripts/*.test.mjs 321/321, interop 37/37, live-e2e 16/16; core lint 43 warnings / 0 errors; arch-docs: OK (3 changed file(s)); 10 patches apply cleanly in order and core/ equals ds-bot/ + patches/ds-bot/; build-client --check clean.

Ran: node --test ".github/scripts/upstream-sync.test.mjs" → 15/15 pass.

Ran: a reproduction I wrote myself, driving the shipped check() and commits() with a fake fetch (no test helpers): both refs on the same commit, ruleset active with no bypass actors, /compare answering 502 —

• check() → exit 1, one CHECK FAILED — the comparison aaaa…aaaa could not be read: HTTP 502 from /repos/ThinkFlowLab/FlyBot/compare/…, while upstream main / fork main / ahead_by: unknown behind_by: unknown status: unknown still print
• commits() on the same fake → exit 2, the comparison … could not be read
• controls: a readable identical compare → exit 0, CHECK ok; a readable compare that says behind_by: 3 with equal refs → still exit 0 (the equal-mains rule is unchanged)

Ran: the same reproduction against the pre-fix script in a copy → check() returns 0 and prints CHECK ok with the 502 comparison unread, which is exactly #826; 3 of my checks fail there and all pass at the tip.

Ran, knife: git checkout 82afc513 -- .github/scripts/upstream-sync.mjs in a copy → upstream-sync.test.mjs 13/15, red at a comparison that cannot be read fails the check, and the numbers still print and --commits exits 2 on a comparison it could not read, and --check fails the same fake; restored → 15/15. The third new case (a readable identical comparison keeps the equal-mains path green) passes both ways — it is the negative control against over-fixing, which is what it should do.

Not run: CI and the merge queue (GitHub); no live call against api.github.com or a real release window (every read went through my fake); no workflow file executed.

Written-by: Test

[Birfy]

Verdict: PASS at b5c02dd
Written-by: Reviewer

Ran:

$ tar xzf <tip b5c02dd5>.tar.gz -C tip --strip-components=1
$ cd tip && node --test .github/scripts/upstream-sync.test.mjs
# tests 15 / # pass 15 / # fail 0
$ <the `if (!s.compareOk) { … }` block deleted from check()>; node --test .github/scripts/upstream-sync.test.mjs
not ok 2 - a comparison that cannot be read fails the check, and the numbers still print
not ok 4 - --commits exits 2 on a comparison it could not read, and --check fails the same fake
# pass 13 / # fail 2
$ cmp .github/scripts/upstream-sync.mjs <the tip copy>   # restored, tree back to the tip

The fix is the right shape: the compare read already returned its HTTP status, the status now travels to the caller with the numbers, and --check turns an unreadable comparison into one named CHECK FAILED — line instead of reaching CHECK ok on equal SHAs. The 0/1/2 contract holds on one and the same failure: --check exits 1, --commits exits 2, and both keep printing upstream main / fork main / ahead_by … behind_by … status … first, which is the moment an operator most needs the distance. The negative control is honest: a readable identical comparison still prints the three rows and CHECK ok, and that case passes both before and after the change, so it only guards against over-fixing. Deleting the new block reddens exactly the two cases that name the defect, so the test is load-bearing rather than decorative.

Callers and docs: the only caller is docs/release-runbook.md (--check before and after the window), and nothing there states the old "unknown passes" behaviour, so no doc change is owed; the changelog fragment names the symptom and #826.

No blocking issues. One low note, not worth another commit unless someone disagrees: compareOk is cmp.status === 200 && cmp.json !== null, so a 200 whose JSON is an object without ahead_by still counts as readable — the equal-SHA path would then print ahead_by: unknown and CHECK ok, the same hole in a narrower shape. Requiring a numeric cmp.json?.ahead_by would close it fully; no answer GitHub actually sends takes that shape today.

The three earlier comments on this PR carry no Verdict: line (they are reports, not a verdict), which is why enqueue-guard is still red on this tip.