From 5cd2fa857a73b3fb5e99f0efe48f50835a811557 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:08:23 -0700 Subject: [PATCH 1/3] feat(review): add suppression-signal data model + memory matcher (#2178-#2181) Adds the review-memory feature: a deterministic finding-fingerprint + suppression matcher that lets a maintainer-dismissed false positive stay suppressed on later re-reviews of the same repo, gated behind review.memory (default off, advisory only). --- .gittensory.yml.example | 14 + .../src/routes/docs.privacy-security.tsx | 1 + apps/gittensory-ui/src/routes/docs.tuning.tsx | 8 + config/examples/gittensory.full.yml | 14 + migrations/0114_review_suppression_memory.sql | 26 ++ src/db/repositories.ts | 102 ++++++++ src/db/schema.ts | 24 ++ src/env.d.ts | 5 + src/queue/processors.ts | 50 +++- src/review/review-memory-match.ts | 106 ++++++++ src/review/review-memory-wire.ts | 69 +++++ src/signals/focus-manifest.ts | 31 ++- src/types.ts | 14 + test/unit/focus-manifest.test.ts | 28 +- test/unit/queue.test.ts | 243 ++++++++++++++++++ test/unit/review-memory-match.test.ts | 150 +++++++++++ test/unit/review-memory-store.test.ts | 102 ++++++++ test/unit/review-memory-wire.test.ts | 105 ++++++++ test/unit/signals-coverage.test.ts | 2 +- worker-configuration.d.ts | 5 +- wrangler.jsonc | 8 + 21 files changed, 1099 insertions(+), 8 deletions(-) create mode 100644 migrations/0114_review_suppression_memory.sql create mode 100644 src/review/review-memory-match.ts create mode 100644 src/review/review-memory-wire.ts create mode 100644 test/unit/review-memory-match.test.ts create mode 100644 test/unit/review-memory-store.test.ts create mode 100644 test/unit/review-memory-wire.test.ts diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 6e9ccae5d6..a2214e827b 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -417,6 +417,14 @@ review: # null. Default: null/false — byte-identical. (#2995) # culture_profile: false + # Repeat-false-positive suppression (#2179, part of #1964). Bool | null. Default: null/false — byte-identical + # (no suppression-store read, no matching). Also requires the operator's GITTENSORY_REVIEW_MEMORY env flag to + # be on -- this manifest field alone cannot enable it. When both are on, an advisory (non-blocking) AI finding + # is matched against this repo's stored review_suppression signals (a maintainer's own past false-positive + # dismissals) before it is surfaced, and demoted/dropped on a match. ADVISORY-ONLY: never applied to gate + # blockers -- it can never change the merge/close disposition. + # memory: false + # Display-only floor for inline AI findings (`critical` | `major` | `minor` | `nitpick`). Findings below the # configured level are suppressed from inline comments — never from gate blockers. Default: null (show all). # min_finding_severity: major @@ -840,6 +848,12 @@ settings: # # (recent_merged_pull_requests). Reference-only grounding, never a gate/scoring input; requires the operator # # flag GITTENSORY_REVIEW_CULTURE_PROFILE. Bool or null. Default: null/false. (#2995) # culture_profile: false +# # When true (AND the operator's GITTENSORY_REVIEW_MEMORY env flag is also on), an advisory (non-blocking) +# # AI finding is matched against this repo's stored review_suppression signals (a maintainer's own past +# # false-positive dismissals) before it is surfaced, and demoted/dropped on a match. ADVISORY-ONLY: never +# # applied to gate blockers -- it can never change the merge/close disposition. Bool or null. +# # Default: null/false. (#2179, part of #1964) +# memory: false # # When true, an inline finding is ALSO tagged with a category (security/correctness/performance/ # # maintainability/tests/style) -- the AI reviewer self-categorizes, with a deterministic path/keyword # # fallback for whatever it omits. Only takes effect when inline_comments is already on. Bool or null. diff --git a/apps/gittensory-ui/src/routes/docs.privacy-security.tsx b/apps/gittensory-ui/src/routes/docs.privacy-security.tsx index f58257f9fc..c215c7b849 100644 --- a/apps/gittensory-ui/src/routes/docs.privacy-security.tsx +++ b/apps/gittensory-ui/src/routes/docs.privacy-security.tsx @@ -87,6 +87,7 @@ GITTENSORY_REVIEW_GROUNDING="true" # CI status + full changed-file GITTENSORY_REVIEW_RAG="true" # codebase vector-index context (needs index) GITTENSORY_REVIEW_IMPACT_MAP="true" # deterministic impact map (needs review.impact_map too) GITTENSORY_REVIEW_CULTURE_PROFILE="true" # repo quality-culture profile (needs review.culture_profile: true) +GITTENSORY_REVIEW_MEMORY="true" # repeat-false-positive suppression (needs review.memory too) GITTENSORY_REVIEW_REPUTATION="true" # submitter-reputation spend control (never shown) GITTENSORY_REVIEW_UNIFIED_COMMENT="true" # one in-place unified PR comment GITTENSORY_REVIEW_ENRICHMENT="true" # external analyzer registry (REES) findings diff --git a/apps/gittensory-ui/src/routes/docs.tuning.tsx b/apps/gittensory-ui/src/routes/docs.tuning.tsx index 7ee6984a8c..94a417fffb 100644 --- a/apps/gittensory-ui/src/routes/docs.tuning.tsx +++ b/apps/gittensory-ui/src/routes/docs.tuning.tsx @@ -153,6 +153,14 @@ function Tuning() { scoring input. Also requires the per-repo review.culture_profile: true opt-in in .gittensory.yml. Per-PR. +
  • + GITTENSORY_REVIEW_MEMORY — repeat-false-positive suppression: matches an + advisory (non-blocking) AI finding against this repo's stored suppression signals (a + maintainer's own past false-positive dismissals) and demotes or drops it before the + unified comment renders. Advisory-only by construction — never applied to gate blockers, + so it can never change the merge/close disposition. Also requires the per-repo{" "} + review.memory: true opt-in in .gittensory.yml. Per-PR. +
  • GITTENSORY_REVIEW_REPUTATION — submitter-reputation spend control. A new, burst, or low-reputation submitter is downgraded to a deterministic-only review; good diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index be99702338..7bfcc48293 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -430,6 +430,14 @@ review: # null. Default: null/false — byte-identical. (#2995) # culture_profile: false + # Repeat-false-positive suppression (#2179, part of #1964). Bool | null. Default: null/false — byte-identical + # (no suppression-store read, no matching). Also requires the operator's GITTENSORY_REVIEW_MEMORY env flag to + # be on -- this manifest field alone cannot enable it. When both are on, an advisory (non-blocking) AI finding + # is matched against this repo's stored review_suppression signals (a maintainer's own past false-positive + # dismissals) before it is surfaced, and demoted/dropped on a match. ADVISORY-ONLY: never applied to gate + # blockers -- it can never change the merge/close disposition. + # memory: false + # Display-only floor for inline AI findings (`critical` | `major` | `minor` | `nitpick`). Findings below the # configured level are suppressed from inline comments — never from gate blockers. Default: null (show all). # min_finding_severity: major @@ -853,6 +861,12 @@ settings: # # (recent_merged_pull_requests). Reference-only grounding, never a gate/scoring input; requires the operator # # flag GITTENSORY_REVIEW_CULTURE_PROFILE. Bool or null. Default: null/false. (#2995) # culture_profile: false +# # When true (AND the operator's GITTENSORY_REVIEW_MEMORY env flag is also on), an advisory (non-blocking) +# # AI finding is matched against this repo's stored review_suppression signals (a maintainer's own past +# # false-positive dismissals) before it is surfaced, and demoted/dropped on a match. ADVISORY-ONLY: never +# # applied to gate blockers -- it can never change the merge/close disposition. Bool or null. +# # Default: null/false. (#2179, part of #1964) +# memory: false # # When true, an inline finding is ALSO tagged with a category (security/correctness/performance/ # # maintainability/tests/style) -- the AI reviewer self-categorizes, with a deterministic path/keyword # # fallback for whatever it omits. Only takes effect when inline_comments is already on. Bool or null. diff --git a/migrations/0114_review_suppression_memory.sql b/migrations/0114_review_suppression_memory.sql new file mode 100644 index 0000000000..54d4b98163 --- /dev/null +++ b/migrations/0114_review_suppression_memory.sql @@ -0,0 +1,26 @@ +-- Review memory (#2178, data-model slice of #1964): a bounded, public-safe per-repo store of "the maintainer +-- already dismissed this as a false positive" suppression signals. A maintainer-authored suppression is keyed +-- by (repo_full_name, category, path_glob, pattern_hash): `category` is the finding's own deterministic `code` +-- (e.g. "ai_review_split", never a private rubric term), `path_glob` narrows the suppression to a path pattern +-- ("" = repo-wide), and `pattern_hash` is a stable hash of the finding's NORMALIZED message (never the raw +-- message itself — no free-form finding text is stored, only its hash, keeping the row public-safe). Recording +-- (writing a row when a maintainer dismisses a finding) and applying (reading rows to suppress a future +-- matching finding) are both SEPARATE slices layered on top of this store — this migration adds ONLY the table +-- + typed row + repository accessors, no recording trigger and no apply-during-review logic. +CREATE TABLE IF NOT EXISTS review_suppression ( + id TEXT PRIMARY KEY, + repo_full_name TEXT NOT NULL, + category TEXT NOT NULL, + path_glob TEXT NOT NULL DEFAULT '', + pattern_hash TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), + created_by TEXT +); +-- Idempotent recording: re-dismissing the SAME finding shape is a no-op upsert (bump created_at), not a +-- duplicate row -- mirrors active_review_tracking's one-row-per-key shape (migrations/0113). +CREATE UNIQUE INDEX IF NOT EXISTS review_suppression_key_unique + ON review_suppression (repo_full_name, category, path_glob, pattern_hash); +-- Per-repo listing + the bounded-row-cap eviction (oldest-first) both scan by repo_full_name ordered by +-- created_at -- this index serves both without a table scan. +CREATE INDEX IF NOT EXISTS review_suppression_repo_created_idx + ON review_suppression (repo_full_name, created_at); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 86c423ea62..223b6014ec 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -51,6 +51,7 @@ import { repositoryAiKeys, repositoryLinearKeys, repositorySettings, + reviewSuppression, scorePreviews, scoringModelSnapshots, signalSnapshots, @@ -149,6 +150,7 @@ import type { RepoSyncStateRecord, RepositorySettings, RepositoryRecord, + ReviewSuppressionRecord, ScorePreviewRecord, ScoringModelSnapshotRecord, SignalSnapshotRecord, @@ -4727,6 +4729,106 @@ export async function terminalizeActiveReviewTracking( return Number(result.meta.changes ?? 0) > 0; } +// Review memory (#2178, data-model slice of #1964). Hard per-repo cap on stored suppression signals — mirrors +// rag.ts's MAX_CHUNKS_PER_REPO discipline (bound a repo-controlled, unboundedly-growable store). A repo that +// keeps dismissing NEW finding shapes evicts its OLDEST suppression first rather than growing forever. +export const MAX_REVIEW_SUPPRESSIONS_PER_REPO = 500; + +function toReviewSuppressionRecord(row: typeof reviewSuppression.$inferSelect): ReviewSuppressionRecord { + return { + id: row.id, + repoFullName: row.repoFullName, + category: row.category, + pathGlob: row.pathGlob, + patternHash: row.patternHash, + createdAt: row.createdAt, + createdBy: row.createdBy, + }; +} + +/** Idempotently record a review-memory suppression signal: a maintainer dismissed a finding matching + * (repoFullName, category, pathGlob, patternHash) as a false positive. Re-recording the SAME key is a true + * no-op upsert (bumps createdAt/createdBy only) — mirrors startActiveReviewTracking's upsert shape — so + * repeatedly dismissing the same recurring finding never creates duplicate rows. After the write, evicts the + * OLDEST rows for this repo beyond MAX_REVIEW_SUPPRESSIONS_PER_REPO (fail-safe: eviction errors are swallowed + * — a failed prune never blocks the recording write that already succeeded). */ +export async function recordReviewSuppression( + env: Env, + input: { repoFullName: string; category: string; pathGlob?: string | null | undefined; patternHash: string; createdBy?: string | null | undefined }, +): Promise { + const repoFullName = boundedString(input.repoFullName, 200); + const category = boundedString(input.category, 200); + const pathGlob = boundedString(input.pathGlob ?? "", 500); + const patternHash = boundedString(input.patternHash, 128); + const db = getDb(env.DB); + const values = { + id: crypto.randomUUID(), + repoFullName, + category, + pathGlob, + patternHash, + createdBy: input.createdBy ?? null, + }; + await db + .insert(reviewSuppression) + .values(values) + .onConflictDoUpdate({ + target: [reviewSuppression.repoFullName, reviewSuppression.category, reviewSuppression.pathGlob, reviewSuppression.patternHash], + set: { createdAt: nowIso(), createdBy: values.createdBy }, + }); + const row = await db + .select() + .from(reviewSuppression) + .where( + and( + eq(reviewSuppression.repoFullName, repoFullName), + eq(reviewSuppression.category, category), + eq(reviewSuppression.pathGlob, pathGlob), + eq(reviewSuppression.patternHash, patternHash), + ), + ) + .get(); + await pruneReviewSuppressionsOverCap(env, repoFullName).catch(() => undefined); + /* v8 ignore next -- the row was just inserted/updated in this same call; a missing read-back would mean D1 + * itself failed silently, not a reachable application branch. */ + return row ? toReviewSuppressionRecord(row) : { ...values, createdAt: nowIso() }; +} + +/** Evict the OLDEST review_suppression rows for repoFullName once the per-repo count exceeds + * MAX_REVIEW_SUPPRESSIONS_PER_REPO — a repo that keeps dismissing new finding shapes never grows this table + * unbounded. Internal to recordReviewSuppression; not exported. */ +async function pruneReviewSuppressionsOverCap(env: Env, repoFullName: string): Promise { + const db = getDb(env.DB); + const rows = await db + .select({ id: reviewSuppression.id }) + .from(reviewSuppression) + .where(eq(reviewSuppression.repoFullName, repoFullName)) + .orderBy(desc(reviewSuppression.createdAt)) + .offset(MAX_REVIEW_SUPPRESSIONS_PER_REPO); + if (rows.length === 0) return; + await db.delete(reviewSuppression).where( + and( + eq(reviewSuppression.repoFullName, repoFullName), + inArray( + reviewSuppression.id, + rows.map((row) => row.id), + ), + ), + ); +} + +/** List every stored suppression signal for repoFullName, newest first. Bounded by `limit` (default 500, + * matching MAX_REVIEW_SUPPRESSIONS_PER_REPO) so a caller can never accidentally request an unbounded scan. */ +export async function listReviewSuppressions(env: Env, repoFullName: string, limit = MAX_REVIEW_SUPPRESSIONS_PER_REPO): Promise { + const rows = await getDb(env.DB) + .select() + .from(reviewSuppression) + .where(eq(reviewSuppression.repoFullName, boundedString(repoFullName, 200))) + .orderBy(desc(reviewSuppression.createdAt)) + .limit(clampInteger(limit, 1, MAX_REVIEW_SUPPRESSIONS_PER_REPO)); + return rows.map(toReviewSuppressionRecord); +} + export async function listGateOutcomes( env: Env, options: { repoFullName?: string; windowDays?: number; now?: string; limit?: number } = {}, diff --git a/src/db/schema.ts b/src/db/schema.ts index f4d7765a40..59a33c9d9a 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -764,6 +764,30 @@ export const activeReviewTracking = sqliteTable( }), ); +// Review memory (#2178, data-model slice of #1964): a bounded, public-safe per-repo store of "the maintainer +// already dismissed this as a false positive" suppression signals (migrations/0114). `category` is the +// finding's own deterministic `code` (never a private rubric term); `pathGlob` narrows the suppression to a +// path pattern ("" = repo-wide); `patternHash` is a stable hash of the finding's NORMALIZED message — the raw +// message itself is never stored, only its hash. One row per (repoFullName, category, pathGlob, patternHash); +// re-recording the same shape upserts (bumps createdAt) rather than duplicating. Read-side matching (#2180) +// and apply-to-findings wiring (#2181) are separate slices layered on top of this store. +export const reviewSuppression = sqliteTable( + "review_suppression", + { + id: text("id").primaryKey(), + repoFullName: text("repo_full_name").notNull(), + category: text("category").notNull(), + pathGlob: text("path_glob").notNull().default(""), + patternHash: text("pattern_hash").notNull(), + createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), + createdBy: text("created_by"), + }, + (table) => ({ + key: uniqueIndex("review_suppression_key_unique").on(table.repoFullName, table.category, table.pathGlob, table.patternHash), + repoCreated: index("review_suppression_repo_created_idx").on(table.repoFullName, table.createdAt), + }), +); + // Agent-layer approval queue (#779). An `auto_with_approval` action the write-actions layer (#778) staged for // a one-tap maintainer accept/reject. At most one row per (repo, pull, action_class). export const agentPendingActions = sqliteTable( diff --git a/src/env.d.ts b/src/env.d.ts index e913740daa..09f016dc5e 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -256,6 +256,11 @@ declare global { * OFF — unset/false performs NO extra D1 read and keeps the reviewer prompt byte-identical (the new branch * is unreachable when off). ADVISORY GROUNDING ONLY: never a gate/scoring input. */ GITTENSORY_REVIEW_CULTURE_PROFILE?: string; + /** Review memory (#2179, part of #1964): operator-level kill-switch for repeat-false-positive suppression, + * ANDed with the per-repo `.gittensory.yml review.memory` opt-in (see review/review-memory-wire's + * isReviewMemoryEnabled / shouldApplyReviewMemory). Default OFF — unset/false performs NO suppression- + * store read and NO matching, byte-identical to today. ADVISORY-ONLY: never applied to gate blockers. */ + GITTENSORY_REVIEW_MEMORY?: string; /** Review-enrichment service (REES): when truthy, the self-host review engine POSTs the PR diff/files to * REES and splices any public-safe brief into the AI reviewer prompt. Requires REES_URL and the repo in * GITTENSORY_REVIEW_REPOS. REES_ANALYZERS is an optional exact comma-list; unset/"all"/"*" lets REES run its diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 56a457163c..3cb0d4ef98 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -62,6 +62,7 @@ import { getGateBlockOutcome, hasActiveReviewForHeadSha, isGlobalAgentFrozen, + listReviewSuppressions, markGateOutcomeOverridden, startActiveReviewTracking, terminalizeActiveReviewTracking, @@ -370,6 +371,7 @@ import { resolveReviewPathInstructions, resolveReviewPreMergeChecks, resolveReviewPromptOverrides, + resolveReviewMemoryManifestToggle, resolveReviewVisualConfig, type FocusManifestFinding, type FocusManifest, @@ -438,6 +440,7 @@ import { buildRepoCultureProfileContext, isRepoCultureProfileEnabled, } from "../review/repo-culture-profile-wire"; +import { applyReviewMemorySuppression, shouldApplyReviewMemory } from "../review/review-memory-wire"; import { buildReviewEnrichment, isEnrichmentEnabled, @@ -7976,6 +7979,7 @@ async function maybePublishPrPublicSurface( let suggestionsEnabledForReview = false; let changedFilesSummaryEnabledForReview = false; let effortScoreEnabledForReview = false; + let reviewMemoryEnabledForReview = false; let findingCategoriesEnabledForReview = false; let minFindingSeverityForReview: ReviewFindingSeverity | null = null; let aiReviewExpected = false; @@ -8489,6 +8493,11 @@ async function maybePublishPrPublicSurface( changedFilesSummaryEnabledForReview = deterministicReviewOverrides.changedFilesSummary; effortScoreEnabledForReview = deterministicReviewOverrides.effortScore; minFindingSeverityForReview = deterministicReviewOverrides.minFindingSeverity; + // review.memory (#2179, part of #1964): deterministic, no-AI -- resolved the same unconditional way as + // changed_files_summary/effort_score above (must apply even when the AI review itself is skipped this + // pass). ANDed with the operator's GITTENSORY_REVIEW_MEMORY kill-switch at the actual apply site below + // (shouldApplyReviewMemory) — this flag alone only carries the per-repo manifest opt-in. + reviewMemoryEnabledForReview = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifestForAutoReview)); maybeAddRequiredAutoReviewSkipHold(env, { settings, advisory, @@ -9683,8 +9692,47 @@ async function maybePublishPrPublicSurface( ); } } + // review.memory (#2181, apply slice of #1964): before the unified comment renders, suppress/demote + // advisory (non-blocking) findings a maintainer already dismissed as false positives for this repo. ONLY + // ever applied to `commentGate.warnings` -- NEVER `commentGate.blockers` -- so this can never change the + // merge/close disposition, matching the ADVISORY-ONLY constraint. Fail-safe: a suppression-store read + // error leaves `renderedGate` as the original, untouched `commentGate` (the catch below never assigns + // renderedGate, so it keeps its `let` initializer). Flag-OFF (default, reviewMemoryEnabledForReview + // false) takes no new branch at all -- zero extra D1 read, byte-identical to today. + let renderedGate = commentGate; + if (reviewMemoryEnabledForReview && commentGate.warnings.length > 0) { + try { + const suppressionSignals = await listReviewSuppressions(env, repoFullName); + const { findings: suppressedWarnings, suppressedCount, demotedCount } = applyReviewMemorySuppression( + commentGate.warnings, + suppressionSignals, + ); + if (suppressedCount > 0 || demotedCount > 0) { + renderedGate = { ...commentGate, warnings: suppressedWarnings }; + incr("gittensory_review_memory_suppressed_total", { repo: repoFullName }); + console.log( + JSON.stringify({ + ev: "review_memory_applied", + repoFullName, + pull: pr.number, + suppressedCount, + demotedCount, + }), + ); + } + } catch (error) { + console.log( + JSON.stringify({ + ev: "review_memory_error", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + } + } deterministicBody = buildUnifiedCommentBody({ - gate: commentGate, + gate: renderedGate, ...(aiReview !== undefined ? { aiReview } : {}), advisoryFindings: advisory.findings, panelRows: rows, diff --git a/src/review/review-memory-match.ts b/src/review/review-memory-match.ts new file mode 100644 index 0000000000..6399b50387 --- /dev/null +++ b/src/review/review-memory-match.ts @@ -0,0 +1,106 @@ +// Review memory (#2180, matching-logic slice of #1964): a pure, deterministic fingerprint over a finding's +// (category, path, message) plus a matcher that decides suppress/demote/keep against a repo's stored +// review_suppression signals (src/db/repositories.ts's listReviewSuppressions, migrations/0114). NO DB I/O and +// NO AI here — this is the pure decision function findings flow through; the store read (#2178) and the +// apply-to-findings wiring that calls this before rendering the unified comment (#2181) are separate slices. +// +// Suppression is scoped by (category, pathGlob): a stored signal's category must match EXACTLY (categories are +// deterministic finding codes, e.g. "ai_review_split" — never fuzzy), and its pathGlob (canonicalized via +// change-guardrail.ts's globToRegExp, the same bounded/ReDoS-safe glob compiler every other maintainer- +// configured path pattern in this codebase uses) must match the finding's own path ("" = repo-wide, always +// matches). Within that scope: an EXACT message-hash match (the finding recurred VERBATIM) is the strongest +// signal → suppress entirely; a category+path match with a DIFFERENT message hash (the finding shape recurs in +// the same place but worded differently) is a weaker signal → demote (still shown, just downgraded) rather +// than silently dropped, so a genuinely new defect at a previously-dismissed spot is never hidden outright. + +import { canonicalize, globToRegExp } from "../signals/change-guardrail"; +import type { ReviewSuppressionRecord } from "../types"; + +/** Bound on the raw message text hashed into a fingerprint — mirrors the codebase's other bounded-input + * discipline (e.g. impact-map-wire.ts's MAX_PROMPT_CHARS) so a pathologically long AI-generated finding body + * can never make fingerprinting itself expensive. */ +const MAX_MESSAGE_LENGTH = 4000; + +/** Normalize a finding message for fingerprinting: lowercase, collapse all whitespace runs to a single space, + * trim, and bound the length. Two findings that differ only in whitespace/case/wording-drift the AI reviewer + * introduces across otherwise-identical re-runs must fingerprint identically, or suppression would never + * actually fire on a recurring finding. */ +export function normalizeFindingMessage(message: string): string { + return message.toLowerCase().replace(/\s+/g, " ").trim().slice(0, MAX_MESSAGE_LENGTH); +} + +/** Normalize a finding path for fingerprinting/matching: canonicalize (case/separator-insensitive, mirrors + * every other path-pattern consumer in this codebase) and default to "" (repo-wide) when absent. */ +export function normalizeFindingPath(path: string | null | undefined): string { + return path ? canonicalize(path) : ""; +} + +/** The minimal, decoupled shape review-memory matching needs from a finding — deliberately NOT `AdvisoryFinding` + * itself, so this module has zero dependency on the gate's own finding type and stays reusable. `category` is + * the finding's own deterministic code (e.g. `ai_review_split`); `path` is optional (absent ⇒ repo-wide). */ +export type ReviewMemoryFindingInput = { + category: string; + path?: string | null | undefined; + message: string; +}; + +/** djb2, a small non-cryptographic string hash — deterministic and collision-resistant enough for this repo- + * scoped, non-security-critical fingerprint (an attacker who can already post arbitrary PR content gains + * nothing from a hash collision here: worst case is one finding wrongly suppressed/kept, never a gate-verdict + * change, since this module is advisory-only by construction). Kept SYNCHRONOUS (no WebCrypto subtle.digest) + * so `fingerprint`/`matchSuppressions` stay pure, sync functions the render path can call without threading + * `await` through buildDualReviewNotes/buildUnifiedCommentBody. */ +function djb2Hex(input: string): string { + let hash = 5381; + for (let i = 0; i < input.length; i += 1) { + hash = ((hash << 5) + hash + input.charCodeAt(i)) >>> 0; + } + return hash.toString(16).padStart(8, "0"); +} + +/** + * Deterministic fingerprint over a finding's (category, normalized path, normalized message). PURE — same + * input always yields the same output, no I/O, no randomness. Two findings with the same category at the same + * path but differently-worded messages intentionally fingerprint DIFFERENTLY (this hash IS the "pattern_hash" + * exact-match key — category+path partial matches are handled by `matchSuppressions`, not by fuzzing this hash). + */ +export function fingerprint(finding: ReviewMemoryFindingInput): string { + const path = normalizeFindingPath(finding.path); + const message = normalizeFindingMessage(finding.message); + return djb2Hex(`v1:${finding.category}:${path}:${message}`); +} + +export type ReviewMemoryMatchResult = "suppress" | "demote" | "keep"; + +/** True when `signal.pathGlob` matches `path` — an empty pathGlob ("" = repo-wide) always matches; otherwise + * the glob is compiled (bounded/ReDoS-safe, see globToRegExp) and tested against the canonicalized path. */ +function pathGlobMatches(pathGlob: string, path: string): boolean { + if (pathGlob === "") return true; + return globToRegExp(pathGlob).test(path); +} + +/** + * Decide how a finding should be treated given this repo's stored suppression signals. PURE — no DB I/O (the + * caller already resolved `signals` via listReviewSuppressions). Bounded — a caller-supplied `signals` array is + * simply iterated once; the bound on ITS size (MAX_REVIEW_SUPPRESSIONS_PER_REPO) is enforced at the store layer + * (#2178), not here. + * + * - `"suppress"`: some signal's category matches exactly, its pathGlob matches the finding's path, AND its + * patternHash equals this finding's own fingerprint — the maintainer dismissed THIS EXACT finding before. + * - `"demote"`: no exact match, but some signal's category+pathGlob scope matches (a different patternHash) — + * the maintainer has dismissed findings from this same category/area before, just not this precise wording. + * - `"keep"`: no signal's category+pathGlob scope matches this finding at all. + */ +export function matchSuppressions(finding: ReviewMemoryFindingInput, signals: ReadonlyArray): ReviewMemoryMatchResult { + if (signals.length === 0) return "keep"; + const path = normalizeFindingPath(finding.path); + const findingHash = fingerprint(finding); + let scopeMatched = false; + for (const signal of signals) { + if (signal.category !== finding.category) continue; + if (!pathGlobMatches(signal.pathGlob, path)) continue; + if (signal.patternHash === findingHash) return "suppress"; + scopeMatched = true; + } + return scopeMatched ? "demote" : "keep"; +} diff --git a/src/review/review-memory-wire.ts b/src/review/review-memory-wire.ts new file mode 100644 index 0000000000..2db0b0cd8d --- /dev/null +++ b/src/review/review-memory-wire.ts @@ -0,0 +1,69 @@ +// Review-memory activation wiring (#2179, config slice of #1964). Mirrors impact-map-wire.ts's +// isImpactMapEnabled: a single GLOBAL env kill-switch the self-host operator controls, ANDed with the per-repo +// `.gittensory.yml review.memory` manifest toggle (resolved via `resolveReviewMemoryManifestToggle`, +// src/signals/focus-manifest.ts) — so a repo can only ever NARROW what the operator has already turned on, +// never widen it. Both OFF by default: with the env flag unset, the suppression store is never read from the +// review path at all (the caller guards on this flag before doing any D1 read or matching), so the review +// stays byte-identical to today. + +import { matchSuppressions, type ReviewMemoryFindingInput } from "./review-memory-match"; +import type { AdvisoryFinding, ReviewSuppressionRecord } from "../types"; + +/** True when repeat-false-positive suppression is enabled at the operator level. Flag-OFF (default) → the + * caller takes no new branch, so no suppression-store read and no matcher call ever happens. Truthy follows + * the codebase convention (`/^(1|true|yes|on)$/i`, same as isImpactMapEnabled / isRagEnabled / + * isSafetyEnabled). */ +export function isReviewMemoryEnabled(env: { GITTENSORY_REVIEW_MEMORY?: string | undefined }): boolean { + return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_MEMORY ?? ""); +} + +/** Resolve whether review-memory suppression should apply for THIS repo/PR: the operator's global env + * kill-switch AND the per-repo manifest opt-in. Neither alone is sufficient — mirrors every other + * converged-feature gate in this codebase (env kill-switch first, then the manifest narrows it further). */ +export function shouldApplyReviewMemory( + env: { GITTENSORY_REVIEW_MEMORY?: string | undefined }, + manifestReviewMemoryEnabled: boolean, +): boolean { + return isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled; +} + +/** Apply-to-findings wiring (#2181, apply slice of #1964). PURE — no DB I/O (the caller already resolved + * `signals` via listReviewSuppressions); the caller wraps the READ side in its own try/catch (fail-safe: a + * store-read error is caught by the caller and this function is never reached at all, so findings pass + * through untouched — see processors.ts). ADVISORY-ONLY BY CONSTRUCTION: the caller must only ever pass this + * the gate's non-blocking `warnings` — NEVER `blockers` — so a suppressed/demoted finding can never affect the + * merge/close disposition. `suppress`-matched findings are DROPPED; `demote`-matched findings are KEPT but + * moved to the END of the list, so an existing `review.max_findings` display cap (if configured) truncates a + * demoted (previously-seen-but-not-identical) finding before a fresh one. Order among non-demoted findings is + * otherwise preserved. */ +export function applyReviewMemorySuppression( + findings: ReadonlyArray, + signals: ReadonlyArray, +): { findings: AdvisoryFinding[]; suppressedCount: number; demotedCount: number } { + if (findings.length === 0 || signals.length === 0) return { findings: [...findings], suppressedCount: 0, demotedCount: 0 }; + const kept: AdvisoryFinding[] = []; + const demoted: AdvisoryFinding[] = []; + let suppressedCount = 0; + for (const finding of findings) { + const result = matchSuppressions(toReviewMemoryFindingInput(finding), signals); + if (result === "suppress") { + suppressedCount += 1; + continue; + } + if (result === "demote") { + demoted.push(finding); + continue; + } + kept.push(finding); + } + return { findings: [...kept, ...demoted], suppressedCount, demotedCount: demoted.length }; +} + +/** Adapt an `AdvisoryFinding` (the gate's own finding shape) to the decoupled `ReviewMemoryFindingInput` the + * matcher needs: `category` is the finding's own deterministic `code`; `AdvisoryFinding` carries no `path` + * today, so every finding fingerprints as repo-wide ("" path) — a future path-anchored finding type can pass + * its own path through once one exists, with zero change to the matcher itself. `message` combines `title` + + * `detail` so two findings with the same title but a different detail body still fingerprint differently. */ +function toReviewMemoryFindingInput(finding: AdvisoryFinding): ReviewMemoryFindingInput { + return { category: finding.code, message: `${finding.title} ${finding.detail}` }; +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index a7ace89f5e..99f50b1c21 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -392,6 +392,18 @@ export type FocusManifestReviewConfig = { * field only opts THIS repo in once the capability itself is enabled). null/false (default, absent) = no * section appended = byte-identical behavior. */ cultureProfile: boolean | null; + /** `review.memory` (#2179, config slice of #1964): when true, gates repeat-false-positive SUPPRESSION — + * before an advisory (non-blocking) AI finding is surfaced in the unified review comment, it is matched + * against this repo's stored `review_suppression` signals (a maintainer's own past false-positive + * dismissals, `src/db/repositories.ts`'s `listReviewSuppressions`, migrations/0114) and demoted/dropped on a + * match (`src/review/review-memory-match.ts`'s `matchSuppressions`). ADVISORY-ONLY BY CONSTRUCTION: it is + * never applied to gate blockers, so it can never change the merge/close disposition — only which + * non-blocking nits render. ALSO requires the global env kill-switch (`isReviewMemoryEnabled`, mirroring + * `isImpactMapEnabled` in `src/review/impact-map-wire.ts`) to be on; the manifest flag alone cannot enable + * it for a self-host operator who hasn't opted in globally. Fail-safe: a suppression-store read error or + * matcher throw leaves findings untouched. null/false (default, absent) ⇒ no suppression lookup at all = + * byte-identical behavior. */ + reviewMemory: boolean | null; /** `review.finding_categories`: when true, an inline finding is ALSO tagged with a category (security/ * correctness/performance/maintainability/tests/style) — the AI reviewer is asked to self-categorize, with a * deterministic path/keyword fallback (`classifyFindingCategory`) covering whatever it omits. Only takes @@ -764,7 +776,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -795,7 +807,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1770,7 +1782,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1814,6 +1826,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const testGeneration = normalizeOptionalBoolean(r.test_generation, "review.test_generation", warnings); const impactMap = normalizeOptionalBoolean(r.impact_map, "review.impact_map", warnings); const cultureProfile = normalizeOptionalBoolean(r.culture_profile, "review.culture_profile", warnings); + const reviewMemory = normalizeOptionalBoolean(r.memory, "review.memory", warnings); const findingCategories = normalizeOptionalBoolean(r.finding_categories, "review.finding_categories", warnings); const minFindingSeverity = normalizeOptionalEnum( r.min_finding_severity, @@ -1848,6 +1861,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo testGeneration !== null || impactMap !== null || cultureProfile !== null || + reviewMemory !== null || findingCategories !== null || minFindingSeverity !== null || maxFindingsPresent(maxFindings) || @@ -1883,6 +1897,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo effortScore, impactMap, cultureProfile, + reviewMemory, findingCategories, minFindingSeverity, maxFindings, @@ -2347,6 +2362,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.testGeneration !== null) out.test_generation = review.testGeneration; if (review.impactMap !== null) out.impact_map = review.impactMap; if (review.cultureProfile !== null) out.culture_profile = review.cultureProfile; + if (review.reviewMemory !== null) out.memory = review.reviewMemory; if (review.findingCategories !== null) out.finding_categories = review.findingCategories; if (review.minFindingSeverity !== null) out.min_finding_severity = review.minFindingSeverity; if (maxFindingsPresent(review.maxFindings)) { @@ -2608,6 +2624,15 @@ export function resolveTestGenerationManifestToggle(manifest: FocusManifest | nu return manifest?.review.testGeneration === true; } +/** Resolve `review.memory` (#2179, config slice of #1964) from a possibly-null manifest (null = load failure ⇒ + * manifest toggle reads as unset/false). Mirrors resolveTestGenerationManifestToggle's shape exactly — true + * ONLY when the manifest explicitly set review.memory: true; null/false/absent ⇒ false. The caller further + * ANDs this with the operator's GITTENSORY_REVIEW_MEMORY kill-switch via isReviewMemoryEnabled + * (src/review/review-memory-wire.ts) before ever reading the suppression store. */ +export function resolveReviewMemoryManifestToggle(manifest: FocusManifest | null): boolean { + return manifest?.review.reviewMemory === true; +} + /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized * so the gate caller resolves them in one place with the null-manifest branch covered here (unit-tested) rather * than inline in the processor. (#review-pre-merge-checks) */ diff --git a/src/types.ts b/src/types.ts index 8eb7824418..96d2410695 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1589,6 +1589,20 @@ export type GateOutcomeRecord = { updatedAt?: string | null | undefined; }; +// Review memory (#2178, data-model slice of #1964). One row per (repoFullName, category, pathGlob, +// patternHash) — a maintainer-dismissed finding shape gittensory should suppress/demote if it recurs. +// Privacy: repo + category (the finding's own deterministic `code`) + a path glob + a message HASH ONLY — +// never the raw finding message/title, never an actor's trust/reward fields. +export type ReviewSuppressionRecord = { + id: string; + repoFullName: string; + category: string; + pathGlob: string; + patternHash: string; + createdAt: string; + createdBy?: string | null | undefined; +}; + export type AgentRecommendationOutcomeSummary = { login: string; generatedAt: string; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index efdfdc1cc0..db1f9e8714 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -34,6 +34,7 @@ import { resolveReviewVisualConfig, repoDocGenerationConfigToJson, resolveTestGenerationManifestToggle, + resolveReviewMemoryManifestToggle, reviewConfigToJson, reviewRecapConfigToJson, settingsOverrideToJson, @@ -362,6 +363,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { testGeneration: "test_generation:", impactMap: "impact_map:", cultureProfile: "culture_profile:", + reviewMemory: "memory:", findingCategories: "finding_categories:", minFindingSeverity: "min_finding_severity:", maxFindings: "max_findings:", @@ -785,7 +787,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -3089,6 +3091,23 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(bad.warnings.some((w) => /review\.culture_profile.*must be a boolean/.test(w))).toBe(true); }); + it("parses review.memory (default OFF), marks present, round-trips, and warns on a non-boolean (#2179)", () => { + expect(parseFocusManifest({ review: { memory: true } }).review.reviewMemory).toBe(true); + const on = parseFocusManifest({ review: { memory: true } }); + expect(on.review.present).toBe(true); // a memory-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { memory: false } }); + expect(off.review.reviewMemory).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.reviewMemory).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { memory: "yes" } }); + expect(bad.review.reviewMemory).toBeNull(); + expect(bad.warnings.some((w) => /review\.memory.*must be a boolean/.test(w))).toBe(true); + }); + it("parses review.finding_categories (default OFF), marks present, round-trips, and warns on a non-boolean (#1958)", () => { expect(parseFocusManifest({ review: { finding_categories: true } }).review.findingCategories).toBe(true); const on = parseFocusManifest({ review: { finding_categories: true } }); @@ -3113,6 +3132,13 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(resolveTestGenerationManifestToggle(parseFocusManifest({ review: { test_generation: true } }))).toBe(true); }); + it("resolves review.memory's manifest toggle to a strict boolean (#2179)", () => { + expect(resolveReviewMemoryManifestToggle(null)).toBe(false); // null manifest (load failure) ⇒ false + expect(resolveReviewMemoryManifestToggle(parseFocusManifest({}))).toBe(false); // absent ⇒ false + expect(resolveReviewMemoryManifestToggle(parseFocusManifest({ review: { memory: false } }))).toBe(false); + expect(resolveReviewMemoryManifestToggle(parseFocusManifest({ review: { memory: true } }))).toBe(true); + }); + it("parses review.min_finding_severity, round-trips, and warns on invalid values (#2048)", () => { const major = parseFocusManifest({ review: { min_finding_severity: "major" } }); expect(major.review.minFindingSeverity).toBe("major"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 7e2c99a3d7..8747f9945f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -51,10 +51,12 @@ import { upsertRepositoryFromGitHub, putCachedAiReview, markAiReviewPublished, + recordReviewSuppression, } from "../../src/db/repositories"; import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAiReviewLock, claimPrActuationLock, contributorEvidenceBatchSize, enrichOpenPullRequestsWithChangedFiles, processJob, reconcileLiveDuplicateSiblings, releaseAiReviewLock, releasePrActuationLock } from "../../src/queue/processors"; import type { PullRequestRecord } from "../../src/types"; import { aiReviewCacheInputFingerprint } from "../../src/review/ai-review-cache-input"; +import { fingerprint as reviewMemoryFingerprint } from "../../src/review/review-memory-match"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; import { persistRegistrySnapshot } from "../../src/registry/sync"; @@ -17078,6 +17080,247 @@ describe("queue processors", () => { } }); + // #2181 (apply slice of #1964): review.memory end-to-end through the real webhook path. A `qualityGateMode: + // "advisory"` + an unreachable `qualityGateMinScore: 100` deterministically produces the + // `readiness_score_below_threshold` ADVISORY (never a blocker — readiness stays advisory-only, see + // rules.test.ts) warning finding on every pass, giving a stable target to record a suppression signal against + // and verify it is (or is not) suppressed from the rendered unified comment. The manifest is seeded DIRECTLY + // via upsertRepoFocusManifest (bypassing the 6h .gittensory.yml fetch cache) so each test's `.gittensory.yml` + // fetch response is never actually needed on the hot path — it only serves as an inert 404 fallback. + async function runReadinessWarningPass(env: Env, opts: { deliveryId: string; headSha: string; reviewMemoryManifest: boolean }) { + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + privateTrustEnabled: true, + autonomy: { update_branch: "auto" }, + qualityGateMode: "advisory", + qualityGateMinScore: 100, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", opts.reviewMemoryManifest ? { review: { memory: true } } : {}); + let postedBody = ""; + let gateFinalized = false; + let failedPostGateMint = false; + const liveCiSpy = vi + .spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl") + .mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") { + return Response.json([ + { + uid: 7, + githubUsername: "oktofeesh1", + githubId: "123", + totalPrs: 4, + totalMergedPrs: 3, + totalOpenPrs: 1, + totalClosedPrs: 0, + totalOpenIssues: 0, + totalClosedIssues: 0, + totalSolvedIssues: 0, + totalValidSolvedIssues: 0, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + hotkey: "must-not-leak", + }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ + repositories: [ + { + repositoryFullName: "JSONbored/gittensory", + totalPrs: "4", + totalMergedPrs: "3", + totalOpenPrs: "1", + totalClosedPrs: "0", + totalOpenIssues: "0", + totalClosedIssues: "0", + isEligible: true, + credibility: "1.000000", + }, + ], + }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.gittensory.yml") { + return new Response("not found", { status: 404 }); + } + if (url.includes("/access_tokens")) { + if (gateFinalized && !failedPostGateMint) { + failedPostGateMint = true; + return new Response("mint failed", { status: 500 }); + } + return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" }); + } + if (url.includes("/pulls/3/files")) + return Response.json([{ filename: "src/cache.ts", additions: 5, deletions: 1, status: "modified" }]); + if (/\/pulls\/3(?:\?|$)/.test(url)) return Response.json({ number: 3, mergeable_state: "clean" }); + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") { + const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string }; + if (body.status !== "in_progress" || body.conclusion) { + gateFinalized = true; + clearInstallationTokenCacheForTest(); + } + return Response.json({ id: 901 }, { status: 201 }); + } + if (url.includes("/check-runs/901") && method === "PATCH") { + gateFinalized = true; + clearInstallationTokenCacheForTest(); + return Response.json({ id: 901 }); + } + if (url.includes("/issues/3/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/3/comments") && method === "POST") { + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + try { + await processJob(env, { + type: "github-webhook", + deliveryId: opts.deliveryId, + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 3, + title: "Fix webhook duplicate delivery again", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: opts.headSha }, + labels: [{ name: "bug" }], + // No linked issue AND no validation evidence -- keeps the readiness score comfortably below the + // unreachable qualityGateMinScore: 100 threshold above, so readiness_score_below_threshold fires + // deterministically regardless of the panel's exact scoring breakdown. + body: "No linked issue, no validation evidence on purpose.", + }, + }, + }); + } finally { + liveCiSpy.mockRestore(); + } + return postedBody; + } + + it("FLAG-OFF (default): review.memory in .gittensory.yml alone never suppresses the readiness warning (operator kill-switch required)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + // review.memory: true in the manifest, but NO GITTENSORY_REVIEW_MEMORY env flag on this env -- byte-identical. + const postedBody = await runReadinessWarningPass(env, { + deliveryId: "review-memory-flag-off", + headSha: "revmem-flag-off", + reviewMemoryManifest: true, + }); + expect(postedBody).toContain("Readiness score is below the configured threshold"); + }); + + it("FLAG-ON: suppresses a readiness warning EXACTLY matching a previously recorded suppression signal", async () => { + const seedEnv = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + // A throwaway pass (flag/manifest both off — byte-identical review path) against a SEPARATE, disposable D1 + // instance just to learn the finding's REAL, LIVE-computed readiness score (a pure function of the fixed + // PR/settings fixture above, so it reproduces identically for the real pass below on its own fresh `env`). + // The rendered nit itself only carries `title`+`action` (see buildDualReviewNotes's gateNits) — the score + // comes from the status chip. + const seedBody = await runReadinessWarningPass(seedEnv, { deliveryId: "review-memory-seed", headSha: "revmem-seed", reviewMemoryManifest: false }); + expect(seedBody).toContain("Readiness score is below the configured threshold"); + const scoreMatch = /readiness (\d+)\/100/.exec(seedBody); + expect(scoreMatch).not.toBeNull(); + const score = Number(scoreMatch![1]); + // Reconstructs buildQualityGateWarning's exact title+detail template (src/rules/advisory.ts) from the live + // score + the qualityGateMinScore: 100 configured above, so the computed patternHash matches the real finding. + const detail = `The public readiness score is ${score}/100, below the repository threshold of 100/100.`; + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", GITTENSORY_REVIEW_MEMORY: "true" }); + await recordReviewSuppression(env, { + repoFullName: "JSONbored/gittensory", + category: "readiness_score_below_threshold", + patternHash: reviewMemoryFingerprint({ + category: "readiness_score_below_threshold", + message: `Readiness score is below the configured threshold ${detail}`, + }), + createdBy: "maintainer1", + }); + // The flag is ON (env + manifest) and the exact-match signal is now stored -- the warning must be + // suppressed from the rendered unified comment. + const postedBody = await runReadinessWarningPass(env, { + deliveryId: "review-memory-flag-on", + headSha: "revmem-flag-on", + reviewMemoryManifest: true, + }); + expect(postedBody).not.toContain("Readiness score is below the configured threshold"); + }); + + it("FLAG-ON: DEMOTES (keeps, but does not suppress) a same-category readiness warning that does not exactly match any stored signal", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", GITTENSORY_REVIEW_MEMORY: "true" }); + // A signal for the SAME category but a patternHash that can never match this PR's real finding -- exercises + // the "demote" (scope-matched, hash-mismatched) branch instead of "suppress". + await recordReviewSuppression(env, { + repoFullName: "JSONbored/gittensory", + category: "readiness_score_below_threshold", + patternHash: "never-matches-the-real-finding", + createdBy: "maintainer1", + }); + const postedBody = await runReadinessWarningPass(env, { + deliveryId: "review-memory-demote", + headSha: "revmem-demote", + reviewMemoryManifest: true, + }); + // Demoted (not suppressed) -- the finding still renders in the comment. + expect(postedBody).toContain("Readiness score is below the configured threshold"); + }); + + it("FLAG-ON, fail-safe: a suppression-store read error leaves the readiness warning untouched rather than throwing", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", GITTENSORY_REVIEW_MEMORY: "true" }); + const listSpy = vi.spyOn(repositoriesModule, "listReviewSuppressions").mockRejectedValue(new Error("D1 unavailable")); + try { + const postedBody = await runReadinessWarningPass(env, { + deliveryId: "review-memory-store-error", + headSha: "revmem-store-error", + reviewMemoryManifest: true, + }); + expect(postedBody).toContain("Readiness score is below the configured threshold"); + } finally { + listSpy.mockRestore(); + } + }); + // #1955: the review-effort minutes persisted onto the public-stats audit event (independent of // review.effort_score, which only gates the unified-comment CHIP) must never block the publish itself when the // estimator throws — the publish still completes and simply omits `reviewEffortMinutes` from the event metadata diff --git a/test/unit/review-memory-match.test.ts b/test/unit/review-memory-match.test.ts new file mode 100644 index 0000000000..8f08487e3f --- /dev/null +++ b/test/unit/review-memory-match.test.ts @@ -0,0 +1,150 @@ +import { describe, expect, it } from "vitest"; +import { fingerprint, matchSuppressions, normalizeFindingMessage, normalizeFindingPath } from "../../src/review/review-memory-match"; +import type { ReviewSuppressionRecord } from "../../src/types"; + +function signal(overrides: Partial = {}): ReviewSuppressionRecord { + return { + id: "sig-1", + repoFullName: "owner/repo", + category: "ai_review_split", + pathGlob: "", + patternHash: "irrelevant", + createdAt: "2026-01-01T00:00:00.000Z", + createdBy: null, + ...overrides, + }; +} + +describe("normalizeFindingMessage", () => { + it("lowercases, collapses whitespace runs, and trims", () => { + expect(normalizeFindingMessage(" Some Message\n\twith\tWHITESPACE ")).toBe("some message with whitespace"); + }); + + it("bounds an extremely long message so fingerprinting stays cheap", () => { + const huge = "a".repeat(10_000); + expect(normalizeFindingMessage(huge).length).toBe(4000); + }); + + it("is a no-op on an already-normalized message", () => { + expect(normalizeFindingMessage("already normal")).toBe("already normal"); + }); +}); + +describe("normalizeFindingPath", () => { + it("canonicalizes a path (case + separator insensitive)", () => { + expect(normalizeFindingPath("Src\\Foo\\Bar.ts")).toBe("src/foo/bar.ts"); + }); + + it("defaults to '' (repo-wide) for null/undefined/empty path", () => { + expect(normalizeFindingPath(null)).toBe(""); + expect(normalizeFindingPath(undefined)).toBe(""); + expect(normalizeFindingPath("")).toBe(""); + }); +}); + +describe("fingerprint (#2180)", () => { + it("is deterministic: the same input always yields the same fingerprint", () => { + const finding = { category: "ai_review_split", path: "src/a.ts", message: "Some finding message." }; + expect(fingerprint(finding)).toBe(fingerprint({ ...finding })); + }); + + it("is stable across whitespace/case-only message differences (normalization applies before hashing)", () => { + const a = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "Some Message" }); + const b = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: " some message " }); + expect(a).toBe(b); + }); + + it("is stable across path separator/case differences (canonicalization applies before hashing)", () => { + const a = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "msg" }); + const b = fingerprint({ category: "ai_review_split", path: "Src\\A.ts", message: "msg" }); + expect(a).toBe(b); + }); + + it("differs when category differs (same path/message)", () => { + const a = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "msg" }); + const b = fingerprint({ category: "ai_consensus_defect", path: "src/a.ts", message: "msg" }); + expect(a).not.toBe(b); + }); + + it("differs when path differs (same category/message)", () => { + const a = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "msg" }); + const b = fingerprint({ category: "ai_review_split", path: "src/b.ts", message: "msg" }); + expect(a).not.toBe(b); + }); + + it("differs when message differs (same category/path)", () => { + const a = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "msg one" }); + const b = fingerprint({ category: "ai_review_split", path: "src/a.ts", message: "msg two" }); + expect(a).not.toBe(b); + }); + + it("treats an absent path the same as an explicitly empty path (both repo-wide)", () => { + const a = fingerprint({ category: "ai_review_split", message: "msg" }); + const b = fingerprint({ category: "ai_review_split", path: "", message: "msg" }); + expect(a).toBe(b); + }); +}); + +describe("matchSuppressions (#2180)", () => { + const finding = { category: "ai_review_split", path: "src/foo/bar.ts", message: "The reviewer flagged X." }; + + it("keep: an empty signal set never matches anything", () => { + expect(matchSuppressions(finding, [])).toBe("keep"); + }); + + it("suppress: an EXACT category+path+patternHash match", () => { + const signals = [signal({ category: finding.category, pathGlob: "src/foo/**", patternHash: fingerprint(finding) })]; + expect(matchSuppressions(finding, signals)).toBe("suppress"); + }); + + it("suppress: a repo-wide (empty pathGlob) exact patternHash match", () => { + const signals = [signal({ category: finding.category, pathGlob: "", patternHash: fingerprint(finding) })]; + expect(matchSuppressions(finding, signals)).toBe("suppress"); + }); + + it("demote: category+path scope matches but the patternHash differs (a different-worded finding at the same spot)", () => { + const signals = [signal({ category: finding.category, pathGlob: "src/foo/**", patternHash: "some-other-hash" })]; + expect(matchSuppressions(finding, signals)).toBe("demote"); + }); + + it("keep: category matches but the pathGlob scope does NOT cover this finding's path", () => { + const signals = [signal({ category: finding.category, pathGlob: "test/**", patternHash: fingerprint(finding) })]; + expect(matchSuppressions(finding, signals)).toBe("keep"); + }); + + it("keep: pathGlob would match but the category differs", () => { + const signals = [signal({ category: "ai_consensus_defect", pathGlob: "src/foo/**", patternHash: fingerprint(finding) })]; + expect(matchSuppressions(finding, signals)).toBe("keep"); + }); + + it("suppress wins over demote when BOTH an exact-hash signal and a scope-only signal exist for the same finding", () => { + const signals = [ + signal({ id: "scope-only", category: finding.category, pathGlob: "src/foo/**", patternHash: "different-hash" }), + signal({ id: "exact", category: finding.category, pathGlob: "src/foo/**", patternHash: fingerprint(finding) }), + ]; + expect(matchSuppressions(finding, signals)).toBe("suppress"); + }); + + it("demote: multiple scope-matching signals, none an exact hash match, still demotes (not suppress)", () => { + const signals = [ + signal({ id: "a", category: finding.category, pathGlob: "src/foo/**", patternHash: "hash-a" }), + signal({ id: "b", category: finding.category, pathGlob: "src/**", patternHash: "hash-b" }), + ]; + expect(matchSuppressions(finding, signals)).toBe("demote"); + }); + + it("keep: a finding with no path (repo-level) only matches a repo-wide ('') or wildcard pathGlob signal", () => { + const repoLevelFinding = { category: "ai_review_inconclusive", message: "no usable verdict" }; + const scopedSignal = signal({ category: repoLevelFinding.category, pathGlob: "src/**", patternHash: "whatever" }); + expect(matchSuppressions(repoLevelFinding, [scopedSignal])).toBe("keep"); + const repoWideSignal = signal({ category: repoLevelFinding.category, pathGlob: "", patternHash: fingerprint(repoLevelFinding) }); + expect(matchSuppressions(repoLevelFinding, [repoWideSignal])).toBe("suppress"); + }); + + it("an over-complex pathGlob (unsafe wildcard count) never matches, degrading to keep/demote rather than throwing", () => { + // globToRegExp compiles an over-complex glob to NEVER_MATCHES rather than a real pattern (ReDoS guard). + const overComplexGlob = "a*b*c*d*e*f*g*h*i*j*k*/**"; + const signals = [signal({ category: finding.category, pathGlob: overComplexGlob, patternHash: fingerprint(finding) })]; + expect(matchSuppressions(finding, signals)).toBe("keep"); + }); +}); diff --git a/test/unit/review-memory-store.test.ts b/test/unit/review-memory-store.test.ts new file mode 100644 index 0000000000..21fa740233 --- /dev/null +++ b/test/unit/review-memory-store.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, it } from "vitest"; +import { MAX_REVIEW_SUPPRESSIONS_PER_REPO, listReviewSuppressions, recordReviewSuppression } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// Review memory (#2178, data-model slice of #1964): insert/list repository accessors over the +// review_suppression table (migrations/0114). No recording-trigger and no apply-during-review logic here -- +// those are separate slices (#2180/#2181) -- this only covers the store itself. +describe("review-memory suppression store (#2178)", () => { + async function rawRow(env: Env, repoFullName: string, category: string, pathGlob: string, patternHash: string) { + return env.DB.prepare("select id, created_at, created_by from review_suppression where repo_full_name = ? and category = ? and path_glob = ? and pattern_hash = ?") + .bind(repoFullName, category, pathGlob, patternHash) + .first<{ id: string; created_at: string; created_by: string | null }>(); + } + + it("records a suppression signal and lists it back for the repo", async () => { + const env = createTestEnv(); + const record = await recordReviewSuppression(env, { + repoFullName: "owner/repo", + category: "ai_review_split", + pathGlob: "src/foo/**", + patternHash: "hash-1", + createdBy: "maintainer1", + }); + expect(record).toMatchObject({ + repoFullName: "owner/repo", + category: "ai_review_split", + pathGlob: "src/foo/**", + patternHash: "hash-1", + createdBy: "maintainer1", + }); + const listed = await listReviewSuppressions(env, "owner/repo"); + expect(listed).toHaveLength(1); + expect(listed[0]).toMatchObject({ category: "ai_review_split", patternHash: "hash-1" }); + }); + + it("defaults pathGlob to empty string (repo-wide) and createdBy to null when omitted", async () => { + const env = createTestEnv(); + const record = await recordReviewSuppression(env, { + repoFullName: "owner/repo", + category: "ai_review_inconclusive", + patternHash: "hash-2", + }); + expect(record.pathGlob).toBe(""); + expect(record.createdBy).toBeNull(); + }); + + it("listReviewSuppressions is empty for a repo with no rows at all", async () => { + const env = createTestEnv(); + expect(await listReviewSuppressions(env, "owner/nothing-here")).toEqual([]); + }); + + it("re-recording the SAME key upserts (bumps createdAt/createdBy) instead of creating a duplicate row", async () => { + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "src/**", patternHash: "hash-3", createdBy: "maintainer1" }); + const firstRow = await rawRow(env, "owner/repo", "ai_review_split", "src/**", "hash-3"); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "src/**", patternHash: "hash-3", createdBy: "maintainer2" }); + const secondRow = await rawRow(env, "owner/repo", "ai_review_split", "src/**", "hash-3"); + expect(secondRow?.id).toBe(firstRow?.id); // same row, not a new insert + expect(secondRow?.created_by).toBe("maintainer2"); // most recent dismissal wins + const listed = await listReviewSuppressions(env, "owner/repo"); + expect(listed).toHaveLength(1); + }); + + it("a DIFFERENT category, pathGlob, or patternHash is a distinct row, not an upsert of an existing one", async () => { + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "src/**", patternHash: "hash-a" }); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_consensus_defect", pathGlob: "src/**", patternHash: "hash-a" }); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "test/**", patternHash: "hash-a" }); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "src/**", patternHash: "hash-b" }); + expect(await listReviewSuppressions(env, "owner/repo")).toHaveLength(4); + }); + + it("scopes listing strictly to the given repo -- another repo's rows never leak in", async () => { + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo-a", category: "ai_review_split", patternHash: "hash-1" }); + await recordReviewSuppression(env, { repoFullName: "owner/repo-b", category: "ai_review_split", patternHash: "hash-1" }); + expect(await listReviewSuppressions(env, "owner/repo-a")).toHaveLength(1); + expect(await listReviewSuppressions(env, "owner/repo-b")).toHaveLength(1); + }); + + it("enforces the per-repo bound: once a repo exceeds MAX_REVIEW_SUPPRESSIONS_PER_REPO rows, the OLDEST are evicted", async () => { + const env = createTestEnv(); + // Insert one MORE than the cap, each a distinct key so none upsert into another. + for (let i = 0; i < MAX_REVIEW_SUPPRESSIONS_PER_REPO + 1; i += 1) { + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: `hash-${i}` }); + } + const listed = await listReviewSuppressions(env, "owner/repo", MAX_REVIEW_SUPPRESSIONS_PER_REPO + 5); + expect(listed.length).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO); + // The very first inserted key ("hash-0") is the oldest and must have been evicted. + expect(listed.some((row) => row.patternHash === "hash-0")).toBe(false); + // The most recently inserted key must survive. + expect(listed.some((row) => row.patternHash === `hash-${MAX_REVIEW_SUPPRESSIONS_PER_REPO}`)).toBe(true); + }); + + it("listReviewSuppressions clamps an out-of-range limit into [1, MAX_REVIEW_SUPPRESSIONS_PER_REPO]", async () => { + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" }); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_consensus_defect", patternHash: "hash-2" }); + expect(await listReviewSuppressions(env, "owner/repo", 0)).toHaveLength(1); + expect(await listReviewSuppressions(env, "owner/repo", 999_999)).toHaveLength(2); + }); +}); diff --git a/test/unit/review-memory-wire.test.ts b/test/unit/review-memory-wire.test.ts new file mode 100644 index 0000000000..9bfe45eed0 --- /dev/null +++ b/test/unit/review-memory-wire.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, it } from "vitest"; +import { applyReviewMemorySuppression, isReviewMemoryEnabled, shouldApplyReviewMemory } from "../../src/review/review-memory-wire"; +import { fingerprint } from "../../src/review/review-memory-match"; +import type { AdvisoryFinding, ReviewSuppressionRecord } from "../../src/types"; + +describe("isReviewMemoryEnabled", () => { + it("is OFF for unset/false and ON for the truthy convention", () => { + expect(isReviewMemoryEnabled({})).toBe(false); + expect(isReviewMemoryEnabled({ GITTENSORY_REVIEW_MEMORY: "false" })).toBe(false); + expect(isReviewMemoryEnabled({ GITTENSORY_REVIEW_MEMORY: "true" })).toBe(true); + expect(isReviewMemoryEnabled({ GITTENSORY_REVIEW_MEMORY: "1" })).toBe(true); + expect(isReviewMemoryEnabled({ GITTENSORY_REVIEW_MEMORY: "on" })).toBe(true); + expect(isReviewMemoryEnabled({ GITTENSORY_REVIEW_MEMORY: "yes" })).toBe(true); + }); +}); + +describe("shouldApplyReviewMemory", () => { + it("requires BOTH the operator env flag AND the per-repo manifest opt-in", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "true" }, true)).toBe(true); + }); + + it("is OFF when the operator flag is on but the manifest didn't opt in", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "true" }, false)).toBe(false); + }); + + it("is OFF when the manifest opted in but the operator flag is off (repo cannot self-enable)", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "false" }, true)).toBe(false); + }); + + it("is OFF when both are off", () => { + expect(shouldApplyReviewMemory({}, false)).toBe(false); + }); +}); + +describe("applyReviewMemorySuppression (#2181)", () => { + function finding(overrides: Partial = {}): AdvisoryFinding { + return { code: "ai_review_split", title: "An AI reviewer flagged a likely blocking defect", severity: "warning", detail: "Some finding detail.", ...overrides }; + } + + function signal(overrides: Partial = {}): ReviewSuppressionRecord { + return { id: "sig-1", repoFullName: "owner/repo", category: "ai_review_split", pathGlob: "", patternHash: "irrelevant", createdAt: "2026-01-01T00:00:00.000Z", createdBy: null, ...overrides }; + } + + function findingHash(f: AdvisoryFinding): string { + return fingerprint({ category: f.code, message: `${f.title} ${f.detail}` }); + } + + it("is a no-op (byte-identical array) when there are no findings", () => { + const result = applyReviewMemorySuppression([], [signal()]); + expect(result).toEqual({ findings: [], suppressedCount: 0, demotedCount: 0 }); + }); + + it("is a no-op (byte-identical array) when there are no stored signals", () => { + const f = finding(); + const result = applyReviewMemorySuppression([f], []); + expect(result).toEqual({ findings: [f], suppressedCount: 0, demotedCount: 0 }); + }); + + it("drops a finding that exactly matches a stored suppression signal", () => { + const f = finding(); + const signals = [signal({ patternHash: findingHash(f) })]; + const result = applyReviewMemorySuppression([f], signals); + expect(result).toEqual({ findings: [], suppressedCount: 1, demotedCount: 0 }); + }); + + it("keeps but moves a scope-matched (category, different message) finding to the END of the list", () => { + const a = finding({ code: "ai_review_split", title: "Finding A", detail: "detail a" }); + const b = finding({ code: "ai_consensus_defect", title: "Finding B", detail: "detail b" }); + // A scope-only match for "a" (same category, different message hash); "b" has no matching signal at all. + const signals = [signal({ category: "ai_review_split", patternHash: "unrelated-hash" })]; + const result = applyReviewMemorySuppression([a, b], signals); + expect(result.suppressedCount).toBe(0); + expect(result.demotedCount).toBe(1); + expect(result.findings).toEqual([b, a]); // demoted "a" moved to the end; "b" (kept) stays first + }); + + it("keeps a finding untouched (in its original position) when nothing matches its category/path scope", () => { + const f = finding({ code: "ai_review_inconclusive" }); + const signals = [signal({ category: "ai_consensus_defect", patternHash: "whatever" })]; + const result = applyReviewMemorySuppression([f], signals); + expect(result).toEqual({ findings: [f], suppressedCount: 0, demotedCount: 0 }); + }); + + it("handles a mix of suppress/demote/keep across several findings in one call", () => { + const suppressMe = finding({ code: "ai_review_split", title: "Suppress me", detail: "d1" }); + const demoteMe = finding({ code: "ai_review_split", title: "Demote me", detail: "d2" }); + const keepMe = finding({ code: "ai_review_inconclusive", title: "Keep me", detail: "d3" }); + const signals = [ + signal({ id: "exact", category: "ai_review_split", patternHash: findingHash(suppressMe) }), + signal({ id: "scope", category: "ai_review_split", patternHash: "some-other-hash" }), + ]; + const result = applyReviewMemorySuppression([suppressMe, demoteMe, keepMe], signals); + expect(result.suppressedCount).toBe(1); + expect(result.demotedCount).toBe(1); + expect(result.findings).toEqual([keepMe, demoteMe]); + }); + + it("never mutates the input findings array or its elements", () => { + const f = finding(); + const findings = [f]; + const signals = [signal({ patternHash: findingHash(f) })]; + applyReviewMemorySuppression(findings, signals); + expect(findings).toEqual([f]); // original array untouched despite f being suppressed in the result + }); +}); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 338fd1bd7f..9f876787e3 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 1e0bf65723..4ec12f8f54 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: db4764f8e94b5a665c1fc749bbbe839d) +// Generated by Wrangler by running `wrangler types` (hash: 1101774c5b8456e1e46f2b918558b1aa) // Runtime types generated with workerd@1.20260701.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { DB: D1Database; @@ -28,6 +28,7 @@ interface __BaseEnv_Env { GITTENSORY_REVIEW_RAG: "false"; GITTENSORY_REVIEW_IMPACT_MAP: "false"; GITTENSORY_REVIEW_CULTURE_PROFILE: "false"; + GITTENSORY_REVIEW_MEMORY: "false"; GITTENSORY_REVIEW_CONTENT_LANE: "false"; GITTENSORY_REVIEW_SELFTUNE: "false"; GITHUB_STATUS_ROLLUP_GRAPHQL: "false"; @@ -53,7 +54,7 @@ type StringifyValues> = { [Binding in keyof EnvType]: EnvType[Binding] extends string ? EnvType[Binding] : string; }; declare namespace NodeJS { - interface ProcessEnv extends StringifyValues> {} + interface ProcessEnv extends StringifyValues> {} } // Begin runtime types diff --git a/wrangler.jsonc b/wrangler.jsonc index 648b6731f2..556deeb50a 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -96,6 +96,14 @@ // gate/scoring input. Also requires the per-repo `.gittensory.yml` review.culture_profile: true opt-in. // Default OFF — flag-OFF performs no extra D1 read and keeps the reviewer prompt byte-identical. "GITTENSORY_REVIEW_CULTURE_PROFILE": "false", + // Review memory (#2179, part of #1964): the operator-level kill-switch for repeat-false-positive + // suppression — before an advisory (non-blocking) AI finding is surfaced, it is matched against this + // repo's stored review_suppression signals (a maintainer's own past false-positive dismissals) and + // demoted/dropped on a match. ANDed with the per-repo `.gittensory.yml review.memory` opt-in — neither + // alone is sufficient. ADVISORY-ONLY: never applied to gate blockers, so it can never change the + // merge/close disposition. Default OFF — flag-OFF performs no suppression-store read and no matching, + // byte-identical to today. + "GITTENSORY_REVIEW_MEMORY": "false", // Convergence (content/registry SURFACE LANE): when truthy AND the repo is in GITTENSORY_REVIEW_REPOS, the // deterministic, AI-FREE surface review drives the gate for registry-submission PRs (metagraphed). Default // OFF (false): the processor takes no new branch + resolves no files, so the gate disposition is byte- From 9df0b5483a0d694d01ea17858d4afc0fb5f370fe Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:28:48 -0700 Subject: [PATCH 2/3] fix(review): fix broken suppression-cap eviction, close codecov/patch gaps pruneReviewSuppressionsOverCap's OFFSET-only query is a syntax error on this driver (a bare OFFSET with no preceding LIMIT), so eviction silently failed on every insert -- the per-repo cap only appeared to hold because listReviewSuppressions clamps its own read-side limit to the same constant. Switches to an in-JS slice over the bounded row set instead of relying on SQL-level OFFSET, and strengthens the existing cap test to assert the underlying row count directly rather than through the limit-clamped read path. --- src/db/repositories.ts | 15 ++++++++++----- test/unit/queue.test.ts | 14 ++++++++++++++ test/unit/review-memory-store.test.ts | 20 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 223b6014ec..21cd7d32b3 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4788,7 +4788,9 @@ export async function recordReviewSuppression( ), ) .get(); - await pruneReviewSuppressionsOverCap(env, repoFullName).catch(() => undefined); + await pruneReviewSuppressionsOverCap(env, repoFullName).catch((error) => { + console.warn("Failed to prune over-cap review suppressions", { repoFullName, error: errorMessage(error) }); + }); /* v8 ignore next -- the row was just inserted/updated in this same call; a missing read-back would mean D1 * itself failed silently, not a reachable application branch. */ return row ? toReviewSuppressionRecord(row) : { ...values, createdAt: nowIso() }; @@ -4799,19 +4801,22 @@ export async function recordReviewSuppression( * unbounded. Internal to recordReviewSuppression; not exported. */ async function pruneReviewSuppressionsOverCap(env: Env, repoFullName: string): Promise { const db = getDb(env.DB); + // Fetch every row for the repo (bounded: never more than MAX+1, since this runs after each insert) and slice + // the overflow off in JS, rather than a SQL OFFSET -- Drizzle's D1 dialect drops a `.limit(-1)` "unbounded + // limit" hint from the emitted SQL entirely, leaving a bare `OFFSET` clause that this driver rejects outright. const rows = await db .select({ id: reviewSuppression.id }) .from(reviewSuppression) .where(eq(reviewSuppression.repoFullName, repoFullName)) - .orderBy(desc(reviewSuppression.createdAt)) - .offset(MAX_REVIEW_SUPPRESSIONS_PER_REPO); - if (rows.length === 0) return; + .orderBy(desc(reviewSuppression.createdAt)); + const overflow = rows.slice(MAX_REVIEW_SUPPRESSIONS_PER_REPO); + if (overflow.length === 0) return; await db.delete(reviewSuppression).where( and( eq(reviewSuppression.repoFullName, repoFullName), inArray( reviewSuppression.id, - rows.map((row) => row.id), + overflow.map((row) => row.id), ), ), ); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 8747f9945f..22f444e576 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -17287,6 +17287,20 @@ describe("queue processors", () => { expect(postedBody).not.toContain("Readiness score is below the configured threshold"); }); + it("FLAG-ON, no stored signals: neither suppresses nor demotes -- the warning renders exactly as if review.memory were off (REGRESSION: the all-clear branch where the store read succeeds but finds nothing to apply)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", GITTENSORY_REVIEW_MEMORY: "true" }); + // Flag is fully ON (env + manifest) and the suppression-store read succeeds, but NO signal has ever been + // recorded for this repo -- applyReviewMemorySuppression's own empty-signals short-circuit returns + // suppressedCount: 0, demotedCount: 0, so processors.ts's "anything to apply?" check is false and + // renderedGate is never reassigned away from the original commentGate. + const postedBody = await runReadinessWarningPass(env, { + deliveryId: "review-memory-no-signals", + headSha: "revmem-no-signals", + reviewMemoryManifest: true, + }); + expect(postedBody).toContain("Readiness score is below the configured threshold"); + }); + it("FLAG-ON: DEMOTES (keeps, but does not suppress) a same-category readiness warning that does not exactly match any stored signal", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", GITTENSORY_REVIEW_MEMORY: "true" }); // A signal for the SAME category but a patternHash that can never match this PR's real finding -- exercises diff --git a/test/unit/review-memory-store.test.ts b/test/unit/review-memory-store.test.ts index 21fa740233..1d7727d911 100644 --- a/test/unit/review-memory-store.test.ts +++ b/test/unit/review-memory-store.test.ts @@ -12,6 +12,13 @@ describe("review-memory suppression store (#2178)", () => { .first<{ id: string; created_at: string; created_by: string | null }>(); } + async function rawCount(env: Env, repoFullName: string): Promise { + const row = await env.DB.prepare("select count(*) as n from review_suppression where repo_full_name = ?") + .bind(repoFullName) + .first<{ n: number }>(); + return row?.n ?? 0; + } + it("records a suppression signal and lists it back for the repo", async () => { const env = createTestEnv(); const record = await recordReviewSuppression(env, { @@ -84,6 +91,11 @@ describe("review-memory suppression store (#2178)", () => { for (let i = 0; i < MAX_REVIEW_SUPPRESSIONS_PER_REPO + 1; i += 1) { await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: `hash-${i}` }); } + // REGRESSION: assert the underlying table itself shrank back to the cap, via a raw count query -- + // listReviewSuppressions clamps its OWN `limit` param to MAX_REVIEW_SUPPRESSIONS_PER_REPO (see the test + // below), which would mask a completely broken eviction (e.g. a query that silently no-ops) by returning + // exactly MAX rows regardless of how many actually remain in the table. + expect(await rawCount(env, "owner/repo")).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO); const listed = await listReviewSuppressions(env, "owner/repo", MAX_REVIEW_SUPPRESSIONS_PER_REPO + 5); expect(listed.length).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO); // The very first inserted key ("hash-0") is the oldest and must have been evicted. @@ -92,6 +104,14 @@ describe("review-memory suppression store (#2178)", () => { expect(listed.some((row) => row.patternHash === `hash-${MAX_REVIEW_SUPPRESSIONS_PER_REPO}`)).toBe(true); }); + it("does NOT prune when a repo is at or under the cap (REGRESSION: pruneReviewSuppressionsOverCap's early-return branch)", async () => { + const env = createTestEnv(); + for (let i = 0; i < 3; i += 1) { + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: `hash-${i}` }); + } + expect(await rawCount(env, "owner/repo")).toBe(3); + }); + it("listReviewSuppressions clamps an out-of-range limit into [1, MAX_REVIEW_SUPPRESSIONS_PER_REPO]", async () => { const env = createTestEnv(); await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" }); From 6d514a6c2824211f7c2b69442ff62ed01d2fa6ff Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:42:28 -0700 Subject: [PATCH 3/3] test(review): cover the swallowed prune-query-failure branch --- test/unit/review-memory-store.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/unit/review-memory-store.test.ts b/test/unit/review-memory-store.test.ts index 1d7727d911..e9e47c210f 100644 --- a/test/unit/review-memory-store.test.ts +++ b/test/unit/review-memory-store.test.ts @@ -112,6 +112,22 @@ describe("review-memory suppression store (#2178)", () => { expect(await rawCount(env, "owner/repo")).toBe(3); }); + it("REGRESSION: a prune-query failure is swallowed -- recordReviewSuppression still returns the newly recorded row instead of throwing", async () => { + const env = createTestEnv(); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + // Only the prune-cap query selects a bare `id` ordered by created_at -- the read-back select() in + // recordReviewSuppression itself selects the full row with no ORDER BY, so this pattern isolates the + // cap-eviction query without breaking the insert/read-back this same call also performs. + if (/select\s+"id"\s+from\s+"review_suppression".*order by.*created_at.*desc/i.test(sql)) { + throw new Error("d1 down"); + } + return realPrepare(sql); + }) as typeof env.DB.prepare; + const record = await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" }); + expect(record).toMatchObject({ repoFullName: "owner/repo", patternHash: "hash-1" }); + }); + it("listReviewSuppressions clamps an out-of-range limit into [1, MAX_REVIEW_SUPPRESSIONS_PER_REPO]", async () => { const env = createTestEnv(); await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" });