fix(gate): the CI merge gate could be switched off by capitalising the repo name #5599
gluecron[bot]🤖 botAI Reviewcommented 1d ago
AI review unavailable
The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.
ccantynzAI Reviewcommented 1d ago
AI Triage
(no summary)
Priority: medium Risk area: mixed
Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)
Suggestions only — nothing has been applied. The PR author stays in control.
Cross-repo impact
See what breaks downstream if this PR merges.
⮌ Merged
This pull request was merged into main.
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts
Four pull requests merged today without their CI being consulted at all. Two of them merged while their runs were still going. Every merge response looked completely normal.
What happened
lookupRepoingate.tsmatched the repository name with exact equality. The stored name isGluecron.com; a caller passinggluecron.com— which every MCP merge in this session did — gotnull. AndrunAllGateChecksturned that null into:{ name: "CI", passed: true, skipped: true, details: "Repository not accessible" }So the CI gate did not fail. It silently stopped existing, for anyone who happened to type the name in a different case. The merge-blocking logic downstream was working perfectly the whole time — it was handed a pass.
Two things had to be true; either alone was survivable
The lookup was case-sensitive. It now delegates to
loadRepoByPath, the platform's one real resolver, instead of keeping a private copy. That also closes a second hole in the same function: the private copy was user-only, so an org-owned repository resolved to null too.Its failure was reported as a pass. An unresolvable repository now blocks, worded so it reads as our problem rather than the user's code. A database error reading run rows blocks too.
The two halves depend on each other, which is why they're one commit. Making an unresolvable repo block while the resolver was still user-only would have made every org repository permanently unmergeable — trading a silent bypass for a silent denial of service.
Reversing a deliberate decision, on the record
A test asserted the old fail-open: "a DB failure degrades to skipped — never blocks a merge on a lookup error." That was considered, and it was wrong.
The old direction's cost is a merge that nothing checked. The new direction's cost is a merge that waits. Those are not comparable — and this platform's own
docs/CI-ATTRIBUTION-GUARANTEE.mdalready settles it: where we cannot tell, we say we cannot tell, never a green that means "we did not look".The fear behind the old behaviour is also smaller than it looks: if the database is unreachable, the merge can't write its own rows either, so it was never going to succeed.
Verification
All three guards are mutation-verified — restoring the case-sensitive lookup, the fabricated pass, or the fail-open catch each fails a specific test.
One of them initially passed against the incident note explaining the bug, so the assertions strip comments first. A test that reads prose is checking that we still describe the property, not that we still have it.