From 1099088601c7c34aa7af167a9f5af01ceb30c97d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:06:08 -0700 Subject: [PATCH 1/2] feat(review): upgrade inlineComments + fixHandoff to full config-as-code substitutes An explicit review.inlineComments/fixHandoff: true|false in .gittensory.yml now fully controls each feature, bypassing the GITTENSORY_REVIEW_REPOS cutover allowlist entirely. Unset stays byte-identical to before (the allowlist alone was never sufficient on its own for either feature). The operator's env flag remains an absolute master kill-switch either way. Closes #4099. --- src/queue/processors.ts | 8 +++--- src/review/fix-handoff.ts | 31 ++++++++++++++--------- src/review/inline-comments.ts | 37 ++++++++++++++++++---------- src/signals/focus-manifest.ts | 11 ++++++--- test/unit/focus-manifest.test.ts | 13 +++++++--- test/unit/inline-comments.test.ts | 23 ++++++++++++----- test/unit/review-fix-handoff.test.ts | 24 +++++++++++------- 7 files changed, 97 insertions(+), 50 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 75c769f19f..c6443a01c3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6921,9 +6921,11 @@ 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). - reviewInlineComments?: boolean | undefined; + // (the per-repo toggle). Precedence (#4099): the operator flag is a kill-switch; an explicit true/false here + // now FULLY controls the feature, bypassing the cutover allowlist; unset (null/undefined) 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 that hasn't touched this setting. + reviewInlineComments?: boolean | null | 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 // whether to ASK the model to self-categorize each inlineFindings item. Absent/false ⇒ byte-identical prompt. diff --git a/src/review/fix-handoff.ts b/src/review/fix-handoff.ts index 3bf40ed4f9..d6a6a439b4 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,23 @@ 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 a MASTER KILL-SWITCH — off ⇒ always false, regardless of the manifest. + * (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. Mirrors `shouldRequestInlineFindings`. `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..9eb546fca3 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,27 @@ 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 a MASTER KILL-SWITCH — off ⇒ always false, regardless of + * the manifest. (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 (`null`/`undefined`) preserves this feature's ORIGINAL + * design exactly: unlike rag/reputation/safety/unifiedComment (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, + manifestToggle: boolean | null | 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..7a03eedf72 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -247,9 +247,12 @@ export function composeManifestReviewInstructions(instructions: string | null, t * failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review caller threads them * in one place with the null-manifest branch covered here (unit-tested) rather than inline in the processor. * (#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. +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean | null; 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 (#4099) preserves the manifest's TRI-STATE (true/false/null) rather than collapsing it — + // `shouldRequestInlineFindings` needs to distinguish an explicit true/false (now FULLY controls the feature, + // bypassing the cutover allowlist) from unset (byte-identical to every repo's behavior before this change: + // the allowlist alone was never sufficient, so unset still resolves to "off"). A null manifest (load failure) + // ⇒ null (unset), same as an absent manifest field. // 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. @@ -268,7 +271,7 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { // cultureProfile resolves the same way (#2995) — true ONLY when the manifest explicitly set // review.culture_profile: true. The caller ANDs this per-repo opt-in with the GITTENSORY_REVIEW_CULTURE_PROFILE // global kill-switch (mirrors how RAG/reputation/grounding compose a global flag with a per-repo override). - return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, inlineCommentsPerCategory: manifest?.review.inlineCommentsPerCategory ?? null, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments ?? null, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, inlineCommentsPerCategory: manifest?.review.inlineCommentsPerCategory ?? null, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.test_generation` (#2189, config slice of #1972) from a possibly-null manifest (null = load diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 536fe9f4f7..ec1fc40793 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3038,11 +3038,16 @@ 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. - 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. + // A null manifest (load failure) yields the byte-identical defaults; suggestions + changed-files summary + + // effort score + impact map + culture profile + finding categories + security focus default OFF (strict + // false). inlineComments (#4099) preserves the manifest's tri-state instead — a null manifest is "unset", not + // an explicit false, so it resolves to null (consumed by shouldRequestInlineFindings's own precedence). + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: null, 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 } }); + // inlineComments (#4099): explicit true/false pass through as-is; absent resolves to null (unset), NOT false — + // distinct from every other flag on this object, which collapses absent to strict false. expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: false } })).inlineComments).toBe(false); - expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: true } })).inlineComments).toBe(true); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBeNull(); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { suggestions: false } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { changed_files_summary: false } })).changedFilesSummary).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); }); }); From 721818699e58ccfe938fe8f30515452deed20dd0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:31:37 -0700 Subject: [PATCH 2/2] fix(review): simplify inlineComments/fixHandoff parity, correct kill-switch docs - Revert the unnecessary boolean|null tri-state on shouldRequestInlineFindings and resolveReviewPromptOverrides's inlineComments field -- the function only ever checks '=== true', so null and undefined were functionally identical; collapsing back to a strict boolean (matching every sibling field) removes complexity that served no purpose, per gittensory-orb review feedback. - Make shouldEmitFixHandoff match shouldRequestInlineFindings exactly (both boolean | undefined, same shape) instead of drifting to boolean | null. - Clarify in both doc comments that the operator's env flag is an ABSOLUTE kill-switch never bypassable by per-repo config, consistent with every other converged feature (resolveConvergedFeature) -- addresses a linked- issue-satisfaction flag about apparent scope drift from the issue text. --- src/queue/processors.ts | 10 +++++----- src/review/fix-handoff.ts | 16 +++++++++------- src/review/inline-comments.ts | 22 ++++++++++++---------- src/signals/focus-manifest.ts | 13 ++++++------- test/unit/focus-manifest.test.ts | 17 ++++++++--------- 5 files changed, 40 insertions(+), 38 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c6443a01c3..f9f77ca22e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6921,11 +6921,11 @@ 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). Precedence (#4099): the operator flag is a kill-switch; an explicit true/false here - // now FULLY controls the feature, bypassing the cutover allowlist; unset (null/undefined) 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 that hasn't touched this setting. - reviewInlineComments?: boolean | null | undefined; + // (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 // whether to ASK the model to self-categorize each inlineFindings item. Absent/false ⇒ byte-identical prompt. diff --git a/src/review/fix-handoff.ts b/src/review/fix-handoff.ts index d6a6a439b4..95dbae4f8e 100644 --- a/src/review/fix-handoff.ts +++ b/src/review/fix-handoff.ts @@ -13,13 +13,15 @@ export function isFixHandoffEnabled(env: { GITTENSORY_REVIEW_FIX_HANDOFF?: strin } /** PURE (#4099): should the reviewer emit fix-handoff blocks for this PR? (1) The operator's - * GITTENSORY_REVIEW_FIX_HANDOFF flag is a MASTER KILL-SWITCH — off ⇒ always false, regardless of the manifest. - * (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. Mirrors `shouldRequestInlineFindings`. `repoFullName` is kept for a - * stable call signature even though it's unused now that the allowlist no longer applies here. */ + * 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 diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index 9eb546fca3..dbd52c4c88 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -29,22 +29,24 @@ export function isInlineCommentsEnabled(env: { GITTENSORY_REVIEW_INLINE_COMMENTS } /** 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 a MASTER KILL-SWITCH — off ⇒ always false, regardless of - * the manifest. (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 (`null`/`undefined`) preserves this feature's ORIGINAL - * design exactly: unlike rag/reputation/safety/unifiedComment (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. */ + * 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 | null | undefined, + manifestToggle: boolean | undefined, ): boolean { void repoFullName; // kept for call-site signature stability, see doc comment above if (!isInlineCommentsEnabled(env)) return false; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 7a03eedf72..4dd99b72de 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -247,12 +247,11 @@ export function composeManifestReviewInstructions(instructions: string | null, t * failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review caller threads them * in one place with the null-manifest branch covered here (unit-tested) rather than inline in the processor. * (#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 | null; 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 (#4099) preserves the manifest's TRI-STATE (true/false/null) rather than collapsing it — - // `shouldRequestInlineFindings` needs to distinguish an explicit true/false (now FULLY controls the feature, - // bypassing the cutover allowlist) from unset (byte-identical to every repo's behavior before this change: - // the allowlist alone was never sufficient, so unset still resolves to "off"). A null manifest (load failure) - // ⇒ null (unset), same as an absent manifest field. +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. `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. @@ -271,7 +270,7 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { // cultureProfile resolves the same way (#2995) — true ONLY when the manifest explicitly set // review.culture_profile: true. The caller ANDs this per-repo opt-in with the GITTENSORY_REVIEW_CULTURE_PROFILE // global kill-switch (mirrors how RAG/reputation/grounding compose a global flag with a per-repo override). - return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments ?? null, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, inlineCommentsPerCategory: manifest?.review.inlineCommentsPerCategory ?? null, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, inlineCommentsPerCategory: manifest?.review.inlineCommentsPerCategory ?? null, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.test_generation` (#2189, config slice of #1972) from a possibly-null manifest (null = load diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index ec1fc40793..1831531a1d 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3038,16 +3038,15 @@ 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; suggestions + changed-files summary + - // effort score + impact map + culture profile + finding categories + security focus default OFF (strict - // false). inlineComments (#4099) preserves the manifest's tri-state instead — a null manifest is "unset", not - // an explicit false, so it resolves to null (consumed by shouldRequestInlineFindings's own precedence). - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: null, 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 } }); - // inlineComments (#4099): explicit true/false pass through as-is; absent resolves to null (unset), NOT false — - // distinct from every other flag on this object, which collapses absent to strict false. + // 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); - expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: true } })).inlineComments).toBe(true); - expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBeNull(); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { suggestions: false } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { changed_files_summary: false } })).changedFilesSummary).toBe(false);