From a321466955f345f37f5e461b6cefdfc201351f5c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:29:17 -0700 Subject: [PATCH 1/4] fix(db): index retention columns and add delete paths for four never-pruned caches (#9083) No RETENTION_POLICY table had a leading index on its retention timestamp, so pruneExpiredRecords's ctid-keyed batched delete forced a full sequential scan of the whole table on Postgres per batch -- large tables blew the prune job's 30-minute timeout and retention fell permanently behind (signal_snapshots alone reached 787MB at ~44KB/row). Rewrite the delete to range over a real, indexable primary key ordered by the retention column instead of rowid/ctid, and add the matching leading index for every policy table via migration 0191. Also close the "no delete path at all" gap for four read-side caches (grounding_file_content_cache, ai_review_cache, ai_slop_cache, linked_issue_satisfaction_cache) that only enforced their TTL at the read site, and add retention for three previously-untracked append-only logs (review_audit, decision_records, orb_webhook_events). Deliberately deferred: pull_request_files, check_summaries, gate_outcomes, agent_runs, and advisories are current-state tables (upserted per natural key, read as live state for open PRs/checks) rather than append-only logs, so they are excluded from this pass per RETENTION_POLICY's own documented scope -- pruning them needs a read-pattern review this PR doesn't do. --- migrations/0191_retention_column_indexes.sql | 33 ++++ src/db/retention.ts | 64 +++++++- src/db/schema.ts | 145 ++++++++++++------ test/unit/retention.test.ts | 149 ++++++++++++++++++- 4 files changed, 344 insertions(+), 47 deletions(-) create mode 100644 migrations/0191_retention_column_indexes.sql diff --git a/migrations/0191_retention_column_indexes.sql b/migrations/0191_retention_column_indexes.sql new file mode 100644 index 000000000..4fa40ebf0 --- /dev/null +++ b/migrations/0191_retention_column_indexes.sql @@ -0,0 +1,33 @@ +-- #9083: no RETENTION_POLICY (src/db/retention.ts) table had an index whose LEADING column is its +-- retention timestamp -- every existing index on these tables either doesn't exist at all +-- (webhook_events, signal_snapshots, score_previews, repo_snapshots) or has the timestamp only in a +-- trailing position (audit_events, ai_usage_events, product_usage_events, +-- github_rate_limit_observations, agent_context_snapshots, notification_deliveries). Paired with the +-- pruneExpiredRecords rewrite (PK-ordered range delete instead of a rowid/ctid semi-join), the inner +-- SELECT in each batched delete is now an index range scan instead of a full table scan. +CREATE INDEX IF NOT EXISTS idx_webhook_events_received_at ON webhook_events(received_at); +CREATE INDEX IF NOT EXISTS idx_audit_events_created_at ON audit_events(created_at); +CREATE INDEX IF NOT EXISTS idx_ai_usage_events_created_at ON ai_usage_events(created_at); +CREATE INDEX IF NOT EXISTS idx_product_usage_events_occurred_at ON product_usage_events(occurred_at); +CREATE INDEX IF NOT EXISTS idx_github_rate_limit_observations_observed_at ON github_rate_limit_observations(observed_at); +CREATE INDEX IF NOT EXISTS idx_signal_snapshots_generated_at ON signal_snapshots(generated_at); +CREATE INDEX IF NOT EXISTS idx_score_previews_generated_at ON score_previews(generated_at); +CREATE INDEX IF NOT EXISTS idx_repo_snapshots_fetched_at ON repo_snapshots(fetched_at); +CREATE INDEX IF NOT EXISTS idx_agent_context_snapshots_created_at ON agent_context_snapshots(created_at); +CREATE INDEX IF NOT EXISTS idx_notification_deliveries_created_at ON notification_deliveries(created_at); +CREATE INDEX IF NOT EXISTS idx_predicted_gate_calls_created_at ON predicted_gate_calls(created_at); + +-- #9083: the four never-pruned caches (grounding_file_content_cache, ai_review_cache, ai_slop_cache, +-- linked_issue_satisfaction_cache) newly gained a retention rule in the same change -- index their +-- timestamp column too so the new prune sweep stays cheap as they grow. +CREATE INDEX IF NOT EXISTS idx_grounding_file_content_cache_fetched_at ON grounding_file_content_cache(fetched_at); +CREATE INDEX IF NOT EXISTS idx_ai_review_cache_created_at ON ai_review_cache(created_at); +CREATE INDEX IF NOT EXISTS idx_ai_slop_cache_created_at ON ai_slop_cache(created_at); +CREATE INDEX IF NOT EXISTS idx_linked_issue_satisfaction_cache_created_at ON linked_issue_satisfaction_cache(created_at); + +-- #9083: the three newly-retained append-only tables (review_audit, decision_records, orb_webhook_events) +-- each already had an index touching their timestamp column, but only in a trailing/combined position -- +-- add the leading single-column index the new PK-ordered delete needs. +CREATE INDEX IF NOT EXISTS idx_review_audit_created_at ON review_audit(created_at); +CREATE INDEX IF NOT EXISTS idx_decision_records_created_at ON decision_records(created_at); +CREATE INDEX IF NOT EXISTS idx_orb_webhook_events_received_at ON orb_webhook_events(received_at); diff --git a/src/db/retention.ts b/src/db/retention.ts index 785753b60..896f53be8 100644 --- a/src/db/retention.ts +++ b/src/db/retention.ts @@ -33,8 +33,63 @@ export const RETENTION_POLICY: readonly RetentionRule[] = [ // driven growth -- predicted-gate-calls.ts deliberately never dedups at write time, see that file's own // header comment) -- same 90-day append-only-log window as audit_events/ai_usage_events above. { table: "predicted_gate_calls", column: "created_at", days: 90 }, + // #9083: four read-side caches with a TTL enforced only at the READ site (getCachedGroundingFileContent / + // getCachedAiReview / getCachedAiSlopFinding / getCachedLinkedIssueSatisfaction) and NO delete path at + // all -- every row lives forever once written. grounding_file_content_cache keys on (repo, path, + // head_sha), so every push mints a fresh head_sha and permanently strands the prior push's blobs (a 20-push + // PR with 15 files leaves 300 dead rows) -- hence the much shorter 2-day window (its own read-side TTL is + // 24h, so 2 days never deletes a row a read could still legitimately hit). The other three key on + // (repo, pull, head_sha[, ...]) with the same "new head_sha orphans the old row" shape but a lower + // per-row footprint (JSON, not file bodies), so they get the same 30-day window as the other + // moderate-volume caches. + { table: "grounding_file_content_cache", column: "fetched_at", days: 2 }, + { table: "ai_review_cache", column: "created_at", days: 30 }, + { table: "ai_slop_cache", column: "created_at", days: 30 }, + { table: "linked_issue_satisfaction_cache", column: "created_at", days: 30 }, + // #9083: shadow-parity audit trail (src/review/parity.ts) -- pure history once a decision is recorded, + // same append-only-log shape and window as audit_events/webhook_events. + { table: "review_audit", column: "created_at", days: 90 }, + // #9083: content-addressed decision records (#8836) are a contributor's evidentiary trail for "clause X + // closed me" -- kept longer than the plain operational logs above so a dispute raised weeks after a + // close still has its record, matching product_usage_events' 180-day window for the same reason. + { table: "decision_records", column: "created_at", days: 180 }, + // #9083: the central Orb App's OWN webhook delivery/dedup log (separate from webhook_events, which is the + // per-repo review app's) -- identical short-lived-idempotency shape, same 90d window. + { table: "orb_webhook_events", column: "received_at", days: 90 }, ]; +// #9083: a real, single-column, indexable primary key for the ordered-range delete below, keyed by table +// name -- NOT the SQLite rowid / Postgres ctid pseudo-column pruneExpiredRecords used to delete by +// exclusively. ctid is a physical row locator, not a value an index can be built over, so +// `ctid IN (SELECT ctid ... LIMIT n)` forces Postgres to Seq-Scan the ENTIRE outer table for every batch +// (the TID-scan fast path only fires for a constant qual, not a correlated subquery) -- on a +// multi-hundred-thousand-row table that scan alone can exceed prune-retention's 30-minute job timeout, +// permanently stalling retention. Every table above with a genuine single-column id gets that column here, +// paired with a leading index on its retention timestamp column (see the accompanying migration) so the +// inner SELECT is an index range scan, not a scan of its own. A table absent from this map (a composite +// primary key, or a caller-supplied ad-hoc rule in a test) falls back to rowid/ctid -- still correct, just +// not scan-optimal, which is acceptable for the lower row counts of the tables that fall back today. +const RETENTION_PK_COLUMN: Readonly> = { + audit_events: "id", + ai_usage_events: "id", + product_usage_events: "id", + github_rate_limit_observations: "id", + signal_snapshots: "id", + score_previews: "id", + repo_snapshots: "id", + agent_context_snapshots: "id", + webhook_events: "delivery_id", + notification_deliveries: "id", + predicted_gate_calls: "id", + review_audit: "id", + decision_records: "id", + orb_webhook_events: "delivery_id", +}; + +function pkColumnFor(table: string): string { + return RETENTION_PK_COLUMN[table] ?? "rowid"; +} + export type PruneResult = { table: string; column: string; cutoff: string; deleted: number }; const SAFE_IDENTIFIER = /^[a-z_]+$/; @@ -87,9 +142,14 @@ export async function pruneExpiredRecords( } let deleted = 0; - // Batched delete by rowid so each statement is bounded; loop until a short batch or the per-run cap. + // Batched delete by a real indexable PK (see RETENTION_PK_COLUMN) ordered by the retention column, so + // each statement is bounded AND the inner SELECT is an index range scan on Postgres, not a ctid-keyed + // correlated subquery that forces a full seq scan of the outer table (#9083). + const pk = pkColumnFor(rule.table); for (;;) { - const result = await env.DB.prepare(`DELETE FROM ${rule.table} WHERE rowid IN (SELECT rowid FROM ${rule.table} WHERE ${retentionWhere(rule)} LIMIT ${batchSize})`) + const result = await env.DB.prepare( + `DELETE FROM ${rule.table} WHERE ${pk} IN (SELECT ${pk} FROM ${rule.table} WHERE ${retentionWhere(rule)} ORDER BY ${rule.column} LIMIT ${batchSize})`, + ) .bind(cutoff) .run(); const changes = Number(result.meta?.changes ?? 0); diff --git a/src/db/schema.ts b/src/db/schema.ts index 82df0178d..969743ae9 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -208,6 +208,9 @@ export const githubRateLimitObservations = sqliteTable( admissionObserved: index("github_rate_limit_observations_admission_observed_idx").on(table.admissionKey, table.observedAt), repoObserved: index("github_rate_limit_observations_repo_observed_idx").on(table.repoFullName, table.observedAt), reset: index("github_rate_limit_observations_reset_idx").on(table.resetAt), + // #9083: a leading index on the retention column itself -- the three above all lead with a different + // column, so RETENTION_POLICY's prune of this table had no usable index (migration 0191). + observed: index("idx_github_rate_limit_observations_observed_at").on(table.observedAt), }), ); @@ -308,19 +311,26 @@ export const repoLabels = sqliteTable( }), ); -export const repoSnapshots = sqliteTable("repo_snapshots", { - id: text("id").primaryKey(), - repoFullName: text("repo_full_name").notNull(), - snapshotKind: text("snapshot_kind").notNull(), - sourceKind: text("source_kind").notNull().default("github"), - fetchedAt: text("fetched_at").notNull(), - primaryLanguage: text("primary_language"), - defaultBranch: text("default_branch"), - openIssuesCount: integer("open_issues_count").notNull().default(0), - openPullRequestsCount: integer("open_pull_requests_count").notNull().default(0), - recentMergedPullRequestsCount: integer("recent_merged_pull_requests_count").notNull().default(0), - payloadJson: text("payload_json").notNull().default("{}"), -}); +export const repoSnapshots = sqliteTable( + "repo_snapshots", + { + id: text("id").primaryKey(), + repoFullName: text("repo_full_name").notNull(), + snapshotKind: text("snapshot_kind").notNull(), + sourceKind: text("source_kind").notNull().default("github"), + fetchedAt: text("fetched_at").notNull(), + primaryLanguage: text("primary_language"), + defaultBranch: text("default_branch"), + openIssuesCount: integer("open_issues_count").notNull().default(0), + openPullRequestsCount: integer("open_pull_requests_count").notNull().default(0), + recentMergedPullRequestsCount: integer("recent_merged_pull_requests_count").notNull().default(0), + payloadJson: text("payload_json").notNull().default("{}"), + }, + (table) => ({ + // #9083: no index at all previously -- RETENTION_POLICY's 90-day prune was an unindexed full scan (migration 0191). + fetched: index("idx_repo_snapshots_fetched_at").on(table.fetchedAt), + }), +); export const registrySnapshots = sqliteTable("registry_snapshots", { id: text("id").primaryKey(), @@ -649,14 +659,22 @@ export const collisionEdges = sqliteTable("collision_edges", { generatedAt: text("generated_at").notNull().$defaultFn(() => nowIso()), }); -export const signalSnapshots = sqliteTable("signal_snapshots", { - id: text("id").primaryKey(), - signalType: text("signal_type").notNull(), - targetKey: text("target_key").notNull(), - repoFullName: text("repo_full_name"), - payloadJson: text("payload_json").notNull().default("{}"), - generatedAt: text("generated_at").notNull().$defaultFn(() => nowIso()), -}); +export const signalSnapshots = sqliteTable( + "signal_snapshots", + { + id: text("id").primaryKey(), + signalType: text("signal_type").notNull(), + targetKey: text("target_key").notNull(), + repoFullName: text("repo_full_name"), + payloadJson: text("payload_json").notNull().default("{}"), + generatedAt: text("generated_at").notNull().$defaultFn(() => nowIso()), + }, + (table) => ({ + // #9083: signal_snapshots had NO index at all -- RETENTION_POLICY's 90-day prune of this table (its + // largest, at ~44KB/row) was an unindexed full scan (migration 0191). + generated: index("idx_signal_snapshots_generated_at").on(table.generatedAt), + }), +); export const agentRuns = sqliteTable( "agent_runs", @@ -723,6 +741,9 @@ export const agentContextSnapshots = sqliteTable( }, (table) => ({ runCreated: index("agent_context_snapshots_run_created_idx").on(table.runId, table.createdAt), + // #9083: the above leads with run_id -- RETENTION_POLICY's 30-day prune had no index leading with + // created_at alone (migration 0191). + created: index("idx_agent_context_snapshots_created_at").on(table.createdAt), }), ); @@ -897,18 +918,27 @@ export const advisories = sqliteTable("advisories", { updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); -export const webhookEvents = sqliteTable("webhook_events", { - deliveryId: text("delivery_id").primaryKey(), - eventName: text("event_name").notNull(), - action: text("action"), - installationId: integer("installation_id"), - repositoryFullName: text("repository_full_name"), - payloadHash: text("payload_hash").notNull(), - status: text("status").notNull(), - errorSummary: text("error_summary"), - receivedAt: text("received_at").notNull().$defaultFn(() => nowIso()), - processedAt: text("processed_at"), -}); +export const webhookEvents = sqliteTable( + "webhook_events", + { + deliveryId: text("delivery_id").primaryKey(), + eventName: text("event_name").notNull(), + action: text("action"), + installationId: integer("installation_id"), + repositoryFullName: text("repository_full_name"), + payloadHash: text("payload_hash").notNull(), + status: text("status").notNull(), + errorSummary: text("error_summary"), + receivedAt: text("received_at").notNull().$defaultFn(() => nowIso()), + processedAt: text("processed_at"), + }, + (table) => ({ + status: index("webhook_events_status_idx").on(table.status), + // #9083: only the status index above existed before -- RETENTION_POLICY's 90-day prune had no index on + // received_at at all (migration 0191). + received: index("idx_webhook_events_received_at").on(table.receivedAt), + }), +); export const orbRelayPending = sqliteTable( "orb_relay_pending", @@ -961,17 +991,24 @@ export const scoringModelSnapshots = sqliteTable("scoring_model_snapshots", { payloadJson: text("payload_json").notNull().default("{}"), }); -export const scorePreviews = sqliteTable("score_previews", { - id: text("id").primaryKey(), - scoringModelSnapshotId: text("scoring_model_snapshot_id").notNull(), - repoFullName: text("repo_full_name").notNull(), - targetType: text("target_type").notNull(), - targetKey: text("target_key").notNull(), - contributorLogin: text("contributor_login"), - inputJson: text("input_json").notNull().default("{}"), - resultJson: text("result_json").notNull().default("{}"), - generatedAt: text("generated_at").notNull().$defaultFn(() => nowIso()), -}); +export const scorePreviews = sqliteTable( + "score_previews", + { + id: text("id").primaryKey(), + scoringModelSnapshotId: text("scoring_model_snapshot_id").notNull(), + repoFullName: text("repo_full_name").notNull(), + targetType: text("target_type").notNull(), + targetKey: text("target_key").notNull(), + contributorLogin: text("contributor_login"), + inputJson: text("input_json").notNull().default("{}"), + resultJson: text("result_json").notNull().default("{}"), + generatedAt: text("generated_at").notNull().$defaultFn(() => nowIso()), + }, + (table) => ({ + // #9083: no index at all previously -- RETENTION_POLICY's 90-day prune was an unindexed full scan (migration 0191). + generated: index("idx_score_previews_generated_at").on(table.generatedAt), + }), +); export const contributorEvidence = sqliteTable("contributor_evidence", { login: text("login").primaryKey(), @@ -1196,6 +1233,9 @@ export const notificationDeliveries = sqliteTable( dedupChannel: uniqueIndex("notification_deliveries_dedup_channel_unique").on(table.dedupKey, table.channel), recipientStatus: index("notification_deliveries_recipient_status_idx").on(table.recipientLogin, table.status), recipientChannelCreated: index("notification_deliveries_recipient_channel_created_idx").on(table.recipientLogin, table.channel, table.createdAt), + // #9083: the three above all lead with recipient_login/dedup_key -- RETENTION_POLICY's 90-day prune had + // no index leading with created_at alone (migration 0191). + created: index("idx_notification_deliveries_created_at").on(table.createdAt), }), ); @@ -1281,6 +1321,9 @@ export const auditEvents = sqliteTable( actorCreated: index("audit_events_actor_created_idx").on(table.actor, table.createdAt), routeCreated: index("audit_events_route_created_idx").on(table.route, table.createdAt), targetKeyCreated: index("audit_events_target_key_created_idx").on(table.targetKey, table.createdAt), + // #9083: the four above all lead with a different column -- RETENTION_POLICY's 90-day prune had no + // index leading with created_at alone (migration 0191). + created: index("idx_audit_events_created_at").on(table.createdAt), }), ); @@ -1313,6 +1356,9 @@ export const productUsageEvents = sqliteTable( eventOccurred: index("product_usage_events_event_occurred_idx").on(table.eventName, table.occurredAt), actorOccurred: index("product_usage_events_actor_occurred_idx").on(table.actorHash, table.occurredAt), repoOccurred: index("product_usage_events_repo_occurred_idx").on(table.repoFullName, table.occurredAt), + // #9083: the five above all lead with a different column -- RETENTION_POLICY's 180-day prune had no + // index leading with occurred_at alone (migration 0191). + occurred: index("idx_product_usage_events_occurred_at").on(table.occurredAt), }), ); @@ -1381,6 +1427,9 @@ export const aiUsageEvents = sqliteTable( // Covers the daily-budget query (sumAiEstimatedNeuronsSince): WHERE status='ok' AND created_at >= ?. // Without it that aggregate full-scans ai_usage_events, which runs on every AI review/summary. statusCreated: index("ai_usage_events_status_created_idx").on(table.status, table.createdAt), + // #9083: the four above all lead with a different column -- RETENTION_POLICY's 90-day prune had no + // index leading with created_at alone (migration 0191). + created: index("idx_ai_usage_events_created_at").on(table.createdAt), }), ); @@ -1408,6 +1457,8 @@ export const aiReviewCache = sqliteTable( }, (table) => ({ primary: primaryKey({ columns: [table.repoFullName, table.pullNumber, table.headSha] }), + // #9083: this cache had a read-side TTL but no delete path at all -- now pruned via RETENTION_POLICY. + created: index("idx_ai_review_cache_created_at").on(table.createdAt), }), ); @@ -1432,6 +1483,8 @@ export const aiSlopCache = sqliteTable( }, (table) => ({ primary: primaryKey({ columns: [table.repoFullName, table.pullNumber, table.headSha] }), + // #9083: this cache had no delete path at all -- now pruned via RETENTION_POLICY. + created: index("idx_ai_slop_cache_created_at").on(table.createdAt), }), ); @@ -1458,6 +1511,8 @@ export const linkedIssueSatisfactionCache = sqliteTable( }, (table) => ({ primary: primaryKey({ columns: [table.repoFullName, table.pullNumber, table.headSha, table.linkedIssueNumber] }), + // #9083: this cache had no delete path at all -- now pruned via RETENTION_POLICY. + created: index("idx_linked_issue_satisfaction_cache_created_at").on(table.createdAt), }), ); @@ -1489,6 +1544,8 @@ export const groundingFileContentCache = sqliteTable( }, (table) => ({ primary: primaryKey({ columns: [table.repoFullName, table.path, table.headSha] }), + // #9083: this cache had a read-side 24h TTL but no delete path at all -- now pruned via RETENTION_POLICY. + fetched: index("idx_grounding_file_content_cache_fetched_at").on(table.fetchedAt), }), ); diff --git a/test/unit/retention.test.ts b/test/unit/retention.test.ts index 1df140982..f2d877f0d 100644 --- a/test/unit/retention.test.ts +++ b/test/unit/retention.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from "vitest"; import { createApp } from "../../src/api/routes"; import { getDb } from "../../src/db/client"; import { dedupeSignalSnapshots, pruneExpiredRecords, RETENTION_POLICY } from "../../src/db/retention"; -import { agentContextSnapshots, aiUsageEvents, webhookEvents } from "../../src/db/schema"; +import { agentContextSnapshots, aiReviewCache, aiSlopCache, aiUsageEvents, groundingFileContentCache, linkedIssueSatisfactionCache, webhookEvents } from "../../src/db/schema"; import { processJob, runRetentionPrune } from "../../src/queue/processors"; import { REPO_FOCUS_MANIFEST_SIGNAL, REPO_PUBLIC_FOCUS_MANIFEST_SIGNAL } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; @@ -192,6 +192,153 @@ describe("pruneExpiredRecords", () => { const rows = await env.DB.prepare("SELECT id FROM predicted_gate_calls").all<{ id: string }>(); expect(rows.results.map((row) => row.id)).toEqual(["pgc-recent"]); }); + + // #9083: four read-side caches had a read-side TTL but no delete path anywhere — every row lived forever. + // Each now has a RETENTION_POLICY entry; these regression tests exercise the actual policy windows (not + // an ad-hoc override) so a future edit to RETENTION_POLICY that drops one of these entries fails loudly. + it("prunes grounding_file_content_cache older than its 2-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + const db = getDb(env.DB); + await db.insert(groundingFileContentCache).values([ + { repoFullName: "acme/widgets", path: "a.ts", headSha: "sha-old", content: "old", fetchedAt: daysAgo(3) }, + { repoFullName: "acme/widgets", path: "a.ts", headSha: "sha-new", content: "new", fetchedAt: daysAgo(1) }, + ]); + + const rule = RETENTION_POLICY.find((r) => r.table === "grounding_file_content_cache"); + expect(rule).toEqual({ table: "grounding_file_content_cache", column: "fetched_at", days: 2 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT head_sha FROM grounding_file_content_cache").all<{ head_sha: string }>(); + expect(rows.results.map((row) => row.head_sha)).toEqual(["sha-new"]); + }); + + it("prunes ai_review_cache and ai_slop_cache older than their 30-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + const db = getDb(env.DB); + await db.insert(aiReviewCache).values([ + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-old", aiReviewMode: "full", notes: "n", reviewerCount: 1, createdAt: daysAgo(40) }, + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-new", aiReviewMode: "full", notes: "n", reviewerCount: 1, createdAt: daysAgo(1) }, + ]); + await db.insert(aiSlopCache).values([ + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-old", inputFingerprint: "f", status: "ok", createdAt: daysAgo(40) }, + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-new", inputFingerprint: "f", status: "ok", createdAt: daysAgo(1) }, + ]); + + const reviewRule = RETENTION_POLICY.find((r) => r.table === "ai_review_cache"); + const slopRule = RETENTION_POLICY.find((r) => r.table === "ai_slop_cache"); + expect(reviewRule).toEqual({ table: "ai_review_cache", column: "created_at", days: 30 }); + expect(slopRule).toEqual({ table: "ai_slop_cache", column: "created_at", days: 30 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [reviewRule!, slopRule!] }); + expect(results.find((r) => r.table === "ai_review_cache")?.deleted).toBe(1); + expect(results.find((r) => r.table === "ai_slop_cache")?.deleted).toBe(1); + const reviewRows = await env.DB.prepare("SELECT head_sha FROM ai_review_cache").all<{ head_sha: string }>(); + expect(reviewRows.results.map((row) => row.head_sha)).toEqual(["sha-new"]); + const slopRows = await env.DB.prepare("SELECT head_sha FROM ai_slop_cache").all<{ head_sha: string }>(); + expect(slopRows.results.map((row) => row.head_sha)).toEqual(["sha-new"]); + }); + + it("prunes linked_issue_satisfaction_cache older than its 30-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + const db = getDb(env.DB); + await db.insert(linkedIssueSatisfactionCache).values([ + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-old", linkedIssueNumber: 5, inputFingerprint: "f", status: "ok", createdAt: daysAgo(40) }, + { repoFullName: "acme/widgets", pullNumber: 1, headSha: "sha-new", linkedIssueNumber: 5, inputFingerprint: "f", status: "ok", createdAt: daysAgo(1) }, + ]); + + const rule = RETENTION_POLICY.find((r) => r.table === "linked_issue_satisfaction_cache"); + expect(rule).toEqual({ table: "linked_issue_satisfaction_cache", column: "created_at", days: 30 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT head_sha FROM linked_issue_satisfaction_cache").all<{ head_sha: string }>(); + expect(rows.results.map((row) => row.head_sha)).toEqual(["sha-new"]); + }); + + // #9083: review_audit, decision_records, and orb_webhook_events were append-only logs with no retention + // rule at all. All three are raw-SQL-only tables (RAW_SQL_ONLY_TABLES in check-schema-drift.ts), so seed + // via raw SQL like the predicted_gate_calls test above. + it("prunes review_audit older than its 90-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at) + VALUES + ('ra-old', 'acme/widgets', 'acme/widgets#1', 'gate_decision', 'merge', 'loopover-native', 'sha1', 'success', ?), + ('ra-recent', 'acme/widgets', 'acme/widgets#1', 'gate_decision', 'merge', 'loopover-native', 'sha2', 'success', ?)`, + ) + .bind(daysAgo(100), daysAgo(1)) + .run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "review_audit"); + expect(rule).toEqual({ table: "review_audit", column: "created_at", days: 90 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT id FROM review_audit").all<{ id: string }>(); + expect(rows.results.map((row) => row.id)).toEqual(["ra-recent"]); + }); + + it("prunes decision_records older than its 180-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES + ('dr-old', 'acme/widgets', 1, 'sha1', 'close', 'missing_linked_issue', 'digest1', '{}', ?), + ('dr-recent', 'acme/widgets', 1, 'sha2', 'close', 'missing_linked_issue', 'digest2', '{}', ?)`, + ) + .bind(daysAgo(200), daysAgo(1)) + .run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "decision_records"); + expect(rule).toEqual({ table: "decision_records", column: "created_at", days: 180 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT id FROM decision_records").all<{ id: string }>(); + expect(rows.results.map((row) => row.id)).toEqual(["dr-recent"]); + }); + + it("prunes orb_webhook_events older than its 90-day window and keeps recent rows (#9083)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO orb_webhook_events (delivery_id, event_name, payload_hash, status, received_at) + VALUES + ('owe-old', 'push', 'h', 'processed', ?), + ('owe-recent', 'push', 'h', 'processed', ?)`, + ) + .bind(daysAgo(100), daysAgo(1)) + .run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "orb_webhook_events"); + expect(rule).toEqual({ table: "orb_webhook_events", column: "received_at", days: 90 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT delivery_id FROM orb_webhook_events").all<{ delivery_id: string }>(); + expect(rows.results.map((row) => row.delivery_id)).toEqual(["owe-recent"]); + }); + + // #9083: pruneExpiredRecords now deletes via a real PK (RETENTION_PK_COLUMN) ordered by the retention + // column instead of rowid/ctid, so this pins that a table absent from that map (composite-PK caches, or + // any future ad-hoc policy entry) still falls back to the rowid path correctly rather than erroring. + it("falls back to rowid ordering for a table with no RETENTION_PK_COLUMN entry", async () => { + const env = createTestEnv(); + const db = getDb(env.DB); + await db.insert(aiReviewCache).values([ + { repoFullName: "acme/widgets", pullNumber: 2, headSha: "sha-a", aiReviewMode: "full", notes: "n", reviewerCount: 1, createdAt: daysAgo(50) }, + { repoFullName: "acme/widgets", pullNumber: 2, headSha: "sha-b", aiReviewMode: "full", notes: "n", reviewerCount: 1, createdAt: daysAgo(45) }, + ]); + // ai_review_cache has a composite primary key (repo, pull, head_sha) so it is intentionally absent from + // RETENTION_PK_COLUMN; the delete must still succeed via the rowid fallback. + const results = await pruneExpiredRecords(env, { + nowMs: NOW, + policy: [{ table: "ai_review_cache", column: "created_at", days: 30 }], + }); + expect(results[0]?.deleted).toBe(2); + const remaining = await env.DB.prepare("SELECT count(*) AS n FROM ai_review_cache").first<{ n: number }>(); + expect(remaining?.n).toBe(0); + }); }); describe("dedupeSignalSnapshots", () => { From 071f7491dbe9ff8f12bf7aeba62802f36f7f17c1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:33:57 -0700 Subject: [PATCH 2/4] fix(webhook): stop treating stuck queued/superseded deliveries as permanent duplicates (#9054) The dedup guard in enqueueWebhookByEnv only ever exempted 'error' rows from suppression. A row left at 'queued' (the insert happened but WEBHOOKS.send() was lost) or 'superseded' (overwritten by a later coalesced delivery) could never be redelivered: every GitHub retry and every operator "Redeliver" click carries the same delivery_id + payload hash, hits the guard, and is silently discarded as a no-op duplicate forever -- including check_suite.completed, the maybeReReviewOnCiCompletion auto-merge trigger. getWebhookEvent now also returns receivedAt, and a 'queued'/'superseded' row past a 10-minute staleness window is treated the same as an 'error' row: never suppressed. A migration purges the historical backlog of rows this bug already stuck permanently (dead by definition once a full day old), since replaying them is not useful and the code fix means no future delivery can get stuck this way again. Deferred: an active cron sweep that proactively re-triggers GitHub's own redelivery API for still-stuck rows was not implemented -- webhook_events does not persist the raw payload (by design, to avoid growing the exact blob-retention problem #9083 addresses), and this fix already ensures any future stuck delivery is redeliverable well inside GitHub's redelivery window without needing an active poke. --- ...0192_purge_stuck_queued_webhook_events.sql | 24 +++++ src/db/repositories.ts | 5 ++ src/github/webhook.ts | 25 +++++- test/unit/webhook.test.ts | 87 +++++++++++++++++++ 4 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 migrations/0192_purge_stuck_queued_webhook_events.sql diff --git a/migrations/0192_purge_stuck_queued_webhook_events.sql b/migrations/0192_purge_stuck_queued_webhook_events.sql new file mode 100644 index 000000000..e84746d8a --- /dev/null +++ b/migrations/0192_purge_stuck_queued_webhook_events.sql @@ -0,0 +1,24 @@ +-- #9054: one-off purge of webhook_events rows PERMANENTLY stuck at 'queued'/'superseded' by the dedup-guard +-- bug fixed alongside this migration (src/github/webhook.ts's enqueueWebhookByEnv). Before that fix, a row +-- left at 'queued' (the insert happened but WEBHOOKS.send() was lost, or the enqueued job vanished) or +-- 'superseded' (overwritten by a later coalesced delivery before either was claimed) could NEVER be +-- redelivered: every GitHub retry and every operator "Redeliver" click carries the identical delivery_id and +-- (usually) the identical payload hash, hit the dedup guard, and was silently discarded as a no-op +-- duplicate, forever. +-- +-- On the live box this incident left 5,547 such rows (4,900 of them check_suite.completed -- the +-- maybeReReviewOnCiCompletion auto-merge trigger), all from a ~3.5-week cutover-era window that was already +-- quiescent by the time this was diagnosed. Replaying them is not useful: the code-level fix means any +-- FUTURE stuck delivery becomes redeliverable after STALE_QUEUED_WEBHOOK_MS (10 minutes) rather than never, +-- so there is no ongoing gap to backfill, and the PRs behind this historical backlog are long since +-- resolved one way or another. Simply deleting them (rather than replaying) is what the issue itself called +-- for, so the stuck-delivery metric is meaningful again going forward. +-- +-- Applies once, wherever/whenever this migration runs: any 'queued'/'superseded' row still unprocessed a +-- full day after receipt is dead by definition (real processing completes in well under a second), so this +-- is safe as a general one-time data-hygiene sweep on any environment, and a no-op on a fresh database with +-- no history to purge. +DELETE FROM webhook_events + WHERE status IN ('queued', 'superseded') + AND processed_at IS NULL + AND received_at < datetime('now', '-1 day'); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 12666adfa..f2c22755e 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -6687,6 +6687,10 @@ export async function getWebhookEvent( deliveryId: string; payloadHash: string; status: string; + // #9054: receivedAt lets the dedup guard (src/github/webhook.ts) tell a genuinely fresh duplicate apart + // from a row permanently stuck at 'queued'/'superseded' -- without it, a stuck row could never be + // redelivered (every retry/manual "Redeliver" was silently discarded as a no-op duplicate, forever). + receivedAt: string; } | null> { const db = getDb(env.DB); const [row] = await db.select().from(webhookEvents).where(eq(webhookEvents.deliveryId, deliveryId)).limit(1); @@ -6695,6 +6699,7 @@ export async function getWebhookEvent( deliveryId: row.deliveryId, payloadHash: row.payloadHash, status: row.status, + receivedAt: row.receivedAt, }; } diff --git a/src/github/webhook.ts b/src/github/webhook.ts index c7fda17a7..71537b675 100644 --- a/src/github/webhook.ts +++ b/src/github/webhook.ts @@ -10,6 +10,20 @@ import { getSelfHostRequestTraceParent } from "../selfhost/trace-context"; import { isNonActionableWebhookNoise } from "./self-authored"; const DEFAULT_MAX_WEBHOOK_BODY_BYTES = 1024 * 1024; +// #9054: how long a 'queued'/'superseded' webhook_events row may sit unprocessed before a redelivery of the +// SAME delivery_id is allowed to bypass the dedup guard below. Real processing (record row -> WEBHOOKS.send) +// completes in well under a second, so 10 minutes is generous headroom against a merely-slow-but-still- +// in-flight delivery while still being far short of "permanently stuck." +const STALE_QUEUED_WEBHOOK_MS = 10 * 60 * 1000; + +/** True once `receivedAt` is older than {@link STALE_QUEUED_WEBHOOK_MS}. A malformed/unparseable timestamp + * (should never happen -- receivedAt is always written by nowIso()) fails closed to "not stale" so it never + * weakens the existing dedup guard. */ +function isStaleUnprocessedWebhook(receivedAt: string): boolean { + const receivedMs = Date.parse(receivedAt); + if (Number.isNaN(receivedMs)) return false; + return Date.now() - receivedMs > STALE_QUEUED_WEBHOOK_MS; +} const WEBHOOK_METRIC_EVENTS = new Set([ "check_run", "check_suite", @@ -157,11 +171,20 @@ export async function enqueueWebhookByEnv(env: Env, deliveryId: string, eventNam } const existingEvent = await getWebhookEvent(env, deliveryId); + // #9054: a 'queued'/'superseded' row that has sat unprocessed past STALE_QUEUED_WEBHOOK_MS is treated the + // same as an 'error' row below -- never suppressed. The queued-insert-then-WEBHOOKS.send() window (or a + // coalesce supersede in pg-queue.ts) can lose the underlying job with no error ever recorded, and without + // this escape hatch that row was PERMANENTLY un-redeliverable: every GitHub retry and every operator + // "Redeliver" click carries the same delivery_id + payload hash, hits this guard, and is silently + // discarded as a no-op duplicate forever. A delivery this stale always wins over reprocessing risk -- + // the request already has the current payload in hand, so re-recording and re-enqueuing it is strictly an + // improvement over leaving the row stuck. + const isStaleStuck = !!existingEvent && (existingEvent.status === "queued" || existingEvent.status === "superseded") && isStaleUnprocessedWebhook(existingEvent.receivedAt); // Suppress redelivery of an already-processed event (on success its payloadHash is overwritten to a // "processed" sentinel, so a hash match alone misses it and the event re-runs its side effects) or one // still in flight with the same payload. "error" rows are never suppressed so a failed enqueue/processing // can still be retried (#789). - if (existingEvent && existingEvent.status !== "error" && (existingEvent.status === "processed" || existingEvent.payloadHash === payloadHash)) { + if (existingEvent && !isStaleStuck && existingEvent.status !== "error" && (existingEvent.status === "processed" || existingEvent.payloadHash === payloadHash)) { recordWebhookEnqueueMetric(eventName, payload.action, "duplicate"); return "duplicate"; } diff --git a/test/unit/webhook.test.ts b/test/unit/webhook.test.ts index 9bb38745b..0a689f843 100644 --- a/test/unit/webhook.test.ts +++ b/test/unit/webhook.test.ts @@ -4,6 +4,7 @@ import { enqueueWebhookByEnv, handleGitHubWebhook, handleOrbRelay } from "../../ import { getWebhookEvent, recordWebhookEvent } from "../../src/db/repositories"; import { relaySignature } from "../../src/orb/relay"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; +import { sha256Hex } from "../../src/utils/crypto"; import { clearSelfHostRequestTraceParent, setSelfHostRequestTraceParent, @@ -259,6 +260,92 @@ describe("github webhook dedup (#789)", () => { expect(sendCount).toBe(0); // not re-enqueued expect(await renderMetrics()).toContain('loopover_webhook_enqueue_total{action="opened",event="pull_request",result="duplicate"} 1'); }); + + // #9054: a 'queued'/'superseded' row with no staleness escape was PERMANENTLY un-redeliverable -- every + // GitHub retry and every operator "Redeliver" click carried the identical delivery_id + payload and was + // silently discarded as a no-op duplicate forever, because the guard above only ever excluded 'error' rows. + it("re-enqueues a delivery stuck 'queued' past the stale window instead of suppressing it as a duplicate (#9054)", async () => { + const env = createTestEnv(); + let sendCount = 0; + env.WEBHOOKS = { + send: async () => { + sendCount += 1; + }, + } as unknown as Queue; + const rawBody = JSON.stringify({ action: "completed", repository: { full_name: "JSONbored/gittensory" } }); + const payloadHash = await sha256Hex(rawBody); + await recordWebhookEvent(env, { deliveryId: "stuck-queued-1", eventName: "check_suite", payloadHash, status: "queued" }); + // Backdate receivedAt past the 10-minute staleness window -- recordWebhookEvent always stamps "now". + await env.DB.prepare("UPDATE webhook_events SET received_at = ? WHERE delivery_id = ?") + .bind(new Date(Date.now() - 11 * 60 * 1000).toISOString(), "stuck-queued-1") + .run(); + + const result = await enqueueWebhookByEnv(env, "stuck-queued-1", "check_suite", rawBody); + + expect(result).toBe("queued"); + expect(sendCount).toBe(1); // actually re-enqueued, not silently discarded + const event = await getWebhookEvent(env, "stuck-queued-1"); + expect(event?.status).toBe("queued"); + }); + + it("re-enqueues a delivery stuck 'superseded' past the stale window instead of suppressing it as a duplicate (#9054)", async () => { + const env = createTestEnv(); + let sendCount = 0; + env.WEBHOOKS = { + send: async () => { + sendCount += 1; + }, + } as unknown as Queue; + const rawBody = JSON.stringify({ action: "synchronize", repository: { full_name: "JSONbored/gittensory" } }); + const payloadHash = await sha256Hex(rawBody); + await recordWebhookEvent(env, { deliveryId: "stuck-superseded-1", eventName: "pull_request", payloadHash, status: "superseded" }); + await env.DB.prepare("UPDATE webhook_events SET received_at = ? WHERE delivery_id = ?") + .bind(new Date(Date.now() - 11 * 60 * 1000).toISOString(), "stuck-superseded-1") + .run(); + + const result = await enqueueWebhookByEnv(env, "stuck-superseded-1", "pull_request", rawBody); + + expect(result).toBe("queued"); + expect(sendCount).toBe(1); + }); + + it("still suppresses a 'queued' row as a duplicate while inside the stale window (no regression)", async () => { + const env = createTestEnv(); + let sendCount = 0; + env.WEBHOOKS = { + send: async () => { + sendCount += 1; + }, + } as unknown as Queue; + const rawBody = JSON.stringify({ action: "completed", repository: { full_name: "JSONbored/gittensory" } }); + const payloadHash = await sha256Hex(rawBody); + // recordWebhookEvent stamps receivedAt as "now" -- freshly queued, well inside the 10-minute window. + await recordWebhookEvent(env, { deliveryId: "fresh-queued-1", eventName: "check_suite", payloadHash, status: "queued" }); + + const result = await enqueueWebhookByEnv(env, "fresh-queued-1", "check_suite", rawBody); + + expect(result).toBe("duplicate"); + expect(sendCount).toBe(0); + }); + + it("treats an unparseable receivedAt as not-stale (defensive fail-closed branch)", async () => { + const env = createTestEnv(); + let sendCount = 0; + env.WEBHOOKS = { + send: async () => { + sendCount += 1; + }, + } as unknown as Queue; + const rawBody = JSON.stringify({ action: "completed", repository: { full_name: "JSONbored/gittensory" } }); + const payloadHash = await sha256Hex(rawBody); + await recordWebhookEvent(env, { deliveryId: "garbage-received-at-1", eventName: "check_suite", payloadHash, status: "queued" }); + await env.DB.prepare("UPDATE webhook_events SET received_at = 'not-a-timestamp' WHERE delivery_id = ?").bind("garbage-received-at-1").run(); + + const result = await enqueueWebhookByEnv(env, "garbage-received-at-1", "check_suite", rawBody); + + expect(result).toBe("duplicate"); // Date.parse -> NaN -> isStaleUnprocessedWebhook fails closed to false + expect(sendCount).toBe(0); + }); }); describe("github webhook queue isolation (#audit-webhook-queue)", () => { From 03cc68aac1ff02e629608631913c830b987320cc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:25:26 -0700 Subject: [PATCH 3/4] fix(governor): consume cadenceFactor to scale rate-limit cadence instead of hard-denying (#9062) selfReputationThrottle's own doc comment promises a soft cadence throttle that "a recovering ratio restores it -- never a hard permanent ban", but the chokepoint ignored cadenceFactor entirely and hard-denied on ANY throttled verdict. Since submissions are the only source of new decided outcomes, and governor_reputation_history never decayed, a miner that hit the throttle band could never submit again to dilute its ratio back down -- an absorbing, permanent self-ban. evaluateGovernorChokepoint now consumes a non-floored cadenceFactor by scaling the per-repo write-rate-limit window instead of denying; the hard deny is reserved for the extreme "floored" ratio only. The miner-lib wrapper advances its rate-limit bucket against the SAME scaled policy the decision was evaluated with, so state stays consistent with the verdict it recorded. governor_reputation_history also gains a 14-day half-life decay (applied at both read and increment time), so even a floored ratio ages back below the sample-size floor over calendar time with zero new submissions, delivering the "recovering ratio" contract the module already documented. --- .../src/governor/chokepoint.ts | 108 ++++++++++++++---- .../loopover-engine/test/chokepoint.test.ts | 35 +++++- .../loopover-miner/lib/governor-chokepoint.ts | 6 +- packages/loopover-miner/lib/governor-state.ts | 43 ++++++- test/unit/miner-attempt-input-builder.test.ts | 34 +++++- test/unit/miner-governor-chokepoint.test.ts | 20 +++- test/unit/miner-governor-state.test.ts | 105 ++++++++++++++++- 7 files changed, 314 insertions(+), 37 deletions(-) diff --git a/packages/loopover-engine/src/governor/chokepoint.ts b/packages/loopover-engine/src/governor/chokepoint.ts index 2ed908dd3..8aa81ba82 100644 --- a/packages/loopover-engine/src/governor/chokepoint.ts +++ b/packages/loopover-engine/src/governor/chokepoint.ts @@ -35,7 +35,7 @@ import { DEFAULT_SELF_REPUTATION_THRESHOLDS, selfReputationThrottle } from "./re import type { OwnSubmissionRecord, SelfPlagiarismCandidate, SelfPlagiarismConfig, SelfPlagiarismVerdict } from "./self-plagiarism.js"; import { DEFAULT_SELF_PLAGIARISM_CONFIG, selfPlagiarismCheck } from "./self-plagiarism.js"; import type { WriteRateLimitBackoffStore, WriteRateLimitBucketStore, WriteRateLimitPolicies, WriteRateLimitVerdict } from "./write-rate-limit.js"; -import { evaluateWriteRateLimit } from "./write-rate-limit.js"; +import { DEFAULT_WRITE_RATE_LIMIT_POLICIES, evaluateWriteRateLimit } from "./write-rate-limit.js"; /** Which stage of the precedence ladder produced the final verdict. */ export type GovernorDecisionStage = @@ -97,6 +97,13 @@ export type GovernorDecisionDetail = { convergence?: PortfolioConvergenceVerdict; reputation?: SelfReputationThrottleDecision; selfPlagiarism?: SelfPlagiarismVerdict; + /** #9062: the rate-limit policies actually used to reach `rateLimit` -- identical to the caller-supplied + * `rateLimitPolicies` (or the engine default) UNLESS a non-floored reputation throttle scaled the + * per-repo window. Present once the rate-limit stage has run (mirrors `rateLimit`'s own presence). The + * stateful miner-lib wrapper (`governor-chokepoint.js`) reads this back to advance the SAME bucket + * arithmetic post-decision that produced this verdict -- reusing the caller's raw, un-scaled policy there + * instead would silently disagree with the decision it is recording state for. */ + effectiveRateLimitPolicies?: WriteRateLimitPolicies; }; export type GovernorDecision = { @@ -110,6 +117,30 @@ export type GovernorDecision = { ledgerEvent: GovernorLedgerEvent; }; +/** #9062: stretch the PER-REPO rate-limit window for `actionClass` by `1 / cadenceFactor` so a throttled + * miner's write cadence on THIS repo degrades proportionally instead of being denied outright -- + * reputation-throttle.ts's own doc comment promises "a recovering ratio restores it -- never a hard + * permanent ban", so consuming `cadenceFactor` here (rather than discarding it, as before) is what actually + * delivers that contract for the common (non-floored) `"throttled"` case. Only the per-repo scope is + * scaled: `RepoOutcomeHistory` is scoped to ONE repo, so a bad ratio there must not also slow the miner's + * unrelated global write velocity across every other repo it works on. Never called with + * `cadenceFactor >= 1` (the caller only invokes this once it has already checked that). */ +function scaleRateLimitPerRepoWindow(policies: WriteRateLimitPolicies, actionClass: string, cadenceFactor: number): WriteRateLimitPolicies { + const perRepoConfig = policies.perRepo[actionClass]; + if (!perRepoConfig) return policies; + // Defensive floor: cadenceFactor is documented as "never 0" by resolveSelfReputationThresholds's clamped + // minCadenceFactor, but that field has no enforced lower bound of its own -- guard the division regardless + // of what a caller-supplied threshold override might set it to. + const safeFactor = cadenceFactor > 0 ? cadenceFactor : 0.001; + return { + ...policies, + perRepo: { + ...policies.perRepo, + [actionClass]: { ...perRepoConfig, windowMs: Math.round(perRepoConfig.windowMs / safeFactor) }, + }, + }; +} + function denyResult(input: { stage: GovernorDecisionStage; reason: string; @@ -184,6 +215,35 @@ export function evaluateGovernorChokepoint(input: GovernorChokepointInput): Gove }; } + const isSelfSubmissionAction = SELF_SUBMISSION_ACTION_CLASSES.has(input.actionClass); + + // #9062: computed here -- BEFORE rate-limit -- rather than after budget/convergence (where its own HARD + // DENY for `reason === "floored"` still stays, preserving the ladder's documented precedence for that + // path) so a non-floored `cadenceFactor` can scale the per-repo rate-limit window below. Discarding + // cadenceFactor and hard-denying on ANY throttled verdict (the pre-#9062 behavior) is exactly what turned a + // recoverable soft throttle into a permanent self-ban: submissions are the miner's only source of new + // decided outcomes, so a miner that can never submit can never dilute a bad ratio back down. + let reputation: SelfReputationThrottleDecision | undefined; + if (isSelfSubmissionAction && input.reputationHistory !== undefined) { + try { + reputation = selfReputationThrottle(input.reputationHistory, input.reputationThresholds ?? DEFAULT_SELF_REPUTATION_THRESHOLDS); + } catch (error) { + return denyResult({ + stage: "internal_error", + reason: `reputation_throttle_calculator_error: ${error instanceof Error ? error.message : String(error)}`, + mode, + detail: baseDetail, + eventType: "denied", + actionClass: input.actionClass, + repoFullName: input.repoFullName, + }); + } + } + const rateLimitPolicies = + reputation && reputation.cadenceFactor < 1 + ? scaleRateLimitPerRepoWindow(input.rateLimitPolicies ?? DEFAULT_WRITE_RATE_LIMIT_POLICIES, input.actionClass, reputation.cadenceFactor) + : input.rateLimitPolicies; + let rateLimit: WriteRateLimitVerdict; try { rateLimit = evaluateWriteRateLimit({ @@ -192,7 +252,7 @@ export function evaluateGovernorChokepoint(input: GovernorChokepointInput): Gove buckets: input.rateLimitBuckets, backoffAttempts: input.rateLimitBackoffAttempts, nowMs: input.nowMs, - ...(input.rateLimitPolicies ? { policies: input.rateLimitPolicies } : {}), + ...(rateLimitPolicies ? { policies: rateLimitPolicies } : {}), ...(input.rateLimitRandomFn ? { randomFn: input.rateLimitRandomFn } : {}), }); } catch (error) { @@ -206,7 +266,11 @@ export function evaluateGovernorChokepoint(input: GovernorChokepointInput): Gove repoFullName: input.repoFullName, }); } - const detailWithRateLimit: GovernorDecisionDetail = { ...baseDetail, rateLimit }; + const detailWithRateLimit: GovernorDecisionDetail = { + ...baseDetail, + rateLimit, + effectiveRateLimitPolicies: rateLimitPolicies ?? DEFAULT_WRITE_RATE_LIMIT_POLICIES, + }; if (!rateLimit.allowed) { return denyResult({ stage: "rate_limit", @@ -216,7 +280,13 @@ export function evaluateGovernorChokepoint(input: GovernorChokepointInput): Gove eventType: "throttled", actionClass: input.actionClass, repoFullName: input.repoFullName, - extraPayload: { retryAfterMs: rateLimit.retryAfterMs, blockedBy: rateLimit.blockedBy }, + extraPayload: { + retryAfterMs: rateLimit.retryAfterMs, + blockedBy: rateLimit.blockedBy, + // Surfaced only when the reputation throttle actually degraded this window, so an operator reading + // the ledger can tell "genuinely busy" apart from "a reputation-scaled cadence caught up with it". + ...(reputation && reputation.cadenceFactor < 1 ? { reputationCadenceFactor: reputation.cadenceFactor } : {}), + }, }); } @@ -275,29 +345,17 @@ export function evaluateGovernorChokepoint(input: GovernorChokepointInput): Gove }); } - const isSelfSubmissionAction = SELF_SUBMISSION_ACTION_CLASSES.has(input.actionClass); - + // #9062: `reputation` was already computed above (before rate-limit, so its cadenceFactor could scale that + // stage's per-repo window). Its own hard-deny stays at its documented ladder position -- AFTER budget/ + // convergence -- but is now reserved for `reason === "floored"` only: the extreme, near-certain-abuse case. + // A plain `"throttled"` verdict already had its cadence effect folded into the rate-limit evaluation above, + // so it no longer denies here at all -- it either already got caught by the tightened rate limit, or it + // didn't need to be, and either way the miner keeps submitting (at a degraded cadence) instead of being + // structurally unable to ever generate the new clean outcomes that would dilute its ratio back down. let detailWithReputation = detailWithConvergence; - // `!== undefined` (not a truthy check): an omitted key means "skip this stage"; any OTHER value the caller - // supplied -- including a bad `null` from a malformed upstream source -- must reach the calculator and, if it - // cannot handle it, fail closed via the catch below, never silently skip. - if (isSelfSubmissionAction && input.reputationHistory !== undefined) { - let reputation: SelfReputationThrottleDecision; - try { - reputation = selfReputationThrottle(input.reputationHistory, input.reputationThresholds ?? DEFAULT_SELF_REPUTATION_THRESHOLDS); - } catch (error) { - return denyResult({ - stage: "internal_error", - reason: `reputation_throttle_calculator_error: ${error instanceof Error ? error.message : String(error)}`, - mode, - detail: detailWithConvergence, - eventType: "denied", - actionClass: input.actionClass, - repoFullName: input.repoFullName, - }); - } + if (reputation) { detailWithReputation = { ...detailWithConvergence, reputation }; - if (reputation.throttled) { + if (reputation.reason === "floored") { return denyResult({ stage: "reputation_throttle", reason: reputation.reason, diff --git a/packages/loopover-engine/test/chokepoint.test.ts b/packages/loopover-engine/test/chokepoint.test.ts index 6ed1f177f..295e64130 100644 --- a/packages/loopover-engine/test/chokepoint.test.ts +++ b/packages/loopover-engine/test/chokepoint.test.ts @@ -106,12 +106,43 @@ test("non-convergence: a stuck item denies before reputation/self-plagiarism run assert.equal(decision.detail.reputation, undefined); }); -test("reputation throttle: a degraded track record denies open_pr before self-plagiarism runs", () => { +// #9062: a non-floored "throttled" ratio used to hard-deny outright, discarding cadenceFactor entirely -- an +// absorbing state, since submissions are the miner's only source of new decided outcomes and a denied miner +// can never submit one. It must now scale the per-repo rate-limit window instead of denying; only the +// extreme "floored" ratio still hard-denies (see the dedicated tests below). +test("reputation throttle: a degraded (non-floored) track record no longer denies -- it scales the per-repo rate-limit window instead (#9062)", () => { const decision = evaluateGovernorChokepoint( - baseInput({ reputationHistory: { decided: 10, unfavorable: 8 } }), + baseInput({ reputationHistory: { decided: 10, unfavorable: 8 } }), // ratio 0.8 -> "throttled", not "floored" + ); + assert.equal(decision.allowed, true, "a non-floored throttle must no longer hard-deny"); + assert.equal(decision.stage, "allow"); + assert.equal(decision.detail.reputation?.reason, "throttled"); + assert.equal(decision.detail.reputation?.cadenceFactor, 0.325); +}); + +test("reputation throttle: the scaled per-repo window denies a write the UNSCALED window would still allow (#9062)", () => { + // Default perRepo open_pr policy is {limit: 3, windowMs: 60_000}. A bucket at its cap with a window that + // started 70s before `nowMs` (10_000) has already ROLLED OVER under the unscaled 60s window (allowed) -- + // but ratio 0.8 gives cadenceFactor 0.325, stretching the window to round(60_000 / 0.325) = 184_615ms, well + // past 70s, so under the SCALED window the bucket has NOT rolled over and is still at its cap (denied). + const decision = evaluateGovernorChokepoint( + baseInput({ + reputationHistory: { decided: 10, unfavorable: 8 }, + rateLimitBuckets: { global: {}, perRepo: { "open_pr:acme/widgets": { count: 3, windowStartMs: -60_000 } } }, + }), + ); + assert.equal(decision.allowed, false); + assert.equal(decision.stage, "rate_limit", "the reputation throttle manifests as a tightened rate-limit denial, not its own stage"); + assert.equal(decision.ledgerEvent.payload?.reputationCadenceFactor, 0.325, "the ledger records WHY the window tightened"); +}); + +test("reputation throttle: a floored ratio (the extreme case) still hard-denies open_pr before self-plagiarism runs (#9062)", () => { + const decision = evaluateGovernorChokepoint( + baseInput({ reputationHistory: { decided: 10, unfavorable: 9 } }), // ratio 0.9 >= floorAtRatio (0.9 default) ); assert.equal(decision.allowed, false); assert.equal(decision.stage, "reputation_throttle"); + assert.equal(decision.detail.reputation?.reason, "floored"); assert.equal(decision.ledgerEvent.eventType, "throttled"); assert.equal(decision.detail.selfPlagiarism, undefined); }); diff --git a/packages/loopover-miner/lib/governor-chokepoint.ts b/packages/loopover-miner/lib/governor-chokepoint.ts index c8d10d975..a4105fcab 100644 --- a/packages/loopover-miner/lib/governor-chokepoint.ts +++ b/packages/loopover-miner/lib/governor-chokepoint.ts @@ -47,7 +47,11 @@ export function evaluateGovernorChokepointGate( input.actionClass, input.repoFullName, input.nowMs, - input.rateLimitPolicies, + // #9062: use the policy the DECISION was actually evaluated against, not the caller's raw + // `input.rateLimitPolicies` -- a non-floored reputation throttle may have scaled the per-repo window + // down before evaluateWriteRateLimit ran, and advancing the bucket with a different (unscaled) window + // than the one that produced this "allow" would silently disagree with its own decision. + decision.detail.effectiveRateLimitPolicies ?? input.rateLimitPolicies, ); rateLimitBackoffAttempts = clearWriteRateLimitBackoff(input.rateLimitBackoffAttempts, input.actionClass, input.repoFullName); } else if (decision.stage === "rate_limit") { diff --git a/packages/loopover-miner/lib/governor-state.ts b/packages/loopover-miner/lib/governor-state.ts index bbb6d4982..8bc941837 100644 --- a/packages/loopover-miner/lib/governor-state.ts +++ b/packages/loopover-miner/lib/governor-state.ts @@ -115,6 +115,40 @@ const DEFAULT_CAP_USAGE: Readonly = Object.freeze({ budgetSpen const DEFAULT_REPUTATION_HISTORY: Readonly = Object.freeze({ decided: 0, unfavorable: 0 }); let defaultGovernorState: GovernorState | null = null; +// #9062: governor_reputation_history was a cumulative, never-decaying counter -- reputation-throttle.ts's own +// doc comment promises "a recovering ratio restores it -- never a hard permanent ban", but with no decay and +// no window the ratio could only ever go UP (submissions are the only source of new decided outcomes, and a +// "floored" ratio hard-denies open_pr, so a miner that hits the floor could never generate the new clean +// outcomes needed to dilute it back down -- an absorbing state). Applying a time-based half-life decays the +// weight of OLD history independent of whether any new submissions happen, so even a fully floored miner's +// ratio recovers automatically over real calendar time and eventually clears the floor/throttle bands on its +// own -- the "recovering ratio" the module's doc already promises, actually delivered. +const REPUTATION_HISTORY_HALF_LIFE_DAYS = 14; +const MS_PER_DAY = 86_400_000; +// Below one full day of elapsed wall-clock time, skip decay entirely rather than applying a near-1.0 factor: +// floating-point truncation in decayReputationHistory's Math.floor could otherwise shave a count off a +// same-process (or same-second) read/write pair -- exactly the cadence every existing round-trip test in this +// file runs at. +const REPUTATION_HISTORY_DECAY_MIN_ELAPSED_DAYS = 1; + +/** Decay `history` by a half-life of {@link REPUTATION_HISTORY_HALF_LIFE_DAYS} for however long has elapsed + * since `updatedAt`, so `governor_reputation_history` (see the module doc above) cannot become an absorbing + * state. Halving preserves the observed ratio while shrinking its absolute weight, so a handful of fresh + * outcomes can outweigh a stale bad streak far sooner than if counts simply accumulated forever. A malformed + * `updatedAt` (should never happen -- it is always written as `new Date().toISOString()`) or an elapsed span + * under the one-day floor returns `history` unchanged. */ +function decayReputationHistory(history: RepoOutcomeHistory, updatedAt: string, nowMs: number): RepoOutcomeHistory { + const updatedMs = Date.parse(updatedAt); + if (!Number.isFinite(updatedMs)) return history; + const elapsedDays = (nowMs - updatedMs) / MS_PER_DAY; + if (elapsedDays < REPUTATION_HISTORY_DECAY_MIN_ELAPSED_DAYS) return history; + const decayFactor = 2 ** (-elapsedDays / REPUTATION_HISTORY_HALF_LIFE_DAYS); + return { + decided: Math.floor(history.decided * decayFactor), + unfavorable: Math.floor(history.unfavorable * decayFactor), + }; +} + export function resolveGovernorStateDbPath(env: Record = process.env): string { return resolveLocalStoreDbPath(defaultDbFileName, "LOOPOVER_MINER_GOVERNOR_STATE_DB", env); } @@ -401,7 +435,10 @@ export function openGovernorState(dbPath: string = resolveGovernorStateDbPath()) const normalizedRepo = normalizeRepoFullName(repoFullName); const row = getReputationStatement.get(normalizedForge, normalizedRepo) as ReputationHistoryRow | undefined; if (!row) return { ...DEFAULT_REPUTATION_HISTORY }; - return { decided: row.decided, unfavorable: row.unfavorable }; + // #9062: decay at READ time too (not just on the next increment) so a miner that never submits again -- + // exactly the "floored" hard-deny scenario reputation-throttle.ts's chokepoint consumer enforces -- still + // sees its ratio recover purely from elapsed time, without needing a write to trigger it. + return decayReputationHistory({ decided: row.decided, unfavorable: row.unfavorable }, row.updated_at, Date.now()); }, saveReputationHistory(repoFullName: string, history: RepoOutcomeHistory, apiBaseUrl?: string): RepoOutcomeHistory { const normalizedForge = normalizeApiBaseUrl(apiBaseUrl); @@ -422,7 +459,9 @@ export function openGovernorState(dbPath: string = resolveGovernorStateDbPath()) const unfavorableDelta = Number.isInteger(delta?.unfavorable) ? Number(delta.unfavorable) : 0; return withTransaction(() => { const row = getReputationStatement.get(normalizedForge, normalizedRepo) as ReputationHistoryRow | undefined; - const prior = row ? { decided: row.decided, unfavorable: row.unfavorable } : { ...DEFAULT_REPUTATION_HISTORY }; + // #9062: decay the PRIOR count before folding in this increment's delta, so the accumulation itself + // never becomes an absorbing 1:1-forever counter. + const prior = row ? decayReputationHistory({ decided: row.decided, unfavorable: row.unfavorable }, row.updated_at, Date.now()) : { ...DEFAULT_REPUTATION_HISTORY }; const decided = prior.decided + decidedDelta; const unfavorable = prior.unfavorable + unfavorableDelta; upsertReputationStatement.run(normalizedForge, normalizedRepo, decided, unfavorable, new Date().toISOString()); diff --git a/test/unit/miner-attempt-input-builder.test.ts b/test/unit/miner-attempt-input-builder.test.ts index 6840874b5..ddbb69b50 100644 --- a/test/unit/miner-attempt-input-builder.test.ts +++ b/test/unit/miner-attempt-input-builder.test.ts @@ -78,13 +78,17 @@ describe("buildAttemptGovernorContext (#5132)", () => { expect(buildAttemptGovernorContext({}, DEFAULT_AMS_POLICY_SPEC)).not.toHaveProperty("reputationHistory"); }); + // #9062: a non-floored ratio (0.8, below the 0.9 floorAtRatio default) no longer hard-denies at the + // reputation-throttle stage -- it scales the per-repo rate-limit window instead. The extreme "floored" + // ratio (0.9) still does, which is what this test now pins to keep proving real history reaches and + // actually gates the chokepoint. it("REGRESSION (#5675): a repo's real unfavorable-outcome streak, threaded through the governor context, throttles the chokepoint", () => { const ctx = buildAttemptGovernorContext( { LOOPOVER_MINER_LIVE_MODE: "live" }, DEFAULT_AMS_POLICY_SPEC, false, undefined, - { decided: 10, unfavorable: 8 }, + { decided: 10, unfavorable: 9 }, ); const decision = evaluateGovernorChokepoint({ actionClass: "open_pr", @@ -99,6 +103,34 @@ describe("buildAttemptGovernorContext (#5132)", () => { }); expect(decision.allowed).toBe(false); expect(decision.stage).toBe("reputation_throttle"); + expect(decision.detail.reputation?.reason).toBe("floored"); + }); + + // #9062: pins the NEW behavior for the common (non-floored) case -- cadenceFactor is consumed to scale + // the per-repo rate-limit window rather than discarded via an outright deny, so the miner can keep + // submitting (at a degraded cadence) and dilute its ratio back down with fresh outcomes. + it("REGRESSION (#9062): a non-floored unfavorable-outcome streak no longer denies -- it reaches allow with a degraded cadenceFactor recorded", () => { + const ctx = buildAttemptGovernorContext( + { LOOPOVER_MINER_LIVE_MODE: "live" }, + DEFAULT_AMS_POLICY_SPEC, + false, + undefined, + { decided: 10, unfavorable: 8 }, + ); + const decision = evaluateGovernorChokepoint({ + actionClass: "open_pr", + repoFullName: "acme/widgets", + nowMs: 10_000, + wouldBeAction: { action: "open_pr", title: "Fix bug" }, + liveModeRepoOptIn: "live", + rateLimitBuckets: { global: {}, perRepo: {} }, + rateLimitBackoffAttempts: {}, + capUsage: { budgetSpent: 0, turnsTaken: 0, elapsedMs: 0 }, + ...ctx, + }); + expect(decision.allowed).toBe(true); + expect(decision.detail.reputation?.reason).toBe("throttled"); + expect(decision.detail.reputation?.cadenceFactor).toBeLessThan(1); }); it("omits rateLimitBuckets/rateLimitBackoffAttempts/capUsage so the persisted governor-state store auto-supplies them", () => { diff --git a/test/unit/miner-governor-chokepoint.test.ts b/test/unit/miner-governor-chokepoint.test.ts index 147bb6190..c75c81af7 100644 --- a/test/unit/miner-governor-chokepoint.test.ts +++ b/test/unit/miner-governor-chokepoint.test.ts @@ -163,14 +163,32 @@ describe("evaluateGovernorChokepointGate (#2340)", () => { expect(result.rateLimitBackoffAttempts).toEqual({}); }); - it("a reputation-throttle denial records to the ledger as throttled before self-plagiarism runs", () => { + // #9062: a non-floored "throttled" ratio no longer hard-denies (it scales the per-repo rate-limit window + // instead -- see chokepoint.ts) -- discarding cadenceFactor and denying on ANY throttled verdict was + // exactly what turned a recoverable soft throttle into a permanent self-ban, since submissions are the + // miner's only source of new decided outcomes and a denied miner can never submit one. + it("a degraded (non-floored) reputation ratio no longer denies -- it reaches allow with a scaled cadence recorded", () => { const ledger = openLedger(); const result = evaluateGovernorChokepointGate(baseInput({ reputationHistory: { decided: 10, unfavorable: 8 } }), { append: (event) => ledger.appendGovernorEvent(event), }); + expect(result.decision.allowed).toBe(true); + expect(result.decision.stage).toBe("allow"); + expect(result.decision.detail.reputation?.reason).toBe("throttled"); + expect(result.decision.detail.reputation?.cadenceFactor).toBeLessThan(1); + expect(result.recorded.eventType).toBe("allowed"); + }); + + it("a floored reputation ratio (the extreme case) still denies to the ledger as throttled before self-plagiarism runs", () => { + const ledger = openLedger(); + const result = evaluateGovernorChokepointGate(baseInput({ reputationHistory: { decided: 10, unfavorable: 9 } }), { + append: (event) => ledger.appendGovernorEvent(event), + }); + expect(result.decision.allowed).toBe(false); expect(result.decision.stage).toBe("reputation_throttle"); + expect(result.decision.detail.reputation?.reason).toBe("floored"); expect(result.recorded.eventType).toBe("throttled"); expect(result.decision.detail.selfPlagiarism).toBeUndefined(); expect(result.rateLimitBuckets).toEqual({ global: {}, perRepo: {} }); diff --git a/test/unit/miner-governor-state.test.ts b/test/unit/miner-governor-state.test.ts index cf43de714..7d2375e34 100644 --- a/test/unit/miner-governor-state.test.ts +++ b/test/unit/miner-governor-state.test.ts @@ -306,9 +306,13 @@ describe("governor-state reputation history (#5134)", () => { updated_at TEXT NOT NULL ) `); - legacy.exec( - "INSERT INTO governor_reputation_history (repo_full_name, decided, unfavorable, updated_at) VALUES ('acme/widgets', 7, 2, '2026-01-01T00:00:00.000Z')", - ); + // #9062: updated_at deliberately close to "now" (not a fixed past date) -- this test is about the + // migration preserving rows across the table rebuild, not about the reputation-history decay this same + // change introduces; a stale hardcoded timestamp would let decay shave these counts down and break the + // exact toEqual assertions below for reasons unrelated to what this test actually covers. + legacy + .prepare("INSERT INTO governor_reputation_history (repo_full_name, decided, unfavorable, updated_at) VALUES ('acme/widgets', 7, 2, ?)") + .run(new Date().toISOString()); legacy.close(); const state = openGovernorState(dbPath); @@ -344,12 +348,15 @@ describe("governor-state reputation history (#5134)", () => { updated_at TEXT NOT NULL ) `); + // #9062: updated_at close to "now" for the same reason as the migration test above -- this test covers + // the corrupt-row-dropped behavior, not reputation-history decay. + const recentIso = new Date().toISOString(); legacy .prepare("INSERT INTO governor_reputation_history (repo_full_name, decided, unfavorable, updated_at) VALUES (?, NULL, NULL, ?)") - .run("acme/corrupt", "2026-01-01T00:00:00.000Z"); + .run("acme/corrupt", recentIso); legacy .prepare("INSERT INTO governor_reputation_history (repo_full_name, decided, unfavorable, updated_at) VALUES (?, ?, ?, ?)") - .run("acme/widgets", 3, 1, "2026-01-01T00:00:00.000Z"); + .run("acme/widgets", 3, 1, recentIso); legacy.close(); let opened: ReturnType | undefined; @@ -466,6 +473,94 @@ describe("governor-state reputation-history increment atomicity (#8855)", () => }); }); +describe("governor-state reputation-history decay (#9062)", () => { + // #9062: governor_reputation_history was a cumulative, never-decaying counter -- three bad early + // contributions meant a permanent, unrecoverable throttle since submissions were the only source of new + // decided outcomes and the ratio could only ever go up. These pin the decay math directly against the raw + // table (bypassing the public API, which always stamps updated_at as "now") so the elapsed-time math is + // actually exercised rather than always hitting the near-zero-elapsed no-op path every other test in this + // file runs at. + function seedRawReputationRow(dbPath: string, decided: number, unfavorable: number, updatedAt: string) { + const raw = new DatabaseSync(dbPath); + raw + .prepare( + "INSERT INTO governor_reputation_history (api_base_url, repo_full_name, decided, unfavorable, updated_at) VALUES (?, ?, ?, ?, ?)", + ) + .run("https://api.github.com", "acme/widgets", decided, unfavorable, updatedAt); + raw.close(); + } + + it("loadReputationHistory decays a row untouched for two half-lives (28 days) to a quarter of its counts", () => { + const state = tempState(); + const now = Date.now(); + const twentyEightDaysAgo = new Date(now - 28 * 86_400_000).toISOString(); + seedRawReputationRow(state.dbPath, 100, 80, twentyEightDaysAgo); + // decayReputationHistory reads Date.now() again internally -- freeze the clock to the SAME instant + // `twentyEightDaysAgo` was computed against, so elapsed-days lands at exactly 28.0 regardless of how much + // real wall-clock time this test itself takes to run (an un-frozen clock made this flaky: any execution + // delay pushes elapsed slightly past 28 days, so decayFactor lands fractionally under 0.25 and + // Math.floor rounds 100*decayFactor down to 24, not 25). + vi.useFakeTimers(); + vi.setSystemTime(now); + try { + // decayFactor = 2^(-28/14) = 0.25 exactly, so no rounding ambiguity: 100*0.25=25, 80*0.25=20. + expect(state.loadReputationHistory("acme/widgets")).toEqual({ decided: 25, unfavorable: 20 }); + } finally { + vi.useRealTimers(); + } + }); + + it("loadReputationHistory does not decay a row less than a day old", () => { + const state = tempState(); + const twelveHoursAgo = new Date(Date.now() - 12 * 3_600_000).toISOString(); + seedRawReputationRow(state.dbPath, 10, 8, twelveHoursAgo); + expect(state.loadReputationHistory("acme/widgets")).toEqual({ decided: 10, unfavorable: 8 }); + }); + + it("incrementReputationHistory decays the prior count before folding in the new delta", () => { + const state = tempState(); + const now = Date.now(); + const twentyEightDaysAgo = new Date(now - 28 * 86_400_000).toISOString(); + seedRawReputationRow(state.dbPath, 100, 80, twentyEightDaysAgo); + // Same clock-freeze rationale as the load-side test above -- pins elapsed-days at exactly 28.0 so the + // prior's decay (and thus the post-increment total) is deterministic regardless of test execution time. + vi.useFakeTimers(); + vi.setSystemTime(now); + try { + // Prior decays to {decided: 25, unfavorable: 20} (see the load-side test above), THEN the delta folds in. + expect(state.incrementReputationHistory("acme/widgets", { decided: 1, unfavorable: 0 })).toEqual({ + decided: 26, + unfavorable: 20, + }); + } finally { + vi.useRealTimers(); + } + }); + + it("REGRESSION (#9062): a floored ratio is no longer a permanent ban -- it ages below minSampleSize and fails back open", async () => { + const { selfReputationThrottle } = await import("@loopover/engine"); + const state = tempState(); + // ratio 9/10 = 0.9 >= floorAtRatio (0.9 default) -- hard-floored under the OLD never-decaying counter, with + // no way for `decided`/`unfavorable` to ever shrink since submissions were the only source of new outcomes + // and a floored miner could never submit one. + const freshFloored = { decided: 10, unfavorable: 9 }; + expect(selfReputationThrottle(freshFloored).reason).toBe("floored"); + + // 30 days later (over two half-lives) with ZERO new submissions -- decay alone, not a new clean outcome -- + // ages the sample size below minSampleSize (5), so the calculator fails OPEN instead of staying floored + // forever: the "recovering ratio restores it -- never a hard permanent ban" contract the module's own doc + // comment already promised. + const thirtyDaysAgo = new Date(Date.now() - 30 * 86_400_000).toISOString(); + seedRawReputationRow(state.dbPath, 10, 9, thirtyDaysAgo); + const decayed = state.loadReputationHistory("acme/widgets"); + expect(decayed.decided).toBeLessThan(5); // below the default minSampleSize + const decision = selfReputationThrottle(decayed); + expect(decision.reason).toBe("insufficient_history"); + expect(decision.throttled).toBe(false); + expect(decision.cadenceFactor).toBe(1); + }); +}); + describe("governor-state own-submission history (#5134)", () => { it("records and lists submissions newest-first", () => { const state = tempState(); From eae7358d036eb3490ccdae4cce5324296e309c1e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:25:43 -0700 Subject: [PATCH 4/4] fix(review): make finding severity load-bearing in check-run display, tighten confidence default, resolve near-miss code names (#9085) Finding severity was purely decorative: a warning-labeled finding (missing_linked_issue, slop_risk_above_threshold) can be the exact reason the gate one-shot-closes a PR, while a critical-labeled one (ai_consensus_defect, under the advisory-only default aiReviewGateMode) can have no gate effect at all. A contributor reading a plain warning icon on the finding that is about to close their PR was being actively misinformed. formatCheckRunOutput/buildCheckRunAnnotations now accept the real blocker codes from the SAME advisory's GateCheckEvaluation (when the caller has one) and use them to correct the displayed severity: an actual blocker always renders at the most alarming level regardless of its authored severity, and a non-blocking finding is capped at warning even if authored critical, so it never cries wolf louder than a genuine one. The parameter is optional and purely additive -- every existing caller that omits it keeps today's exact rendering. Wired the one caller with the evaluation already in scope (processors.ts's check-run publish). Also: - An absent AdvisoryFinding.confidence degraded to 1.0 (maximum certainty) at three gate-side consumption sites, the same "silence is not certainty" anti-pattern CONFIDENCE_WHEN_UNSTATED already fixed at the model-parsing layer. These sites read confidence off a finding object via a path the parser doesn't cover (a producer that omitted it, or the unvalidated cached-advisory JSON parse), so they now share the same 0.5 fallback instead of a second, wrong hardcoded default. - Renamed the near-miss repo_not_registered -> repo_not_cached (mirrors pr_not_cached/issue_not_cached's naming for the identical "not yet synced" shape) to stop it being confused with repo_unregistered, one character away and with the opposite gate consequence (repo_not_cached holds the gate for a human; repo_unregistered is a non-blocking advisory warning). The engine's gate-advisory.ts twin gets the matching confidence-default and rename fixes to stay in lock-step with the host copy. --- .../src/advisory/gate-advisory.ts | 21 ++-- src/github/app.ts | 6 +- src/queue/processors.ts | 5 + src/review/parity-wire.ts | 3 +- src/review/unified-comment-bridge.ts | 3 +- src/rules/advisory.ts | 80 +++++++++++--- test/unit/parity-wire.test.ts | 2 +- test/unit/predicted-gate-engine.test.ts | 15 +-- test/unit/rules.test.ts | 101 ++++++++++++++++-- test/unit/slop.test.ts | 2 +- test/unit/unified-comment-bridge.test.ts | 2 +- 11 files changed, 201 insertions(+), 39 deletions(-) diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index 223d044b9..f47190106 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -39,6 +39,11 @@ function sanitizeForCheckRun(text: string): string { } const DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE = 0.93; +// #9085 (host-parity): mirrors CONFIDENCE_WHEN_UNSTATED (src/services/ai-review.ts) -- kept as a local constant +// rather than an import, matching this file's own divergence design (it deliberately never pulls in the host's +// signals/services subsystem). An ABSENT `finding.confidence` must never read as maximum certainty (1.0): +// "silence is not certainty" (#8833). +const CONFIDENCE_WHEN_UNSTATED = 0.5; /** Exported to mirror the src twin (#8224): loopover's LOOSENABLE_KNOBS registry anchors the slop knob's * shipped value on this constant (divided by 100 onto the corpus's confidence scale). Value unchanged. */ export const DEFAULT_SLOP_BLOCK_THRESHOLD = 60; @@ -196,12 +201,15 @@ export function buildPullRequestAdvisory( const targetKey = pr ? `${repoFullName}#${pr.number}` : `${repoFullName}#unknown`; const findings: AdvisoryFinding[] = []; if (!repo) { + // #9085 (host-parity): renamed from `repo_not_registered` -- one character away from, and with the + // OPPOSITE gate consequence of, `repo_unregistered` below (addRepoFindings). See the host copy + // (src/rules/advisory.ts) for the full rename rationale. findings.push({ - code: "repo_not_registered", + code: "repo_not_cached", severity: "warning", - title: "Repository registration is unknown", - detail: "LoopOver cannot evaluate repo-specific rules until registry data is available.", - action: "Refresh the Gittensor registry snapshot.", + title: "Repository is not yet cached", + detail: "LoopOver has not synced this repository yet, so repo-specific rules cannot be evaluated.", + action: "Wait for the next repo sync, or re-deliver the installation webhook.", }); } else { addRepoFindings(repo, findings); @@ -634,7 +642,8 @@ function isEvaluationBlocker(code: string, policy: GateCheckPolicy): boolean { // pre_merge_check_unresolved: an enforced path-gated pre-merge check whose changed-file set could not be // resolved — loopover cannot evaluate it yet, so the gate is NEUTRAL (held) and re-evaluates on the next // sync, rather than auto-merging past the unverified requirement or hard-closing on a transient miss. (#review-audit) - if (code === "repo_not_registered" || code === "repo_not_seen" || code === "pr_not_cached" || code === "pre_merge_check_unresolved") return true; + // #9085 (host-parity): renamed from `repo_not_registered`. + if (code === "repo_not_cached" || code === "repo_not_seen" || code === "pr_not_cached" || code === "pre_merge_check_unresolved") return true; // cla_check_unresolved (#2564): the CLA-bot check-run's conclusion could not be resolved. Unlike the codes // above (which are never mode-gated), evaluateClaCheck runs for BOTH claGateMode "advisory" and "block" (so // the finding surfaces either way) — only "block" should ever HOLD the gate on an unresolved check-run. @@ -678,7 +687,7 @@ function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPoli if (!gatePolicyBlocks(policy.aiReviewGateMode, "advisory")) return false; if ((policy.aiReviewLowConfidenceDisposition ?? "hold_for_review") === "advisory_only") { const floor = policy.aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE; - const confidence = finding.confidence ?? 1; + const confidence = finding.confidence ?? CONFIDENCE_WHEN_UNSTATED; if (confidence < floor) return false; } return true; diff --git a/src/github/app.ts b/src/github/app.ts index dc0498ed7..3aea62ecc 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -769,6 +769,10 @@ export async function createOrUpdateCheckRun( detailLevel: "minimal" | "standard" = "minimal", annotationContext?: CheckRunAnnotationContext, mode: AgentActionMode = "live", + // #9085: the real blocker codes from THIS SAME advisory's GateCheckEvaluation, when the caller already has + // one (see formatCheckRunOutput's doc comment) -- optional and additive, so an omitted call site keeps + // today's exact authored-severity-only rendering. + blockerCodes?: ReadonlySet, ): Promise { return createOrUpdateNamedCheckRun( env, @@ -778,7 +782,7 @@ export async function createOrUpdateCheckRun( { name: LOOPOVER_CONTEXT_CHECK_NAME, conclusion: advisory.conclusion, - output: formatCheckRunOutput(advisory, detailLevel, annotationContext), + output: formatCheckRunOutput(advisory, detailLevel, annotationContext, blockerCodes), mode, }, ); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a53b1b8c7..1ef1ceb5c 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -12061,6 +12061,11 @@ async function maybePublishPrPublicSurface( pullNumber: pr.number, }, mode, + // #9085: thread the SAME evaluation's real blocker codes through so the informational "context" check + // never shows a lower-alarm icon on a finding that this SAME gate run actually closed the PR for. + // Undefined when the gate wasn't evaluated for this pass (e.g. shouldEvaluateGate false) -- falls + // back to today's authored-severity-only rendering, same as before this change. + gateEvaluation ? new Set(gateEvaluation.blockers.map((blocker) => blocker.code)) : undefined, ); if (checkRunResult?.kind === "permission_missing") { failedOutputs.push({ diff --git a/src/review/parity-wire.ts b/src/review/parity-wire.ts index 4ea3de133..3905f4260 100644 --- a/src/review/parity-wire.ts +++ b/src/review/parity-wire.ts @@ -42,7 +42,8 @@ const NEUTRAL_HOLD_REASON_CODES = [ "ai_review_inconclusive", "oversized_pr", "guardrail_hold", - "repo_not_registered", + // #9085: renamed from `repo_not_registered` (rules/advisory.ts) -- see that rename's own comment there. + "repo_not_cached", "repo_not_seen", "pr_not_cached", "pre_merge_check_unresolved", diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 4987272ca..aa143fc0e 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -146,7 +146,8 @@ export function panelRowsToSignalRows(rows: PublicPrPanelSignalRow[]): UnifiedSi * observations — keep them OUT of the Nits list so the nit count reflects real code review, not boilerplate that * padded nearly every review (#review-accuracy). */ const BOILERPLATE_NIT_CODES = new Set([ - "repo_not_registered", + // #9085: renamed from `repo_not_registered` (rules/advisory.ts) -- see that rename's own comment there. + "repo_not_cached", "repo_not_seen", "pr_not_cached", "pre_merge_check_unresolved", diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 8ad4a5958..2478dc9c1 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -36,6 +36,14 @@ import type { GuardrailPathMatch } from "../signals/change-guardrail"; import { isCodeFile } from "../signals/local-branch"; import { isTestPath } from "../signals/test-evidence"; import { nowIso } from "../utils/json"; +// #9085: an ABSENT `finding.confidence` used to degrade to 1.0 (maximum certainty) at every gate-side +// consumption site below -- exactly the "silence is not certainty" anti-pattern CONFIDENCE_WHEN_UNSTATED's own +// doc comment (ai-review.ts) already fixed at the model-parsing layer (parseReviewConfidence). These sites sit +// one layer downstream of that parser -- reading `finding.confidence` off an AdvisoryFinding object -- and can +// still see it absent/undefined via a path the parser never touches: a producer that forgot to set it, or the +// unvalidated `AdvisoryFinding[]` JSON parse at the advisory cache read site (`src/db/repositories.ts`). Reusing +// the SAME constant (rather than a second hardcoded 0.5) keeps the "what does silence mean" answer singular. +import { CONFIDENCE_WHEN_UNSTATED } from "../services/ai-review"; import { LOOPOVER_GATE_CHECK_NAME } from "../review/check-names"; import { CLA_CHECK_UNRESOLVED_CODE, CLA_CONSENT_MISSING_CODE } from "../review/cla-check"; import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; @@ -258,7 +266,9 @@ export function resolveAiReviewLowConfidenceHold( if ((policy.aiReviewLowConfidenceDisposition ?? "hold_for_review") !== "hold_for_review") return undefined; if (!isAiJudgmentOnlyFailure(evaluation)) return undefined; const floor = policy.aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE; - const belowFloor = evaluation.blockers.some((blocker) => (blocker.confidence ?? 1) < floor); + // #9085: an absent confidence must never read as maximum certainty -- see the CONFIDENCE_WHEN_UNSTATED + // import's doc comment above. + const belowFloor = evaluation.blockers.some((blocker) => (blocker.confidence ?? CONFIDENCE_WHEN_UNSTATED) < floor); if (!belowFloor) return undefined; // #8849: name the calibrated-abstention source when the floor in force is a live risk-control λ̂. const floorSource = policy.aiReviewCloseConfidenceCalibrated === true ? "calibrated risk-control threshold" : "configured close-confidence floor"; @@ -289,7 +299,9 @@ export function resolveAiReviewSalvageableHold( if ((policy.aiReviewLowConfidenceDisposition ?? "hold_for_review") !== "hold_for_review") return undefined; if (!isAiJudgmentOnlyFailure(evaluation)) return undefined; const floor = policy.aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE; - if (evaluation.blockers.some((blocker) => (blocker.confidence ?? 1) < floor)) return undefined; + // #9085: same fail-safe as resolveAiReviewLowConfidenceHold above -- an absent confidence is never + // maximum certainty. + if (evaluation.blockers.some((blocker) => (blocker.confidence ?? CONFIDENCE_WHEN_UNSTATED) < floor)) return undefined; if (salvageability.score < minScore) return undefined; return { reason: `the flagged defect looks fixable and the author's record clears the salvageability floor (${salvageability.score} ≥ ${minScore}: ${salvageability.factors.join("; ")})`, @@ -393,11 +405,18 @@ export function buildPullRequestAdvisory( const findings: AdvisoryFinding[] = []; if (!repo) { findings.push({ - code: "repo_not_registered", + // #9085: renamed from `repo_not_registered` -- one character away from, and with the OPPOSITE gate + // consequence of, `repo_unregistered` below (addRepoFindings): this code fires when LoopOver has NO + // repo record cached at all (app-state hold, see isEvaluationBlocker) and mirrors pr_not_cached/ + // issue_not_cached's naming for the identical "not yet synced" shape; `repo_unregistered` fires when a + // repo record EXISTS but isn't in the Gittensor registry snapshot (a plain non-blocking advisory + // warning, no hold). Same taxonomy-drift shape CONFIGURED_GATE_BLOCKER_SIGNAL_CODES's own doc comment + // above already calls out for slop_risk_above_threshold/surface_lane_reject. + code: "repo_not_cached", severity: "warning", - title: "Repository registration is unknown", - detail: "LoopOver cannot evaluate repo-specific rules until registry data is available.", - action: "Refresh the Gittensor registry snapshot.", + title: "Repository is not yet cached", + detail: "LoopOver has not synced this repository yet, so repo-specific rules cannot be evaluated.", + action: "Wait for the next repo sync, or re-deliver the installation webhook.", }); } else { addRepoFindings(repo, findings); @@ -432,11 +451,12 @@ export function buildIssueAdvisory(repo: RepositoryRecord | null, issue: IssueRe const targetKey = issue ? `${repoFullName}#${issue.number}` : `${repoFullName}#unknown`; const findings: AdvisoryFinding[] = []; if (!repo) { + // #9085: renamed from `repo_not_registered` -- see buildPullRequestAdvisory's identical finding above. findings.push({ - code: "repo_not_registered", + code: "repo_not_cached", severity: "warning", - title: "Repository registration is unknown", - detail: "LoopOver cannot evaluate repo-specific issue rules until registry data is available.", + title: "Repository is not yet cached", + detail: "LoopOver has not synced this repository yet, so repo-specific issue rules cannot be evaluated.", }); } else { addRepoFindings(repo, findings); @@ -506,6 +526,24 @@ function severityToAnnotationLevel(severity: AdvisorySeverity): CheckRunAnnotati return "notice"; } +/** #9085: a finding's OWN authored `severity` is set at creation time and never reconsidered against the + * ACTUAL gate outcome it ends up producing -- the exact drift the issue identified (a `warning`-labeled + * `missing_linked_issue`/`slop_risk_above_threshold` finding that really does one-shot-close the PR, + * alongside a `critical`-labeled `ai_consensus_defect` that, by default (`aiReviewGateMode: advisory`), has + * no gate effect at all). `blockerCodes` -- when the caller has a `GateCheckEvaluation` for this SAME + * advisory in hand (`evaluation.blockers.map(b => b.code)`) -- lets the public check-run rendering below + * correct for that: a finding whose code is a REAL blocker in this evaluation always renders at the most + * prominent level, regardless of its authored severity; a finding that is NOT actually blocking is capped at + * `warning` even if authored `critical`, so a non-enforcing signal never cries wolf louder than a genuine + * one. `blockerCodes` omitted (every existing caller before this change) preserves today's exact + * authored-severity-only rendering -- purely additive, never a behavior change for a caller that doesn't + * pass it. */ +function effectiveDisplaySeverity(finding: AdvisoryFinding, blockerCodes: ReadonlySet | undefined): AdvisorySeverity { + if (!blockerCodes) return finding.severity; + if (blockerCodes.has(finding.code)) return "critical"; + return finding.severity === "critical" ? "warning" : finding.severity; +} + function isCodePath(path: string): boolean { return /\.(ts|tsx|js|jsx|py|go|rs|java|rb|php|cs|cpp|cc|c|h|hpp|swift|kt|m|sql|yaml|yml|json|toml|md|vue|svelte|astro|dart)$/i.test(path); } @@ -544,6 +582,8 @@ export function buildCheckRunAnnotations( advisoryResult: Advisory, annotationContext: CheckRunAnnotationContext | undefined, detailLevel: "minimal" | "standard" = "minimal", + // #9085: see effectiveDisplaySeverity's doc comment above. Optional and additive. + blockerCodes?: ReadonlySet, ): CheckRunAnnotationBuildResult { if (detailLevel === "minimal" || !annotationContext) { return { annotations: [], omittedCount: 0 }; @@ -615,7 +655,7 @@ export function buildCheckRunAnnotations( addCandidate( path, annotationLineForFile(annotatableFiles.find((file) => file.path === path)!) ?? 1, - severityToAnnotationLevel(finding.severity), + severityToAnnotationLevel(effectiveDisplaySeverity(finding, blockerCodes)), finding.title, finding.publicText, finding.alreadyPublicSafe ?? false, @@ -631,6 +671,11 @@ export function formatCheckRunOutput( advisoryResult: Advisory, detailLevel: "minimal" | "standard" = "minimal", annotationContext?: CheckRunAnnotationContext, + // #9085: see effectiveDisplaySeverity's doc comment above. Optional and additive -- pass + // `new Set(gate.blockers.map(b => b.code))` from the SAME evaluation's GateCheckEvaluation when the caller + // has one, so a finding that actually closes the PR is never rendered with a lower-alarm icon than one that + // doesn't. + blockerCodes?: ReadonlySet, ): CheckRunOutput { const title = advisoryResult.conclusion === "success" ? "LoopOver context checked" : "LoopOver context posted"; const summary = "LoopOver public check output is intentionally minimal. Detailed maintainer context is available only through private API/MCP surfaces."; @@ -643,7 +688,11 @@ export function formatCheckRunOutput( } else { const publicLines = advisoryResult.findings.flatMap((f) => { if (!f.publicText) return []; - const label = f.severity === "warning" ? "⚠️" : "ℹ️"; + // #9085: a finding that is an ACTUAL blocker in this evaluation always gets the most alarming icon, + // regardless of its own authored severity -- a contributor must never read a ⚠️/ℹ️ on the exact finding + // that is about to close their PR. Falls back to the pre-#9085 two-icon scheme (⚠️ for `warning`, ℹ️ + // otherwise) when `blockerCodes` is undefined, byte-identical to every caller that doesn't pass it. + const label = blockerCodes?.has(f.code) ? "🚫" : effectiveDisplaySeverity(f, blockerCodes) === "warning" ? "⚠️" : "ℹ️"; // `alreadyPublicSafe` (#7981): a fixed, engineer-authored message with no interpolated contributor/AI // content skips the scrub — see AdvisoryFinding's own doc comment for why (the same reasoning // unified-comment-bridge.ts's gateBlockerLines applies to the PR-comment rendering of this same finding). @@ -652,7 +701,7 @@ export function formatCheckRunOutput( text = publicLines.length === 0 ? "No detailed findings are published in check runs." : publicLines.join("\n"); } - const { annotations, omittedCount } = buildCheckRunAnnotations(advisoryResult, annotationContext, detailLevel); + const { annotations, omittedCount } = buildCheckRunAnnotations(advisoryResult, annotationContext, detailLevel, blockerCodes); if (omittedCount > 0) { text = `${text}\n\n…${omittedCount} more hotspot annotation(s) omitted from inline check output.`; } @@ -1190,7 +1239,8 @@ function isEvaluationBlocker(code: string, policy: GateCheckPolicy): boolean { // pre_merge_check_unresolved: an enforced path-gated pre-merge check whose changed-file set could not be // resolved — loopover cannot evaluate it yet, so the gate is NEUTRAL (held) and re-evaluates on the next // sync, rather than auto-merging past the unverified requirement or hard-closing on a transient miss. (#review-audit) - if (code === "repo_not_registered" || code === "repo_not_seen" || code === "pr_not_cached" || code === "pre_merge_check_unresolved") return true; + // #9085: renamed from `repo_not_registered`. + if (code === "repo_not_cached" || code === "repo_not_seen" || code === "pr_not_cached" || code === "pre_merge_check_unresolved") return true; // cla_check_unresolved (#2564): the CLA-bot check-run's conclusion could not be resolved. Unlike the codes // above (which are never mode-gated), evaluateClaCheck runs for BOTH claGateMode "advisory" and "block" (so // the finding surfaces either way) — only "block" should ever HOLD the gate on an unresolved check-run. @@ -1249,7 +1299,9 @@ function resolveConfiguredGateMode(finding: AdvisoryFinding, policy: GateCheckPo if (mode !== "block") return mode; if ((policy.aiReviewLowConfidenceDisposition ?? "hold_for_review") === "advisory_only") { const floor = policy.aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE; - const confidence = finding.confidence ?? 1; + // #9085: an absent confidence must not silently clear the floor -- see the CONFIDENCE_WHEN_UNSTATED + // import's doc comment above. + const confidence = finding.confidence ?? CONFIDENCE_WHEN_UNSTATED; if (confidence < floor) return "advisory"; } return "block"; diff --git a/test/unit/parity-wire.test.ts b/test/unit/parity-wire.test.ts index 279eb2613..43f733d66 100644 --- a/test/unit/parity-wire.test.ts +++ b/test/unit/parity-wire.test.ts @@ -83,7 +83,7 @@ describe("neutralHoldReasonCode — bounded hold-reason class for a neutral gate }); it("returns the recognized code for each known neutral-hold class (guardrail, size, ai-inconclusive, sync-state)", () => { - for (const code of ["guardrail_hold", "oversized_pr", "ai_review_inconclusive", "repo_not_registered", "repo_not_seen", "pr_not_cached", "pre_merge_check_unresolved", "cla_check_unresolved"]) { + for (const code of ["guardrail_hold", "oversized_pr", "ai_review_inconclusive", "repo_not_cached", "repo_not_seen", "pr_not_cached", "pre_merge_check_unresolved", "cla_check_unresolved"]) { expect(neutralHoldReasonCode({ conclusion: "neutral", warnings: [finding(code)] })).toBe(code); } }); diff --git a/test/unit/predicted-gate-engine.test.ts b/test/unit/predicted-gate-engine.test.ts index a31443663..137aa0826 100644 --- a/test/unit/predicted-gate-engine.test.ts +++ b/test/unit/predicted-gate-engine.test.ts @@ -306,7 +306,7 @@ describe("predicted-gate engine module coverage (#2283)", () => { it("exercises advisory edge cases and gate failures", () => { const missingRepo = buildPullRequestAdvisory(null, PR); - expect(missingRepo.findings.some((f) => f.code === "repo_not_registered")).toBe(true); + expect(missingRepo.findings.some((f) => f.code === "repo_not_cached")).toBe(true); const missingPr = buildPullRequestAdvisory(REPO, null); expect(missingPr.findings.some((f) => f.code === "pr_not_cached")).toBe(true); // #9129 (host-parity): a duplicate_pr_risk finding under block mode now HOLDS (neutral), never closes. @@ -962,7 +962,7 @@ describe("predicted-gate engine module coverage (#2283)", () => { severity: "warning", title: "t", summary: "s", - findings: [{ code: "repo_not_registered", severity: "warning", title: "hold", detail: "hold" }], + findings: [{ code: "repo_not_cached", severity: "warning", title: "hold", detail: "hold" }], generatedAt: "2026-01-01T00:00:00.000Z", }, {}, @@ -1114,7 +1114,7 @@ describe("predicted-gate engine module coverage (#2283)", () => { }; expect(buildPullRequestAdvisory(splitRepo, PR).findings.some((f) => f.code === "issue_discovery_disabled")).toBe(false); expect(buildPullRequestAdvisory(splitRepo, PR).findings.some((f) => f.code === "direct_pr_pool_disabled")).toBe(false); - expect(buildPullRequestAdvisory(null, null).findings.some((f) => f.code === "repo_not_registered")).toBe(true); + expect(buildPullRequestAdvisory(null, null).findings.some((f) => f.code === "repo_not_cached")).toBe(true); expect(evaluateClaCheck(claConfig({ checkRunName: "CLA Bot" }), { checkRunConclusion: "failure" })[0]?.detail).toContain("CLA Bot"); expect(evaluateClaCheck(claConfig({ consentPhrase: "agree" }), { body: "nope" })[0]?.detail).toContain("agree"); @@ -1618,14 +1618,17 @@ describe("predicted-gate engine branch coverage (#2283)", () => { { ...block, aiReviewLowConfidenceDisposition: "advisory_only", aiReviewCloseConfidence: 0.93 }, ), ).toBe(true); - // A finding with no confidence reported at all defaults to fully-confident (?? 1), so it still blocks - // even under advisory_only regardless of the configured floor. + // #9085 (host-parity): a finding with no confidence reported at all used to default to fully-confident + // (?? 1), so it still blocked even under advisory_only regardless of the configured floor -- "the model + // said nothing" read as maximum certainty. It now defaults to CONFIDENCE_WHEN_UNSTATED (0.5), below the + // default 0.93 floor, so advisory_only correctly demotes it to non-blocking like any other sub-floor + // confidence. expect( gateAdvisoryInternals.isConfiguredGateBlocker(finding("ai_consensus_defect"), { ...block, aiReviewLowConfidenceDisposition: "advisory_only", }), - ).toBe(true); + ).toBe(false); expect(gateAdvisoryInternals.isConfiguredGateBlocker(finding("manifest_linked_issue_required"), advisory)).toBe(false); expect(gateAdvisoryInternals.isConfiguredGateBlocker(finding("manifest_linked_issue_required"), block)).toBe(true); expect(gateAdvisoryInternals.isConfiguredGateBlocker(finding("manifest_missing_tests"), advisory)).toBe(false); diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index e1a015797..a0c3acf6f 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -187,8 +187,8 @@ describe("advisory rules", () => { }); it("handles uncached PR and issue advisories for unknown repositories", () => { - expect(buildPullRequestAdvisory(null, null).findings.map((finding) => finding.code)).toEqual(["repo_not_registered", "pr_not_cached"]); - expect(buildIssueAdvisory(null, null).findings.map((finding) => finding.code)).toEqual(["repo_not_registered", "issue_not_cached"]); + expect(buildPullRequestAdvisory(null, null).findings.map((finding) => finding.code)).toEqual(["repo_not_cached", "pr_not_cached"]); + expect(buildIssueAdvisory(null, null).findings.map((finding) => finding.code)).toEqual(["repo_not_cached", "issue_not_cached"]); }); it("warns when an issue already has linked PRs", () => { @@ -487,7 +487,7 @@ describe("advisory rules", () => { // App-state findings (repo not synced, PR not cached) must NOT block a contributor on the app's // own state — the gate is neutral and re-evaluates automatically. - expect(advisory.findings.map((finding) => finding.code)).toEqual(expect.arrayContaining(["repo_not_registered", "pr_not_cached"])); + expect(advisory.findings.map((finding) => finding.code)).toEqual(expect.arrayContaining(["repo_not_cached", "pr_not_cached"])); expect(gate.conclusion).toBe("neutral"); expect(gate.blockers).toEqual([]); expect(output.title).toBe("LoopOver Orb Review Agent — not evaluated yet"); @@ -692,12 +692,16 @@ describe("advisory rules", () => { expect(evaluateGateCheck(advisory, { aiReviewGateMode: "block", aiReviewLowConfidenceDisposition: "advisory_only", aiReviewCloseConfidence: 0.4 }).conclusion).toBe("failure"); }); - it("an absent confidence degrades to 1.0 (at-or-above any floor), matching an at-or-above-floor confidence", () => { + // #9085: an absent confidence used to degrade to 1.0 (maximum certainty) here -- "the model said nothing" + // silently cleared even a custom close-confidence floor. It now degrades to CONFIDENCE_WHEN_UNSTATED + // (0.5), which sits BELOW the 0.93 default floor, so `advisory_only` now correctly treats a sub-floor + // absent confidence the same as any other sub-floor confidence: non-blocking. + it("an absent confidence degrades to CONFIDENCE_WHEN_UNSTATED (0.5) — sub-floor, matching any other sub-floor confidence", () => { const advisory = { ...buildPullRequestAdvisory(repo, null), findings: [{ code: "ai_consensus_defect", title: "t", severity: "critical" as const, detail: "d" }], }; - expect(evaluateGateCheck(advisory, { aiReviewGateMode: "block", aiReviewLowConfidenceDisposition: "advisory_only" }).conclusion).toBe("failure"); + expect(evaluateGateCheck(advisory, { aiReviewGateMode: "block", aiReviewLowConfidenceDisposition: "advisory_only" }).conclusion).toBe("success"); }); it("aiReviewGateMode !== block stays non-blocking regardless of disposition (unchanged from today)", () => { @@ -778,9 +782,12 @@ describe("advisory rules", () => { expect(resolveAiReviewLowConfidenceHold(evaluation, {})).toBeDefined(); }); - it("an absent confidence degrades to 1.0 — never holds", () => { + // #9085: an absent confidence used to degrade to 1.0 (never below the default 0.93 floor, so it never + // held). It now degrades to CONFIDENCE_WHEN_UNSTATED (0.5) -- below the floor -- so the default + // `hold_for_review` disposition correctly holds it, the same as any other sub-floor confidence. + it("an absent confidence degrades to CONFIDENCE_WHEN_UNSTATED (0.5) — sub-floor, so it holds under the default disposition", () => { const evaluation = failure([finding("ai_consensus_defect")]); - expect(resolveAiReviewLowConfidenceHold(evaluation, {})).toBeUndefined(); + expect(resolveAiReviewLowConfidenceHold(evaluation, {})).toBeDefined(); }); it("never holds a non-failure conclusion or an empty blocker list", () => { @@ -1235,6 +1242,86 @@ describe("advisory rules", () => { expect(output.text).not.toContain("Critical finding"); }); + // #9085: severity is otherwise decorative -- a `warning`-labeled finding can be the exact reason the gate + // closed the PR, while a `critical`-labeled one (e.g. ai_consensus_defect under the advisory-only default) + // can have no gate effect at all. `blockerCodes` -- the real blocker codes from this SAME advisory's + // GateCheckEvaluation, when the caller has one -- lets the public check-run text correct for that. + describe("formatCheckRunOutput blockerCodes (#9085)", () => { + const advisory = buildPullRequestAdvisory(null, null); + const withFindings = (findings: import("../../src/types").AdvisoryFinding[]) => ({ ...advisory, findings }); + + it("omitting blockerCodes preserves today's exact two-icon rendering (byte-identical for every existing caller)", () => { + const output = formatCheckRunOutput( + withFindings([ + { code: "missing_linked_issue", title: "t", severity: "warning", detail: "d", publicText: "warn text" }, + { code: "ai_consensus_defect", title: "t", severity: "critical", detail: "d", publicText: "critical text" }, + ]), + "standard", + ); + expect(output.text).toBe("⚠️ warn text\nℹ️ critical text"); + }); + + it("a warning-labeled finding that IS an actual blocker in this evaluation renders with the most alarming icon", () => { + const output = formatCheckRunOutput( + withFindings([{ code: "missing_linked_issue", title: "t", severity: "warning", detail: "d", publicText: "warn text" }]), + "standard", + undefined, + new Set(["missing_linked_issue"]), + ); + expect(output.text).toBe("🚫 warn text"); + }); + + it("a critical-labeled finding that is NOT actually blocking is capped at the warning icon, never crying wolf", () => { + const output = formatCheckRunOutput( + withFindings([{ code: "ai_consensus_defect", title: "t", severity: "critical", detail: "d", publicText: "critical text" }]), + "standard", + undefined, + new Set(), // no real blockers in this evaluation + ); + expect(output.text).toBe("⚠️ critical text"); + }); + + it("an info-labeled finding stays info even when blockerCodes is provided and it isn't a blocker", () => { + const output = formatCheckRunOutput( + withFindings([{ code: "some_info_code", title: "t", severity: "info", detail: "d", publicText: "info text" }]), + "standard", + undefined, + new Set(), + ); + expect(output.text).toBe("ℹ️ info text"); + }); + }); + + describe("buildCheckRunAnnotations blockerCodes (#9085)", () => { + const repoForAnnotations: RepositoryRecord = repo; + const files: PullRequestFileRecord[] = [ + { repoFullName: repo.fullName, pullNumber: 50, path: "src/a.ts", additions: 3, deletions: 0, changes: 3, payload: {} }, + ]; + + it("a non-blocker finding still maps to its own authored annotation level when blockerCodes is provided", () => { + const advisory = { + ...buildPullRequestAdvisory(repoForAnnotations, null), + findings: [{ code: "some_warning_code", title: "t", severity: "warning" as const, detail: "d", publicText: "warn text" }], + }; + const { annotations } = buildCheckRunAnnotations(advisory, { files, collisions: emptyCollisions(), pullNumber: 50 }, "standard", new Set()); + expect(annotations.find((a) => a.message === "warn text")?.annotation_level).toBe("warning"); + }); + + it("a finding that IS an actual blocker in this evaluation always maps to 'failure', regardless of its authored severity", () => { + const advisory = { + ...buildPullRequestAdvisory(repoForAnnotations, null), + findings: [{ code: "missing_linked_issue", title: "t", severity: "warning" as const, detail: "d", publicText: "warn text" }], + }; + const { annotations } = buildCheckRunAnnotations( + advisory, + { files, collisions: emptyCollisions(), pullNumber: 50 }, + "standard", + new Set(["missing_linked_issue"]), + ); + expect(annotations.find((a) => a.message === "warn text")?.annotation_level).toBe("failure"); + }); + }); + it("separates issue-discovery-only issues from clean split-lane issue advisories", () => { const issue: IssueRecord = { repoFullName: repo.fullName, diff --git a/test/unit/slop.test.ts b/test/unit/slop.test.ts index cc288fcb2..a464837fa 100644 --- a/test/unit/slop.test.ts +++ b/test/unit/slop.test.ts @@ -31,7 +31,7 @@ const FORBIDDEN_PUBLIC_TERMS = // without a minScore (see DEFAULT_SLOP_BLOCK_THRESHOLD in src/rules/advisory.ts). const DEFAULT_SLOP_BLOCK_THRESHOLD = 60; -// A PR advisory with no app-state findings — overriding findings to [] avoids the `repo_not_registered` +// A PR advisory with no app-state findings — overriding findings to [] avoids the `repo_not_cached` // short-circuit so evaluateGateCheck reaches the slop blocker (mirrors the rules.test.ts pattern). const cleanAdvisory = () => ({ ...buildPullRequestAdvisory(null, null), findings: [] }); diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 4882a119b..cb51a9b76 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -280,7 +280,7 @@ describe("buildDualReviewNotes", () => { aiReview: { notes: "Looks fine." }, warnings: [ { code: "missing_linked_issue", severity: "warning", title: "No linked issue detected", detail: "...", action: "Link it." }, - { code: "repo_not_registered", severity: "warning", title: "Registration is not available in the local LoopOver cache", detail: "..." }, + { code: "repo_not_cached", severity: "warning", title: "Registration is not available in the local LoopOver cache", detail: "..." }, { code: "real_code_nit", severity: "warning", title: "Missing test", detail: "...", action: "Add a test." }, ], recommendation: "merge",