diff --git a/src/env.d.ts b/src/env.d.ts index 9d8be57d7f..08ad29ece0 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -261,10 +261,11 @@ declare global { * unreachable when off). Even when ON, retrieval is INERT until the self-host vector index is populated for * the repo (a cold/missing index degrades to no context). */ GITTENSORY_REVIEW_RAG?: string; - /** Deterministic impact map (#2184, part of #1971): operator-level kill-switch, ANDed with the per-repo - * `.gittensory.yml review.impact_map` opt-in (see review/impact-map-wire's isImpactMapEnabled / - * shouldComputeImpactMap). Default OFF — unset/false performs NO symbol extraction, NO RAG query, and adds - * NO comment/prompt section, byte-identical to today. */ + /** Deterministic impact map (#2184, part of #1971): operator-level master kill-switch; the per-repo + * `.gittensory.yml review.impact_map` opt-in fully decides once this is on (#4102 precedence — see + * review/impact-map-wire's isImpactMapEnabled / shouldComputeImpactMap). Default OFF — unset/false + * performs NO symbol extraction, NO RAG query, and adds NO comment/prompt section, byte-identical to + * today. */ GITTENSORY_REVIEW_IMPACT_MAP?: string; /** Repo quality-culture profile (#2995): when truthy, the AI reviewer prompt gains an ADDITIVE "REPO * QUALITY-CULTURE PROFILE" reference block — typical merged-PR size + common accepted labels, derived diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 399d48a7e2..5eb085849b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6934,10 +6934,11 @@ export async function runAiReviewForAdvisory( // manifest. Self-host only — overrides that repo's claude-code/codex model+effort, taking priority over the // operator's global env vars. Absent/all-null ⇒ byte-identical (global env var, then provider default). reviewSelfHostAiModel?: SelfHostAiModelConfig | undefined; - // `.gittensory.yml` review.impact_map (#2184/#2186), resolved by the caller from the cached manifest. ANDed - // here with the operator's GITTENSORY_REVIEW_IMPACT_MAP flag (shouldComputeImpactMap) to decide whether to - // compute the deterministic impact map and splice it into the reviewer prompt as additive reference - // context. Absent/false ⇒ byte-identical reviewer prompt (no impact-map computation, no RAG query for it). + // `.gittensory.yml` review.impact_map (#2184/#2186), resolved by the caller from the cached manifest. + // Precedence (#4102): the operator's GITTENSORY_REVIEW_IMPACT_MAP flag is a master kill-switch, never + // bypassable by config (shouldComputeImpactMap); an explicit true/false here then fully controls whether to + // compute the deterministic impact map and splice it into the reviewer prompt. Absent ⇒ byte-identical + // reviewer prompt (no impact-map computation, no RAG query for it). reviewImpactMap?: boolean | undefined; // `.gittensory.yml` review.culture_profile (#2995), resolved by the caller from the cached manifest. ANDed // here with the GITTENSORY_REVIEW_CULTURE_PROFILE global flag to decide whether to append the repo's @@ -7136,16 +7137,17 @@ export async function runAiReviewForAdvisory( : undefined; const ragTelemetry = ragContextResult?.telemetry ?? emptyReviewRagTelemetry(false); - // Deterministic impact map (#2184/#2186), ANDed operator env flag + per-repo review.impact_map opt-in - // (shouldComputeImpactMap). Reuses the SAME changed files this pass already resolved — no extra fetch. - // Flag-OFF (default) → NO new branch: no symbol extraction, no RAG query, and `impactMapContext` is left - // undefined so the prompt is byte-identical to today. Fully fail-safe (computeImpactMap never throws; a - // missing/cold RAG index degrades to an empty impact map, which formats to "" and appends nothing). + // Deterministic impact map (#2184/#2186): operator env kill-switch + per-repo review.impact_map opt-in + // (shouldComputeImpactMap, #4102 precedence). Reuses the SAME changed files this pass already resolved — no + // extra fetch. Flag-OFF (default) → NO new branch: no symbol extraction, no RAG query, and + // `impactMapContext` is left undefined so the prompt is byte-identical to today. Fully fail-safe + // (computeImpactMap never throws; a missing/cold RAG index degrades to an empty impact map, which formats + // to "" and appends nothing). let impactMapContext: string | undefined; // The computed entries are ALSO threaded out of this function (#1971) so the publish site can render the // "Impact map" collapsible from the exact same array — no second RAG query. Empty when the feature is off. let impactMapEntries: ImpactMapEntry[] = []; - if (shouldComputeImpactMap(env, args.reviewImpactMap === true)) { + if (shouldComputeImpactMap(env, args.reviewImpactMap)) { const [impactMapProject, impactMapRepo] = splitRepoForRag(args.repoFullName); const changedSymbols = extractChangedSymbols( files.map((file) => ({ @@ -9299,7 +9301,7 @@ async function maybePublishPrPublicSurface( // Impact map (#2182-#2186): queries the SAME live vector index RAG does (computeImpactMap issues // its own retrieveContextWithMetrics calls), so it can go stale for the SAME head SHA exactly like // RAG — a repo with it active also bypasses the AI-review result cache. - impactMap: shouldComputeImpactMap(env, reviewImpactMap === true), + impactMap: shouldComputeImpactMap(env, reviewImpactMap), }; const dynamicReviewContextActive = dynamicReviewFeatures.grounding || diff --git a/src/review/impact-map-wire.ts b/src/review/impact-map-wire.ts index f52d2756d0..7c7a43c932 100644 --- a/src/review/impact-map-wire.ts +++ b/src/review/impact-map-wire.ts @@ -1,9 +1,11 @@ -// Impact-map activation wiring (#2184, config slice of #1971). Mirrors rag-wire.ts's isRagEnabled: a single -// GLOBAL env kill-switch the self-host operator controls, ANDed with the per-repo `.gittensory.yml -// review.impact_map` manifest toggle (resolved via `resolveReviewPromptOverrides`'s `impactMap` field) — 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, impact-map computation is never invoked from the review path at all (the caller -// guards on this flag before doing any RAG query or rendering), so the review stays byte-identical to today. +// Impact-map activation wiring (#2184, config slice of #1971; activation precedence fixed for #4102). Default +// OFF: the operator flag GITTENSORY_REVIEW_IMPACT_MAP is a master kill-switch, and the per-repo `.gittensory.yml +// review.impactMap` toggle (resolved via `resolveReviewPromptOverrides`'s `impactMap` field) fully controls +// activation by itself when explicitly set — impact-map has never had a `GITTENSORY_REVIEW_REPOS` cutover +// allowlist fallback (unlike rag/reputation/grounding/safety/unifiedComment), so there is nothing for a repo +// toggle to bypass. With the env flag unset, impact-map computation is never invoked from the review path at +// all (the caller guards on this flag before doing any RAG query or rendering), so the review stays +// byte-identical to today. // // Also hosts the AI-review grounding formatter (#2186): `formatImpactMapPromptSection` turns // `computeImpactMap`'s output into the bounded "IMPACT MAP" block spliced into the reviewer's user prompt via @@ -20,14 +22,23 @@ export function isImpactMapEnabled(env: { GITTENSORY_REVIEW_IMPACT_MAP?: string return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_IMPACT_MAP ?? ""); } -/** Resolve whether impact-map computation should run 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 (#4102): should impact-map computation run for THIS repo/PR? (1) The operator's + * GITTENSORY_REVIEW_IMPACT_MAP 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.impactMap` override (`true`/`false`) FULLY controls the feature by itself once the kill-switch is + * on — impact-map has never had a `GITTENSORY_REVIEW_REPOS` cutover-allowlist fallback, so there is no + * allowlist for a repo toggle to bypass. (3) `manifestToggle` unset (`undefined`) preserves this feature's + * ORIGINAL always-off-unless-both-set default exactly: with no allowlist to fall back to, an unset manifest + * toggle stays `false`, byte-identical to every repo's behavior before this change. Exactly mirrors + * `shouldRequestInlineFindings`/`shouldEmitFixHandoff`'s shape and precedence (src/review/inline-comments.ts, + * src/review/fix-handoff.ts). */ export function shouldComputeImpactMap( env: { GITTENSORY_REVIEW_IMPACT_MAP?: string | undefined }, - manifestImpactMapEnabled: boolean, + manifestToggle: boolean | undefined, ): boolean { - return isImpactMapEnabled(env) && manifestImpactMapEnabled; + if (!isImpactMapEnabled(env)) return false; + return manifestToggle === true; } /** Hard cap on entries actually formatted into the AI-review prompt section — bounds prompt-token cost diff --git a/test/unit/impact-map-wire.test.ts b/test/unit/impact-map-wire.test.ts index b90207933d..f95c36a0cd 100644 --- a/test/unit/impact-map-wire.test.ts +++ b/test/unit/impact-map-wire.test.ts @@ -13,22 +13,34 @@ describe("isImpactMapEnabled", () => { }); }); -describe("shouldComputeImpactMap", () => { - it("requires BOTH the operator env flag AND the per-repo manifest opt-in", () => { +describe("shouldComputeImpactMap (#4102 precedence: env kill-switch, then manifestToggle === true fully decides)", () => { + it("is ON when the operator flag is on and the manifest explicitly opted in", () => { expect(shouldComputeImpactMap({ GITTENSORY_REVIEW_IMPACT_MAP: "true" }, true)).toBe(true); }); - it("is OFF when the operator flag is on but the manifest didn't opt in", () => { + it("is OFF when the operator flag is on but the manifest explicitly opted out", () => { expect(shouldComputeImpactMap({ GITTENSORY_REVIEW_IMPACT_MAP: "true" }, false)).toBe(false); }); - it("is OFF when the manifest opted in but the operator flag is off (repo cannot self-enable)", () => { + it("is OFF when the operator flag is on but the manifest toggle is unset (regression: preserves the ORIGINAL always-off default — there is no allowlist fallback for impactMap)", () => { + expect(shouldComputeImpactMap({ GITTENSORY_REVIEW_IMPACT_MAP: "true" }, undefined)).toBe(false); + }); + + it("is OFF when the manifest opted in but the operator flag is off — the env flag is an absolute master kill-switch no per-repo config can bypass", () => { expect(shouldComputeImpactMap({ GITTENSORY_REVIEW_IMPACT_MAP: "false" }, true)).toBe(false); }); + it("is OFF when the operator flag is unset and the manifest opted in — the kill-switch still wins", () => { + expect(shouldComputeImpactMap({}, true)).toBe(false); + }); + it("is OFF when both are off", () => { expect(shouldComputeImpactMap({}, false)).toBe(false); }); + + it("is OFF when both are unset", () => { + expect(shouldComputeImpactMap({}, undefined)).toBe(false); + }); }); describe("formatImpactMapPromptSection (#2186)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 72928a2903..913c1852a0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6247,10 +6247,10 @@ describe("queue processors", () => { AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", - // Both gates on: the global capability switch, and (like culture-profile above, unlike + // Both gates on: the global kill-switch, and (like culture-profile above, unlike // grounding/enrichment/RAG/reputation which are env-only) the per-repo `.gittensory.yml` opt-in mocked // below, so `dynamicReviewFeatures.impactMap` (src/queue/processors.ts) actually evaluates - // shouldComputeImpactMap's `&&` right-hand side true, not just short-circuits. + // shouldComputeImpactMap's `manifestToggle === true` arm true, not just the kill-switch short-circuit. GITTENSORY_REVIEW_IMPACT_MAP: "true", }); await persistRegistrySnapshot(