Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 21 additions & 12 deletions src/review/review-memory-wire.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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 }; }
Expand Down
20 changes: 12 additions & 8 deletions test/unit/review-memory-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down