diff --git a/src/review/feature-activation.ts b/src/review/feature-activation.ts index 292f3b9a1..61d4baade 100644 --- a/src/review/feature-activation.ts +++ b/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,10 +90,11 @@ const FEATURE_GLOBAL_FLAG: Record 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> = { safety: "forceOnOnly", grounding: "allowlistRequired", + screenshots: "allowlistRequired", }; /** @@ -101,8 +102,8 @@ const FEATURE_MODE: Partial> * 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, diff --git a/test/unit/feature-activation.test.ts b/test/unit/feature-activation.test.ts index 4734fdac4..22b325397 100644 --- a/test/unit/feature-activation.test.ts +++ b/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 diff --git a/test/unit/visual-wire.test.ts b/test/unit/visual-wire.test.ts index a25a1a67f..ae2f684cd 100644 --- a/test/unit/visual-wire.test.ts +++ b/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 = { 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 = { 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 = { 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 = { features: { ...noOverride.features, present: true, screenshots: false } }; expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: repo } as Env, forcedOff, "screenshots", repo)).toBe(false); });