diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 75c769f19f..f9f77ca22e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6921,8 +6921,10 @@ export async function runAiReviewForAdvisory( // positively scope the AI review. Empty ⇒ every non-excluded file is reviewed (byte-identical). Gate unaffected. reviewPathFilters?: string[] | undefined; // `.gittensory.yml` review.inline_comments (#inline-comments), resolved by the caller from the cached manifest - // (the per-repo toggle). ANDed here with the operator flag + cutover allowlist to decide whether to ASK the - // model for line-anchored inline findings. Absent/false ⇒ the reviewer prompt is byte-identical (no findings). + // (the per-repo toggle). Precedence (#4099): the operator flag is a master kill-switch, never bypassable by + // config; an explicit true/false here now fully controls the feature, bypassing the cutover allowlist; unset + // stays byte-identical to every repo's behavior before this change (the allowlist alone was never sufficient + // on its own). Absent ⇒ the reviewer prompt is byte-identical (no findings) for every repo untouched by this. reviewInlineComments?: boolean | undefined; // `.gittensory.yml` review.finding_categories (#1958), resolved by the caller from the cached manifest. ANDed // here with reviewInlineComments (a category has nothing to categorize without an inline finding) to decide diff --git a/src/review/fix-handoff.ts b/src/review/fix-handoff.ts index 3bf40ed4f9..95dbae4f8e 100644 --- a/src/review/fix-handoff.ts +++ b/src/review/fix-handoff.ts @@ -1,10 +1,10 @@ // Fix-handoff blocks (#2176, config slice for #1962) — copy-paste remediation guidance the reviewer can emit -// ALONGSIDE the decision summary. Default OFF at every layer, mirroring the inline-comments precedent: the operator -// flag GITTENSORY_REVIEW_FIX_HANDOFF, the per-repo convergence cutover allowlist, AND the per-repo `.gittensory.yml` -// review.fixHandoff toggle are ALL ANDed before a fix-handoff block is ever emitted. 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. - -import { isConvergenceRepoAllowed } from "./cutover-gate"; +// ALONGSIDE the decision summary. Default OFF: the operator flag GITTENSORY_REVIEW_FIX_HANDOFF is a master +// kill-switch, and the per-repo `.gittensory.yml` review.fixHandoff toggle (#4099) fully controls activation by +// 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. /** 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). */ @@ -12,14 +12,25 @@ export function isFixHandoffEnabled(env: { GITTENSORY_REVIEW_FIX_HANDOFF?: strin return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_FIX_HANDOFF ?? ""); } -/** PURE: should the reviewer emit fix-handoff blocks for this PR? True ONLY when ALL THREE gates pass — the per-repo - * `.gittensory.yml` toggle (`manifestToggle`), the operator flag, AND the convergence cutover allowlist — so the - * feature is off by default at every layer. Mirrors shouldRequestInlineFindings, keeping the three-way gate in one - * unit-testable place. */ +/** PURE (#4099): should the reviewer emit fix-handoff blocks for this PR? (1) The operator's + * GITTENSORY_REVIEW_FIX_HANDOFF flag is an absolute MASTER KILL-SWITCH — off ⇒ always false, regardless of the + * manifest, and no per-repo config can bypass it (consistent with every other converged feature — see + * `resolveConvergedFeature` in `feature-activation.ts`). (2) An explicit per-repo `.gittensory.yml` + * `review.fixHandoff` override (`true`/`false`) now FULLY controls the feature by itself — a repo can turn this + * 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. */ 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 + // comment above. env: { GITTENSORY_REVIEW_FIX_HANDOFF?: string | undefined; GITTENSORY_REVIEW_REPOS?: string | undefined }, repoFullName: string, manifestToggle: boolean | undefined, ): boolean { - return manifestToggle === true && isFixHandoffEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); + void repoFullName; // kept for call-site signature stability, see doc comment above + if (!isFixHandoffEnabled(env)) return false; + return manifestToggle === true; } diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index a42b15714f..dbd52c4c88 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -1,15 +1,15 @@ // Quiet inline PR review comments (#inline-comments) — the CodeRabbit-style line-level layer ON TOP OF the // decision summary. Posts the AI reviewer's line-anchored findings as a single NON-BLOCKING review (GitHub // `event: COMMENT`, never REQUEST_CHANGES/APPROVE), so a contributor sees exactly what to fix on a resubmission -// without the gate or its verdict ever changing. Default OFF at BOTH layers: the operator flag -// GITTENSORY_REVIEW_INLINE_COMMENTS (+ the per-repo GITTENSORY_REVIEW_REPOS cutover allowlist) AND the per-repo -// `.gittensory.yml` review.inline_comments toggle — the caller ANDs all three to decide whether to ASK the model -// for inline findings AND passes the same resolved gate to the write boundary. 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. +// without the gate or its verdict ever changing. Default OFF: the operator flag GITTENSORY_REVIEW_INLINE_COMMENTS +// is a master kill-switch, and the per-repo `.gittensory.yml` review.inline_comments toggle (#4099) fully +// controls activation by itself when explicitly set — the GITTENSORY_REVIEW_REPOS 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). 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. import { createPullRequestReviewComments } from "../github/pr-actions"; -import { isConvergenceRepoAllowed } from "./cutover-gate"; import { formatInlineCommentSeverityLabel } from "./inline-comment-label"; import { resolveInlineCommentAnchor, rightLinesByPath } from "./inline-comment-range"; import { addedLinesByPath, anchoredSuggestionBlock } from "./inline-suggestion-anchor"; @@ -28,16 +28,29 @@ export function isInlineCommentsEnabled(env: { GITTENSORY_REVIEW_INLINE_COMMENTS return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_INLINE_COMMENTS ?? ""); } -/** PURE: should the reviewer be asked to emit line-anchored inline findings for this PR? True ONLY when ALL THREE - * gates pass — the per-repo `.gittensory.yml` toggle (`manifestToggle`), the operator flag, AND the cutover - * allowlist — so the feature is off by default at every layer. Keeps the three-way gate in one unit-testable - * place instead of inline in the review path. */ +/** PURE (#4099): should the reviewer be asked to emit line-anchored inline findings for this PR? (1) The + * operator's GITTENSORY_REVIEW_INLINE_COMMENTS flag is an absolute MASTER KILL-SWITCH — off ⇒ always false, + * regardless of the manifest, and no per-repo config can bypass it (consistent with every other converged + * feature — see `resolveConvergedFeature` in `feature-activation.ts`). (2) An explicit per-repo + * `.gittensory.yml` `review.inlineComments` override (`true`/`false`) now FULLY controls the feature by itself + * — a repo can turn this on without needing the GITTENSORY_REVIEW_REPOS cutover allowlist at all. (3) + * `manifestToggle` unset (`undefined`) preserves this feature's ORIGINAL design exactly: unlike + * 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. */ 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 + // the doc comment above. env: { GITTENSORY_REVIEW_INLINE_COMMENTS?: string | undefined; GITTENSORY_REVIEW_REPOS?: string | undefined }, repoFullName: string, manifestToggle: boolean | undefined, ): boolean { - return manifestToggle === true && isInlineCommentsEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); + void repoFullName; // kept for call-site signature stability, see doc comment above + if (!isInlineCommentsEnabled(env)) return false; + return manifestToggle === true; } /** PURE (#1956): should a `suggestion` be rendered as a GitHub-native ` ```suggestion ` block? This is an diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 927c8d1306..4dd99b72de 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -249,7 +249,9 @@ export function composeManifestReviewInstructions(instructions: string | null, t * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043 / #selfhost-ai-model-override / #1956) */ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; effortScore: boolean; impactMap: boolean; cultureProfile: boolean; findingCategories: boolean; inlineCommentsPerCategory: number | null; minFindingSeverity: ReviewFindingSeverity | null; maxFindings: MaxFindingsConfig; commentVerbosity: CommentVerbosity | null; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { // inlineComments resolves to a strict boolean — true ONLY when the manifest explicitly set review.inline_comments: - // true; null/false/absent ⇒ false. The caller ANDs this per-repo toggle with the operator flag + cutover allowlist. + // true; null/false/absent ⇒ false. `shouldRequestInlineFindings` (#4099) only ever checks `=== true`, so null + // and false are functionally identical to it — collapsing here (matching every sibling field below) is simpler + // than plumbing a tri-state through for a distinction nothing downstream actually consumes. // securityFocus resolves the same way — true ONLY when the manifest explicitly set review.security_focus: true. // suggestions resolves the same way (#1956) — the caller further ANDs it with the already-resolved // inlineComments gate, since a suggestion has nothing to attach to without an inline comment. diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 536fe9f4f7..1831531a1d 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3038,7 +3038,11 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, changed_files_summary: true, effort_score: true, impact_map: true, culture_profile: true, finding_categories: true, comment_verbosity: "detailed", path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, changedFilesSummary: true, effortScore: true, impactMap: true, cultureProfile: true, findingCategories: true, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: "detailed", pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); - // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + changed-files summary + effort score + impact map + culture profile + finding categories + security focus default OFF. + // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + + // changed-files summary + effort score + impact map + culture profile + finding categories + security focus + // all default OFF (strict false) — inlineComments collapses the same way as every sibling flag on this + // object (#4099: shouldRequestInlineFindings only ever checks `=== true`, so null/false/absent are + // functionally identical to it; no tri-state needed here). expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, effortScore: false, impactMap: false, cultureProfile: false, findingCategories: false, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); // An explicit false / absent toggle both resolve to the strict-boolean false. expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: false } })).inlineComments).toBe(false); diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index f2391b6459..23335324d7 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -20,14 +20,25 @@ describe("isInlineCommentsEnabled (#inline-comments)", () => { }); }); -describe("shouldRequestInlineFindings (#inline-comments)", () => { +describe("shouldRequestInlineFindings (#inline-comments / #4099)", () => { const on = { GITTENSORY_REVIEW_INLINE_COMMENTS: "true", GITTENSORY_REVIEW_REPOS: "acme/widgets" }; - it("requires ALL THREE gates: the per-repo manifest toggle, the operator flag, and the cutover allowlist", () => { + it("operator flag is a master kill-switch — off ⇒ always false regardless of the manifest toggle", () => { + expect(shouldRequestInlineFindings({ GITTENSORY_REVIEW_REPOS: "acme/widgets" }, "acme/widgets", true)).toBe(false); + expect(shouldRequestInlineFindings({}, "acme/widgets", true)).toBe(false); + }); + + it("REGRESSION (#4099): unset manifest toggle stays false regardless of the cutover allowlist — byte-identical to before this change (being allowlisted was never sufficient on its own)", () => { + expect(shouldRequestInlineFindings(on, "acme/widgets", undefined)).toBe(false); + expect(shouldRequestInlineFindings(on, "other/repo", undefined)).toBe(false); + }); + + it("(#4099) an explicit manifest toggle: true fully controls the feature, even for a repo NOT on the cutover allowlist", () => { expect(shouldRequestInlineFindings(on, "acme/widgets", true)).toBe(true); - expect(shouldRequestInlineFindings(on, "acme/widgets", false)).toBe(false); // manifest toggle off - expect(shouldRequestInlineFindings(on, "acme/widgets", undefined)).toBe(false); // manifest toggle absent - expect(shouldRequestInlineFindings({ GITTENSORY_REVIEW_REPOS: "acme/widgets" }, "acme/widgets", true)).toBe(false); // operator flag off - expect(shouldRequestInlineFindings(on, "other/repo", true)).toBe(false); // repo not allowlisted + expect(shouldRequestInlineFindings(on, "other/repo", true)).toBe(true); + }); + + it("(#4099) an explicit manifest toggle: false forces the feature off, even for an allowlisted repo", () => { + expect(shouldRequestInlineFindings(on, "acme/widgets", false)).toBe(false); }); }); diff --git a/test/unit/review-fix-handoff.test.ts b/test/unit/review-fix-handoff.test.ts index fd6e87cbf9..7533adaceb 100644 --- a/test/unit/review-fix-handoff.test.ts +++ b/test/unit/review-fix-handoff.test.ts @@ -31,21 +31,27 @@ describe("review.fixHandoff config toggle (#2176)", () => { }); }); -describe("fix-handoff env kill-switch + resolver (#2176)", () => { +describe("fix-handoff env kill-switch + resolver (#2176 / #4099)", () => { it("isFixHandoffEnabled: only truthy env values enable", () => { for (const v of ["1", "true", "yes", "on", "TRUE"]) expect(isFixHandoffEnabled({ GITTENSORY_REVIEW_FIX_HANDOFF: v })).toBe(true); for (const v of ["0", "false", "off", "", undefined]) expect(isFixHandoffEnabled({ GITTENSORY_REVIEW_FIX_HANDOFF: v })).toBe(false); }); - it("shouldEmitFixHandoff: true ONLY when manifest toggle AND env flag AND cutover allowlist all pass", () => { - // all three on + it("operator flag is a master kill-switch — off ⇒ always false regardless of the manifest toggle", () => { + expect(shouldEmitFixHandoff({ GITTENSORY_REVIEW_FIX_HANDOFF: "0", GITTENSORY_REVIEW_REPOS: ON }, ON, true)).toBe(false); + }); + + it("REGRESSION (#4099): unset manifest toggle stays false regardless of the cutover allowlist — byte-identical to before this change (being allowlisted was never sufficient on its own)", () => { + expect(shouldEmitFixHandoff(ALLOW, ON, undefined)).toBe(false); + expect(shouldEmitFixHandoff({ GITTENSORY_REVIEW_FIX_HANDOFF: "1", GITTENSORY_REVIEW_REPOS: "other/repo" }, ON, undefined)).toBe(false); + }); + + it("(#4099) an explicit manifest toggle: true fully controls the feature, even for a repo NOT on the cutover allowlist", () => { expect(shouldEmitFixHandoff(ALLOW, ON, true)).toBe(true); - // manifest toggle off / undefined + expect(shouldEmitFixHandoff({ GITTENSORY_REVIEW_FIX_HANDOFF: "1", GITTENSORY_REVIEW_REPOS: "other/repo" }, ON, true)).toBe(true); + }); + + it("(#4099) an explicit manifest toggle: false forces the feature off, even for an allowlisted repo", () => { expect(shouldEmitFixHandoff(ALLOW, ON, false)).toBe(false); - expect(shouldEmitFixHandoff(ALLOW, ON, undefined)).toBe(false); - // env flag off - expect(shouldEmitFixHandoff({ GITTENSORY_REVIEW_FIX_HANDOFF: "0", GITTENSORY_REVIEW_REPOS: ON }, ON, true)).toBe(false); - // repo not on the cutover allowlist - expect(shouldEmitFixHandoff({ GITTENSORY_REVIEW_FIX_HANDOFF: "1", GITTENSORY_REVIEW_REPOS: "other/repo" }, ON, true)).toBe(false); }); });