diff --git a/src/review/feature-activation.ts b/src/review/feature-activation.ts index 34bec05c52..43d6f559ed 100644 --- a/src/review/feature-activation.ts +++ b/src/review/feature-activation.ts @@ -6,11 +6,13 @@ // (`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. EXCEPTION: +// 2. Per-repo `features:` override — `true`/`false` forces the feature on/off for this repo. EXCEPTIONS: // `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). +// `grounding` can fetch full post-change file contents for the AI prompt, so the operator allowlist remains +// mandatory; a repo override may only disable grounding inside that allowlist. // 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. // @@ -38,7 +40,8 @@ const FEATURE_GLOBAL_FLAG: Record 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. + * `safety` is asymmetric: an override can only force it ON, never force it OFF (#2269). `grounding` is also + * asymmetric in the opposite direction: a repo override can only force it OFF, never bypass the operator allowlist. */ export function resolveConvergedFeature( env: Env, @@ -50,9 +53,11 @@ export function resolveConvergedFeature( 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); + const allowlisted = isConvergenceRepoAllowed(env, repoFullName); + if (feature === "safety") return override === true || allowlisted; + if (feature === "grounding") return allowlisted && override !== false; if (override !== null) return override; // explicit per-repo on/off - return isConvergenceRepoAllowed(env, repoFullName); // back-compat allowlist default + return allowlisted; // back-compat allowlist default } /** diff --git a/test/unit/feature-activation.test.ts b/test/unit/feature-activation.test.ts index 65acca57d4..ff1755c064 100644 --- a/test/unit/feature-activation.test.ts +++ b/test/unit/feature-activation.test.ts @@ -80,6 +80,20 @@ describe("resolveConvergedFeature — safety is force-on-only, never force-off ( }); }); +describe("resolveConvergedFeature — grounding remains allowlist-bound", () => { + it("does not let a repo manifest force grounding ON outside the operator allowlist", () => { + const e = env({ GITTENSORY_REVIEW_GROUNDING: "true", GITTENSORY_REVIEW_REPOS: "other/repo" }); + expect(resolveConvergedFeature(e, manifestWith({ grounding: true }), "grounding", REPO)).toBe(false); + }); + + it("allows an allowlisted repo to enable grounding by default and force it OFF per repo", () => { + const e = env({ GITTENSORY_REVIEW_GROUNDING: "true", GITTENSORY_REVIEW_REPOS: REPO }); + expect(resolveConvergedFeature(e, manifestWith({}), "grounding", REPO)).toBe(true); + expect(resolveConvergedFeature(e, manifestWith({ grounding: true }), "grounding", REPO)).toBe(true); + expect(resolveConvergedFeature(e, manifestWith({ grounding: false }), "grounding", 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.