diff --git a/.gittensory.yml.example b/.gittensory.yml.example index d6654849ac..c1e54b9192 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -1106,17 +1106,20 @@ settings: # auto_pause_after_reviewed_commits: 3 # Per-repo activation overrides for the converged review features that ship behind a deployment-wide -# GITTENSORY_REVIEW_* env kill-switch (rag/reputation/unifiedComment/safety/grounding/e2eTests/improvementSignal). -# Each key is `true` (force on for this repo, subject to the env flag still being enabled), `false` (force off), -# or omitted (falls back to the GITTENSORY_REVIEW_REPOS allowlist default -- an operator who sets nothing keeps -# today's behavior). Exception: `safety` is force-on-only -- an untrusted repo-controlled `false` is treated as -# "no opinion" rather than an active force-off (#2269), since a lower-trust actor must never be able to silently -# defeat the operator's own security-hardening enablement. `e2eTests` (#4190) and `improvementSignal` (#4738, -# foundation phase of the #4737 PR-improvement-signal epic) are plain symmetric overrides like -# rag/reputation/unifiedComment -- neither AI-generated test content nor a read-only advisory quality-delta -# signal carries a security-hardening rationale to protect from a repo-controlled override, so there's no -# force-on/force-off asymmetry for either. `improvementSignal` is activation wiring only for now -- no tier -# reads the resolved value yet. +# GITTENSORY_REVIEW_* env kill-switch (rag/reputation/unifiedComment/safety/grounding/e2eTests/screenshots/ +# improvementSignal). Each key is `true` (force on for this repo, subject to the env flag still being +# enabled), `false` (force off), or omitted (falls back to the GITTENSORY_REVIEW_REPOS allowlist default -- +# an operator who sets nothing keeps today's behavior). Exception: `safety` is force-on-only -- an untrusted +# repo-controlled `false` is treated as "no opinion" rather than an active force-off (#2269), since a +# lower-trust actor must never be able to silently defeat the operator's own security-hardening enablement. +# `e2eTests` (#4190), `screenshots` (#4616), and `improvementSignal` (#4738, foundation phase of the #4737 +# PR-improvement-signal epic) are plain symmetric overrides like rag/reputation/unifiedComment -- none carries +# a security-hardening or full-file-fetch rationale (unlike safety/grounding) that would justify a force-on/ +# force-off asymmetry. `screenshots` sits UNDER the separate, richer `review.visual.*` block further below +# (route/preview-URL config, and `review.visual.enabled: false` as an always-available additional force-off); +# this key only answers "does capture run for this repo at all," the same question its siblings answer for +# their own feature. `improvementSignal` is activation wiring only for now -- no tier reads the resolved +# value yet. # features: # rag: true # reputation: false @@ -1124,6 +1127,7 @@ settings: # safety: true # grounding: true # e2eTests: false +# screenshots: false # improvementSignal: false # Registry-review lane (#2435): lets a self-hosted maintainer point gittensory at their OWN structured diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index 48f3a74d42..06fadd51d2 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -1119,17 +1119,20 @@ settings: # auto_pause_after_reviewed_commits: 3 # Per-repo activation overrides for the converged review features that ship behind a deployment-wide -# GITTENSORY_REVIEW_* env kill-switch (rag/reputation/unifiedComment/safety/grounding/e2eTests/improvementSignal). -# Each key is `true` (force on for this repo, subject to the env flag still being enabled), `false` (force off), -# or omitted (falls back to the GITTENSORY_REVIEW_REPOS allowlist default -- an operator who sets nothing keeps -# today's behavior). Exception: `safety` is force-on-only -- an untrusted repo-controlled `false` is treated as -# "no opinion" rather than an active force-off (#2269), since a lower-trust actor must never be able to silently -# defeat the operator's own security-hardening enablement. `e2eTests` (#4190) and `improvementSignal` (#4738, -# foundation phase of the #4737 PR-improvement-signal epic) are plain symmetric overrides like -# rag/reputation/unifiedComment -- neither AI-generated test content nor a read-only advisory quality-delta -# signal carries a security-hardening rationale to protect from a repo-controlled override, so there's no -# force-on/force-off asymmetry for either. `improvementSignal` is activation wiring only for now -- no tier -# reads the resolved value yet. +# GITTENSORY_REVIEW_* env kill-switch (rag/reputation/unifiedComment/safety/grounding/e2eTests/screenshots/ +# improvementSignal). Each key is `true` (force on for this repo, subject to the env flag still being +# enabled), `false` (force off), or omitted (falls back to the GITTENSORY_REVIEW_REPOS allowlist default -- +# an operator who sets nothing keeps today's behavior). Exception: `safety` is force-on-only -- an untrusted +# repo-controlled `false` is treated as "no opinion" rather than an active force-off (#2269), since a +# lower-trust actor must never be able to silently defeat the operator's own security-hardening enablement. +# `e2eTests` (#4190), `screenshots` (#4616), and `improvementSignal` (#4738, foundation phase of the #4737 +# PR-improvement-signal epic) are plain symmetric overrides like rag/reputation/unifiedComment -- none carries +# a security-hardening or full-file-fetch rationale (unlike safety/grounding) that would justify a force-on/ +# force-off asymmetry. `screenshots` sits UNDER the separate, richer `review.visual.*` block further below +# (route/preview-URL config, and `review.visual.enabled: false` as an always-available additional force-off); +# this key only answers "does capture run for this repo at all," the same question its siblings answer for +# their own feature. `improvementSignal` is activation wiring only for now -- no tier reads the resolved +# value yet. # features: # rag: true # reputation: false @@ -1137,6 +1140,7 @@ settings: # safety: true # grounding: true # e2eTests: false +# screenshots: false # improvementSignal: false # Registry-review lane (#2435): lets a self-hosted maintainer point gittensory at their OWN structured diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index 071f3b9ecd..3eb5dbce57 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -208,22 +208,34 @@ export type CopycatGateMode = "off" | "warn" | "label" | "block"; // (#4100) is now migrated too — its original "coupled to the merge/close DISPOSITION path" blocker was the // removed AI CI-refutation path (grounding-wire.ts's aiCiRefutationActive is now a vestigial historical- // compatibility helper with zero real callers); grounding today only shapes reviewer PROMPT content, same -// shape as rag/reputation. `screenshots` has its own richer `visual:` block instead (review.visual.enabled, -// #4083) since it carries more than a single boolean. contentLane got its own richer `contentLane:` block below -// (#2435) instead of a boolean here, since it resolves to a whole RegistryLaneSpec, not an on/off toggle — see -// resolveRegistryLaneSpec in review/content-lane/spec-resolver.ts. `selftune` (#4104) ALSO deliberately lives -// outside this block, as its own top-level `review.selftune` field below — it has no `GITTENSORY_REVIEW_REPOS` -// allowlist to fall back to (its own repo scoping is `isAgentConfigured`, a different consent boundary), so it -// doesn't fit this resolver's env-kill-switch → override → allowlist-default shape; see `selfTuneRepos` in -// `review/selftune-wire.ts`. `e2eTests` (#4190, part of the #4189 E2E-test-generation epic) fits this shape -// exactly as a plain symmetric override — unlike `safety`/`grounding` it has no force-on-only or force-off-only -// floor/ceiling, since AI-generated test content carries no security-hardening or full-file-fetch rationale to -// protect from a repo-controlled override. `improvementSignal` (#4738, foundation phase of the #4737 PR- -// improvement-signal epic) is likewise a plain symmetric override: it is a READ-ONLY advisory quality-delta -// signal, not a security control, so a repo-level `false` behaves like any other plain override with no -// floor/ceiling. This is activation wiring only -- no tier reads the resolved value yet (sibling sub-issues -// #4739-#4746 build the deterministic/LLM/panel behavior that will gate on it). -export const CONVERGED_FEATURE_KEYS = ["rag", "reputation", "unifiedComment", "safety", "grounding", "e2eTests", "improvementSignal"] as const; +// shape as rag/reputation. contentLane got its own richer `contentLane:` block below (#2435) instead of a +// boolean here, since it resolves to a whole RegistryLaneSpec, not an on/off toggle — see +// resolveRegistryLaneSpec in review/content-lane/spec-resolver.ts (its own precedence already matches this +// block's env-kill-switch → override → allowlist-default shape one-for-one; it just isn't literally routed +// through resolveConvergedFeature yet — a disclosed, low-priority fast-follow, #4616). `selftune` (#4104) +// ALSO deliberately lives outside this block, as its own top-level `review.selftune` field below — it has no +// `GITTENSORY_REVIEW_REPOS` allowlist to fall back to (its own repo scoping is `isAgentConfigured`, a +// different consent boundary), so it doesn't fit this resolver's env-kill-switch → override → allowlist- +// default shape; see `selfTuneRepos` in `review/selftune-wire.ts`. `e2eTests` (#4190, part of the #4189 +// E2E-test-generation epic) fits this shape exactly as a plain symmetric override — unlike `safety`/ +// `grounding` it has no force-on-only or force-off-only floor/ceiling, since AI-generated test content +// carries no security-hardening or full-file-fetch rationale to protect from a repo-controlled override. +// `screenshots` (#4616) joined this block for the SAME reason `e2eTests` fits it plainly: capturing a +// before/after render of the PR's own web-visible files carries no security-hardening or full-file-fetch +// rationale either, so it gets the standard override, not an asymmetric one. Before #4616 it had NO +// `features:` override at all (env flag AND allowlist only) despite being documented right next to its six +// siblings in `.gittensory.yml.example` — a self-hoster who guessed `features.screenshots: true` (a natural +// guess given the sibling keys) found it silently did nothing. `features.screenshots` is layered UNDER the +// separate, richer `review.visual.*` block (route/preview-URL config, #3609/#3610, and `review.visual.enabled: +// false` as an always-available additional force-off, #4083) — that block still narrows/disables capture +// AFTER this key decides whether capture is even attempted for the repo at all; the two are independent and +// `review.visual.enabled` keeps its own existing force-off-only semantics untouched by this change. +// `improvementSignal` (#4738, foundation phase of the #4737 PR-improvement-signal epic) is likewise a plain +// symmetric override: it is a READ-ONLY advisory quality-delta signal, not a security control, so a repo- +// level `false` behaves like any other plain override with no floor/ceiling. This is activation wiring only +// -- no tier reads the resolved value yet (sibling sub-issues #4739-#4746 build the deterministic/LLM/panel +// behavior that will gate on it). +export const CONVERGED_FEATURE_KEYS = ["rag", "reputation", "unifiedComment", "safety", "grounding", "e2eTests", "screenshots", "improvementSignal"] as const; export type ConvergedFeatureKey = (typeof CONVERGED_FEATURE_KEYS)[number]; /** Per-repo activation overrides for the converged review features (`features:` block). `true`/`false` force the @@ -967,6 +979,7 @@ const EMPTY_FEATURES_CONFIG: FocusManifestFeaturesConfig = { safety: null, grounding: null, e2eTests: null, + screenshots: null, improvementSignal: null, }; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e785d78f65..9253c5fca6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -385,7 +385,6 @@ import { // unchanged -- those tests are deeply interspersed with unrelated ones in that file, not in a cleanly // extractable describe block, so relocating them is deliberately deferred rather than forced into this PR. export { claimPrActuationLock, releasePrActuationLock } from "./transient-locks"; -import { screenshotsAllowed } from "../review/visual-wire"; import { isVisualPath } from "../review/visual/paths"; import { buildCapture, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture"; import { @@ -515,7 +514,7 @@ import { resolveE2eTestGenInstructions, runGittensoryE2eTestGeneration } from ". import { commitE2eTestToPrBranch } from "../github/e2e-test-commit"; import { buildRepoCultureProfileContext, - isRepoCultureProfileEnabled, + shouldApplyRepoCultureProfile, } from "../review/repo-culture-profile-wire"; import { applyReviewMemorySuppression, getCachedReviewSuppressions, invalidateReviewSuppressionCache, shouldApplyReviewMemory } from "../review/review-memory-wire"; import { @@ -7837,10 +7836,9 @@ export async function runAiReviewForAdvisory( // (typical PR size, common accepted labels) and appends it as additive grounding — exactly like RAG. Both // gates OFF (default) → NO new branch: no D1 read, and `cultureProfileContext` is left undefined so the // prompt is byte-identical to today. Fully fail-safe (any error/insufficient-history degrades to ""). - const cultureProfileContext = - isRepoCultureProfileEnabled(env) && args.reviewCultureProfile === true - ? await buildRepoCultureProfileContext(env, args.repoFullName) - : undefined; + const cultureProfileContext = shouldApplyRepoCultureProfile(env, args.reviewCultureProfile === true) + ? await buildRepoCultureProfileContext(env, args.repoFullName) + : undefined; // Review-enrichment (#1472, flag-gated by GITTENSORY_REVIEW_ENRICHMENT + REES_URL). POST the PR to the external // REES for the heavy/external analysis the reviewer can't run (dependency CVEs, secrets, license/EOL/supply-chain); // its public-safe brief splices into the prompt next to grounding + RAG. Flag-OFF (default) → no call, no branch, @@ -10668,7 +10666,7 @@ async function maybePublishPrPublicSurface( // invalidation) can refresh independently of this PR's head SHA, exactly like RAG's vector index — // so a repo with it active also bypasses the AI-review result cache rather than fingerprinting a // value that can't prove freshness. - cultureProfile: isRepoCultureProfileEnabled(env) && reviewCultureProfile === true, + cultureProfile: shouldApplyRepoCultureProfile(env, reviewCultureProfile === true), // 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. @@ -11652,9 +11650,11 @@ async function maybePublishPrPublicSurface( gate: commentGate, duplicateWinnerEnabled, }); - // Visual before/after capture (visual-capture port). Fires ONLY when (1) the global flag + per-repo - // cutover gate both allow it (screenshotsAllowed) AND (2) the PR touches WEB-VISIBLE files (isVisualPath - // — frontend pages / public OG images; backend .ts/.md/.json PRs never qualify). Fully wrapped in + // Visual before/after capture (visual-capture port). Fires ONLY when (1) the "screenshots" converged + // feature resolves active for this repo (resolveConvergedFeature — the global flag AND (a per-repo + // `features.screenshots` override OR the cutover allowlist default), #4616; reuses the manifest this + // pass already loaded above, no extra fetch) AND (2) the PR touches WEB-VISIBLE files (isVisualPath — + // frontend pages / public OG images; backend .ts/.md/.json PRs never qualify). Fully wrapped in // try/catch + defaults to [] so a capture failure (render timeout, missing binding, GitHub hiccup) can // NEVER sink the review — it just omits the "Visual preview" section. Flag-OFF (default) ⇒ this block is // skipped entirely and the unified comment is byte-identical. @@ -11662,7 +11662,7 @@ async function maybePublishPrPublicSurface( const visualFiles = unifiedFiles .map((file) => file.path) .filter(isVisualPath); - if (screenshotsAllowed(env, repoFullName) && visualFiles.length > 0) { + if (resolveConvergedFeature(env, repoFocusManifestForComment, "screenshots", repoFullName) && visualFiles.length > 0) { try { const token = await createInstallationToken(env, installationId); // review.visual (#3609 / #3610): an explicit per-repo preview-URL template / route list. Absent config diff --git a/src/review/feature-activation.ts b/src/review/feature-activation.ts index 58506c19f7..292f3b9a10 100644 --- a/src/review/feature-activation.ts +++ b/src/review/feature-activation.ts @@ -1,23 +1,33 @@ -// Per-repo activation resolver for the converged review features (phase 2 of the per-repo migration). +// Shared per-PR advisory-feature activation resolver (#4616, generalizing the phase-2 per-repo migration this +// file originally shipped for just the `features:`-block keys). // -// Before: each feature ran when `isXEnabled(env)` (a global env flag) AND `isConvergenceRepoAllowed(env, repo)` -// (the GITTENSORY_REVIEW_REPOS allowlist) were both true — coarse, all-or-nothing per repo, and configured only -// via env. Now a self-host operator toggles features individually PER REPO in the container-private `.gittensory.yml` -// (`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. 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. +// Before this file existed, each feature ran when `isXEnabled(env)` (a global env flag) AND +// `isConvergenceRepoAllowed(env, repo)` (the GITTENSORY_REVIEW_REPOS allowlist) were both true — coarse, +// all-or-nothing per repo, configured only via env. `resolveConvergedFeature` replaced that for the six +// `features:`-block keys (now seven, see below) with a per-repo `.gittensory.yml` override. But that migration +// left ~10 OTHER per-PR advisory capabilities (screenshots, impactMap, reviewMemory, cultureProfile, +// inlineComments, fixHandoff, …) each re-implementing their OWN hand-rolled version of the same boolean +// arithmetic outside this file, with at least four subtly different precedence shapes and no single place +// documenting which feature uses which (#4616's config-sprawl audit finding — already the root cause of one +// production incident, see GITTENSORY_PUBLIC_STATS_REPOS's doc comment in env.d.ts). // -// `resolveConvergedFeature` is the pure core (takes the already-loaded manifest). `convergedFeatureActive` is the -// async convenience that loads the cached focus manifest itself — used at call sites that don't already hold one. +// `resolveFeatureActivation` below is now the ONE pure core every one of those precedence shapes reduces to. +// `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. +// - `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 +// (`review:`, not `features:`) than the seven `ConvergedFeatureKey`s above, so they were never candidates +// for literally becoming `ConvergedFeatureKey`s — renaming an operator's existing yml key would itself be a +// behavior break — but they share the exact same underlying arithmetic as "standard" mode with the +// allowlist input pinned to `false` (a `manifestOnly` feature can never be force-activated by +// GITTENSORY_REVIEW_REPOS the way its `features:`-block cousins can). +// +// `convergedFeatureActive` is the async convenience that loads the cached focus manifest itself for +// `resolveConvergedFeature`'s callers that don't already hold one. import { isConvergenceRepoAllowed } from "./cutover-gate"; import { isE2eTestGenerationEnabled } from "./e2e-test-gen-wire"; import { isGroundingEnabled } from "./grounding-wire"; @@ -26,9 +36,46 @@ import { isRagEnabled } from "./rag-wire"; import { isReputationEnabled } from "./reputation-wire"; import { isSafetyEnabled } from "./safety"; import { isUnifiedReviewCommentEnabled } from "./unified-comment-bridge"; +import { isScreenshotsEnabled } from "./visual-wire"; import type { ConvergedFeatureKey, FocusManifest } from "../signals/focus-manifest"; 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. + * - `"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). + * - `"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 + * from day one, with no `GITTENSORY_REVIEW_REPOS` role ever defined for it (see + * {@link resolveManifestOnlyFeature}). + */ +export type FeatureActivationMode = "standard" | "forceOnOnly" | "allowlistRequired" | "manifestOnly"; + +/** + * The single pure precedence core (#4616) every per-PR advisory-feature activation check in this codebase now + * shares — env kill-switch, then a per-repo override, then (for two of the four modes) an allowlist default. + * Deliberately takes already-resolved primitives, not `Env` or a raw `FocusManifest`, specifically so a future + * per-tenant resolution path (e.g. a tenant DB row standing in for the global env var, or a tenant's own + * allowlist standing in for `GITTENSORY_REVIEW_REPOS`) can supply the same three booleans without this + * function — or either of its two callers' precedence logic below — changing at all. + */ +export function resolveFeatureActivation(globalFlagOn: boolean, override: boolean | null, allowlisted: boolean, mode: FeatureActivationMode): boolean { + if (!globalFlagOn) return false; // master kill-switch — off ⇒ false regardless of override, allowlist, or mode + if (mode === "forceOnOnly") return override === true || allowlisted; + if (mode === "allowlistRequired") return allowlisted && override !== false; + if (mode === "manifestOnly") return override === true; + return override ?? allowlisted; // "standard" +} + /** The master kill-switch (global env flag) for each converged feature, keyed by the manifest `features:` key. */ const FEATURE_GLOBAL_FLAG: Record boolean> = { rag: isRagEnabled, @@ -37,15 +84,25 @@ const FEATURE_GLOBAL_FLAG: Record boolean> = safety: isSafetyEnabled, grounding: isGroundingEnabled, e2eTests: isE2eTestGenerationEnabled, + screenshots: isScreenshotsEnabled, improvementSignal: isImprovementSignalEnabled, }; +/** 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. */ +const FEATURE_MODE: Partial> = { + safety: "forceOnOnly", + grounding: "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 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). `grounding` is also - * asymmetric in the opposite direction: a repo override can only force it OFF, never bypass the operator allowlist. + * 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. */ export function resolveConvergedFeature( env: Env, @@ -53,15 +110,26 @@ export function resolveConvergedFeature( feature: ConvergedFeatureKey, repoFullName: string, ): boolean { - if (!FEATURE_GLOBAL_FLAG[feature](env)) return false; // master kill-switch + const globalFlagOn = FEATURE_GLOBAL_FLAG[feature](env); + if (!globalFlagOn) return false; // master kill-switch — short-circuits before the allowlist check below 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. 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 allowlisted; // back-compat allowlist default + return resolveFeatureActivation(globalFlagOn, override, allowlisted, FEATURE_MODE[feature] ?? "standard"); +} + +/** + * Resolve a "manifest-only" advisory feature (#4616): the operator's global env kill-switch AND an EXPLICIT + * per-repo `.gittensory.yml` `review.*` opt-in — no `GITTENSORY_REVIEW_REPOS` allowlist role at all. Shared by + * every `review:`-block feature that was never given an allowlist fallback: impactMap (`shouldComputeImpactMap`, + * impact-map-wire.ts), reviewMemory (`shouldApplyReviewMemory`, review-memory-wire.ts), cultureProfile + * (`shouldApplyRepoCultureProfile`, repo-culture-profile-wire.ts), inlineComments (`shouldRequestInlineFindings`, + * inline-comments.ts, #4099), and fixHandoff (`shouldEmitFixHandoff`, fix-handoff.ts, #4099). `override` accepts + * `undefined` (in addition to the pure core's `boolean | null`) purely so callers can pass a manifest field + * straight through without normalizing it first — `undefined` and `null` are both "unset" here, identically to + * every one of those five wire modules' own prior, independently-hand-rolled `=== true` check. + */ +export function resolveManifestOnlyFeature(globalFlagOn: boolean, override: boolean | null | undefined): boolean { + return resolveFeatureActivation(globalFlagOn, override ?? null, false, "manifestOnly"); } /** diff --git a/src/review/fix-handoff.ts b/src/review/fix-handoff.ts index 95dbae4f8e..afde531d5c 100644 --- a/src/review/fix-handoff.ts +++ b/src/review/fix-handoff.ts @@ -4,7 +4,11 @@ // itself when explicitly set — the per-repo convergence cutover allowlist no longer applies to this feature (an // unset manifest toggle preserves the ORIGINAL always-off default; it was never sufficient to be allowlisted // alone). This is the config/gate slice: pure resolvers only — no emission/render here (that is a separate -// slice), so the gate/verdict is never touched. +// slice), so the gate/verdict is never touched. `shouldEmitFixHandoff` is the "manifestOnly" precedence shape +// (#4616) — see `resolveManifestOnlyFeature`/`FeatureActivationMode` in `./feature-activation` for the shared +// core this, and four sibling `review:`-block features, now delegate to. + +import { resolveManifestOnlyFeature } from "./feature-activation"; /** True when the operator enabled fix-handoff globally. Flag-OFF (default) ⇒ the caller never emits fix-handoff * blocks. Truthy follows the codebase convention (same regex as isInlineCommentsEnabled). */ @@ -20,8 +24,9 @@ export function isFixHandoffEnabled(env: { GITTENSORY_REVIEW_FIX_HANDOFF?: strin * on without needing the GITTENSORY_REVIEW_REPOS cutover allowlist at all. (3) `manifestToggle` unset * (`undefined`) preserves this feature's ORIGINAL design exactly: being on the allowlist alone was never * sufficient, so this stays `false` regardless of the allowlist, byte-identical to every repo's behavior before - * this change. Exactly mirrors `shouldRequestInlineFindings`'s shape and precedence. `repoFullName` is kept for - * a stable call signature even though it's unused now that the allowlist no longer applies here. */ + * this change. Exactly mirrors `shouldRequestInlineFindings`'s shape and precedence (both are now the SAME + * `resolveManifestOnlyFeature` call, #4616). `repoFullName` is kept for a stable call signature even though + * it's unused now that the allowlist no longer applies here. */ export function shouldEmitFixHandoff( // GITTENSORY_REVIEW_REPOS is accepted (not just GITTENSORY_REVIEW_FIX_HANDOFF) purely for call-site signature // stability with existing callers/tests that pass a wider env object -- it's no longer read, see the doc @@ -31,6 +36,5 @@ export function shouldEmitFixHandoff( manifestToggle: boolean | undefined, ): boolean { void repoFullName; // kept for call-site signature stability, see doc comment above - if (!isFixHandoffEnabled(env)) return false; - return manifestToggle === true; + return resolveManifestOnlyFeature(isFixHandoffEnabled(env), manifestToggle); } diff --git a/src/review/impact-map-wire.ts b/src/review/impact-map-wire.ts index f52d2756d0..ea8fb1f9a1 100644 --- a/src/review/impact-map-wire.ts +++ b/src/review/impact-map-wire.ts @@ -4,12 +4,16 @@ // 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. +// `shouldComputeImpactMap` is the "manifestOnly" precedence shape (#4616) — see +// `resolveManifestOnlyFeature`/`FeatureActivationMode` in `./feature-activation` for the shared core this, and +// four sibling `review:`-block features, now delegate to. // // 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 // `GittensoryAiReviewInput.impactMapContext` (src/services/ai-review.ts), exactly like `formatRetrievedContext` // does for RAG's own retrieval block. +import { resolveManifestOnlyFeature } from "./feature-activation"; import type { ImpactMapEntry } from "./impact-map"; /** True when impact-map computation is enabled at the operator level. Flag-OFF (default) → the caller takes @@ -27,7 +31,7 @@ export function shouldComputeImpactMap( env: { GITTENSORY_REVIEW_IMPACT_MAP?: string | undefined }, manifestImpactMapEnabled: boolean, ): boolean { - return isImpactMapEnabled(env) && manifestImpactMapEnabled; + return resolveManifestOnlyFeature(isImpactMapEnabled(env), manifestImpactMapEnabled); } /** Hard cap on entries actually formatted into the AI-review prompt section — bounds prompt-token cost diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index c86df32396..2baf0e770c 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -7,9 +7,12 @@ // applies to this feature (an unset manifest toggle preserves the ORIGINAL always-off default; it was never // sufficient to be allowlisted alone). Fully FAIL-SAFE: a finding whose line is not a commentable line in the PR // diff is dropped (GitHub 422s otherwise), and any API error degrades to "no inline comments" — it NEVER throws -// and NEVER touches the gate. +// and NEVER touches the gate. `shouldRequestInlineFindings` is the "manifestOnly" precedence shape (#4616) — +// see `resolveManifestOnlyFeature`/`FeatureActivationMode` in `./feature-activation` for the shared core this, +// and four sibling `review:`-block features, now delegate to. import { createPullRequestReviewComments } from "../github/pr-actions"; +import { resolveManifestOnlyFeature } from "./feature-activation"; import { formatInlineCommentSeverityLabel } from "./inline-comment-label"; import { resolveInlineCommentAnchor, rightLinesByPath } from "./inline-comment-range"; import { addedLinesByPath, anchoredSuggestionBlock } from "./inline-suggestion-anchor"; @@ -38,8 +41,9 @@ export function isInlineCommentsEnabled(env: { GITTENSORY_REVIEW_INLINE_COMMENTS * rag/reputation/safety/unifiedComment/grounding (which already fall back to the cutover allowlist when their * manifest field is unset), inline comments have always required an EXPLICIT per-repo opt-in — being on the * allowlist alone was never sufficient, so this stays `false` regardless of the allowlist, byte-identical to - * every repo's behavior before this change. `repoFullName` is kept for a stable call signature even though it's - * unused now that the allowlist no longer applies here. */ + * every repo's behavior before this change (now expressed as `resolveManifestOnlyFeature`'s `"manifestOnly"` + * mode, #4616 — see `feature-activation.ts`). `repoFullName` is kept for a stable call signature even though + * it's unused now that the allowlist no longer applies here. */ export function shouldRequestInlineFindings( // GITTENSORY_REVIEW_REPOS is accepted (not just GITTENSORY_REVIEW_INLINE_COMMENTS) purely for call-site // signature stability with existing callers/tests that pass a wider env object -- it's no longer read, see @@ -49,8 +53,7 @@ export function shouldRequestInlineFindings( manifestToggle: boolean | undefined, ): boolean { void repoFullName; // kept for call-site signature stability, see doc comment above - if (!isInlineCommentsEnabled(env)) return false; - return manifestToggle === true; + return resolveManifestOnlyFeature(isInlineCommentsEnabled(env), manifestToggle); } /** PURE (#1956): should a `suggestion` be rendered as a GitHub-native ` ```suggestion ` block? This is an diff --git a/src/review/repo-culture-profile-wire.ts b/src/review/repo-culture-profile-wire.ts index e896f331f1..0d261f3c7e 100644 --- a/src/review/repo-culture-profile-wire.ts +++ b/src/review/repo-culture-profile-wire.ts @@ -10,11 +10,14 @@ // default OFF) gates whether the capability exists AT ALL, and the per-repo `.gittensory.yml` // `review.culture_profile` boolean (see signals/focus-manifest.ts) opts a specific repo in once the global // switch is on. Both default OFF/absent ⇒ this module is never invoked, no D1 read happens, and the reviewer -// prompt is byte-identical to today. +// prompt is byte-identical to today. `shouldApplyRepoCultureProfile` is the "manifestOnly" precedence shape +// (#4616) — see `resolveManifestOnlyFeature`/`FeatureActivationMode` in `./feature-activation` for the shared +// core this, and four sibling `review:`-block features, now delegate to. // // ADVISORY GROUNDING ONLY (house rule + #2995 requirement): this NEVER becomes a gate/scoring input. It only // ever appends a reference-only block to the AI reviewer's USER prompt, exactly like the RAG/grounding/ // enrichment sections it sits alongside in `services/ai-review.ts`'s buildUserPrompt. +import { resolveManifestOnlyFeature } from "./feature-activation"; import { extractRepoCultureProfile, type RepoCultureProfile } from "./repo-culture-profile"; import { neutralizePromptInjection } from "./prompt-injection"; @@ -24,6 +27,18 @@ export function isRepoCultureProfileEnabled(env: { GITTENSORY_REVIEW_CULTURE_PRO return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_CULTURE_PROFILE ?? ""); } +/** Resolve whether culture-profile grounding should apply for THIS repo/PR: the operator's global env + * kill-switch AND the per-repo manifest opt-in. Neither alone is sufficient — mirrors + * `shouldComputeImpactMap` / `shouldApplyReviewMemory` (#4616), the same "manifestOnly" shape. Previously + * inlined at each of its two call sites in src/queue/processors.ts as `isRepoCultureProfileEnabled(env) && + * x === true`; centralized here so the precedence lives in exactly one place, like every sibling feature. */ +export function shouldApplyRepoCultureProfile( + env: { GITTENSORY_REVIEW_CULTURE_PROFILE?: string | undefined }, + manifestCultureProfileEnabled: boolean, +): boolean { + return resolveManifestOnlyFeature(isRepoCultureProfileEnabled(env), manifestCultureProfileEnabled); +} + /** Format a present profile into the reviewer-prompt block. Mirrors `formatRetrievedContext`'s * self-labelled, reference-only framing so the model treats it the same way it treats RAG context. */ export function formatRepoCultureProfileSection(profile: RepoCultureProfile): string { diff --git a/src/review/review-memory-wire.ts b/src/review/review-memory-wire.ts index a3b5bb94e4..ffb138a997 100644 --- a/src/review/review-memory-wire.ts +++ b/src/review/review-memory-wire.ts @@ -4,9 +4,12 @@ // src/signals/focus-manifest.ts) — 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, the suppression store is never read from the // review path at all (the caller guards on this flag before doing any D1 read or matching), so the review -// stays byte-identical to today. +// stays byte-identical to today. `shouldApplyReviewMemory` is the "manifestOnly" precedence shape (#4616) — +// see `resolveManifestOnlyFeature`/`FeatureActivationMode` in `./feature-activation` for the shared core this, +// and four sibling `review:`-block features, now delegate to. import { listReviewSuppressions, recordAuditEvent } from "../db/repositories"; +import { resolveManifestOnlyFeature } from "./feature-activation"; import { matchSuppressions, type ReviewMemoryFindingInput } from "./review-memory-match"; import type { AdvisoryFinding, ReviewSuppressionRecord } from "../types"; import { incr } from "../selfhost/metrics"; @@ -26,7 +29,7 @@ export function shouldApplyReviewMemory( env: { GITTENSORY_REVIEW_MEMORY?: string | undefined }, manifestReviewMemoryEnabled: boolean, ): boolean { - return isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled; + return resolveManifestOnlyFeature(isReviewMemoryEnabled(env), manifestReviewMemoryEnabled); } // Short in-isolate TTL cache for listReviewSuppressions (#4508), mirroring rag.ts's chunkCountCache: repeated // unified-comment renders for the same repo within a short window (the 3 independent maybePublishPrPublicSurface diff --git a/src/review/visual-wire.ts b/src/review/visual-wire.ts index 469a85e942..7600d7642d 100644 --- a/src/review/visual-wire.ts +++ b/src/review/visual-wire.ts @@ -1,29 +1,24 @@ -// Convergence (visual capture) feature flag + per-repo gate wiring. +// Convergence (visual capture) feature flag wiring. // // Single env switch: GITTENSORY_REVIEW_SCREENSHOTS. Default OFF (unset/"false") — when OFF the processor // never calls buildCapture, so the review path is byte-identical to today. Truthy follows the codebase // convention (`/^(1|true|yes|on)$/i`, same as isSafetyEnabled / isUnifiedReviewCommentEnabled). // -// As with every other per-PR converged feature, capture runs on a given PR's repo ONLY IF the global flag is -// ON *AND* the repo is in the per-repo cutover allowlist (GITTENSORY_REVIEW_REPOS). The AND with -// isConvergenceRepoAllowed is MANDATORY — it lets the cutover roll forward/back one repo at a time and keeps -// a globally-on-but-not-listed deploy dormant. - -import { isConvergenceRepoAllowed } from "./cutover-gate"; +// `screenshots` is a `ConvergedFeatureKey` (#4616): per-repo activation — the global flag here AND (a per-repo +// `.gittensory.yml` `features.screenshots` override OR the `GITTENSORY_REVIEW_REPOS` cutover allowlist +// default) — is resolved by `resolveConvergedFeature` / `convergedFeatureActive` in `./feature-activation`, +// the SAME shared resolver every other converged feature goes through; this file only owns the flag itself +// (`FEATURE_GLOBAL_FLAG`'s `screenshots` entry). Before #4616 this file also exported a hand-rolled +// `screenshotsAllowed` (env flag AND allowlist, no `features:` override at all — screenshots was not yet a +// `ConvergedFeatureKey`); call sites now call `resolveConvergedFeature(env, manifest, "screenshots", +// repoFullName)` directly, exactly like `e2eTests`' own call sites already did before this change. +// +// `review.visual.enabled` / `review.visual.production_url` (#3609/#3610/#4083) are a SEPARATE, richer per-repo +// config layer (route/preview-URL details, plus an always-available additional force-off) that narrows +// capture AFTER this key decides whether it is attempted for the repo at all — see resolveVisualCaptureConfig +// in src/queue/processors.ts. That layer's existing force-off-only semantics are unchanged by #4616. /** True when the visual-capture global flag is enabled. Flag-OFF (default) → no capture is attempted. */ export function isScreenshotsEnabled(env: { GITTENSORY_REVIEW_SCREENSHOTS?: string | undefined }): boolean { return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_SCREENSHOTS ?? ""); } - -/** - * True when visual capture is allowed for `repoFullName`: the global flag is ON *AND* the repo is in the - * per-repo cutover allowlist. Both must hold — a globally-on flag alone never activates capture for an - * unlisted repo (the dormant default). - */ -export function screenshotsAllowed( - env: { GITTENSORY_REVIEW_SCREENSHOTS?: string | undefined; GITTENSORY_REVIEW_REPOS?: string | undefined }, - repoFullName: string, -): boolean { - return isScreenshotsEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); -} diff --git a/test/unit/feature-activation.test.ts b/test/unit/feature-activation.test.ts index 32ed75d708..4734fdac47 100644 --- a/test/unit/feature-activation.test.ts +++ b/test/unit/feature-activation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { convergedFeatureActive, resolveConvergedFeature } from "../../src/review/feature-activation"; +import { convergedFeatureActive, resolveConvergedFeature, resolveFeatureActivation, resolveManifestOnlyFeature, type FeatureActivationMode } from "../../src/review/feature-activation"; import { CONVERGED_FEATURE_KEYS, type ConvergedFeatureKey, type FocusManifest } from "../../src/signals/focus-manifest"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; @@ -14,6 +14,7 @@ const FLAG: Record = { safety: "GITTENSORY_REVIEW_SAFETY", grounding: "GITTENSORY_REVIEW_GROUNDING", e2eTests: "GITTENSORY_REVIEW_E2E_TESTS", + screenshots: "GITTENSORY_REVIEW_SCREENSHOTS", improvementSignal: "GITTENSORY_REVIEW_IMPROVEMENT_SIGNAL", }; @@ -29,11 +30,93 @@ function manifestWith(features: Partial>): safety: null, grounding: null, e2eTests: null, + screenshots: null, improvementSignal: null, } as FocusManifest["features"]; return { features: { ...base, ...features, present: Object.keys(features).length > 0 } }; } +// ── resolveFeatureActivation (#4616) — the single pure core every mode below reduces to ────────────────────── + +describe("resolveFeatureActivation — the shared pure core (#4616)", () => { + it("the master kill-switch wins outright regardless of mode, override, or allowlist", () => { + const modes: FeatureActivationMode[] = ["standard", "forceOnOnly", "allowlistRequired", "manifestOnly"]; + for (const mode of modes) { + expect(resolveFeatureActivation(false, true, true, mode)).toBe(false); + expect(resolveFeatureActivation(false, null, true, mode)).toBe(false); + expect(resolveFeatureActivation(false, false, false, mode)).toBe(false); + } + }); + + describe('mode "standard"', () => { + it("an explicit override fully controls in both directions", () => { + expect(resolveFeatureActivation(true, true, false, "standard")).toBe(true); // override bypasses a non-allowlisted repo + expect(resolveFeatureActivation(true, false, true, "standard")).toBe(false); // override forces off an allowlisted repo + }); + it("falls back to the allowlist when override is unset (null)", () => { + expect(resolveFeatureActivation(true, null, true, "standard")).toBe(true); + expect(resolveFeatureActivation(true, null, false, "standard")).toBe(false); + }); + }); + + describe('mode "forceOnOnly" (safety\'s shape, #2269)', () => { + it("override can force ON even when NOT allowlisted", () => { + expect(resolveFeatureActivation(true, true, false, "forceOnOnly")).toBe(true); + }); + it("override=false is downgraded to \"no opinion\" — falls through to the allowlist instead of forcing off", () => { + expect(resolveFeatureActivation(true, false, true, "forceOnOnly")).toBe(true); // allowlisted wins despite override=false + expect(resolveFeatureActivation(true, false, false, "forceOnOnly")).toBe(false); // not allowlisted either ⇒ off + }); + it("unset override falls back to the allowlist, same as standard", () => { + expect(resolveFeatureActivation(true, null, true, "forceOnOnly")).toBe(true); + expect(resolveFeatureActivation(true, null, false, "forceOnOnly")).toBe(false); + }); + }); + + describe('mode "allowlistRequired" (grounding\'s shape)', () => { + it("the allowlist is a hard requirement — override=true can NEVER bypass it", () => { + expect(resolveFeatureActivation(true, true, false, "allowlistRequired")).toBe(false); + }); + it("within an allowlisted repo, override may additionally force OFF", () => { + expect(resolveFeatureActivation(true, false, true, "allowlistRequired")).toBe(false); + expect(resolveFeatureActivation(true, null, true, "allowlistRequired")).toBe(true); + expect(resolveFeatureActivation(true, true, true, "allowlistRequired")).toBe(true); + }); + }); + + describe('mode "manifestOnly" (the five review:-block features, #4616)', () => { + it("there is no allowlist role at all — an allowlisted repo with no override still stays off", () => { + expect(resolveFeatureActivation(true, null, true, "manifestOnly")).toBe(false); + }); + it("an explicit override===true is the only way to activate, allowlist status notwithstanding", () => { + expect(resolveFeatureActivation(true, true, false, "manifestOnly")).toBe(true); + expect(resolveFeatureActivation(true, true, true, "manifestOnly")).toBe(true); + }); + it("override===false stays off exactly like unset (both are \"not explicitly true\")", () => { + expect(resolveFeatureActivation(true, false, true, "manifestOnly")).toBe(false); + expect(resolveFeatureActivation(true, false, false, "manifestOnly")).toBe(false); + }); + }); +}); + +// ── resolveManifestOnlyFeature (#4616) — the thin review:-block adapter over the core above ────────────────── + +describe("resolveManifestOnlyFeature — env kill-switch AND an explicit manifest opt-in, no allowlist role", () => { + it("requires BOTH the global flag and an explicit override===true", () => { + expect(resolveManifestOnlyFeature(true, true)).toBe(true); + }); + it("is OFF when the flag is on but the override is unset (undefined) or absent (null)", () => { + expect(resolveManifestOnlyFeature(true, undefined)).toBe(false); + expect(resolveManifestOnlyFeature(true, null)).toBe(false); + }); + it("is OFF when the override is explicitly true but the global flag is off (a repo cannot self-enable)", () => { + expect(resolveManifestOnlyFeature(false, true)).toBe(false); + }); + it("is OFF when the override is explicitly false", () => { + expect(resolveManifestOnlyFeature(true, false)).toBe(false); + }); +}); + describe("resolveConvergedFeature — env kill-switch → per-repo override → allowlist default", () => { it("returns false when the global env flag is off, regardless of a per-repo override or the allowlist", () => { // flag off, override true, repo allowlisted → still off (kill-switch wins). diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 6e236070a8..e5fd6fbc96 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -415,6 +415,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { safety: "safety:", grounding: "grounding:", e2eTests: "e2eTests:", + screenshots: "screenshots:", improvementSignal: "improvementSignal:", } satisfies Record, string>; @@ -830,7 +831,7 @@ describe("compileFocusManifestPolicy", () => { gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, - features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null, grounding: null, e2eTests: null, improvementSignal: null }, + features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, reviewRecap: { present: false, enabled: false, cadenceDays: 7 }, diff --git a/test/unit/repo-culture-profile-wiring.test.ts b/test/unit/repo-culture-profile-wiring.test.ts index 19db25e0df..0d6977fb7d 100644 --- a/test/unit/repo-culture-profile-wiring.test.ts +++ b/test/unit/repo-culture-profile-wiring.test.ts @@ -8,6 +8,7 @@ import { buildRepoCultureProfileContext, formatRepoCultureProfileSection, isRepoCultureProfileEnabled, + shouldApplyRepoCultureProfile, } from "../../src/review/repo-culture-profile-wire"; import { createTestEnv } from "../helpers/d1"; import type { Advisory, RecentMergedPullRequestRecord, RepositorySettings } from "../../src/types"; @@ -65,6 +66,26 @@ describe("isRepoCultureProfileEnabled", () => { }); }); +// ── shouldApplyRepoCultureProfile (#4616) ─────────────────────────────────────────────────────── + +describe("shouldApplyRepoCultureProfile", () => { + it("requires BOTH the operator env flag AND the per-repo manifest opt-in", () => { + expect(shouldApplyRepoCultureProfile({ GITTENSORY_REVIEW_CULTURE_PROFILE: "true" }, true)).toBe(true); + }); + + it("is OFF when the operator flag is on but the manifest didn't opt in", () => { + expect(shouldApplyRepoCultureProfile({ GITTENSORY_REVIEW_CULTURE_PROFILE: "true" }, false)).toBe(false); + }); + + it("is OFF when the manifest opted in but the operator flag is off (repo cannot self-enable)", () => { + expect(shouldApplyRepoCultureProfile({ GITTENSORY_REVIEW_CULTURE_PROFILE: "false" }, true)).toBe(false); + }); + + it("is OFF when both are off", () => { + expect(shouldApplyRepoCultureProfile({}, false)).toBe(false); + }); +}); + // ── formatRepoCultureProfileSection ───────────────────────────────────────────────────────────── describe("formatRepoCultureProfileSection", () => { diff --git a/test/unit/visual-wire.test.ts b/test/unit/visual-wire.test.ts index a261ef4dab..a25a1a67fd 100644 --- a/test/unit/visual-wire.test.ts +++ b/test/unit/visual-wire.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from "vitest"; -import { isScreenshotsEnabled, screenshotsAllowed } from "../../src/review/visual-wire"; +import { isScreenshotsEnabled } from "../../src/review/visual-wire"; +import { resolveConvergedFeature } from "../../src/review/feature-activation"; +import type { FocusManifest } from "../../src/signals/focus-manifest"; describe("isScreenshotsEnabled", () => { it("is OFF by default (unset / empty / false)", () => { @@ -18,26 +20,42 @@ describe("isScreenshotsEnabled", () => { }); }); -describe("screenshotsAllowed (global flag AND per-repo cutover gate)", () => { +// #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)", () => { 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 }, + }; - it("requires BOTH the global flag and the repo allowlist", () => { - // Global on, repo allowlisted → allowed. - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: repo }, repo)).toBe(true); + it("requires BOTH the global flag and the repo allowlist when no override is set", () => { + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: repo } as Env, noOverride, "screenshots", repo)).toBe(true); }); - it("is false when the global flag is OFF even if the repo is allowlisted", () => { - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "false", GITTENSORY_REVIEW_REPOS: repo }, repo)).toBe(false); - expect(screenshotsAllowed({ GITTENSORY_REVIEW_REPOS: repo }, repo)).toBe(false); + it("is false when the global flag is OFF even if the repo is allowlisted (master kill-switch)", () => { + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "false", GITTENSORY_REVIEW_REPOS: repo } as Env, noOverride, "screenshots", repo)).toBe(false); + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_REPOS: repo } as Env, noOverride, "screenshots", repo)).toBe(false); }); - it("is false when the repo is NOT allowlisted even if the global flag is ON (dormant default)", () => { - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "true" }, repo)).toBe(false); - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "" }, repo)).toBe(false); - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "JSONbored/other" }, repo)).toBe(false); + it("is false when the repo is NOT allowlisted and no override is set, even if the global flag is ON (dormant default)", () => { + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true" } as Env, noOverride, "screenshots", repo)).toBe(false); + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "" } as Env, noOverride, "screenshots", repo)).toBe(false); + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true", GITTENSORY_REVIEW_REPOS: "JSONbored/other" } as Env, noOverride, "screenshots", repo)).toBe(false); }); it("matches the repo case-insensitively within the allowlist", () => { - expect(screenshotsAllowed({ GITTENSORY_REVIEW_SCREENSHOTS: "on", GITTENSORY_REVIEW_REPOS: "jsonbored/GITTENSORY" }, repo)).toBe(true); + 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", () => { + const forcedOn: Pick = { features: { ...noOverride.features, present: true, screenshots: true } }; + expect(resolveConvergedFeature({ GITTENSORY_REVIEW_SCREENSHOTS: "true" } as Env, forcedOn, "screenshots", "not/allowlisted")).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); }); });