From 60ab394a2ed303e64f5dfbbfc6f1fc066303afa5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 13:43:38 -0700 Subject: [PATCH] =?UTF-8?q?fix(orb):=20stop=20two=20wrong-close=20paths=20?= =?UTF-8?q?=E2=80=94=20superseded=20cancelled=20checks=20and=20permanently?= =?UTF-8?q?-remembered=20linked-issue=20violations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #9053 -- CONFIRMED live, upgraded from the issue's "suspected". Verified on a real PR head that GET /commits/{sha}/check-runs returns runs from MULTIPLE suites even under GitHub's default filter=latest: 11 runs across 6 suites, 10 of them from older suites. Combined with ci.yml's `concurrency: cancel-in-progress: true`, a same-SHA re-trigger (reopened / ready_for_review / labeled -- and ORB ITSELF applies labels) starts a new suite and cancels the first, leaving conclusion:"cancelled" runs on the head SHA in the OLD suite. "cancelled" is in CI_FAILING_CONCLUSIONS and the dedupe key deliberately includes the suite id (so a genuine failure is never hidden by a later success), so those stale runs landed in failingDetails beside the new suite's green ones -> ciState "failed" -> willClose on ciFailed -> one-shot auto-close of a PR whose CI is green. The executor's recheck re-derived the same verdict, confirming rather than catching it. "CI is failing" is the most common close reason in the ledger, so the false-positive cost is high. Fix drops a cancelled run only when a strictly NEWER suite ran the same (name, app). A cancellation with no superseding attempt still fails; genuine failure / timed_out / startup_failure conclusions are never dropped, so the cross-suite non-collapse keeps doing its job; a non-numeric or absent suite id is not comparable and is conservatively kept. #9029 -- linked_issue_hard_rule_violated_at was written the first time ANY pass observed a violation and NEVER cleared, so an ephemeral, benign issue state condemned a PR for its whole lifetime: a maintainer who momentarily self-assigns the linked issue to triage it stamped a permanent one-shot-close marker on every PR linking it, and un-assigning did not exonerate. The only escape was disabling every hard rule repo-wide. The persistence exists to stop a stamp-then-dodge (edit the body to unlink the ineligible issue after being caught), so it is kept and narrowed to exactly that case. Migration 0184 records WHICH issues were linked at violation time; the merge exonerates only when the SAME set is still linked and the live rules now pass (the improvement came from the issue side, which the author does not control), and the marker is then cleared. A changed linked set -- swapped, unlinked, or extended -- still stands, as do pre-#9029 rows with no snapshot, where a dodge cannot be ruled out. POLICY REVERSAL, called out explicitly: this inverts an existing regression test that asserted an unchanged body whose linked issue merely lost its assignee STILL closed. That is precisely the harm #9029 documents. The sibling test covering the real dodge (body edited to unlink) is unchanged and still passes. Closes #9029 Closes #9053 Tests: 4 CI regressions (superseded-cancelled is ignored including when the newer suite appears FIRST in array order, a lone cancelled run still fails, a genuine older-suite failure still reports, non-comparable suite ids are kept), 5 merge-discriminator unit tests, 3 repository-level tests for the snapshot writer/clearer, and an exoneration-clear-failure test proving a failed D1 write never becomes a wrong close. 100% line+branch coverage on all added lines; 1715/1715 across 12 suites; all drift gates green. --- .../0184_linked_issue_violation_snapshot.sql | 14 +++ src/db/repositories.ts | 35 +++++- src/db/schema.ts | 4 + src/github/backfill.ts | 50 ++++++++- src/queue/processors.ts | 13 ++- src/review/linked-issue-hard-rules.ts | 33 +++++- src/types.ts | 2 + test/unit/backfill-2.test.ts | 103 +++++++++++++++++ test/unit/linked-issue-hard-rules.test.ts | 104 ++++++++++++++++++ test/unit/queue.test.ts | 60 ++++++++-- 10 files changed, 405 insertions(+), 13 deletions(-) create mode 100644 migrations/0184_linked_issue_violation_snapshot.sql diff --git a/migrations/0184_linked_issue_violation_snapshot.sql b/migrations/0184_linked_issue_violation_snapshot.sql new file mode 100644 index 0000000000..a4d602214f --- /dev/null +++ b/migrations/0184_linked_issue_violation_snapshot.sql @@ -0,0 +1,14 @@ +-- #9029: the linked-issue hard-rule violation marker (linked_issue_hard_rule_violated_at) is written the first +-- time ANY pass observes a violation and is then NEVER cleared, so an ephemeral, benign issue state condemns a +-- PR for its entire lifetime. Concrete case from the audit: a maintainer momentarily self-assigns the linked +-- issue to triage it, the next pass stamps "issue assigned to someone else", and the PR is permanently marked +-- for a one-shot close -- removing the assignment does not exonerate it. That punishes a contributor for the +-- maintainer's own action, with no recourse short of disabling every hard rule repo-wide. +-- +-- The persistence itself is deliberate and must stay: it exists so a contributor cannot stamp-then-dodge by +-- editing the PR body to unlink the ineligible issue after the violation was observed. The missing piece is the +-- ability to tell those two situations apart. Recording WHICH linked issues were observed at violation time +-- makes that a decidable question: if the PR still links the SAME issue set and the live rules now pass, the +-- change happened on the ISSUE side (assignee removed, label added, reopened) and the PR is exonerated; if the +-- linked set CHANGED, the author edited the link and the remembered violation stands. +ALTER TABLE pull_requests ADD COLUMN linked_issue_hard_rule_violation_issues_json TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index ad9543e0d7..692db5bf60 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4229,13 +4229,43 @@ export async function markPullRequestMergeBlocked(env: Env, fullName: string, nu * the body or the linked issue's state changing after this call is a no-op here: the PR already proved itself in * violation once and stays that way for its lifetime. A no-op (0 rows affected) when the PR row doesn't exist yet * is safe -- the caller only reaches this after a live violation was just evaluated against an existing row. */ -export async function markPullRequestLinkedIssueHardRuleViolated(env: Env, fullName: string, number: number, reason: string): Promise { +export async function markPullRequestLinkedIssueHardRuleViolated( + env: Env, + fullName: string, + number: number, + // Nullable: the default lives here rather than at the call site so the fallback is directly unit-testable + // (a violation with no reason is not reachable through the live evaluator today, but the column is + // NOT NULL-tolerant by contract and a future rule could omit one). + reason: string | null | undefined, + // #9029: the linked-issue set observed at violation time, COALESCE'd like the siblings so it always describes + // the FIRST violation. Without it a later pass cannot tell an issue-side state change (exonerate) from an + // author link edit (violation stands) -- see mergeLinkedIssueHardRuleWithPersistedViolation. + linkedIssues?: readonly number[] | undefined, +): Promise { const db = getDb(env.DB); + const snapshot = jsonString([...new Set(linkedIssues ?? [])].sort((a, b) => a - b)); await db .update(pullRequests) .set({ linkedIssueHardRuleViolatedAt: sql`COALESCE(${pullRequests.linkedIssueHardRuleViolatedAt}, ${nowIso()})`, - linkedIssueHardRuleViolationReason: sql`COALESCE(${pullRequests.linkedIssueHardRuleViolationReason}, ${reason.slice(0, 280)})`, + linkedIssueHardRuleViolationReason: sql`COALESCE(${pullRequests.linkedIssueHardRuleViolationReason}, ${(reason ?? "the linked issue is not eligible for a community PR").slice(0, 280)})`, + linkedIssueHardRuleViolationIssuesJson: sql`COALESCE(${pullRequests.linkedIssueHardRuleViolationIssuesJson}, ${snapshot})`, + updatedAt: nowIso(), + }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))); +} + +/** #9029: clear a remembered linked-issue hard-rule violation once a later pass proves it was an issue-side, + * benign state change rather than an author dodge. Only ever called with that determination already made + * (see mergeLinkedIssueHardRuleWithPersistedViolation's exoneration branch). */ +export async function clearPullRequestLinkedIssueHardRuleViolation(env: Env, fullName: string, number: number): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ + linkedIssueHardRuleViolatedAt: null, + linkedIssueHardRuleViolationReason: null, + linkedIssueHardRuleViolationIssuesJson: null, updatedAt: nowIso(), }) .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))); @@ -6844,6 +6874,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull lastRegatedAt: row.lastRegatedAt, lastPublishedSurfaceSha: row.lastPublishedSurfaceSha, linkedIssueHardRuleViolatedAt: row.linkedIssueHardRuleViolatedAt, + linkedIssueHardRuleViolationIssues: parseJson(row.linkedIssueHardRuleViolationIssuesJson, []), linkedIssueHardRuleViolationReason: row.linkedIssueHardRuleViolationReason, visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha, screenshotTablePresenceSatisfied: parseJson<{ headSha: string; evidenceFingerprint: string } | null>(row.screenshotTablePresenceSatisfiedJson, null), diff --git a/src/db/schema.ts b/src/db/schema.ts index cd23d68d9b..46e156ba2b 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -420,6 +420,10 @@ export const pullRequests = sqliteTable( // pairing with merge_blocked_sha) -- so a later close can still cite the concrete rule even if the live // re-parse can no longer reproduce it (the issue was unlinked or its state changed). linkedIssueHardRuleViolationReason: text("linked_issue_hard_rule_violation_reason"), + // #9029: the linked-issue set observed AT violation time. Lets a later pass distinguish "the issue's state + // changed underneath the contributor" (same issues still linked, live rules now pass ⇒ exonerate) from + // "the author edited the link to dodge the remembered violation" (issue set changed ⇒ violation stands). + linkedIssueHardRuleViolationIssuesJson: text("linked_issue_hard_rule_violation_issues_json"), // Visual-capture gate satisfaction (#4110): the head SHA at which the bot's before/after capture pipeline // (review.visual.enabled) last produced a REAL before+after render pair (not a placeholder/failed/pending // shot) for this PR. Lets the deterministic screenshotTableGate treat a successful automated capture as diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 68eb09632f..0cb7e31398 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2877,6 +2877,54 @@ function dedupeLatestCheckRunsByIdentity(checkRuns: ReadonlyArray