fix(review): keep screenshots allowlist-bound

src/review/feature-activation.ts

@@ -15,8 +15,8 @@
 // `resolveConvergedFeature` and `resolveManifestOnlyFeature` are the two thin adapters over it in actual use:
 //   - `resolveConvergedFeature` — the `features:`-block keys (rag/reputation/unifiedComment/safety/grounding/
 //     e2eTests/screenshots): env kill-switch → per-repo `features:` override → `GITTENSORY_REVIEW_REPOS`
-//     allowlist default. Safety and grounding are the two NAMED exceptions this shape has always had (#2269);
-//     see `FEATURE_MODE` below.
+//     allowlist default. Safety, grounding, and screenshots are the named exceptions this shape has; see
+//     `FEATURE_MODE` below.
 //   - `resolveManifestOnlyFeature` — the `review:`-block keys with NO allowlist role at all (impactMap /
 //     reviewMemory / cultureProfile / inlineComments / fixHandoff): env kill-switch → an EXPLICIT per-repo
 //     `review.*` opt-in is the only way to activate. These live under a different `.gittensory.yml` namespace
@@ -44,14 +44,14 @@ import { loadRepoFocusManifest } from "../signals/focus-manifest-loader";
  * The four per-feature activation precedence shapes actually in use across gittensory's advisory review
  * capabilities (#4616):
  *  - `"standard"`: `override` fully controls (`true` forces on, `false` forces off); `null` (unset) falls back
- *    to `allowlisted`. rag / reputation / unifiedComment / e2eTests / screenshots.
+ *    to `allowlisted`. rag / reputation / unifiedComment / e2eTests / improvementSignal.
  *  - `"forceOnOnly"`: `override` can only force ON (bypassing the allowlist); an untrusted `false` is "no
  *    opinion" and falls through to `allowlisted` — for a feature where a lower-trust, repo-controlled override
  *    must never be able to silently defeat the operator's own enablement. safety (#2269).
  *  - `"allowlistRequired"`: `allowlisted` is a hard requirement regardless of `override`; an override may
  *    ADDITIONALLY force OFF within an allowlisted repo, never force ON outside it. grounding (fetches full
- *    post-change file contents for the AI prompt, so a repo override alone must never bypass the operator's
- *    own allowlist).
+ *    post-change file contents for the AI prompt) and screenshots (launches browser rendering and stores
+ *    publicly embedded images), so a repo override alone must never bypass the operator's own allowlist.
  *  - `"manifestOnly"`: there is no allowlist role at all (`allowlisted` is never consulted); an explicit
  *    `override === true` is the ONLY way to activate. impactMap / reviewMemory / cultureProfile /
  *    inlineComments / fixHandoff — each shipped as an explicit-opt-in-only `.gittensory.yml` `review.*` toggle
@@ -90,19 +90,20 @@ const FEATURE_GLOBAL_FLAG: Record<ConvergedFeatureKey, (env: Env) => boolean> =
 
 /** The named per-feature exceptions to `resolveConvergedFeature`'s default `"standard"` precedence — every
  *  `ConvergedFeatureKey` not listed here uses `"standard"`. See {@link FeatureActivationMode}'s doc comment for
- *  why each of these two needs its own asymmetric shape. */
+ *  why each of these features needs its own asymmetric shape. */
 const FEATURE_MODE: Partial<Record<ConvergedFeatureKey, FeatureActivationMode>> = {
   safety: "forceOnOnly",
   grounding: "allowlistRequired",
+  screenshots: "allowlistRequired",
 };
 
 /**
  * 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 `features:`-block precedence lives
  * (delegating the actual arithmetic to {@link resolveFeatureActivation}). 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). `grounding` is also asymmetric in the opposite
- * direction: a repo override can only force it OFF, never bypass the operator allowlist.
+ * an override can only force it ON, never force it OFF (#2269). `grounding` and `screenshots` are asymmetric in
+ * the opposite direction: a repo override can only force them OFF, never bypass the operator allowlist.
  */
 export function resolveConvergedFeature(
   env: Env,

test/unit/feature-activation.test.ts

@@ -188,6 +188,20 @@ describe("resolveConvergedFeature — grounding remains allowlist-bound", () =>
   });
 });
 
