diff --git a/migrations/0184_linked_issue_violation_snapshot.sql b/migrations/0184_linked_issue_violation_snapshot.sql new file mode 100644 index 000000000..a4d602214 --- /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 ad9543e0d..692db5bf6 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 cd23d68d9..46e156ba2 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 68eb09632..0cb7e3139 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2877,6 +2877,54 @@ function dedupeLatestCheckRunsByIdentity(checkRuns: ReadonlyArray