fix(review): make the safety feature override force-on-only

src/review/feature-activation.ts

@@ -6,7 +6,11 @@
 // (`features:` block). The precedence, highest to lowest:
 //   1. GLOBAL env flag (GITTENSORY_REVIEW_*) — a MASTER KILL-SWITCH. Off ⇒ the feature never runs anywhere,
 //      regardless of any per-repo override (so an operator keeps one deploy-wide off switch per feature).
-//   2. Per-repo `features:` override — `true`/`false` forces the feature on/off for this repo.
+//   2. Per-repo `features:` override — `true`/`false` forces the feature on/off for this repo. EXCEPTION:
+//      `safety` (prompt-injection defanging) is security-critical and `.gittensory.yml` lives in the repo
+//      itself, writable by a lower-trust actor than the operator — so a repo override may only TIGHTEN
+//      (force-on) the operator's global enablement, never loosen it. `features.safety: false` is treated as
+//      "no opinion" (falls through to the allowlist default below) rather than an active force-off (#2269).
 //   3. `GITTENSORY_REVIEW_REPOS` allowlist — the back-compat DEFAULT when the manifest says nothing, so a repo
 //      that sets no `features:` block behaves exactly as it did before this change.
 //
@@ -32,6 +36,7 @@ const FEATURE_GLOBAL_FLAG: Record<ConvergedFeatureKey, (env: Env) => boolean> =
  * Resolve whether a converged feature is active for a repo, given the already-loaded manifest (or null). Pure +
  * synchronous so it carries no I/O and is the single unit-tested place the precedence lives. Precedence: env
  * kill-switch (off ⇒ false) → per-repo `features:` override → `GITTENSORY_REVIEW_REPOS` allowlist default.
+ * `safety` is asymmetric: an override can only force it ON, never force it OFF (#2269) — see the file header.
  */
 export function resolveConvergedFeature(
   env: Env,
@@ -41,6 +46,9 @@ export function resolveConvergedFeature(
 ): boolean {
   if (!FEATURE_GLOBAL_FLAG[feature](env)) return false; // master kill-switch
   const override = manifest?.features?.[feature] ?? null;
+  // Security-critical: a repo-controlled override must not silently defeat the operator's global enablement.
+  // `false` is downgraded to "no opinion" so it falls through to the allowlist default instead of forcing off.
+  if (feature === "safety") return override === true || isConvergenceRepoAllowed(env, repoFullName);
   if (override !== null) return override; // explicit per-repo on/off
   return isConvergenceRepoAllowed(env, repoFullName); // back-compat allowlist default
 }

test/unit/feature-activation.test.ts

@@ -56,6 +56,29 @@ describe("resolveConvergedFeature — env kill-switch → per-repo override →
   });
 });
 
+describe("resolveConvergedFeature — safety is force-on-only, never force-off (#2269)", () => {
+  it("ignores a repo override that tries to force safety OFF, falling through to the allowlist default", () => {
+    // Operator enabled safety globally AND allowlisted this repo — a repo-controlled override must not defeat it.
+    const allowlisted = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: REPO });
+    expect(resolveConvergedFeature(allowlisted, manifestWith({ safety: false }), "safety", REPO)).toBe(true);
+
+    // Not allowlisted: the override is still ignored (treated as "no opinion"), so the allowlist default (off) applies.
+    // This is off for the same reason a bare `manifestWith({})` would be off here — not because the override "worked".
+    const notAllowlisted = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: "other/repo" });
+    expect(resolveConvergedFeature(notAllowlisted, manifestWith({ safety: false }), "safety", REPO)).toBe(false);
+  });
+
+  it("still honors a repo override that forces safety ON, even when the repo is not allowlisted", () => {
+    const e = env({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: "other/repo" });
+    expect(resolveConvergedFeature(e, manifestWith({ safety: true }), "safety", REPO)).toBe(true);
+  });
+
+  it("still respects the master kill-switch — a true override cannot turn safety on when the global flag is off", () => {
+    const e = env({ GITTENSORY_REVIEW_REPOS: REPO }); // GITTENSORY_REVIEW_SAFETY unset
+    expect(resolveConvergedFeature(e, manifestWith({ safety: true }), "safety", REPO)).toBe(false);
+  });
+});
+
 describe("convergedFeatureActive — async (loads the cached manifest)", () => {
   it("short-circuits to false WITHOUT loading the manifest when the env flag is off", async () => {
     // DB-less env: if it tried to load the manifest it would throw; returning false proves the short-circuit.
@@ -73,4 +96,10 @@ describe("convergedFeatureActive — async (loads the cached manifest)", () => {
     const e = createTestEnv({ GITTENSORY_REVIEW_RAG: "true", GITTENSORY_REVIEW_REPOS: REPO });
     expect(await convergedFeatureActive(e, REPO, "rag")).toBe(true);
   });
+
+  it("applies the safety force-on-only exception through the async DB-backed path too (#2269)", async () => {
+    const e = createTestEnv({ GITTENSORY_REVIEW_SAFETY: "true", GITTENSORY_REVIEW_REPOS: REPO });
+    await upsertRepoFocusManifest(e, REPO, { features: { safety: false } });
+    expect(await convergedFeatureActive(e, REPO, "safety")).toBe(true); // override ignored, allowlist wins
+  });
 });