+describe("resolveConvergedFeature — screenshots remain allowlist-bound", () => {
+  it("does not let a repo manifest force screenshots ON outside the operator allowlist", () => {
+    const e = env({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "other/repo" });
+    expect(resolveConvergedFeature(e, manifestWith({ screenshots: true }), "screenshots", REPO)).toBe(false);
+  });
+
+  it("allows an allowlisted repo to enable screenshots by default and force them OFF per repo", () => {
+    const e = env({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: REPO });
+    expect(resolveConvergedFeature(e, manifestWith({}), "screenshots", REPO)).toBe(true);
+    expect(resolveConvergedFeature(e, manifestWith({ screenshots: true }), "screenshots", REPO)).toBe(true);
+    expect(resolveConvergedFeature(e, manifestWith({ screenshots: false }), "screenshots", REPO)).toBe(false);
+  });
+});
+
 describe("resolveConvergedFeature — improvementSignal is a plain symmetric override (#4738)", () => {
   // The full resolution matrix the #4738 acceptance criteria calls out explicitly: env off; env on + no
   // override; env on + repo true; env on + repo false. improvementSignal has no safety/grounding-style

test/unit/visual-wire.test.ts

@@ -20,14 +20,10 @@ describe("isScreenshotsEnabled", () => {
   });
 });
 
-// #4616: screenshots is now a `ConvergedFeatureKey` — its per-repo activation decision (global flag AND (a
-// `features.screenshots` override OR the GITTENSORY_REVIEW_REPOS allowlist default)) is resolved through the
-// SAME shared `resolveConvergedFeature` every other converged feature uses (see feature-activation.test.ts for
-// the exhaustive, feature-key-agnostic precedence suite this inherits automatically). The dedicated
-// `screenshotsAllowed` helper this file used to test (env flag AND allowlist ONLY, no `features:` override at
-// all) was removed as part of that migration; these are its former assertions, ported onto the new call shape
-// and extended with the override case `screenshotsAllowed` never had.
-describe("screenshots converged-feature activation (env flag AND (features.screenshots override OR the repo cutover allowlist), #4616)", () => {
+// #4616: screenshots is a `ConvergedFeatureKey`, but browser rendering remains allowlist-bound:
+// `features.screenshots` may opt an allowlisted repo out, but it must not let an unallowlisted repo bypass the
+// operator-controlled GITTENSORY_REVIEW_REPOS rollout boundary.
+describe("screenshots converged-feature activation (env flag AND repo cutover allowlist, with manifest opt-out)", () => {
   const repo = "JSONbored/gittensory";
   const noOverride: Pick<FocusManifest, "features"> = {
     features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null },
@@ -52,9 +48,15 @@ describe("screenshots converged-feature activation (env flag AND (features.scree
     expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "on", GITTENSORY_REVIEW_REPOS: "jsonbored/GITTENSORY" } as Env, noOverride, "screenshots", repo)).toBe(true);
   });
 
-  it("(#4616) a `features.screenshots` override now fully controls the feature, even for a repo NOT on the allowlist — the gap this migration fixes", () => {
+  it("does not let a `features.screenshots` override bypass the repo allowlist", () => {
     const forcedOn: Pick<FocusManifest, "features"> = { features: { ...noOverride.features, present: true, screenshots: true } };
-    expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true" } as Env, forcedOn, "screenshots", "not/allowlisted")).toBe(true);
+    expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true" } as Env, forcedOn, "screenshots", "not/allowlisted")).toBe(false);
+    expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "JSONbored/other" } as Env, forcedOn, "screenshots", repo)).toBe(false);
+  });
+
+  it("allows an allowlisted repo to enable screenshots by default and force it OFF per repo", () => {
+    const forcedOn: Pick<FocusManifest, "features"> = { features: { ...noOverride.features, present: true, screenshots: true } };
+    expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: repo } as Env, forcedOn, "screenshots", repo)).toBe(true);
     const forcedOff: Pick<FocusManifest, "features"> = { features: { ...noOverride.features, present: true, screenshots: false } };
     expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: repo } as Env, forcedOff, "screenshots", repo)).toBe(false);
   });