feat(review): upgrade inlineComments + fixHandoff to full config-as-code substitutes (#4116)

* 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.

* 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.

Author: JSONbored

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

src/review/fix-handoff.ts

@@ -1,25 +1,36 @@
 // 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). */
 export function isFixHandoffEnabled(env: { GITTENSORY_REVIEW_FIX_HANDOFF?: string | undefined }): boolean {
   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;
 }

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

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.

test/unit/focus-manifest.test.ts

@@ -3039,7 +3039,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);

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);
   });
 });
 

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);
   });
 });