From 870fe09d59e279e012b26815a2a675eec35615a1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 19:41:25 -0700 Subject: [PATCH] fix(review): make review.memory a full config-as-code substitute (#4101) shouldApplyReviewMemory previously required GITTENSORY_REVIEW_MEMORY AND the per-repo manifest toggle (a repo could only narrow, never replace, the env var). Match the inlineComments/fixHandoff shape (#4099): the env flag is now an absolute kill-switch, and an explicit review.memory true/false fully controls the feature by itself. review.memory has never had a GITTENSORY_REVIEW_REPOS cutover allowlist, so an unset manifest toggle stays byte-identical to the original always-off default. --- src/queue/processors.ts | 5 +++-- src/review/review-memory-wire.ts | 33 ++++++++++++++++++---------- test/unit/review-memory-wire.test.ts | 20 ++++++++++------- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 399d48a7e2..19501443c5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9033,8 +9033,9 @@ async function maybePublishPrPublicSurface( inlineCommentsPerCategoryForReview = deterministicReviewOverrides.inlineCommentsPerCategory; // 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. + // pass). The operator's GITTENSORY_REVIEW_MEMORY kill-switch at the actual apply site below + // (shouldApplyReviewMemory) is absolute; an explicit `review.memory` manifest toggle then fully decides + // (#4101, same precedence as shouldEmitFixHandoff) — this flag alone only carries the per-repo manifest opt-in. reviewMemoryEnabledForReview = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifestForAutoReview)); // review.fixHandoff emission (#1962): resolved the same unconditional way as the deterministic sections above, // ANDing the per-repo `review.fixHandoff` manifest opt-in with the operator's GITTENSORY_REVIEW_FIX_HANDOFF diff --git a/src/review/review-memory-wire.ts b/src/review/review-memory-wire.ts index ea64b67f5a..0b4c0f4fa4 100644 --- a/src/review/review-memory-wire.ts +++ b/src/review/review-memory-wire.ts @@ -1,10 +1,11 @@ -// 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. +// Review-memory activation wiring (#2179, config slice of #1964). Default OFF: the operator flag +// GITTENSORY_REVIEW_MEMORY is a master kill-switch, and the per-repo `.gittensory.yml` review.memory toggle +// (#4101, same shape as inlineComments/fixHandoff, #4099) fully controls activation by itself when explicitly +// set — there has never been a GITTENSORY_REVIEW_REPOS cutover allowlist for this feature (an unset manifest +// toggle preserves the ORIGINAL always-off default; it was never sufficient to be allowlisted alone, and in +// fact no allowlist fallback ever existed for review memory in the first place). 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"; @@ -17,14 +18,22 @@ export function isReviewMemoryEnabled(env: { GITTENSORY_REVIEW_MEMORY?: string | 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). */ +/** PURE (#4101): should review-memory suppression apply for THIS repo/PR? (1) The operator's + * GITTENSORY_REVIEW_MEMORY flag is an absolute MASTER KILL-SWITCH — off ⇒ always false, regardless of the + * manifest, and no per-repo config can bypass it (consistent with every other converged feature — see + * `resolveConvergedFeature` in `feature-activation.ts`). (2) An explicit per-repo `.gittensory.yml` + * `review.memory` override (`true`/`false`) now FULLY controls the feature by itself — a repo can turn this on + * without needing any allowlist at all. (3) `manifestToggle` unset (`undefined`) preserves this feature's + * ORIGINAL design exactly: review memory has never had a GITTENSORY_REVIEW_REPOS cutover allowlist to fall + * back to (unlike rag/reputation/safety/unifiedComment/grounding), so this stays `false` regardless, byte- + * identical to every repo's behavior before this change. Exactly mirrors `shouldEmitFixHandoff`'s shape and + * precedence. */ export function shouldApplyReviewMemory( env: { GITTENSORY_REVIEW_MEMORY?: string | undefined }, - manifestReviewMemoryEnabled: boolean, + manifestToggle: boolean | undefined, ): boolean { - return isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled; + if (!isReviewMemoryEnabled(env)) return false; + return manifestToggle === true; } const RESOLVE_FINDING_CODE = /^[a-z][a-z0-9_]{0,199}$/; export function normalizeResolveFindingRef(raw: string | null | undefined): { ok: true; scope: "whole_pr" } | { ok: true; scope: "single"; findingCode: string } | { ok: false; reason: "malformed_finding_id" } { const trimmed = (raw ?? "").trim(); if (trimmed.length === 0) return { ok: true, scope: "whole_pr" }; const normalized = trimmed.toLowerCase().replace(/^finding-/, ""); if (!RESOLVE_FINDING_CODE.test(normalized)) return { ok: false, reason: "malformed_finding_id" }; return { ok: true, scope: "single", findingCode: normalized }; } diff --git a/test/unit/review-memory-wire.test.ts b/test/unit/review-memory-wire.test.ts index 9bfe45eed0..72fe53afef 100644 --- a/test/unit/review-memory-wire.test.ts +++ b/test/unit/review-memory-wire.test.ts @@ -14,21 +14,25 @@ describe("isReviewMemoryEnabled", () => { }); }); -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); +describe("shouldApplyReviewMemory (#4101)", () => { + it("operator flag is a master kill-switch — off ⇒ always false regardless of the manifest toggle", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "false" }, true)).toBe(false); }); - 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("REGRESSION (#4101): unset manifest toggle stays false — byte-identical to the ORIGINAL required-AND behavior (review memory has never had a GITTENSORY_REVIEW_REPOS cutover allowlist to fall back to)", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "true" }, undefined)).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("(#4101) an explicit manifest toggle: true fully controls the feature", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "true" }, true)).toBe(true); + }); + + it("(#4101) an explicit manifest toggle: false forces the feature off, even with the operator flag on", () => { + expect(shouldApplyReviewMemory({ GITTENSORY_REVIEW_MEMORY: "true" }, false)).toBe(false); }); it("is OFF when both are off", () => { - expect(shouldApplyReviewMemory({}, false)).toBe(false); + expect(shouldApplyReviewMemory({}, undefined)).toBe(false); }); });